From 7da9176fb275eb3bc0a04ae73373d6e00d496a86 Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Wed, 27 Nov 2024 02:27:48 +0100 Subject: [PATCH] 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 --- .../dashboard/dashboard_controller.go | 9 +- .../dashboard/dashboard_controller_actions.go | 3 +- .../datasciencepipelines_controller.go | 16 +- .../components/kueue/kueue_controller.go | 16 +- .../modelregistry/modelregistry_controller.go | 13 +- controllers/components/ray/ray_controller.go | 16 +- .../trainingoperator_controller.go | 16 +- .../trustyai/trustyai_controller.go | 16 +- .../actions/deploy/action_deploy.go | 65 +++++--- .../deploy/action_deploy_cache_test.go | 1 + .../actions/deploy/action_deploy_test.go | 9 +- pkg/controller/actions/gc/action_gc.go | 29 ++-- .../actions/gc/action_gc_support.go | 11 +- pkg/controller/actions/gc/action_gc_test.go | 43 +++-- .../updatestatus/action_update_status.go | 19 ++- .../updatestatus/action_update_status_test.go | 153 +++++++++++++++++- pkg/controller/handlers/handlers.go | 27 +++- .../predicates/component/component.go | 42 ++++- .../reconciler/component_reconciler.go | 6 +- .../component_reconciler_support.go | 48 +++--- pkg/metadata/annotations/annotations.go | 9 +- pkg/resources/resources.go | 14 ++ pkg/utils/test/fakeclient/fakeclient.go | 2 + tests/e2e/dashboard_test.go | 15 +- tests/e2e/datasciencepipelines_test.go | 3 +- tests/e2e/kueue_test.go | 3 +- tests/e2e/modelregistry_test.go | 17 +- tests/e2e/ray_test.go | 3 +- tests/e2e/trainingoperator_test.go | 3 +- tests/e2e/trustyai_test.go | 3 +- 30 files changed, 423 insertions(+), 207 deletions(-) diff --git a/controllers/components/dashboard/dashboard_controller.go b/controllers/components/dashboard/dashboard_controller.go index 8f640611bec..c7f6d42ee50 100644 --- 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 index ef05bd1302c..0458803ee0a 100644 --- 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 index 492ac54a75a..eb4adfdd207 100644 --- 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 index 53d2254c816..35e1d90fd99 100644 --- 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 index c59a01df0a7..eaf8ffab906 100644 --- 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 index fac23edcd3d..1b9239992f8 100644 --- 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 index 3da6cb466de..63c3059aede 100644 --- 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 index 947c1c933b8..23913a8eab5 100644 --- 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 index dcd3a7eb2ba..2728098e8ff 100644 --- 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 index 75a04eabddd..6c53fdb5f9d 100644 --- 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()) diff --git a/pkg/controller/actions/deploy/action_deploy_test.go b/pkg/controller/actions/deploy/action_deploy_test.go index 11c18f4f54c..e441da7559c 100644 --- a/pkg/controller/actions/deploy/action_deploy_test.go +++ b/pkg/controller/actions/deploy/action_deploy_test.go @@ -3,6 +3,7 @@ package deploy_test import ( "context" "strconv" + "strings" "testing" "github.com/blang/semver/v4" @@ -20,6 +21,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy" "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" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/fakeclient" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/matchers/jq" @@ -78,12 +80,9 @@ func TestDeployAction(t *testing.T) { err = cl.Get(ctx, client.ObjectKeyFromObject(obj1), obj1) g.Expect(err).ShouldNot(HaveOccurred()) - dh, err := types.HashStr(&rr) - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(obj1).Should(And( - jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.ComponentGeneration, strconv.FormatInt(rr.Instance.GetGeneration(), 10)), - jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.ComponentHash, dh), + jq.Match(`.metadata.labels."%s" == "%s"`, labels.ComponentPartOf, strings.ToLower(componentsv1.DashboardKind)), + jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.InstanceGeneration, strconv.FormatInt(rr.Instance.GetGeneration(), 10)), jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.PlatformVersion, "1.2.3"), jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.PlatformType, string(cluster.OpenDataHub)), )) diff --git a/pkg/controller/actions/gc/action_gc.go b/pkg/controller/actions/gc/action_gc.go index 3e51bdac613..c92d4e7efff 100644 --- a/pkg/controller/actions/gc/action_gc.go +++ b/pkg/controller/actions/gc/action_gc.go @@ -12,7 +12,8 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions" odhTypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" + odhLabels "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/services/gc" ) @@ -82,28 +83,30 @@ func (a *Action) run(ctx context.Context, rr *odhTypes.ReconciliationRequest) er return nil } - controllerName := strings.ToLower(rr.Instance.GetObjectKind().GroupVersionKind().Kind) + kind, err := resources.KindForObject(rr.Client.Scheme(), rr.Instance) + if err != nil { + return err + } + + controllerName := strings.ToLower(kind) CyclesTotal.WithLabelValues(controllerName).Inc() - ch, err := odhTypes.HashStr(rr) - if err != nil { - return err + selector := a.selector + if selector == nil { + selector = labels.SelectorFromSet(map[string]string{ + odhLabels.ComponentPartOf: strings.ToLower(kind), + }) } deleted, err := a.gc.Run( ctx, - a.selector, + selector, func(ctx context.Context, obj unstructured.Unstructured) (bool, error) { if slices.Contains(a.unremovables, obj.GroupVersionKind()) { return false, nil } - objectHash := obj.GetAnnotations()[annotations.ComponentHash] - if objectHash != "" { - return objectHash != ch, nil - } - return a.predicateFn(rr, obj) }, ) @@ -128,7 +131,9 @@ func NewAction(opts ...ActionOpts) actions.Fn { opt(&action) } - action.selector = labels.SelectorFromSet(action.labels) + if len(action.labels) > 0 { + action.selector = labels.SelectorFromSet(action.labels) + } // TODO: refactor if action.gc == nil { diff --git a/pkg/controller/actions/gc/action_gc_support.go b/pkg/controller/actions/gc/action_gc_support.go index ea2124e1edd..6ee3da9ebff 100644 --- a/pkg/controller/actions/gc/action_gc_support.go +++ b/pkg/controller/actions/gc/action_gc_support.go @@ -18,9 +18,10 @@ func DefaultPredicate(rr *odhTypes.ReconciliationRequest, obj unstructured.Unstr pv := resources.GetAnnotation(&obj, odhAnnotations.PlatformVersion) pt := resources.GetAnnotation(&obj, odhAnnotations.PlatformType) - cg := resources.GetAnnotation(&obj, odhAnnotations.ComponentGeneration) + ig := resources.GetAnnotation(&obj, odhAnnotations.InstanceGeneration) + iu := resources.GetAnnotation(&obj, odhAnnotations.InstanceUID) - if pv == "" || pt == "" || cg == "" { + if pv == "" || pt == "" || ig == "" || iu == "" { return false, nil } @@ -32,7 +33,11 @@ func DefaultPredicate(rr *odhTypes.ReconciliationRequest, obj unstructured.Unstr return true, nil } - g, err := strconv.Atoi(cg) + if iu != string(rr.Instance.GetUID()) { + return true, nil + } + + g, err := strconv.Atoi(ig) if err != nil { return false, fmt.Errorf("cannot determine generation: %w", err) } diff --git a/pkg/controller/actions/gc/action_gc_test.go b/pkg/controller/actions/gc/action_gc_test.go index 6b9abd9d72b..8f41200148f 100644 --- a/pkg/controller/actions/gc/action_gc_test.go +++ b/pkg/controller/actions/gc/action_gc_test.go @@ -2,6 +2,7 @@ package gc_test import ( "context" + "strings" "testing" "github.com/blang/semver/v4" @@ -17,6 +18,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + apytypes "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" ctrlCli "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" @@ -29,6 +31,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client" "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" gcSvc "github.com/opendatahub-io/opendatahub-operator/v2/pkg/services/gc" . "github.com/onsi/gomega" @@ -74,7 +77,7 @@ func TestGcAction(t *testing.T) { metricsMatcher gTypes.GomegaMatcher labels map[string]string options []gc.ActionOpts - hashFn func(*types.ReconciliationRequest) (string, error) + uidFn func(request *types.ReconciliationRequest) string }{ { name: "should delete leftovers", @@ -82,6 +85,7 @@ func TestGcAction(t *testing.T) { generated: true, matcher: Satisfy(k8serr.IsNotFound), metricsMatcher: BeNumerically("==", 1), + uidFn: func(rr *types.ReconciliationRequest) string { return string(rr.Instance.GetUID()) }, }, { name: "should not delete resources because same annotations", @@ -89,6 +93,7 @@ func TestGcAction(t *testing.T) { generated: true, matcher: Not(HaveOccurred()), metricsMatcher: BeNumerically("==", 1), + uidFn: func(rr *types.ReconciliationRequest) string { return string(rr.Instance.GetUID()) }, }, { name: "should not delete resources because of no generated resources have been detected", @@ -96,6 +101,7 @@ func TestGcAction(t *testing.T) { generated: false, matcher: Not(HaveOccurred()), metricsMatcher: BeNumerically("==", 0), + uidFn: func(rr *types.ReconciliationRequest) string { return string(rr.Instance.GetUID()) }, }, { name: "should not delete resources because of selector", @@ -105,6 +111,7 @@ func TestGcAction(t *testing.T) { metricsMatcher: BeNumerically("==", 1), labels: map[string]string{"foo": "bar"}, options: []gc.ActionOpts{gc.WithLabel("foo", "baz")}, + uidFn: func(rr *types.ReconciliationRequest) string { return string(rr.Instance.GetUID()) }, }, { name: "should not delete resources because of unremovable type", @@ -113,6 +120,7 @@ func TestGcAction(t *testing.T) { matcher: Not(HaveOccurred()), metricsMatcher: BeNumerically("==", 1), options: []gc.ActionOpts{gc.WithUnremovables(gvk.ConfigMap)}, + uidFn: func(rr *types.ReconciliationRequest) string { return string(rr.Instance.GetUID()) }, }, { name: "should not delete resources because of predicate", @@ -125,24 +133,23 @@ func TestGcAction(t *testing.T) { return unstructured.GroupVersionKind() != gvk.ConfigMap, nil }, )}, + uidFn: func(rr *types.ReconciliationRequest) string { return string(rr.Instance.GetUID()) }, }, { - name: "should delete leftovers because of hash", + name: "should delete leftovers because of UID", version: semver.Version{Major: 0, Minor: 1, Patch: 0}, generated: true, matcher: Satisfy(k8serr.IsNotFound), metricsMatcher: BeNumerically("==", 1), - hashFn: func(rr *types.ReconciliationRequest) (string, error) { - return xid.New().String(), nil - }, + uidFn: func(rr *types.ReconciliationRequest) string { return xid.New().String() }, }, { - name: "should not delete leftovers because of hash", + name: "should not delete leftovers because of UID", version: semver.Version{Major: 0, Minor: 1, Patch: 0}, generated: true, matcher: Not(HaveOccurred()), metricsMatcher: BeNumerically("==", 1), - hashFn: types.HashStr, + uidFn: func(rr *types.ReconciliationRequest) string { return string(rr.Instance.GetUID()) }, }, } @@ -153,6 +160,7 @@ func TestGcAction(t *testing.T) { g := NewWithT(t) nsn := xid.New().String() + id := xid.New().String() gci := gcSvc.New( cli, @@ -175,6 +183,7 @@ func TestGcAction(t *testing.T) { NotTo(HaveOccurred()) rr := types.ReconciliationRequest{ + Client: cli, DSCI: &dsciv1.DSCInitialization{ ObjectMeta: metav1.ObjectMeta{ Generation: 1, @@ -187,6 +196,7 @@ func TestGcAction(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Generation: 1, + UID: apytypes.UID(id), }, }, Release: cluster.Release{ @@ -198,23 +208,24 @@ func TestGcAction(t *testing.T) { Generated: tt.generated, } - ch := "" - if tt.hashFn != nil { - ch, err = tt.hashFn(&rr) - g.Expect(err).NotTo(HaveOccurred()) + l := make(map[string]string) + for k, v := range tt.labels { + l[k] = v } + l[labels.ComponentPartOf] = strings.ToLower(componentsv1.DashboardKind) + cm := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "gc-cm", Namespace: nsn, Annotations: map[string]string{ - annotations.ComponentGeneration: "1", - annotations.ComponentHash: ch, - annotations.PlatformVersion: "0.1.0", - annotations.PlatformType: string(cluster.OpenDataHub), + annotations.InstanceGeneration: "1", + annotations.InstanceUID: tt.uidFn(&rr), + annotations.PlatformVersion: "0.1.0", + annotations.PlatformType: string(cluster.OpenDataHub), }, - Labels: tt.labels, + Labels: l, }, } diff --git a/pkg/controller/actions/updatestatus/action_update_status.go b/pkg/controller/actions/updatestatus/action_update_status.go index d6287774496..f44b43625ef 100644 --- a/pkg/controller/actions/updatestatus/action_update_status.go +++ b/pkg/controller/actions/updatestatus/action_update_status.go @@ -3,6 +3,7 @@ package updatestatus import ( "context" "fmt" + "strings" appsv1 "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/api/meta" @@ -12,6 +13,8 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) const ( @@ -40,8 +43,18 @@ func WithSelectorLabels(values map[string]string) ActionOpts { } func (a *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error { - if len(a.labels) == 0 { - return nil + l := make(map[string]string, len(a.labels)) + for k, v := range a.labels { + l[k] = v + } + + if l[labels.ComponentPartOf] == "" { + kind, err := resources.KindForObject(rr.Client.Scheme(), rr.Instance) + if err != nil { + return err + } + + l[labels.ComponentPartOf] = strings.ToLower(kind) } obj, ok := rr.Instance.(types.ResourceObject) @@ -55,7 +68,7 @@ func (a *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error ctx, deployments, client.InNamespace(rr.DSCI.Spec.ApplicationsNamespace), - client.MatchingLabels(a.labels), + client.MatchingLabels(l), ) if err != nil { diff --git a/pkg/controller/actions/updatestatus/action_update_status_test.go b/pkg/controller/actions/updatestatus/action_update_status_test.go index fdde2ffc52c..e10d098ee34 100644 --- a/pkg/controller/actions/updatestatus/action_update_status_test.go +++ b/pkg/controller/actions/updatestatus/action_update_status_test.go @@ -3,6 +3,7 @@ package updatestatus_test import ( "context" + "strings" "testing" "github.com/onsi/gomega/gstruct" @@ -42,7 +43,7 @@ func TestUpdateStatusActionNotReady(t *testing.T) { Name: "my-deployment", Namespace: ns, Labels: map[string]string{ - labels.K8SCommon.PartOf: "foo", + labels.ComponentPartOf: ns, }, }, Status: appsv1.DeploymentStatus{ @@ -59,7 +60,7 @@ func TestUpdateStatusActionNotReady(t *testing.T) { Name: "my-deployment-2", Namespace: ns, Labels: map[string]string{ - labels.K8SCommon.PartOf: "foo", + labels.ComponentPartOf: ns, }, }, Status: appsv1.DeploymentStatus{ @@ -72,7 +73,7 @@ func TestUpdateStatusActionNotReady(t *testing.T) { g.Expect(err).ShouldNot(HaveOccurred()) action := updatestatus.NewAction( - updatestatus.WithSelectorLabel(labels.K8SCommon.PartOf, "foo")) + updatestatus.WithSelectorLabel(labels.ComponentPartOf, ns)) rr := types.ReconciliationRequest{ Client: cl, @@ -113,7 +114,7 @@ func TestUpdateStatusActionReady(t *testing.T) { Name: "my-deployment", Namespace: ns, Labels: map[string]string{ - labels.K8SCommon.PartOf: "foo", + labels.ComponentPartOf: ns, }, }, Status: appsv1.DeploymentStatus{ @@ -130,7 +131,7 @@ func TestUpdateStatusActionReady(t *testing.T) { Name: "my-deployment-2", Namespace: ns, Labels: map[string]string{ - labels.K8SCommon.PartOf: "foo", + labels.ComponentPartOf: ns, }, }, Status: appsv1.DeploymentStatus{ @@ -143,7 +144,7 @@ func TestUpdateStatusActionReady(t *testing.T) { g.Expect(err).ShouldNot(HaveOccurred()) action := updatestatus.NewAction( - updatestatus.WithSelectorLabel(labels.K8SCommon.PartOf, "foo")) + updatestatus.WithSelectorLabel(labels.ComponentPartOf, ns)) rr := types.ReconciliationRequest{ Client: cl, @@ -167,3 +168,143 @@ func TestUpdateStatusActionReady(t *testing.T) { ), ) } + +func TestUpdateStatusActionReadyAutoSelector(t *testing.T) { + g := NewWithT(t) + + ctx := context.Background() + ns := xid.New().String() + + cl, err := fakeclient.New( + &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: gvk.Deployment.GroupVersion().String(), + Kind: gvk.Deployment.Kind, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-deployment", + Namespace: ns, + Labels: map[string]string{ + labels.ComponentPartOf: strings.ToLower(componentsv1.DashboardKind), + }, + }, + Status: appsv1.DeploymentStatus{ + Replicas: 1, + ReadyReplicas: 1, + }, + }, + &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: gvk.Deployment.GroupVersion().String(), + Kind: gvk.Deployment.Kind, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-deployment-2", + Namespace: ns, + Labels: map[string]string{ + labels.ComponentPartOf: strings.ToLower(componentsv1.DashboardKind), + }, + }, + Status: appsv1.DeploymentStatus{ + Replicas: 1, + ReadyReplicas: 1, + }, + }, + ) + + g.Expect(err).ShouldNot(HaveOccurred()) + + action := updatestatus.NewAction() + + rr := types.ReconciliationRequest{ + Client: cl, + Instance: &componentsv1.Dashboard{}, + DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}}, + DSC: &dscv1.DataScienceCluster{}, + Release: cluster.Release{Name: cluster.OpenDataHub}, + } + + err = action(ctx, &rr) + g.Expect(err).ShouldNot(HaveOccurred()) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(rr.Instance).Should( + WithTransform( + matchers.ExtractStatusCondition(status.ConditionTypeReady), + gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{ + "Status": Equal(metav1.ConditionTrue), + "Reason": Equal(updatestatus.ReadyReason), + }), + ), + ) +} + +func TestUpdateStatusActionNotReadyNotFound(t *testing.T) { + g := NewWithT(t) + + ctx := context.Background() + ns := xid.New().String() + + cl, err := fakeclient.New( + &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: gvk.Deployment.GroupVersion().String(), + Kind: gvk.Deployment.Kind, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-deployment", + Namespace: ns, + Labels: map[string]string{ + labels.ComponentPartOf: ns, + }, + }, + Status: appsv1.DeploymentStatus{ + Replicas: 1, + ReadyReplicas: 1, + }, + }, + &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: gvk.Deployment.GroupVersion().String(), + Kind: gvk.Deployment.Kind, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-deployment-2", + Namespace: ns, + Labels: map[string]string{ + labels.ComponentPartOf: ns, + }, + }, + Status: appsv1.DeploymentStatus{ + Replicas: 1, + ReadyReplicas: 1, + }, + }, + ) + + g.Expect(err).ShouldNot(HaveOccurred()) + + action := updatestatus.NewAction() + + rr := types.ReconciliationRequest{ + Client: cl, + Instance: &componentsv1.Dashboard{}, + DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}}, + DSC: &dscv1.DataScienceCluster{}, + Release: cluster.Release{Name: cluster.OpenDataHub}, + } + + err = action(ctx, &rr) + g.Expect(err).ShouldNot(HaveOccurred()) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(rr.Instance).Should( + WithTransform( + matchers.ExtractStatusCondition(status.ConditionTypeReady), + gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{ + "Status": Equal(metav1.ConditionFalse), + "Reason": Equal(updatestatus.DeploymentsNotReadyReason), + }), + ), + ) +} diff --git a/pkg/controller/handlers/handlers.go b/pkg/controller/handlers/handlers.go index 080372244ba..5dad19f4c2d 100644 --- a/pkg/controller/handlers/handlers.go +++ b/pkg/controller/handlers/handlers.go @@ -9,14 +9,33 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -func LabelToName(label string) handler.EventHandler { +func LabelToName(key string) handler.EventHandler { return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request { - objLabels := a.GetLabels() - if len(objLabels) == 0 { + values := a.GetLabels() + if len(values) == 0 { return []reconcile.Request{} } - name := objLabels[label] + name := values[key] + if name == "" { + return []reconcile.Request{} + } + + return []reconcile.Request{{ + NamespacedName: types.NamespacedName{ + Name: name, + }, + }} + }) +} +func AnnotationToName(key string) handler.EventHandler { + return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request { + values := obj.GetAnnotations() + if len(values) == 0 { + return []reconcile.Request{} + } + + name := values[key] if name == "" { return []reconcile.Request{} } diff --git a/pkg/controller/predicates/component/component.go b/pkg/controller/predicates/component/component.go index 10bfcd3b9f1..5a69f213cd0 100644 --- a/pkg/controller/predicates/component/component.go +++ b/pkg/controller/predicates/component/component.go @@ -11,22 +11,52 @@ func ForLabel(name string, value string) predicate.Funcs { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - labelList := e.Object.GetLabels() + values := e.Object.GetLabels() - if v, exist := labelList[name]; exist && v == value { + if v, exist := values[name]; exist && v == value { return true } return false }, UpdateFunc: func(e event.UpdateEvent) bool { - oldLabels := e.ObjectOld.GetLabels() - if v, exist := oldLabels[name]; exist && v == value { + oldValues := e.ObjectOld.GetLabels() + if v, exist := oldValues[name]; exist && v == value { return true } - newLabels := e.ObjectNew.GetLabels() - if v, exist := newLabels[name]; exist && v == value { + newValues := e.ObjectNew.GetLabels() + if v, exist := newValues[name]; exist && v == value { + return true + } + + return false + }, + } +} + +func ForAnnotation(name string, value string) predicate.Funcs { + return predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + return false + }, + DeleteFunc: func(e event.DeleteEvent) bool { + values := e.Object.GetAnnotations() + + if v, exist := values[name]; exist && v == value { + return true + } + + return false + }, + UpdateFunc: func(e event.UpdateEvent) bool { + oldValues := e.ObjectOld.GetAnnotations() + if v, exist := oldValues[name]; exist && v == value { + return true + } + + newValues := e.ObjectNew.GetAnnotations() + if v, exist := newValues[name]; exist && v == value { return true } diff --git a/pkg/controller/reconciler/component_reconciler.go b/pkg/controller/reconciler/component_reconciler.go index 57cd973b744..d35aa1f1947 100644 --- a/pkg/controller/reconciler/component_reconciler.go +++ b/pkg/controller/reconciler/component_reconciler.go @@ -42,7 +42,11 @@ type ComponentReconciler struct { instanceFactory func() (components.ComponentObject, error) } -func NewComponentReconciler(mgr manager.Manager, name string, object components.ComponentObject) (*ComponentReconciler, error) { +func NewComponentReconciler( + mgr manager.Manager, + name string, + object components.ComponentObject, +) (*ComponentReconciler, error) { oc, err := odhClient.NewFromManager(mgr) if err != nil { return nil, err diff --git a/pkg/controller/reconciler/component_reconciler_support.go b/pkg/controller/reconciler/component_reconciler_support.go index 586dc25cc2e..3fc2bafd733 100644 --- a/pkg/controller/reconciler/component_reconciler_support.go +++ b/pkg/controller/reconciler/component_reconciler_support.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/hashicorp/go-multierror" "golang.org/x/exp/slices" "k8s.io/apimachinery/pkg/runtime/schema" ctrl "sigs.k8s.io/controller-runtime" @@ -18,6 +19,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/handlers" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates/component" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates/generation" + "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" ) @@ -39,6 +41,7 @@ var ( type forInput struct { object components.ComponentObject options []builder.ForOption + gvk schema.GroupVersionKind } type watchInput struct { @@ -80,20 +83,26 @@ type ComponentReconcilerBuilder struct { input forInput watches []watchInput predicates []predicate.Predicate - ownerName string componentName string actions []actions.Fn finalizers []actions.Fn + errors error } -func ComponentReconcilerFor(mgr ctrl.Manager, ownerName string, object components.ComponentObject, opts ...builder.ForOption) *ComponentReconcilerBuilder { +func ComponentReconcilerFor(mgr ctrl.Manager, object components.ComponentObject, opts ...builder.ForOption) *ComponentReconcilerBuilder { crb := ComponentReconcilerBuilder{ - mgr: mgr, - ownerName: ownerName, - input: forInput{ - object: object, - options: slices.Clone(opts), - }, + mgr: mgr, + } + + gvk, err := mgr.GetClient().GroupVersionKindFor(object) + if err != nil { + crb.errors = multierror.Append(crb.errors, fmt.Errorf("unable to determine GVK: %w", err)) + } + + crb.input = forInput{ + object: object, + options: slices.Clone(opts), + gvk: gvk, } return &crb @@ -124,17 +133,17 @@ func (b *ComponentReconcilerBuilder) Watches(object client.Object, opts ...Watch } if in.eventHandler == nil { - // use the components.opendatahub.io/part-of label to find out + // use the platform.opendatahub.io/instance.name label to find out // the owner - in.eventHandler = handlers.LabelToName(labels.ComponentPartOf) + in.eventHandler = handlers.AnnotationToName(annotations.InstanceName) } if len(in.predicates) == 0 { in.predicates = append(in.predicates, predicate.And( defaultPredicate, // use the components.opendatahub.io/part-of label to filter - // events not related to the owner - component.ForLabel(labels.ComponentPartOf, b.ownerName), + // events not related to the owner type + component.ForLabel(labels.ComponentPartOf, strings.ToLower(b.input.gvk.Kind)), )) } @@ -184,18 +193,13 @@ func (b *ComponentReconcilerBuilder) WithEventFilter(p predicate.Predicate) *Com } func (b *ComponentReconcilerBuilder) Build(_ context.Context) (*ComponentReconciler, error) { + if b.errors != nil { + return nil, b.errors + } + name := b.componentName if name == "" { - kinds, _, err := b.mgr.GetScheme().ObjectKinds(b.input.object) - if err != nil { - return nil, err - } - if len(kinds) != 1 { - return nil, fmt.Errorf("expected exactly one kind of object, got %d", len(kinds)) - } - - name = kinds[0].Kind - name = strings.ToLower(name) + name = strings.ToLower(b.input.gvk.Kind) } r, err := NewComponentReconciler(b.mgr, name, b.input.object) diff --git a/pkg/metadata/annotations/annotations.go b/pkg/metadata/annotations/annotations.go index a39de39bb9e..7ebd755d077 100644 --- a/pkg/metadata/annotations/annotations.go +++ b/pkg/metadata/annotations/annotations.go @@ -18,8 +18,9 @@ const ( const ManagementStateAnnotation = "component.opendatahub.io/management-state" const ( - ComponentGeneration = "components.opendatahub.io/component-generation" - ComponentHash = "components.opendatahub.io/component-hash" - PlatformVersion = "platform.opendatahub.io/version" - PlatformType = "platform.opendatahub.io/type" + PlatformVersion = "platform.opendatahub.io/version" + PlatformType = "platform.opendatahub.io/type" + InstanceGeneration = "platform.opendatahub.io/instance.generation" + InstanceName = "platform.opendatahub.io/instance.name" + InstanceUID = "platform.opendatahub.io/instance.uid" ) diff --git a/pkg/resources/resources.go b/pkg/resources/resources.go index a9df8414d13..1c0cde96b88 100644 --- a/pkg/resources/resources.go +++ b/pkg/resources/resources.go @@ -16,6 +16,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) func ToUnstructured(obj any) (*unstructured.Unstructured, error) { @@ -228,3 +229,16 @@ func Hash(in *unstructured.Unstructured) ([]byte, error) { func EncodeToString(in []byte) string { return "v" + base64.RawURLEncoding.EncodeToString(in) } + +func KindForObject(scheme *runtime.Scheme, obj runtime.Object) (string, error) { + if obj.GetObjectKind().GroupVersionKind().Kind != "" { + return obj.GetObjectKind().GroupVersionKind().Kind, nil + } + + gvk, err := apiutil.GVKForObject(obj, scheme) + if err != nil { + return "", fmt.Errorf("failed to get GVK: %w", err) + } + + return gvk.Kind, nil +} diff --git a/pkg/utils/test/fakeclient/fakeclient.go b/pkg/utils/test/fakeclient/fakeclient.go index 8cbef38a411..121512e1d76 100644 --- a/pkg/utils/test/fakeclient/fakeclient.go +++ b/pkg/utils/test/fakeclient/fakeclient.go @@ -12,6 +12,7 @@ import ( ctrlClient "sigs.k8s.io/controller-runtime/pkg/client" clientFake "sigs.k8s.io/controller-runtime/pkg/client/fake" + componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) @@ -21,6 +22,7 @@ func New(objs ...ctrlClient.Object) (*client.Client, error) { utilruntime.Must(corev1.AddToScheme(scheme)) utilruntime.Must(appsv1.AddToScheme(scheme)) utilruntime.Must(rbacv1.AddToScheme(scheme)) + utilruntime.Must(componentsv1.AddToScheme(scheme)) fakeMapper := meta.NewDefaultRESTMapper(scheme.PreferredVersionAllGroups()) for gvk := range scheme.AllKnownTypes() { diff --git a/tests/e2e/dashboard_test.go b/tests/e2e/dashboard_test.go index f13930ce8e4..7ce9ba85b68 100644 --- a/tests/e2e/dashboard_test.go +++ b/tests/e2e/dashboard_test.go @@ -2,6 +2,7 @@ package e2e_test import ( "fmt" + "strings" "testing" "time" @@ -193,7 +194,7 @@ func (d *DashboardTestCtx) validateOperandsOwnerReferences(t *testing.T) { d.List( gvk.Deployment, client.InNamespace(d.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.DashboardInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.DashboardKind)}, ), ).Should(And( HaveLen(1), @@ -229,7 +230,7 @@ func (d *DashboardTestCtx) validateOperandsDynamicallyWatchedResources(t *testin g.Eventually( d.List( gvk.OdhApplication, - client.MatchingLabels{labels.ComponentPartOf: componentsv1.DashboardInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.DashboardKind)}, ), ).Should(And( HaveEach( @@ -244,7 +245,7 @@ func (d *DashboardTestCtx) validateUpdateOperandsResources(t *testing.T) { appDeployments, err := d.kubeClient.AppsV1().Deployments(d.applicationsNamespace).List( d.ctx, metav1.ListOptions{ - LabelSelector: labels.ComponentPartOf + "=" + componentsv1.DashboardInstanceName, + LabelSelector: labels.ComponentPartOf + "=" + strings.ToLower(componentsv1.DashboardKind), }, ) @@ -278,7 +279,7 @@ func (d *DashboardTestCtx) validateUpdateOperandsResources(t *testing.T) { d.List( gvk.Deployment, client.InNamespace(d.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.DashboardInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.DashboardKind)}, ), ).Should(And( HaveLen(1), @@ -291,7 +292,7 @@ func (d *DashboardTestCtx) validateUpdateOperandsResources(t *testing.T) { d.List( gvk.Deployment, client.InNamespace(d.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.DashboardInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.DashboardKind)}, ), ).WithTimeout(30 * time.Second).WithPolling(1 * time.Second).Should(And( HaveLen(1), @@ -308,7 +309,7 @@ func (d *DashboardTestCtx) validateDashboardDisabled(t *testing.T) { d.List( gvk.Deployment, client.InNamespace(d.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.DashboardInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.DashboardKind)}, ), ).Should( HaveLen(1), @@ -326,7 +327,7 @@ func (d *DashboardTestCtx) validateDashboardDisabled(t *testing.T) { d.List( gvk.Deployment, client.InNamespace(d.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.DashboardInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.DashboardKind)}, ), ).Should( BeEmpty(), diff --git a/tests/e2e/datasciencepipelines_test.go b/tests/e2e/datasciencepipelines_test.go index c0509d4fe6a..7ddfd1c938d 100644 --- a/tests/e2e/datasciencepipelines_test.go +++ b/tests/e2e/datasciencepipelines_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "reflect" + "strings" "testing" "time" @@ -200,7 +201,7 @@ func (tc *DataSciencePipelinesTestCtx) validateDataSciencePipelinesReady() error func (tc *DataSciencePipelinesTestCtx) testUpdateOnDataSciencePipelinesResources() error { appDeployments, err := tc.testCtx.kubeClient.AppsV1().Deployments(tc.testCtx.applicationsNamespace).List(tc.testCtx.ctx, metav1.ListOptions{ - LabelSelector: labels.ComponentPartOf + "=" + tc.testDataSciencePipelinesInstance.Name, + LabelSelector: labels.ComponentPartOf + "=" + strings.ToLower(tc.testDataSciencePipelinesInstance.Kind), }) if err != nil { return err diff --git a/tests/e2e/kueue_test.go b/tests/e2e/kueue_test.go index dcd050226ba..f445986df53 100644 --- a/tests/e2e/kueue_test.go +++ b/tests/e2e/kueue_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "reflect" + "strings" "testing" "time" @@ -164,7 +165,7 @@ func (tc *KueueTestCtx) testUpdateOnKueueResources() error { // Test Updating Kueue Replicas appDeployments, err := tc.testCtx.kubeClient.AppsV1().Deployments(tc.testCtx.applicationsNamespace).List(tc.testCtx.ctx, metav1.ListOptions{ - LabelSelector: labels.ComponentPartOf + "=" + tc.testKueueInstance.Name, + LabelSelector: labels.ComponentPartOf + "=" + strings.ToLower(tc.testKueueInstance.Kind), }) if err != nil { return err diff --git a/tests/e2e/modelregistry_test.go b/tests/e2e/modelregistry_test.go index 48231381358..708c6ae8467 100644 --- a/tests/e2e/modelregistry_test.go +++ b/tests/e2e/modelregistry_test.go @@ -2,6 +2,7 @@ package e2e_test import ( "fmt" + "strings" "testing" "time" @@ -199,7 +200,7 @@ func (mr *ModelRegistryTestCtx) validateOperandsOwnerReferences(t *testing.T) { mr.List( gvk.Deployment, client.InNamespace(mr.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.ModelRegistryInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.ModelRegistryKind)}, ), ).Should(And( HaveLen(1), @@ -215,7 +216,7 @@ func (mr *ModelRegistryTestCtx) validateOperandsWatchedResources(t *testing.T) { g.Eventually( mr.List( gvk.ServiceMeshMember, - client.MatchingLabels{labels.ComponentPartOf: componentsv1.ModelRegistryInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.ModelRegistryKind)}, ), ).Should(And( HaveLen(1), @@ -251,7 +252,7 @@ func (mr *ModelRegistryTestCtx) validateOperandsDynamicallyWatchedResources(t *t g.Eventually( mr.List( gvk.ServiceMeshMember, - client.MatchingLabels{labels.ComponentPartOf: componentsv1.ModelRegistryInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.ModelRegistryKind)}, ), ).Should(And( HaveLen(1), @@ -267,7 +268,7 @@ func (mr *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t * appDeployments, err := mr.kubeClient.AppsV1().Deployments(mr.applicationsNamespace).List( mr.ctx, metav1.ListOptions{ - LabelSelector: labels.ComponentPartOf + "=" + componentsv1.ModelRegistryInstanceName, + LabelSelector: labels.ComponentPartOf + "=" + strings.ToLower(componentsv1.ModelRegistryKind), }, ) @@ -302,7 +303,7 @@ func (mr *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t * mr.List( gvk.Deployment, client.InNamespace(mr.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.ModelRegistryInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.ModelRegistryKind)}, ), ).Should(And( HaveLen(1), @@ -315,7 +316,7 @@ func (mr *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t * mr.List( gvk.Deployment, client.InNamespace(mr.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.ModelRegistryInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.ModelRegistryKind)}, ), ).WithTimeout(30 * time.Second).WithPolling(1 * time.Second).Should(And( HaveLen(1), @@ -361,7 +362,7 @@ func (mr *ModelRegistryTestCtx) validateModelRegistryDisabled(t *testing.T) { mr.List( gvk.Deployment, client.InNamespace(mr.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.ModelRegistryInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.ModelRegistryKind)}, ), ).Should( HaveLen(1), @@ -379,7 +380,7 @@ func (mr *ModelRegistryTestCtx) validateModelRegistryDisabled(t *testing.T) { mr.List( gvk.Deployment, client.InNamespace(mr.applicationsNamespace), - client.MatchingLabels{labels.ComponentPartOf: componentsv1.ModelRegistryInstanceName}, + client.MatchingLabels{labels.ComponentPartOf: strings.ToLower(componentsv1.ModelRegistryKind)}, ), ).Should( BeEmpty(), diff --git a/tests/e2e/ray_test.go b/tests/e2e/ray_test.go index 1183c3ac35f..cd62819e2fe 100644 --- a/tests/e2e/ray_test.go +++ b/tests/e2e/ray_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "reflect" + "strings" "testing" "time" @@ -164,7 +165,7 @@ func (tc *RayTestCtx) testUpdateOnRayResources() error { // Test Updating Ray Replicas appDeployments, err := tc.testCtx.kubeClient.AppsV1().Deployments(tc.testCtx.applicationsNamespace).List(tc.testCtx.ctx, metav1.ListOptions{ - LabelSelector: labels.ComponentPartOf + "=" + tc.testRayInstance.Name, + LabelSelector: labels.ComponentPartOf + "=" + strings.ToLower(tc.testRayInstance.Kind), }) if err != nil { return err diff --git a/tests/e2e/trainingoperator_test.go b/tests/e2e/trainingoperator_test.go index f4cb0543d3f..683bcf84694 100644 --- a/tests/e2e/trainingoperator_test.go +++ b/tests/e2e/trainingoperator_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "reflect" + "strings" "testing" "time" @@ -164,7 +165,7 @@ func (tc *TrainingOperatorTestCtx) testUpdateOnTrainingOperatorResources() error // Test Updating TrainingOperator Replicas appDeployments, err := tc.testCtx.kubeClient.AppsV1().Deployments(tc.testCtx.applicationsNamespace).List(tc.testCtx.ctx, metav1.ListOptions{ - LabelSelector: labels.ComponentPartOf + "=" + tc.testTrainingOperatorInstance.Name, + LabelSelector: labels.ComponentPartOf + "=" + strings.ToLower(tc.testTrainingOperatorInstance.Kind), }) if err != nil { return err diff --git a/tests/e2e/trustyai_test.go b/tests/e2e/trustyai_test.go index 4f6c33dceb9..7882d764d98 100644 --- a/tests/e2e/trustyai_test.go +++ b/tests/e2e/trustyai_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "reflect" + "strings" "testing" "time" @@ -164,7 +165,7 @@ func (tc *TrustyaiTestCtx) testUpdateOnTrustyaiResources() error { // Test Updating Trustyai Replicas appDeployments, err := tc.testCtx.kubeClient.AppsV1().Deployments(tc.testCtx.applicationsNamespace).List(tc.testCtx.ctx, metav1.ListOptions{ - LabelSelector: labels.ComponentPartOf + "=" + tc.testTrustyaiInstance.Name, + LabelSelector: labels.ComponentPartOf + "=" + strings.ToLower(tc.testTrustyaiInstance.Kind), }) if err != nil { return err