feat: add predicates on deployment (opendatahub-io#1172)

* 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]>
zdtsw-forking · Aug 27, 2024 · 6f82d01 · 6f82d01
1 parent 8f3d013
commit 6f82d01
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 26 deletions.
diff --git a/controllers/datasciencecluster/datasciencecluster_controller.go b/controllers/datasciencecluster/datasciencecluster_controller.go
@@ -55,6 +55,7 @@ import (
 	"github.com/opendatahub-io/opendatahub-operator/v2/components/datasciencepipelines"
 	"github.com/opendatahub-io/opendatahub-operator/v2/controllers/status"
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster"
+	annotations "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/upgrade"
 )
@@ -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 == "redhat-ods-applications" || namespace == "opendatahub") { //nolint:goconst
+			return false
+		}
+		return true
+	},
+}
+
+// reduce unnecessary reconcile triggered by odh component's deployment change due to ManagedByODHOperator annotation.
+var componentDeploymentPredicates = predicate.Funcs{
+	UpdateFunc: func(e event.UpdateEvent) bool {
+		namespace := e.ObjectNew.GetNamespace()
+		if namespace == "opendatahub" || namespace == "redhat-ods-applications" {
+			oldManaged, oldExists := e.ObjectOld.GetAnnotations()[annotations.ManagedByODHOperator]
+			newManaged := e.ObjectNew.GetAnnotations()[annotations.ManagedByODHOperator]
+			// only reoncile if annotation from "not exist" to "set to true", or from "non-true" value to "true"
+			if newManaged == "true" && (!oldExists || oldManaged != "true") {
+				return true
+			}
 			return false
 		}
 		return true
@@ -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 == "redhat-ods-applications" || namespace == "opendatahub") {
 			return false
 		}
 		return true
@@ -457,7 +477,9 @@ func (r *DataScienceClusterReconciler) SetupWithManager(ctx context.Context, mgr
 		Owns(
 			&rbacv1.ClusterRoleBinding{},
 			builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRBPredicates))).
-		Owns(&appsv1.Deployment{}).
+		Owns(
+			&appsv1.Deployment{},
+			builder.WithPredicates(componentDeploymentPredicates)).
 		Owns(&corev1.PersistentVolumeClaim{}).
 		Owns(
 			&corev1.Service{},

diff --git a/controllers/dscinitialization/dscinitialization_controller.go b/controllers/dscinitialization/dscinitialization_controller.go
@@ -380,7 +380,6 @@ var CMContentChangedPredicate = predicate.Funcs{
 
 var DSCDeletionPredicate = predicate.Funcs{
 	DeleteFunc: func(e event.DeleteEvent) bool {
-
 		return true
 	},
 }

diff --git a/pkg/deploy/deploy.go b/pkg/deploy/deploy.go
@@ -204,30 +204,31 @@ func manageResource(ctx context.Context, cli client.Client, res *resource.Resour
 
 	found, err := getResource(ctx, cli, res)
 
-	if err != nil {
-		if !k8serr.IsNotFound(err) {
-			return err
-		}
-		// Create resource if it doesn't exist and component enabled
+	if err == nil {
+		// when resource is found
 		if enabled {
-			return createResource(ctx, cli, res, owner)
+			// Exception to not update kserve with managed annotation
+			// do not reconcile kserve resource with annotation "opendatahub.io/managed: false"
+			// TODO: remove this exception when we define managed annotation across odh
+			if found.GetAnnotations()[annotations.ManagedByODHOperator] == "false" && componentName == "kserve" {
+				return nil
+			}
+			return updateResource(ctx, cli, res, found, owner)
 		}
-		// Skip if resource doesn't exist and component is disabled
-		return nil
+		// Delete resource if it exists or do nothing if not found
+		return handleDisabledComponent(ctx, cli, found, componentName)
 	}
 
-	// when resource is found
+	if !k8serr.IsNotFound(err) {
+		return err
+	}
+
+	// Create resource when component enabled
 	if enabled {
-		// Exception to not update kserve with managed annotation
-		// do not reconcile kserve resource with annotation "opendatahub.io/managed: false"
-		// TODO: remove this exception when we define managed annotation across odh
-		if found.GetAnnotations()[annotations.ManagedByODHOperator] == "false" && componentName == "kserve" {
-			return nil
-		}
-		return updateResource(ctx, cli, res, found, owner)
+		return createResource(ctx, cli, res, owner)
 	}
-	// Delete resource if it exists and component is disabled
-	return handleDisabledComponent(ctx, cli, found, componentName)
+	// Skip if resource doesn't exist and component is disabled
+	return nil
 }
 
 func getResource(ctx context.Context, cli client.Client, obj *resource.Resource) (*unstructured.Unstructured, error) {
@@ -287,15 +288,15 @@ func createResource(ctx context.Context, cli client.Client, res *resource.Resour
 	return cli.Create(ctx, obj)
 }
 
+// Exception to skip ODHDashboardConfig CR reconcile.
 func updateResource(ctx context.Context, cli client.Client, res *resource.Resource, found *unstructured.Unstructured, owner metav1.Object) error {
-	// Skip ODHDashboardConfig Update
 	if found.GetKind() == "OdhDashboardConfig" {
 		return nil
 	}
 
-	// only reconcile whiltelistedFields if the existing resource has annoation set to "true"
-	// all other cases, whiltelistedfields will be skipped by ODH operator
-	if managed, exists := found.GetAnnotations()[annotations.ManagedByODHOperator]; !exists || managed != "true" {
+	// Operator reconcile allowedListfield only when resource is managed by operator(annotation is true)
+	// all other cases: no annotation at all, required annotation not present, of annotation is non-true value, skip reconcile
+	if managed := found.GetAnnotations()[annotations.ManagedByODHOperator]; managed != "true" {
 		if err := skipUpdateOnAllowlistedFields(res); err != nil {
 			return err
 		}
@@ -336,11 +337,13 @@ func updateLabels(found, obj *unstructured.Unstructured) {
 	obj.SetLabels(foundLabels)
 }
 
+// preformPatch works for update cases.
 func performPatch(ctx context.Context, cli client.Client, obj, found *unstructured.Unstructured, owner metav1.Object) error {
 	data, err := json.Marshal(obj)
 	if err != nil {
 		return err
 	}
+	// force owner to be default-dsc/default-dsci
 	return cli.Patch(ctx, found, client.RawPatch(types.ApplyPatchType, data), client.ForceOwnership, client.FieldOwner(owner.GetName()))
 }