fix(feature): only relies on desired state for reconcilation #1103
Conversation
bartoszmajsak
requested review from
aslakknutsen,
zdtsw,
israel-hdez and
cam-garrison
bartoszmajsak
force-pushed
the
feature-reconcile-fix
branch
from
a820929
to
673fb94
Compare
|
Putting on |
bartoszmajsak
force-pushed
the
feature-reconcile-fix
branch
from
673fb94
to
538fed1
Compare
This change ensures that management state is based exclusively on a resource defined in desired state, i.e. the one provided by the controller itself. This can be, but is not limited to, manifest files provided through embedded file system. Although the actual instance on the cluster (the target) might be in a different state, we intentionally do not address this scenario due to the lack of clear requirements on the extent to which users can modify such resource. This change allows to take control over previously undefined resources, so those which are created by `Feature` API without explicit management state defined. As long as original Feature does not have `Managed()` called in the builder chain, nor used manifests have the label explicitly defined the behaviour remain unchanged, that means the resources being part of such a feature are not managed and thus not reconciled after initial apply. `opendatahub.io/managed` is already exposed as an annotation and used by [other components](https://docs.redhat.com/en/documentation/red_hat_openshift_ai_self-managed/2.10/html/installing_and_uninstalling_openshift_ai_self-managed/preparing-openshift-ai-for-ibm-cpd_prepare-openshift-ai-ibm-cpd#editing-model-inferencing-configuration-ibm-cpd_prepare-openshift-ai-ibm-cpd). Follow up to opendatahub-io#974
bartoszmajsak
force-pushed
the
feature-reconcile-fix
branch
from
538fed1
to
1d2658d
Compare
| if isUnmanaged(source) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
As discussed offline with @zdtsw, this is somewhat redundant as we return false at the end. We do have this behaviour covered in tests, but I was wondering if it wouldn't be beneficial to keep this check in the original code as a contract so that when the requirements about relying on target state will become clear this will be explicitly visible. I am on the verge here.
that has been clarified now - annotation that is. |
There was a problem hiding this comment.
So, this changes from looking at the bundled manifests, rather than looking at the resource in the cluster.
I'm fine with the change, as it is backwards compatible. Although, it should be noted that in the documented case the annotation is editable by the user to disengage reconciliation, while in Features developers have the control. So, it is two different meanings. Just to keep in mind.
Correct, and note it is not used anywhere in the
This is why I mentioned in the godocs, that the requirements are not clear yet - we only handle it for one specific case for KServe kustomize manifests through |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cam-garrison, 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:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit e12d7d7
into
opendatahub-io:incubation
Here I have to defer to @zdtsw and @VaishnaviHire about the godocs. If you mean about the doc-comments in the code, I also appreciate clarification there for us, the developers. About the other docs mentioned in the main comment, I guess the policy is that only documented uses are supported; so those docs should be fine as is. |
…ahub-io#1103) This change ensures that management state is based exclusively on a resource defined in desired state, i.e. the one provided by the controller itself. This can be, but is not limited to, manifest files provided through embedded file system. Although the actual instance on the cluster (the target) might be in a different state, we intentionally do not address this scenario due to the lack of clear requirements on the extent to which users can modify such resource. This change allows to take control over previously undefined resources, so those which are created by `Feature` API without explicit management state defined. As long as original Feature does not have `Managed()` called in the builder chain, nor used manifests have the label explicitly defined the behaviour remain unchanged, that means the resources being part of such a feature are not managed and thus not reconciled after initial apply. `opendatahub.io/managed` is already exposed as an annotation and used by [other components](https://docs.redhat.com/en/documentation/red_hat_openshift_ai_self-managed/2.10/html/installing_and_uninstalling_openshift_ai_self-managed/preparing-openshift-ai-for-ibm-cpd_prepare-openshift-ai-ibm-cpd#editing-model-inferencing-configuration-ibm-cpd_prepare-openshift-ai-ibm-cpd). Follow up to opendatahub-io#974 (cherry picked from commit e12d7d7)
…ahub-io#1103) This change ensures that management state is based exclusively on a resource defined in desired state, i.e. the one provided by the controller itself. This can be, but is not limited to, manifest files provided through embedded file system. Although the actual instance on the cluster (the target) might be in a different state, we intentionally do not address this scenario due to the lack of clear requirements on the extent to which users can modify such resource. This change allows to take control over previously undefined resources, so those which are created by `Feature` API without explicit management state defined. As long as original Feature does not have `Managed()` called in the builder chain, nor used manifests have the label explicitly defined the behaviour remain unchanged, that means the resources being part of such a feature are not managed and thus not reconciled after initial apply. `opendatahub.io/managed` is already exposed as an annotation and used by [other components](https://docs.redhat.com/en/documentation/red_hat_openshift_ai_self-managed/2.10/html/installing_and_uninstalling_openshift_ai_self-managed/preparing-openshift-ai-for-ibm-cpd_prepare-openshift-ai-ibm-cpd#editing-model-inferencing-configuration-ibm-cpd_prepare-openshift-ai-ibm-cpd). Follow up to opendatahub-io#974 (cherry picked from commit e12d7d7)
| func isManaged(obj *unstructured.Unstructured) bool { | ||
| managed, isDefined := obj.GetAnnotations()[annotations.ManagedByODHOperator] | ||
| return isDefined && managed == "true" | ||
| } |
There was a problem hiding this comment.
So, how is the 3rd state, !isDefined, which is neither managed nor unmanaged called? :)
There was a problem hiding this comment.
then shouldReconcile() return false in the case of !IsDefined , right?
There was a problem hiding this comment.
then
shouldReconcile() return false in the case of !IsDefined , right?
Yes. As far as I understand #1103 (comment) you discussed it offline and kept it on purpose. It just looks a bit weird from the reader's prospective :)
In general, as mentioned in the comment, shouldReconcile is just isManaged(), and Unmanaged probably means !isManaged().
There was a problem hiding this comment.
I admit my 🧠 was tripping at the time of writing it. Happy to discuss how to make it better.
Description
This change ensures that the "management mode" is based exclusively on a resource defined as the desired state, i.e. the one provided by the controller itself. This can be, but is not limited to, manifest files provided through an embedded file system.
Although the actual instance on the cluster (the target) might be in a different state (the user might want to opt-out from reconcilation by changing the label), we intentionally do not address this scenario due to the lack of clear requirements on how users can modify such resources.
This change allows control over previously undefined resources, so those created by
FeatureAPI without explicit management state defined. As long as the original Feature does not haveManaged()called in the builder chain, nor used manifests have the label explicitly defined the behaviour remains unchanged, which means the resources being part of such a feature are not managed and thus not reconciled after initial apply.opendatahub.io/managedis already exposed as an annotation and used by other components.Follow up to #974
How Has This Been Tested?
Integration tests are included in
tests/integration/features/resources_int_test.goScreenshot or short clip
Merge criteria
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).