fix(DSC): do not reconcile resource if it has a special annotation #879

zdtsw · 2024-02-21T15:15:51Z

ref: https://issues.redhat.com/browse/RHOAIENG-3111, https://issues.redhat.com/browse/RHOAIENG-1262

for kserve to work with raw deployment
see #864: inferenceServiceConfigMap.Data["deploy"] = string(deployDataBytes)

Description

if a resource has annotation opendatahub.io/managed: 'false' it will not be reconcile by DSC
if a resource has annoation opendatahub.io/managed: 'false' it will not be revert after upgrade ODH
we still set owererefence on this resource regardless it has this annotation or not, so it gets GC when component is Removed.

How Has This Been Tested?

live build quay.io/wenzhou/opendatahub-operator-catalog:v2.8.3111-1

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful 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

rawdeployment Signed-off-by: Wen Zhou <wenzhou@redhat.com>

controllers/datasciencecluster/datasciencecluster_controller.go

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

ykaliuta · 2024-02-21T16:15:04Z

/lgtm

danielezonca · 2024-02-21T16:52:41Z

controllers/datasciencecluster/datasciencecluster_controller.go

@@ -335,6 +335,10 @@ var configMapPredicates = predicate.Funcs{
 		if e.ObjectNew.GetName() == "prometheus" && e.ObjectNew.GetNamespace() == "redhat-ods-monitoring" {
 			return false
 		}
+		// Do not reconcile on kserver's inferenceservice-config CM updates, for rawdeployment
+		if e.ObjectNew.GetName() == "inferenceservice-config" && (e.ObjectNew.GetNamespace() == "redhat-ods-applications" || e.ObjectNew.GetNamespace() == "opendatahub") {


Please correct if I'm wrong but I'm not sure if this if condition works like we need: during an upgrade (i.e. from version 2.1 to 2.2) this value should not be overridden otherwise the user will need to restore the configuration after every release.

@etirelli
Is this the predicate that you mentioned be that can be used to bypass the reconcile?
Is there any way (like an annotation?) to let the user opt-out the reconcile? This configMap is something the user might want to update even if it is not something that we officially document yet

@VaishnaviHire
FYI

originally with this change, my thought goes with "changes in one specific CM should not lead to unnecessary reconcile of DSC"
but after some offline talk, I think the real problem we want to resolve here is:
user can update CM's data and not get reverted back during the same ODH release and even update ODH upgrade to a new release.

so i think in this case we will need 2 changes:

prevent DSC reconcile because of this CM has been updated

skip ownerreference on some resources
and the only thing i can think of for (2) is to use an annotation as indication, which user can add it when they are updating CM's data

Out of curiosity, why the namespace test is needed?

BTW, I think the ownership annotation is still needed, because IMO uninstalling the component should still collect and remove the CM. So, IMO, the annotation should simply control whether cli.Patch should be skipped in the managedResource func of pkg/deploy/deploy.go.

@israel-hdez

Out of curiosity, why the namespace test is needed?

the part to check if it is in opendatahub or in redhat-ods-applications?
only to narrow down if the CM is in these 2 namespaces. do you see it is possible that users create a different application namespace and deploy kseve into that?

good point @israel-hdez
if not set ownerreference, then users will need to cleanup these resource by themselves when they Removed component or uninstall ODH.
we need to be clear:

if annoation is set by the users, what consequence it will be

if component manifests has it set by default, do we need to state this in the release docs to user
leave above ^ to @danielezonca
and i start to lean towards your idea to "keep ownerreference but just skip patch"
this is mainly because i am thinking, in the first case, annoation is post added by user, so when it first gets applied to the cluster (by component set as Managed) it should get ownerreference already. to skip it , only works the annotation has been added in the manifests out of the box.

Well overall the use case is to let user customise this configMap and at that point the operator should not reconcile it anymore.
About the delete I agree that it is fine to keep removing it because it is tied to KServe so if the user remove KServe it is fine to remove it.

I'm fine with an exception (if) or an annotation to opt-out. The "skip patch" should work but we need to make sure it works also when we update from one version to another (i.e. 2.7 -> 2.8)

Note: this feature for now is a bit "advanced" and not really something we would like to document for "normal usage" but it can be necessary for hotfixes or ad hoc solutions and at the moment the only way to make it work (as far as I know) is to scale the operator to 0 that is definitely big as impact.

i made another updates on this PR:

we will keep adding owerreference (same logic as before)

we do not patch resource if it has annotation set

even upgrade to a new version of ODH, it consists with same customized content if annotation exists

when component is Removed, such resource will be removed as well.

This logic seems good to me (but I have not tested the PR)

Why would you need to test it manually? Can we add some automated tests to cover those scenarios? Is it a big effort?

annotation to false Signed-off-by: Wen Zhou <wenzhou@redhat.com>

ykaliuta · 2024-02-21T21:17:12Z

pkg/deploy/deploy.go

@@ -271,7 +271,8 @@ func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Un
 	if apierrs.IsNotFound(err) {
 		// Set the owner reference for garbage collection
 		// Skip set on CRD, e.g. we should not delete notebook CRD if we delete DSC instance
-		if found.GetKind() != "CustomResourceDefinition" {
+		// Skip on resource if has annotation "opendatahub.io/managed: false"
+		if found.GetKind() != "CustomResourceDefinition" || found.GetAnnotations()["opendatahub.io/managed"] != "false" {


is the intention here "if it is CustomResourceDefinition OR there is managed=false annotation, then skip setting the reference"? If so should the negation be with AND then?

i was not sure in this case should annoation take over CRD kind or CRD kind should always be skipped regardless annoation.
now i think probably the later is more safe, so we do not change behavior, and we can decide what we want later.
i will do the change, what i want to do now:

if it is CRD regardless annotation, skip

if it is not CRD and has annoation to false, skip

othercases, set ownerreference

- we still set ownerreference on resources - we only skip reconcile if resource has defined annotation Signed-off-by: Wen Zhou <wenzhou@redhat.com>

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

ykaliuta · 2024-02-22T14:54:14Z

/lgtm

israel-hdez · 2024-02-22T17:14:19Z

pkg/deploy/deploy.go

@@ -287,6 +287,11 @@ func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Un
 		return nil
 	}

+	if found.GetAnnotations()["opendatahub.io/managed"] == "false" {


@zdtsw There is another Patch in line 262. Looks like it is for sub-components.
I'm not sure... does it make sense to also put a condition there?

The other patch is responsible for removing the ownerreferrences and cleaning up the resources when a component is set to Removed . I think we still want to cleanup the resource with "opendatahub.io/managed" annotation...correct?

If so, we don't need to update any condition.

Is there any particular reason for "opendatahub.io/managed" to be an annotation instead of a label? With the latter, we could query such resources easier. Can the purpose of this be documented as part of this PR? This could help us decide what's the most feasible approach here.

Also if it's not documented it becomes a super-power known only by chosen ones :)

just found out label opendatahub.io/managed has been used by several components already.
so if we want to make it a label than annotation, we need to find a different key for it.

VaishnaviHire · 2024-02-22T18:22:18Z

Hi @danielezonca @israel-hdez and @Jooho(forRHOAIENG-1262 ), for the annotation is this something that only should be added by the manifests i.e if a user adds this annotation we should revert it? Or do we support this annotation irrespective of it being added by the user or component manifests?

israel-hdez · 2024-02-22T18:54:58Z

for the annotation is this something that only should be added by the manifests i.e if a user adds this annotation we should revert it?

AFAIK, we didn't define this. I think we need to discuss this.

That said, we don't have plans to further update our manifests for the upcoming release. So, for now, we DO need to have the possibility for the user adding the annotation.

zdtsw · 2024-02-22T19:00:52Z

for the annotation is this something that only should be added by the manifests i.e if a user adds this annotation we should revert it?

AFAIK, we didn't define this. I think we need to discuss this.

That said, we don't have plans to further update our manifests for the upcoming release. So, for now, we DO need to have the possibility for the user adding the annotation.

From my understanding, purely from this special case:
annotation will not be added into manifests yaml which becomes default value built into operator's image
it will be up to user to add it which is happening after operator installed and kserve gets installed.
in long term, we will need a broad discussion and inform all component for awareness of this annotation.

israel-hdez · 2024-02-22T19:24:27Z

it will be up to user to add it which is happening after operator installed and kserve gets installed.
in long term, we will need a broad discussion and inform all component for awareness of this annotation.

➕ 1️⃣

VaishnaviHire · 2024-02-22T19:50:17Z

for the annotation is this something that only should be added by the manifests i.e if a user adds this annotation we should revert it?

AFAIK, we didn't define this. I think we need to discuss this.
That said, we don't have plans to further update our manifests for the upcoming release. So, for now, we DO need to have the possibility for the user adding the annotation.

From my understanding, purely from this special case: annotation will not be added into manifests yaml which becomes default value built into operator's image it will be up to user to add it which is happening after operator installed and kserve gets installed. in long term, we will need a broad discussion and inform all component for awareness of this annotation.

Seems like we need more inputs regarding the general behavior, and immediate fix is needed for Kserve, lets update the condition to

if found.GetAnnotations()["opendatahub.io/managed"] == "false" && componentName == "kserve"{
}

WDYT @israel-hdez @zdtsw @danielezonca @etirelli ?

israel-hdez · 2024-02-22T20:15:42Z

Seems like we need more inputs regarding the general behavior, and immediate fix is needed for Kserve, lets update the condition to
if found.GetAnnotations()["opendatahub.io/managed"] == "false" && componentName == "kserve"{
}

It is fine for me, given the limited time we have. And looking forward for the discussion involving the other components.

zdtsw · 2024-02-23T09:50:42Z

Seems like we need more inputs regarding the general behavior, and immediate fix is needed for Kserve, lets update the condition to
if found.GetAnnotations()["opendatahub.io/managed"] == "false" && componentName == "kserve"{
}
It is fine for me, given the limited time we have. And looking forward for the discussion involving the other components.

yes i agree this, we can limit it to only kserve case till we want to make it a solution might be useful across other components

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw · 2024-02-26T07:04:58Z

/test opendatahub-operator-e2e

zdtsw · 2024-02-26T09:00:23Z

/test opendatahub-operator-e2e

VaishnaviHire

/lgtm

openshift-ci · 2024-02-26T15:22:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VaishnaviHire

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~~ [VaishnaviHire]

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

…pendatahub-io#879) * fix(DSC+Kserve): do not reconcile inferenceservice-config for rawdeployment Signed-off-by: Wen Zhou <wenzhou@redhat.com> * fix: wrong logic Signed-off-by: Wen Zhou <wenzhou@redhat.com> * feat(deploy): do not set ownerreference if resource has sepcial annotation to false Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: change logic to --if kind is CRD do not care annotation * update: change logic - we still set ownerreference on resources - we only skip reconcile if resource has defined annotation Signed-off-by: Wen Zhou <wenzhou@redhat.com> * fix(typo) Signed-off-by: Wen Zhou <wenzhou@redhat.com> * Update: limite scope of annoation to only kserve resource for now Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 20f4ad0)

* Update Owners and Owwners-aliases (opendatahub-io#869) (cherry picked from commit 7f477a3) * fix(DSC): do not reconcile resource if it has a special annotation (opendatahub-io#879) * fix(DSC+Kserve): do not reconcile inferenceservice-config for rawdeployment Signed-off-by: Wen Zhou <wenzhou@redhat.com> * fix: wrong logic Signed-off-by: Wen Zhou <wenzhou@redhat.com> * feat(deploy): do not set ownerreference if resource has sepcial annotation to false Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: change logic to --if kind is CRD do not care annotation * update: change logic - we still set ownerreference on resources - we only skip reconcile if resource has defined annotation Signed-off-by: Wen Zhou <wenzhou@redhat.com> * fix(typo) Signed-off-by: Wen Zhou <wenzhou@redhat.com> * Update: limite scope of annoation to only kserve resource for now Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 20f4ad0) * fix(feature): use correct error variable name (opendatahub-io#882) (cherry picked from commit 0d8bd14) * allow setting default deployment mode for Kserve in DSC (opendatahub-io#864) * allow setting default deployment mode for Kserve in DSC Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * move kserve config logic to separate file + enhancements Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * revert dev image set in operator CSV Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * only setup kserve config if component is enabled Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * bug fix Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * address PR feedback Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * cleanup Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * fix lint error Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * set default value for Kserve defaultDeploymentMode to be empty Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * more pr feedback Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * update bundle Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * enhance documentation Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> * add readme for dev preview Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> --------- Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com> (cherry picked from commit cebd287) * Update bundle --------- Co-authored-by: Wen Zhou <wenzhou@redhat.com> Co-authored-by: Ivan Necas <necasik@gmail.com> Co-authored-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>

…pendatahub-io#879) * fix(DSC+Kserve): do not reconcile inferenceservice-config for rawdeployment Signed-off-by: Wen Zhou <wenzhou@redhat.com> * fix: wrong logic Signed-off-by: Wen Zhou <wenzhou@redhat.com> * feat(deploy): do not set ownerreference if resource has sepcial annotation to false Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: change logic to --if kind is CRD do not care annotation * update: change logic - we still set ownerreference on resources - we only skip reconcile if resource has defined annotation Signed-off-by: Wen Zhou <wenzhou@redhat.com> * fix(typo) Signed-off-by: Wen Zhou <wenzhou@redhat.com> * Update: limite scope of annoation to only kserve resource for now Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 20f4ad0)

fix(DSC+Kserve): do not reconcile inferenceservice-config for

00dc28d

rawdeployment Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw requested a review from VaishnaviHire February 21, 2024 15:15

openshift-ci bot requested a review from etirelli February 21, 2024 15:16

zdtsw requested review from danielezonca and VedantMahabaleshwarkar and removed request for etirelli February 21, 2024 15:16

ykaliuta reviewed Feb 21, 2024

View reviewed changes

controllers/datasciencecluster/datasciencecluster_controller.go Outdated Show resolved Hide resolved

fix: wrong logic

72dc2aa

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

openshift-ci bot assigned ykaliuta Feb 21, 2024

openshift-ci bot added the lgtm label Feb 21, 2024

danielezonca reviewed Feb 21, 2024

View reviewed changes

feat(deploy): do not set ownerreference if resource has sepcial

e9c7533

annotation to false Signed-off-by: Wen Zhou <wenzhou@redhat.com>

openshift-ci bot removed the lgtm label Feb 21, 2024

ykaliuta reviewed Feb 21, 2024

View reviewed changes

zdtsw added 2 commits February 22, 2024 07:45

update: change logic to --if kind is CRD do not care annotation

52f59e2

update: change logic

0e59d04

- we still set ownerreference on resources - we only skip reconcile if resource has defined annotation Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw changed the title ~~fix(DSC+Kserve): do not reconcile inferenceservice-config for rawdeployment~~ fix(DSC+Kserve): do not reconcile resource if it has a special annotation Feb 22, 2024

zdtsw changed the title ~~fix(DSC+Kserve): do not reconcile resource if it has a special annotation~~ fix(DSC): do not reconcile resource if it has a special annotation Feb 22, 2024

fix(typo)

43ad6b1

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw added the rhoai-2.8 label Feb 22, 2024

openshift-ci bot added the lgtm label Feb 22, 2024

israel-hdez reviewed Feb 22, 2024

View reviewed changes

Update: limite scope of annoation to only kserve resource for now

855c965

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

openshift-ci bot removed the lgtm label Feb 25, 2024

Merge branch 'incubation' into jira/3111

ad6399b

VaishnaviHire approved these changes Feb 26, 2024

View reviewed changes

openshift-ci bot assigned VaishnaviHire Feb 26, 2024

openshift-ci bot added the lgtm label Feb 26, 2024

openshift-ci bot added the approved label Feb 26, 2024

VaishnaviHire merged commit 20f4ad0 into opendatahub-io:incubation Feb 26, 2024
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(DSC): do not reconcile resource if it has a special annotation #879

fix(DSC): do not reconcile resource if it has a special annotation #879

zdtsw commented Feb 21, 2024 •

edited

ykaliuta commented Feb 21, 2024

danielezonca Feb 21, 2024 •

edited

zdtsw Feb 21, 2024

israel-hdez Feb 21, 2024

israel-hdez Feb 21, 2024

zdtsw Feb 22, 2024

zdtsw Feb 22, 2024

danielezonca Feb 22, 2024

zdtsw Feb 22, 2024

danielezonca Feb 22, 2024

bartoszmajsak Feb 22, 2024

ykaliuta Feb 21, 2024

zdtsw Feb 22, 2024

ykaliuta commented Feb 22, 2024

israel-hdez Feb 22, 2024

VaishnaviHire Feb 22, 2024

bartoszmajsak Feb 22, 2024

zdtsw Feb 23, 2024

VaishnaviHire commented Feb 22, 2024

israel-hdez commented Feb 22, 2024

zdtsw commented Feb 22, 2024

israel-hdez commented Feb 22, 2024 •

edited

VaishnaviHire commented Feb 22, 2024 •

edited

israel-hdez commented Feb 22, 2024

zdtsw commented Feb 23, 2024

zdtsw commented Feb 26, 2024

zdtsw commented Feb 26, 2024

VaishnaviHire left a comment

openshift-ci bot commented Feb 26, 2024

fix(DSC): do not reconcile resource if it has a special annotation #879

fix(DSC): do not reconcile resource if it has a special annotation #879

Conversation

zdtsw commented Feb 21, 2024 • edited

Description

How Has This Been Tested?

Merge criteria:

ykaliuta commented Feb 21, 2024

danielezonca Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykaliuta commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishnaviHire commented Feb 22, 2024

israel-hdez commented Feb 22, 2024

zdtsw commented Feb 22, 2024

israel-hdez commented Feb 22, 2024 • edited

VaishnaviHire commented Feb 22, 2024 • edited

israel-hdez commented Feb 22, 2024

zdtsw commented Feb 23, 2024

zdtsw commented Feb 26, 2024

zdtsw commented Feb 26, 2024

VaishnaviHire left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 26, 2024

zdtsw commented Feb 21, 2024 •

edited

danielezonca Feb 21, 2024 •

edited

israel-hdez commented Feb 22, 2024 •

edited

VaishnaviHire commented Feb 22, 2024 •

edited