feat: reworks Feature to be generic data container #1052

bartoszmajsak · 2024-06-12T13:15:03Z

Description

Important

For the high-level overview please have a look at README.md which is part of this PR. It's a good starting point :)

Feature Struct Improvements

Eliminates the .Spec field which was a single point of coupling with various domain structs used across the project.
Introduces a generic map to store keys relevant to a given feature, which are utilized in templates and other functions.

Feature Builder Changes

Renamed a few functions to better reflect their intent:
- entry builder function feature.CreateFeature becomes feature.Define
- Load() becomes Create()
.WithData builder function serves as an entry point to define the contents of a given feature. This chain method allows adding key-values to the feature's context. These values are later used in templates and can also be accessed in pre/post conditions and for programmatic resource creation.

Additional Changes

Simplifies and decouples the use of FeatureHandler. It is no longer necessary to pass it to the Feature builder to group features together.
Introduces a new FeatureRegistry interface, allowing convenient addition of feature sets to the handler via the FeatureProvider function.
Introduces a convention for defining FeatureData for specific parts of the platform (e.g., Serverless or Service Mesh setup). This approach enhances readability and promotes the reuse of commonly used data entries.
Adds a README.md file providing a high-level overview of the Feature DSL.

JIRA issue:

https://issues.redhat.com/browse/RHOAIENG-4579

How Has This Been Tested?

Besides automated tests, I enabled KServe component and tested sample ISVC using this script

Warning

There seems to be regression in KServe/ODH Model Controller deployment as described in opendatahub-io/odh-model-controller#233 opendatahub-io/odh-model-controller#234. Follow workarounds to see that it all works as intended.

Merge criteria:

Have a meaningful commit messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

bartoszmajsak · 2024-06-12T13:17:02Z

Important

Deliberately put do-not-merge/hold label as I want to solicit feedback from multiple people and avoid one LGTM resulting in auto-merge. I consider it ready to review. This was in my backlog for a long time and the code I wasn't really happy with, so I want to end up with something which is flexible and easy to use.

pkg/upgrade/upgrade.go

pkg/feature/cert.go

components/kserve/servicemesh_setup.go

AjayJagan

thanks for this @bartoszmajsak .

bartoszmajsak · 2024-07-09T19:29:30Z

I think it's been incubating for long enough, removing hold label. Thank you everyone for feedback here and through other channels. Waiting for the final round of review!

bartoszmajsak · 2024-07-10T10:02:51Z

/retest

Feature Struct Improvements - Eliminates the `.Spec` field, eliminating a single point of coupling with various domain structs used across the project. - Introduces a generic map to store keys relevant to a given feature, which are utilized in templates and other functions. Feature Builder Changes - Renamed few functions to better reflect their intent - entry builder function `feature.CreateFeature` becomes `feature.Define` - Load() becomes Create() - `.WithData` builder function serves as an entry point to define the contents of a given feature. This chain method allows adding key-values to the feature's context. These values are later used in templates and can also be accessed in pre/post conditions and for programmatic resource creation. Additional Changes - Simplifies and decouples the use of `FeatureHandler`. It is no longer necessary to patss it to the Feature builder to group features together. - Introduces a new `FeatureRegistry` interface, allowing convenient addition of feature sets to the handler via the `FeatureProvider` function. - Introduces a convention for defining `FeatureData` for specific parts of the platform (e.g., Serverless or Service Mesh setup). This approach enhances readability and promotes the reuse of commonly used data entries. - Adds a `README.md` file providing a high-level overview of the `Feature DSL`.

openshift-ci · 2024-07-10T11:42:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AjayJagan, israel-hdez, zdtsw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [AjayJagan,zdtsw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

With the opendatahub-io#1052 refactoring, the order of features added to the Registry was accidentally changed. It results in failing of metrics collection feature which expects SMCP to be created first, but the creation runs afterwards. The setup is eventually consistent, as the reconcile will retry, so this not a bug per se, but results in unnecassary errors. This fix ensures features are ordered as before and levarages `.EnabledWhen` instead of wrapping features in `if`s.

With the #1052 refactoring, the order of features added to the Registry was accidentally changed. It results in failing of metrics collection feature which expects SMCP to be created first, but the creation runs afterwards. The setup is eventually consistent, as the reconcile will retry, so this not a bug per se, but results in unnecassary errors. This fix ensures features are ordered as before and levarages `.EnabledWhen` instead of wrapping features in `if`s.

If the authorization provider namespace is not specified in the DSCI the default is constructed to be `application-namespace-auth-provider`, e.g. `opendatahub-auth-provider`. With the opendatahub-io#1052 refactoring, the regression has been introduced where the value is directly read from the spec instead of being dynamically constructed based on the rule described above. This is manifested with the following error, as the feature mistakenly waits for pods across all namespaces (because of list option for namespace being `corev1.NamespaceAll == ""`). This obviously rarely is true, especially for large clusters. ```json Failed applying [enable-proxy-injection-in-authorino-deployment]: 1 error occurred: * client rate limiter Wait returned an error: context deadline exceeded ``` leading to failure of reconciling this feature. The fix is to read the namespace from `FeatureData` instead, where the defaulting logic is defined.

If the authorization provider namespace is not specified in the DSCI the default is constructed to be `application-namespace-auth-provider`, e.g. `opendatahub-auth-provider`. With the opendatahub-io#1052 refactoring, the regression has been introduced where the value is directly read from the spec instead of being dynamically constructed based on the rule described above. This is manifested with the following error, as the feature mistakenly waits for pods across all namespaces (because of list option for namespace being `corev1.NamespaceAll == ""`). This obviously rarely is true, especially for large clusters. ```json Failed applying [enable-proxy-injection-in-authorino-deployment]: 1 error occurred: * client rate limiter Wait returned an error: context deadline exceeded ``` leading to failure of reconciling this feature. The fix is to read the namespace from `FeatureData` instead, where the defaulting logic is defined. Fixes https://issues.redhat.com/browse/RHOAIENG-10268

…e value (#1135) * fix: fixes authz patch injection feature precondition If the authorization provider namespace is not specified in the DSCI the default is constructed to be `application-namespace-auth-provider`, e.g. `opendatahub-auth-provider`. With the #1052 refactoring, the regression has been introduced where the value is directly read from the spec instead of being dynamically constructed based on the rule described above. This is manifested with the following error, as the feature mistakenly waits for pods across all namespaces (because of list option for namespace being `corev1.NamespaceAll == ""`). This obviously rarely is true, especially for large clusters. ```json Failed applying [enable-proxy-injection-in-authorino-deployment]: 1 error occurred: * client rate limiter Wait returned an error: context deadline exceeded ``` leading to failure of reconciling this feature. The fix is to read the namespace from `FeatureData` instead, where the defaulting logic is defined. Fixes https://issues.redhat.com/browse/RHOAIENG-10268 * Update controllers/dscinitialization/servicemesh_setup.go Co-authored-by: Wen Zhou <[email protected]> --------- Co-authored-by: Wen Zhou <[email protected]>

Feature Struct Improvements - Eliminates the `.Spec` field, eliminating a single point of coupling with various domain structs used across the project. - Introduces a generic map to store keys relevant to a given feature, which are utilized in templates and other functions. Feature Builder Changes - Renamed few functions to better reflect their intent - entry builder function `feature.CreateFeature` becomes `feature.Define` - Load() becomes Create() - `.WithData` builder function serves as an entry point to define the contents of a given feature. This chain method allows adding key-values to the feature's context. These values are later used in templates and can also be accessed in pre/post conditions and for programmatic resource creation. Additional Changes - Simplifies and decouples the use of `FeatureHandler`. It is no longer necessary to patss it to the Feature builder to group features together. - Introduces a new `FeatureRegistry` interface, allowing convenient addition of feature sets to the handler via the `FeatureProvider` function. - Introduces a convention for defining `FeatureData` for specific parts of the platform (e.g., Serverless or Service Mesh setup). This approach enhances readability and promotes the reuse of commonly used data entries. - Adds a `README.md` file providing a high-level overview of the `Feature DSL`. (cherry picked from commit 973ff74)

…o#1118) With the opendatahub-io#1052 refactoring, the order of features added to the Registry was accidentally changed. It results in failing of metrics collection feature which expects SMCP to be created first, but the creation runs afterwards. The setup is eventually consistent, as the reconcile will retry, so this not a bug per se, but results in unnecassary errors. This fix ensures features are ordered as before and levarages `.EnabledWhen` instead of wrapping features in `if`s. (cherry picked from commit d6f25c7)

…e value (opendatahub-io#1135) * fix: fixes authz patch injection feature precondition If the authorization provider namespace is not specified in the DSCI the default is constructed to be `application-namespace-auth-provider`, e.g. `opendatahub-auth-provider`. With the opendatahub-io#1052 refactoring, the regression has been introduced where the value is directly read from the spec instead of being dynamically constructed based on the rule described above. This is manifested with the following error, as the feature mistakenly waits for pods across all namespaces (because of list option for namespace being `corev1.NamespaceAll == ""`). This obviously rarely is true, especially for large clusters. ```json Failed applying [enable-proxy-injection-in-authorino-deployment]: 1 error occurred: * client rate limiter Wait returned an error: context deadline exceeded ``` leading to failure of reconciling this feature. The fix is to read the namespace from `FeatureData` instead, where the defaulting logic is defined. Fixes https://issues.redhat.com/browse/RHOAIENG-10268 * Update controllers/dscinitialization/servicemesh_setup.go Co-authored-by: Wen Zhou <[email protected]> --------- Co-authored-by: Wen Zhou <[email protected]> (cherry picked from commit 7034768)

Feature Struct Improvements - Eliminates the `.Spec` field, eliminating a single point of coupling with various domain structs used across the project. - Introduces a generic map to store keys relevant to a given feature, which are utilized in templates and other functions. Feature Builder Changes - Renamed few functions to better reflect their intent - entry builder function `feature.CreateFeature` becomes `feature.Define` - Load() becomes Create() - `.WithData` builder function serves as an entry point to define the contents of a given feature. This chain method allows adding key-values to the feature's context. These values are later used in templates and can also be accessed in pre/post conditions and for programmatic resource creation. Additional Changes - Simplifies and decouples the use of `FeatureHandler`. It is no longer necessary to patss it to the Feature builder to group features together. - Introduces a new `FeatureRegistry` interface, allowing convenient addition of feature sets to the handler via the `FeatureProvider` function. - Introduces a convention for defining `FeatureData` for specific parts of the platform (e.g., Serverless or Service Mesh setup). This approach enhances readability and promotes the reuse of commonly used data entries. - Adds a `README.md` file providing a high-level overview of the `Feature DSL`. (cherry picked from commit 973ff74)

…o#1118) With the opendatahub-io#1052 refactoring, the order of features added to the Registry was accidentally changed. It results in failing of metrics collection feature which expects SMCP to be created first, but the creation runs afterwards. The setup is eventually consistent, as the reconcile will retry, so this not a bug per se, but results in unnecassary errors. This fix ensures features are ordered as before and levarages `.EnabledWhen` instead of wrapping features in `if`s. (cherry picked from commit d6f25c7)

…e value (opendatahub-io#1135) * fix: fixes authz patch injection feature precondition If the authorization provider namespace is not specified in the DSCI the default is constructed to be `application-namespace-auth-provider`, e.g. `opendatahub-auth-provider`. With the opendatahub-io#1052 refactoring, the regression has been introduced where the value is directly read from the spec instead of being dynamically constructed based on the rule described above. This is manifested with the following error, as the feature mistakenly waits for pods across all namespaces (because of list option for namespace being `corev1.NamespaceAll == ""`). This obviously rarely is true, especially for large clusters. ```json Failed applying [enable-proxy-injection-in-authorino-deployment]: 1 error occurred: * client rate limiter Wait returned an error: context deadline exceeded ``` leading to failure of reconciling this feature. The fix is to read the namespace from `FeatureData` instead, where the defaulting logic is defined. Fixes https://issues.redhat.com/browse/RHOAIENG-10268 * Update controllers/dscinitialization/servicemesh_setup.go Co-authored-by: Wen Zhou <[email protected]> --------- Co-authored-by: Wen Zhou <[email protected]> (cherry picked from commit 7034768)

bartoszmajsak added the do-not-merge/hold label Jun 12, 2024

bartoszmajsak requested review from aslakknutsen, zdtsw, israel-hdez and cam-garrison June 12, 2024 13:15

openshift-ci bot requested review from ajaypratap003 and Sara4994 June 12, 2024 13:15

bartoszmajsak requested review from AjayJagan and removed request for ajaypratap003 and Sara4994 June 12, 2024 13:17

bartoszmajsak commented Jun 12, 2024

View reviewed changes

pkg/upgrade/upgrade.go Outdated Show resolved Hide resolved

bartoszmajsak commented Jun 12, 2024

View reviewed changes

pkg/feature/cert.go Outdated Show resolved Hide resolved

bartoszmajsak force-pushed the feature-spec-container branch from d5f0313 to d51444a Compare June 14, 2024 10:14

aslakknutsen reviewed Jun 14, 2024

View reviewed changes

components/kserve/servicemesh_setup.go Show resolved Hide resolved

bartoszmajsak force-pushed the feature-spec-container branch 3 times, most recently from a148731 to 6d1ead1 Compare June 18, 2024 09:34

AjayJagan approved these changes Jun 19, 2024

View reviewed changes

openshift-ci bot assigned AjayJagan Jun 19, 2024

openshift-ci bot added lgtm approved labels Jun 19, 2024

openshift-merge-robot added the needs-rebase label Jun 19, 2024

bartoszmajsak force-pushed the feature-spec-container branch from 6d1ead1 to 1cb8332 Compare June 20, 2024 13:02

openshift-ci bot removed the lgtm label Jun 20, 2024

openshift-merge-robot removed the needs-rebase label Jun 20, 2024

bartoszmajsak force-pushed the feature-spec-container branch 2 times, most recently from 7c5219d to 5a0c315 Compare June 20, 2024 14:48

openshift-merge-robot added the needs-rebase label Jun 20, 2024

bartoszmajsak force-pushed the feature-spec-container branch from 5a0c315 to 573f99e Compare June 20, 2024 14:57

bartoszmajsak removed the do-not-merge/hold label Jul 9, 2024

bartoszmajsak force-pushed the feature-spec-container branch 2 times, most recently from 50d83fc to 58fc075 Compare July 9, 2024 20:29

openshift-merge-robot added the needs-rebase label Jul 10, 2024

bartoszmajsak force-pushed the feature-spec-container branch 2 times, most recently from 8d2da19 to 0265341 Compare July 10, 2024 09:43

openshift-merge-robot removed the needs-rebase label Jul 10, 2024

bartoszmajsak force-pushed the feature-spec-container branch from 0265341 to e45dbea Compare July 10, 2024 11:25

zdtsw approved these changes Jul 10, 2024

View reviewed changes

openshift-ci bot assigned zdtsw Jul 10, 2024

openshift-ci bot added the lgtm label Jul 10, 2024

openshift-merge-bot bot merged commit 973ff74 into opendatahub-io:incubation Jul 10, 2024
8 checks passed

bartoszmajsak mentioned this pull request Jul 12, 2024

fix: ensures SMCP is created before any other features #1118

Merged

5 tasks

bartoszmajsak mentioned this pull request Jul 23, 2024

fix: authz patch injection feature precondition uses correct namespace value #1135

Merged

5 tasks

bartoszmajsak deleted the feature-spec-container branch July 30, 2024 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reworks Feature to be generic data container #1052

feat: reworks Feature to be generic data container #1052

bartoszmajsak commented Jun 12, 2024 •

edited

Loading

bartoszmajsak commented Jun 12, 2024

AjayJagan left a comment

bartoszmajsak commented Jul 9, 2024

bartoszmajsak commented Jul 10, 2024

openshift-ci bot commented Jul 10, 2024

feat: reworks Feature to be generic data container #1052

feat: reworks Feature to be generic data container #1052

Conversation

bartoszmajsak commented Jun 12, 2024 • edited Loading

Description

Feature Struct Improvements

Feature Builder Changes

Additional Changes

JIRA issue:

How Has This Been Tested?

Merge criteria:

bartoszmajsak commented Jun 12, 2024

AjayJagan left a comment

Choose a reason for hiding this comment

bartoszmajsak commented Jul 9, 2024

bartoszmajsak commented Jul 10, 2024

openshift-ci bot commented Jul 10, 2024

bartoszmajsak commented Jun 12, 2024 •

edited

Loading