Reduce component reconciler's action verbosity (#1388)

* Reduce component reconciler's action verbosity * Use kind instead of resource name for selectors * Use handlers.AnnotationToName to map from an event to a resource * Fix TrustyAI e2e
opendatahub-io · Nov 27, 2024 · 7da9176 · 7da9176
1 parent 89bf20a
commit 7da9176
Show file tree

Hide file tree

Showing 30 changed files with 423 additions and 207 deletions.
diff --git a/controllers/components/dashboard/dashboard_controller.go b/controllers/components/dashboard/dashboard_controller.go
@@ -44,7 +44,7 @@ import (
 func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error {
 	componentName := computeComponentName()
 
-	_, err := reconciler.ComponentReconcilerFor(mgr, componentsv1.DashboardInstanceName, &componentsv1.Dashboard{}).
+	_, err := reconciler.ComponentReconcilerFor(mgr, &componentsv1.Dashboard{}).
 		// operands - owned
 		Owns(&corev1.ConfigMap{}).
 		Owns(&corev1.Secret{}).
@@ -105,16 +105,11 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
 		WithAction(customizeResources).
 		WithAction(deploy.NewAction(
 			deploy.WithCache(),
-			deploy.WithFieldOwner(componentsv1.DashboardInstanceName),
-			deploy.WithLabel(labels.ComponentPartOf, componentsv1.DashboardInstanceName),
-		)).
-		WithAction(updatestatus.NewAction(
-			updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.DashboardInstanceName),
 		)).
+		WithAction(updatestatus.NewAction()).
 		WithAction(updateStatus).
 		// must be the final action
 		WithAction(gc.NewAction(
-			gc.WithLabel(labels.ComponentPartOf, componentsv1.DashboardInstanceName),
 			gc.WithUnremovables(gvk.OdhDashboardConfig),
 		)).
 		Build(ctx)

diff --git a/controllers/components/dashboard/dashboard_controller_actions.go b/controllers/components/dashboard/dashboard_controller_actions.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	routev1 "github.com/openshift/api/route/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -110,7 +111,7 @@ func updateStatus(ctx context.Context, rr *odhtypes.ReconciliationRequest) error
 		&rl,
 		client.InNamespace(rr.DSCI.Spec.ApplicationsNamespace),
 		client.MatchingLabels(map[string]string{
-			labels.ComponentPartOf: componentsv1.DashboardInstanceName,
+			labels.ComponentPartOf: strings.ToLower(componentsv1.DashboardKind),
 		}),
 	)
 

diff --git a/controllers/components/datasciencepipelines/datasciencepipelines_controller.go b/controllers/components/datasciencepipelines/datasciencepipelines_controller.go
@@ -48,11 +48,7 @@ var (
 )
 
 func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error {
-	_, err := reconciler.ComponentReconcilerFor(
-		mgr,
-		componentsv1.DataSciencePipelinesInstanceName,
-		&componentsv1.DataSciencePipelines{},
-	).
+	_, err := reconciler.ComponentReconcilerFor(mgr, &componentsv1.DataSciencePipelines{}).
 		// customized Owns() for Component with new predicates
 		Owns(&corev1.ConfigMap{}).
 		Owns(&corev1.Secret{}).
@@ -77,16 +73,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
 		)).
 		WithAction(deploy.NewAction(
 			deploy.WithCache(),
-			deploy.WithFieldOwner(componentsv1.DataSciencePipelinesInstanceName),
-			deploy.WithLabel(labels.ComponentPartOf, componentsv1.DataSciencePipelinesInstanceName),
-		)).
-		WithAction(updatestatus.NewAction(
-			updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.DataSciencePipelinesInstanceName),
 		)).
+		WithAction(updatestatus.NewAction()).
 		// must be the final action
-		WithAction(gc.NewAction(
-			gc.WithLabel(labels.ComponentPartOf, componentsv1.DataSciencePipelinesInstanceName),
-		)).
+		WithAction(gc.NewAction()).
 		Build(ctx)
 
 	if err != nil {

diff --git a/controllers/components/kueue/kueue_controller.go b/controllers/components/kueue/kueue_controller.go
@@ -39,11 +39,7 @@ import (
 )
 
 func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error {
-	_, err := reconciler.ComponentReconcilerFor(
-		mgr,
-		componentsv1.KueueInstanceName,
-		&componentsv1.Kueue{},
-	).
+	_, err := reconciler.ComponentReconcilerFor(mgr, &componentsv1.Kueue{}).
 		// customized Owns() for Component with new predicates
 		Owns(&corev1.ConfigMap{}).
 		Owns(&corev1.Secret{}).
@@ -70,16 +66,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
 		)).
 		WithAction(deploy.NewAction(
 			deploy.WithCache(),
-			deploy.WithFieldOwner(componentsv1.KueueInstanceName),
-			deploy.WithLabel(labels.ComponentPartOf, componentsv1.KueueInstanceName),
-		)).
-		WithAction(updatestatus.NewAction(
-			updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.KueueInstanceName),
 		)).
+		WithAction(updatestatus.NewAction()).
 		// must be the final action
-		WithAction(gc.NewAction(
-			gc.WithLabel(labels.ComponentPartOf, componentsv1.KueueInstanceName),
-		)).
+		WithAction(gc.NewAction()).
 		Build(ctx)
 
 	if err != nil {

diff --git a/controllers/components/modelregistry/modelregistry_controller.go b/controllers/components/modelregistry/modelregistry_controller.go
@@ -43,11 +43,7 @@ import (
 )
 
 func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error {
-	_, err := reconciler.ComponentReconcilerFor(
-		mgr,
-		componentsv1.ModelRegistryInstanceName,
-		&componentsv1.ModelRegistry{},
-	).
+	_, err := reconciler.ComponentReconcilerFor(mgr, &componentsv1.ModelRegistry{}).
 		Owns(&corev1.ConfigMap{}).
 		Owns(&corev1.Secret{}).
 		Owns(&rbacv1.Role{}).
@@ -91,16 +87,11 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
 		WithAction(customizeResources).
 		WithAction(deploy.NewAction(
 			deploy.WithCache(),
-			deploy.WithFieldOwner(componentsv1.ModelRegistryInstanceName),
-			deploy.WithLabel(labels.ComponentPartOf, componentsv1.ModelRegistryInstanceName),
-		)).
-		WithAction(updatestatus.NewAction(
-			updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.ModelRegistryInstanceName),
 		)).
+		WithAction(updatestatus.NewAction()).
 		WithAction(updateStatus).
 		// must be the final action
 		WithAction(gc.NewAction(
-			gc.WithLabel(labels.ComponentPartOf, componentsv1.ModelRegistryInstanceName),
 			gc.WithUnremovables(gvk.ServiceMeshMember),
 		)).
 		Build(ctx)

diff --git a/controllers/components/ray/ray_controller.go b/controllers/components/ray/ray_controller.go
@@ -37,11 +37,7 @@ import (
 )
 
 func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error {
-	_, err := reconciler.ComponentReconcilerFor(
-		mgr,
-		componentsv1.RayInstanceName,
-		&componentsv1.Ray{},
-	).
+	_, err := reconciler.ComponentReconcilerFor(mgr, &componentsv1.Ray{}).
 		// customized Owns() for Component with new predicates
 		Owns(&corev1.ConfigMap{}).
 		Owns(&corev1.Secret{}).
@@ -63,16 +59,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
 		)).
 		WithAction(deploy.NewAction(
 			deploy.WithCache(),
-			deploy.WithFieldOwner(componentsv1.RayInstanceName),
-			deploy.WithLabel(labels.ComponentPartOf, componentsv1.RayInstanceName),
-		)).
-		WithAction(updatestatus.NewAction(
-			updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.RayInstanceName),
 		)).
+		WithAction(updatestatus.NewAction()).
 		// must be the final action
-		WithAction(gc.NewAction(
-			gc.WithLabel(labels.ComponentPartOf, componentsv1.RayInstanceName),
-		)).
+		WithAction(gc.NewAction()).
 		Build(ctx)
 
 	if err != nil {

diff --git a/controllers/components/trainingoperator/trainingoperator_controller.go b/controllers/components/trainingoperator/trainingoperator_controller.go
@@ -37,11 +37,7 @@ import (
 )
 
 func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error {
-	_, err := reconciler.ComponentReconcilerFor(
-		mgr,
-		componentsv1.TrainingOperatorInstanceName,
-		&componentsv1.TrainingOperator{},
-	).
+	_, err := reconciler.ComponentReconcilerFor(mgr, &componentsv1.TrainingOperator{}).
 		// customized Owns() for Component with new predicates
 		Owns(&corev1.ConfigMap{}).
 		Owns(&promv1.PodMonitor{}).
@@ -60,16 +56,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
 		)).
 		WithAction(deploy.NewAction(
 			deploy.WithCache(),
-			deploy.WithFieldOwner(componentsv1.TrainingOperatorInstanceName),
-			deploy.WithLabel(labels.ComponentPartOf, componentsv1.TrainingOperatorInstanceName),
-		)).
-		WithAction(updatestatus.NewAction(
-			updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.TrainingOperatorInstanceName),
 		)).
+		WithAction(updatestatus.NewAction()).
 		// must be the final action
-		WithAction(gc.NewAction(
-			gc.WithLabel(labels.ComponentPartOf, componentsv1.TrainingOperatorInstanceName),
-		)).
+		WithAction(gc.NewAction()).
 		Build(ctx)
 
 	if err != nil {

diff --git a/controllers/components/trustyai/trustyai_controller.go b/controllers/components/trustyai/trustyai_controller.go
@@ -36,11 +36,7 @@ import (
 )
 
 func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error {
-	_, err := reconciler.ComponentReconcilerFor(
-		mgr,
-		componentsv1.TrustyAIInstanceName,
-		&componentsv1.TrustyAI{},
-	).
+	_, err := reconciler.ComponentReconcilerFor(mgr, &componentsv1.TrustyAI{}).
 		// customized Owns() for Component with new predicates
 		Owns(&corev1.ConfigMap{}).
 		Owns(&corev1.ServiceAccount{}).
@@ -61,16 +57,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
 		)).
 		WithAction(deploy.NewAction(
 			deploy.WithCache(),
-			deploy.WithFieldOwner(componentsv1.TrustyAIInstanceName),
-			deploy.WithLabel(labels.ComponentPartOf, componentsv1.TrustyAIInstanceName),
-		)).
-		WithAction(updatestatus.NewAction(
-			updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.TrustyAIInstanceName),
 		)).
+		WithAction(updatestatus.NewAction()).
 		// must be the final action
-		WithAction(gc.NewAction(
-			gc.WithLabel(labels.ComponentPartOf, componentsv1.TrustyAIInstanceName),
-		)).
+		WithAction(gc.NewAction()).
 		Build(ctx)
 
 	if err != nil {

diff --git a/pkg/controller/actions/deploy/action_deploy.go b/pkg/controller/actions/deploy/action_deploy.go
@@ -21,6 +21,7 @@ import (
 	odhClient "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client"
 	odhTypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types"
 	"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/resources"
 )
 
@@ -111,22 +112,15 @@ func (a *Action) run(ctx context.Context, rr *odhTypes.ReconciliationRequest) er
 		a.cache.Sync()
 	}
 
-	controllerName := strings.ToLower(rr.Instance.GetObjectKind().GroupVersionKind().Kind)
-
-	deployHash, err := odhTypes.HashStr(rr)
+	kind, err := resources.KindForObject(rr.Client.Scheme(), rr.Instance)
 	if err != nil {
-		return fmt.Errorf("unable to compute reauest hash: %w", err)
+		return err
 	}
 
-	deployAnnotations := map[string]string{
-		annotations.ComponentGeneration: strconv.FormatInt(rr.Instance.GetGeneration(), 10),
-		annotations.ComponentHash:       deployHash,
-		annotations.PlatformType:        string(rr.Release.Name),
-		annotations.PlatformVersion:     rr.Release.Version.String(),
-	}
+	controllerName := strings.ToLower(kind)
 
 	for i := range rr.Resources {
-		ok, err := a.deploy(ctx, rr, rr.Resources[i], deployAnnotations)
+		ok, err := a.deploy(ctx, rr, rr.Resources[i])
 		if err != nil {
 			return fmt.Errorf("failure deploying %s: %w", rr.Resources[i], err)
 		}
@@ -143,7 +137,6 @@ func (a *Action) deploy(
 	ctx context.Context,
 	rr *odhTypes.ReconciliationRequest,
 	obj unstructured.Unstructured,
-	deployAnnotations map[string]string,
 ) (bool, error) {
 	current, lookupErr := a.lookup(ctx, rr.Client, obj)
 	if lookupErr != nil {
@@ -156,9 +149,27 @@ func (a *Action) deploy(
 		return false, nil
 	}
 
+	fo := a.fieldOwner
+	if fo == "" {
+		kind, err := resources.KindForObject(rr.Client.Scheme(), rr.Instance)
+		if err != nil {
+			return false, err
+		}
+
+		fo = strings.ToLower(kind)
+	}
+
 	resources.SetLabels(&obj, a.labels)
 	resources.SetAnnotations(&obj, a.annotations)
-	resources.SetAnnotations(&obj, deployAnnotations)
+	resources.SetAnnotation(&obj, annotations.InstanceGeneration, strconv.FormatInt(rr.Instance.GetGeneration(), 10))
+	resources.SetAnnotation(&obj, annotations.InstanceName, rr.Instance.GetName())
+	resources.SetAnnotation(&obj, annotations.InstanceUID, string(rr.Instance.GetUID()))
+	resources.SetAnnotation(&obj, annotations.PlatformType, string(rr.Release.Name))
+	resources.SetAnnotation(&obj, annotations.PlatformVersion, rr.Release.Version.String())
+
+	if resources.GetLabel(&obj, labels.ComponentPartOf) == "" && fo != "" {
+		resources.SetLabel(&obj, labels.ComponentPartOf, fo)
+	}
 
 	// backup copy for caching
 	origObj := obj.DeepCopy()
@@ -205,9 +216,21 @@ func (a *Action) deploy(
 
 		switch a.deployMode {
 		case ModePatch:
-			deployedObj, err = a.patch(ctx, rr.Client, &obj, current)
+			deployedObj, err = a.patch(
+				ctx,
+				rr.Client,
+				&obj,
+				current,
+				client.ForceOwnership,
+				client.FieldOwner(fo))
 		case ModeSSA:
-			deployedObj, err = a.apply(ctx, rr.Client, &obj, current)
+			deployedObj, err = a.apply(
+				ctx,
+				rr.Client,
+				&obj,
+				current,
+				client.ForceOwnership,
+				client.FieldOwner(fo))
 		default:
 			err = fmt.Errorf("unsupported deploy mode %s", a.deployMode)
 		}
@@ -278,6 +301,7 @@ func (a *Action) patch(
 	c *odhClient.Client,
 	obj *unstructured.Unstructured,
 	old *unstructured.Unstructured,
+	opts ...client.PatchOption,
 ) (*unstructured.Unstructured, error) {
 	logf.FromContext(ctx).V(3).Info("patch",
 		"gvk", obj.GroupVersionKind(),
@@ -305,7 +329,7 @@ func (a *Action) patch(
 		}
 
 	default:
-		// do noting
+		// do nothing
 		break
 	}
 
@@ -324,8 +348,7 @@ func (a *Action) patch(
 			ctx,
 			old,
 			client.RawPatch(types.ApplyPatchType, data),
-			client.ForceOwnership,
-			client.FieldOwner(a.fieldOwner),
+			opts...,
 		)
 
 		if err != nil {
@@ -341,6 +364,7 @@ func (a *Action) apply(
 	c *odhClient.Client,
 	obj *unstructured.Unstructured,
 	old *unstructured.Unstructured,
+	opts ...client.PatchOption,
 ) (*unstructured.Unstructured, error) {
 	logf.FromContext(ctx).V(3).Info("apply",
 		"gvk", obj.GroupVersionKind(),
@@ -369,15 +393,14 @@ func (a *Action) apply(
 			return nil, fmt.Errorf("failed to merge Deployment %s/%s: %w", obj.GetNamespace(), obj.GetName(), err)
 		}
 	default:
-		// do noting
+		// do nothing
 		break
 	}
 
 	err := c.Apply(
 		ctx,
 		obj,
-		client.ForceOwnership,
-		client.FieldOwner(a.fieldOwner),
+		opts...,
 	)
 
 	if err != nil {

diff --git a/pkg/controller/actions/deploy/action_deploy_cache_test.go b/pkg/controller/actions/deploy/action_deploy_cache_test.go
@@ -40,6 +40,7 @@ func TestDeployWithCacheAction(t *testing.T) {
 	utilruntime.Must(corev1.AddToScheme(s))
 	utilruntime.Must(appsv1.AddToScheme(s))
 	utilruntime.Must(apiextensionsv1.AddToScheme(s))
+	utilruntime.Must(componentsv1.AddToScheme(s))
 
 	projectDir, err := envtestutil.FindProjectRoot()
 	g.Expect(err).NotTo(HaveOccurred())