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]> (cherry picked from commit 6f82d01)
red-hat-data-services · Aug 29, 2024 · 011fc69 · 011fc69
1 parent c4446d6
commit 011fc69
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 31 deletions.
diff --git a/controllers/datasciencecluster/datasciencecluster_controller.go b/controllers/datasciencecluster/datasciencecluster_controller.go
@@ -56,6 +56,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"
 )
@@ -382,7 +383,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
@@ -392,7 +411,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" {
+		namespace := e.ObjectNew.GetNamespace()
+		if e.ObjectNew.GetName() == "odh-model-controller" && (namespace == "redhat-ods-applications" || namespace == "opendatahub") {
 			return false
 		}
 		return true
@@ -453,13 +473,29 @@ func (r *DataScienceClusterReconciler) SetupWithManager(ctx context.Context, mgr
 		For(&dscv1.DataScienceCluster{}).
 		Owns(&corev1.Namespace{}).
 		Owns(&corev1.Secret{}).
-		Owns(&corev1.ConfigMap{}, builder.WithPredicates(configMapPredicates)).
-		Owns(&networkingv1.NetworkPolicy{}, builder.WithPredicates(networkpolicyPredicates)).
-		Owns(&rbacv1.Role{}, builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRolePredicates))).
-		Owns(&rbacv1.RoleBinding{}, builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRBPredicates))).
-		Owns(&rbacv1.ClusterRole{}, builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRolePredicates))).
-		Owns(&rbacv1.ClusterRoleBinding{}, builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRBPredicates))).
-		Owns(&appsv1.Deployment{}).
+		Owns(
+			&corev1.ConfigMap{},
+			builder.WithPredicates(configMapPredicates),
+		).
+		Owns(
+			&networkingv1.NetworkPolicy{},
+			builder.WithPredicates(networkpolicyPredicates),
+		).
+		Owns(
+			&rbacv1.Role{},
+			builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRolePredicates))).
+		Owns(
+			&rbacv1.RoleBinding{},
+			builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRBPredicates))).
+		Owns(
+			&rbacv1.ClusterRole{},
+			builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRolePredicates))).
+		Owns(
+			&rbacv1.ClusterRoleBinding{},
+			builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRBPredicates))).
+		Owns(
+			&appsv1.Deployment{},
+			builder.WithPredicates(componentDeploymentPredicates)).
 		Owns(&corev1.PersistentVolumeClaim{}).
 		Owns(&corev1.Service{}, builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshGeneralPredicates))).
 		Owns(&appsv1.StatefulSet{}).

diff --git a/pkg/deploy/deploy.go b/pkg/deploy/deploy.go
@@ -200,30 +200,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) {
@@ -283,15 +284,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
 		}
@@ -332,11 +333,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()))
 }