feat: add predicates on deployment #1172

zdtsw · 2024-08-12T16:39:42Z

Description

~~rename updateResoruce() to CreateOrUpdateResource()~~
~~use annoataion RHOAIApplicationNamespace and ODHApplicationNamespace than hardcode~~

add predicate.Funcs for deployments to reduce reconciles on all components
~~use switch for better readiness in CreateOrUpdateResource for annotation handling~~

How Has This Been Tested?

local build: quay.io/wenzhou/opendatahub-operator-catalog:v2.13.11024-1

create default dsc in a new opendatahub namespace
manually delete component deployment, it got reconciled
update component replica/resource, it got remained
remove kserve and create again with rawdeployment
check inference CM is still able to set domainname with annotation

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
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

lburgazzoli · 2024-08-22T07:29:18Z

controllers/datasciencecluster/datasciencecluster_controller.go

@@ -376,7 +395,8 @@ var configMapPredicates = predicate.Funcs{
 // a workaround for 2.5 due to odh-model-controller serivceaccount keeps updates with label.
 var saPredicates = predicate.Funcs{
 	UpdateFunc: func(e event.UpdateEvent) bool {
-		if e.ObjectNew.GetName() == "odh-model-controller" && (e.ObjectNew.GetNamespace() == "redhat-ods-applications" || e.ObjectNew.GetNamespace() == "opendatahub") {
+		namespace := e.ObjectNew.GetNamespace()
+		if e.ObjectNew.GetName() == "odh-model-controller" && (namespace == cluster.RHOAIApplicationNamespace || namespace == cluster.ODHApplicationNamespace) {


Wonder if we can just use the operator namespace env var which is set on the deployment, that woudl avoid having to reference a specific list of possible namespace and rely on the namespace where the operator runs instead.

sorry, after a second review, I think my comment was incorrect in the assumption that the namespace were the one where the operator runs. However, I think it would be better to take the namespace from the DSCI instead of having some hardcoded values.

yeah, i have reverted the change and can have another PR to work on the namespace part

lburgazzoli · 2024-08-22T07:38:05Z

pkg/deploy/deploy.go

-	// all other cases, whiltelistedfields will be skipped by ODH operator
-	if managed, exists := found.GetAnnotations()[annotations.ManagedByODHOperator]; !exists || managed != "true" {
+	// handl allowlisted fields with annoation
+	switch managed, exists := found.GetAnnotations()[annotations.ManagedByODHOperator]; {


found.GetAnnotations() may return nil

in this case( when no annotation set at all) , it falls into case !exists as managed == "" and exists is false, right?
which is the same: users can config these fields

yeah sorry, I was thinking in could panic but in fact, you can dereference a map value even on nil ptr.
So wonder if you can just skip the check on exists, since in fact what matter is only managed != "true"

the logic from "before" is correct: ( !exists || managed != "true" ) => call skipUpdateOnAllowlistedFields
if this is easier to understand i can put it back than changing to "switch" ?

but we cannot only check on managed != "true" case

But refactoring (changes without amending functionality) and changing the predicate are different things. Does it make sense to move the condition to own function?

but we cannot only check on managed != "true" case

but if the annotation does not exist, it will return an empty string which actually satisfies the managed != "true" condition, isn't it ?

i see what you mean now. yes, then it is either managed == true or the rest (covers no annotation, this annotation not exist, this annotation is set to non-true)

lburgazzoli · 2024-08-22T07:57:23Z

pkg/deploy/deploy.go

+		// Create resource if it doesn't exist OR reconcile component if exists
+		return CreateOrUpdateResource(ctx, cli, res, found, owner)
+	}
+	// Component id removed: delete resource if it exists or do nothing if not found


this is not easy to understand as there is a found object but also a check on the IsNotFound error, so the do nothing if not found is a little bit misleading. IMHO, the getResource function should honor the ErrNotFound and there should be no need to check if found != nil

ykaliuta · 2024-08-22T08:44:19Z

controllers/datasciencecluster/datasciencecluster_controller.go

@@ -366,7 +367,25 @@ var configMapPredicates = predicate.Funcs{
 			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") {
+		namespace := e.ObjectNew.GetNamespace()
+		if e.ObjectNew.GetName() == "inferenceservice-config" && (namespace == cluster.RHOAIApplicationNamespace || namespace == cluster.ODHApplicationNamespace) {


Should it use namespace from DSCI?

yeah these are not directly for the predicates part.
i ran into some lint warning saying the name have been used multiple times, thats why i create const for these,
but i think you are right, it should be break out into a different PR and get them from dsci somehow

ykaliuta · 2024-08-22T08:45:59Z

pkg/deploy/deploy.go

What the point of this file changes? For me they make it less readable. And at the first glance I do not see they are related to the rest so we can discuss them in a separate PR.

BTW, I still do not support that part of 0261670 ("chore: refactor on some logic for component manifests resources (#1121)")

i did some rework on the deploy part

ykaliuta · 2024-08-22T10:39:27Z

pkg/deploy/deploy.go

@@ -202,31 +202,31 @@ func manageResource(ctx context.Context, cli client.Client, res *resource.Resour
 	}

 	found, err := getResource(ctx, cli, res)
+	if !k8serr.IsNotFound(err) {


Will it be triggered by nil?

thanks! missed this

- rename updateResoruce() to CreateOrUpdateResource() - use annoataion RHOAIApplicationNamespace and ODHApplicationNamespace than hardcode - add predicate.Funcs for deployments to reduce reconciles on all components - use switch for better readiness in CreateOrUpdateResource for annoataion handling Signed-off-by: Wen Zhou <[email protected]>

Signed-off-by: Wen Zhou <[email protected]>

- need to deal with this with another PR to get appliation namespace Signed-off-by: Wen Zhou <[email protected]>

Signed-off-by: Wen Zhou <[email protected]>

openshift-ci · 2024-08-27T14:20:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ykaliuta

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

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

* update: logic for manage resource with skip unnecessary reconcile - rename updateResoruce() to CreateOrUpdateResource() - use annoataion RHOAIApplicationNamespace and ODHApplicationNamespace than hardcode - add predicate.Funcs for deployments to reduce reconciles on all components - use switch for better readiness in CreateOrUpdateResource for annoataion handling Signed-off-by: Wen Zhou <[email protected]> * revert: back out annotation for application namespace Signed-off-by: Wen Zhou <[email protected]> * lint: skip lint on const with multiple hardcode value - need to deal with this with another PR to get appliation namespace Signed-off-by: Wen Zhou <[email protected]> * refactor: deploy with manage resource part Signed-off-by: Wen Zhou <[email protected]> --------- Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit 6f82d01)

* update: logic for manage resource with skip unnecessary reconcile - add predicate.Funcs for deployments to reduce reconciles on all components Signed-off-by: Wen Zhou <[email protected]> --------- Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit 6f82d01) Signed-off-by: Wen Zhou <[email protected]>

* update: logic for manage resource with skip unnecessary reconcile - add predicate.Funcs for deployments to reduce reconciles on all components --------- (cherry picked from commit 6f82d01) Signed-off-by: Wen Zhou <[email protected]>

openshift-ci bot requested review from mattmahoneyrh and ykaliuta August 12, 2024 16:39

zdtsw force-pushed the chore_93 branch from 3e87aad to 2df832d Compare August 19, 2024 15:56

zdtsw requested review from VaishnaviHire and removed request for mattmahoneyrh August 19, 2024 15:56

zdtsw changed the title ~~update: logic for manage resource~~ update: logic for manage resource with predicates on deployment Aug 19, 2024

zdtsw force-pushed the chore_93 branch from 2df832d to 3bf06f8 Compare August 22, 2024 07:22

zdtsw changed the title ~~update: logic for manage resource with predicates on deployment~~ refactor: logic for manage resource with predicates on deployment Aug 22, 2024

lburgazzoli reviewed Aug 22, 2024

View reviewed changes

ykaliuta reviewed Aug 22, 2024

View reviewed changes

zdtsw force-pushed the chore_93 branch from 8a647de to da3d5b4 Compare August 22, 2024 10:01

zdtsw changed the title ~~refactor: logic for manage resource with predicates on deployment~~ feat: add predicates on deployment Aug 22, 2024

zdtsw force-pushed the chore_93 branch from 2c6c1cd to 181f67e Compare August 22, 2024 10:34

ykaliuta reviewed Aug 22, 2024

View reviewed changes

zdtsw force-pushed the chore_93 branch from 181f67e to 3ee613e Compare August 22, 2024 15:59

zdtsw added 4 commits August 27, 2024 15:53

revert: back out annotation for application namespace

6ef1e2d

Signed-off-by: Wen Zhou <[email protected]>

lint: skip lint on const with multiple hardcode value

df127bd

- need to deal with this with another PR to get appliation namespace Signed-off-by: Wen Zhou <[email protected]>

refactor: deploy with manage resource part

6efe013

Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the chore_93 branch from 3ee613e to 6efe013 Compare August 27, 2024 13:54

zdtsw requested review from lburgazzoli and ykaliuta August 27, 2024 13:54

ykaliuta approved these changes Aug 27, 2024

View reviewed changes

openshift-ci bot assigned ykaliuta Aug 27, 2024

openshift-ci bot added the lgtm label Aug 27, 2024

openshift-ci bot added the approved label Aug 27, 2024

openshift-merge-bot bot merged commit 6f82d01 into opendatahub-io:incubation Aug 27, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add predicates on deployment #1172

feat: add predicates on deployment #1172

zdtsw commented Aug 12, 2024 •

edited

Loading

lburgazzoli Aug 22, 2024

lburgazzoli Aug 22, 2024

zdtsw Aug 22, 2024

lburgazzoli Aug 22, 2024

zdtsw Aug 22, 2024

lburgazzoli Aug 22, 2024

zdtsw Aug 22, 2024

ykaliuta Aug 22, 2024

lburgazzoli Aug 22, 2024 •

edited

Loading

zdtsw Aug 22, 2024

lburgazzoli Aug 22, 2024

ykaliuta Aug 22, 2024

zdtsw Aug 22, 2024

ykaliuta Aug 22, 2024

ykaliuta Aug 22, 2024

zdtsw Aug 22, 2024

ykaliuta Aug 22, 2024

zdtsw Aug 22, 2024

openshift-ci bot commented Aug 27, 2024

feat: add predicates on deployment #1172

feat: add predicates on deployment #1172

Conversation

zdtsw commented Aug 12, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

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

lburgazzoli Aug 22, 2024 • edited Loading

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

openshift-ci bot commented Aug 27, 2024

zdtsw commented Aug 12, 2024 •

edited

Loading

lburgazzoli Aug 22, 2024 •

edited

Loading