From ec93aa2761fa34953c0c8b09d3f5aa225366a41a Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Fri, 24 Jan 2025 10:35:07 +0100 Subject: [PATCH] Improve Kserve's FeatureTraker handing - Make it possible to set FT's OwnerReference as Controller reference so the kubernetes garbage collector can block owner deletion till the FT has been deleted - Add an Kserver reconciler action to remove legacy ownership on DSCI/DSC is any and related e2e tests --- .../components/kserve/kserve_controller.go | 14 +-- .../kserve/kserve_controller_actions.go | 30 +++++ .../components/kserve/kserve_support.go | 12 ++ pkg/cluster/gvk/gvk.go | 6 + pkg/cluster/meta.go | 6 + .../actions/deploy/action_deploy.go | 2 +- .../actions/deploy/action_deploy_support.go | 55 --------- .../deploy/action_deploy_support_test.go | 108 ------------------ pkg/controller/reconciler/reconciler.go | 42 +++++++ pkg/feature/builder.go | 20 +++- pkg/feature/feature.go | 24 +++- pkg/feature/feature_tracker_handler.go | 30 +++-- pkg/feature/handler.go | 3 + pkg/resources/resources.go | 53 +++++++++ pkg/resources/resources_test.go | 108 +++++++++++++++++- tests/e2e/kserve_test.go | 71 +++++++++++- 16 files changed, 387 insertions(+), 197 deletions(-) diff --git a/controllers/components/kserve/kserve_controller.go b/controllers/components/kserve/kserve_controller.go index 6c0b2bcf0a7..9ec335e4491 100644 --- a/controllers/components/kserve/kserve_controller.go +++ b/controllers/components/kserve/kserve_controller.go @@ -29,7 +29,6 @@ import ( extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" @@ -67,17 +66,7 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. // changes. The compareHashPredicate ensures that we don't needlessly enqueue // requests if there are no changes that we don't care about. Owns(&templatev1.Template{}, reconciler.WithPredicates(hash.Updated())). - // The FeatureTrackers are created slightly differently, and have - // ownerRefs set by controllerutil.SetOwnerReference() rather than - // controllerutil.SetControllerReference(), which means that the default - // eventHandler for Owns won't work, so a slightly modified variant is - // added here - Owns(&featuresv1.FeatureTracker{}, reconciler.WithEventHandler( - handler.EnqueueRequestForOwner( - mgr.GetScheme(), - mgr.GetRESTMapper(), - &componentApi.Kserve{}, - ))). + Owns(&featuresv1.FeatureTracker{}). Owns(&networkingv1.NetworkPolicy{}). Owns(&monitoringv1.ServiceMonitor{}). Owns(&admissionregistrationv1.MutatingWebhookConfiguration{}). @@ -155,6 +144,7 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. WithAction(checkPreConditions). WithAction(initialize). WithAction(devFlags). + WithAction(removeLegacyFeatureTrackerOwnerRef). WithAction(configureServerless). WithAction(configureServiceMesh). WithAction(kustomize.NewAction( diff --git a/controllers/components/kserve/kserve_controller_actions.go b/controllers/components/kserve/kserve_controller_actions.go index b12562a0f87..d7fbc4f9b16 100644 --- a/controllers/components/kserve/kserve_controller_actions.go +++ b/controllers/components/kserve/kserve_controller_actions.go @@ -9,11 +9,14 @@ import ( operatorv1 "github.com/openshift/api/operator/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + k8serr "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" + featuresv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" @@ -22,6 +25,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) func checkPreConditions(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { @@ -137,6 +141,32 @@ func devFlags(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { return nil } +func removeLegacyFeatureTrackerOwnerRef(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { + ftNames := []string{ + rr.DSCI.Spec.ApplicationsNamespace + "-serverless-serving-deployment", + rr.DSCI.Spec.ApplicationsNamespace + "-serverless-net-istio-secret-filtering", + rr.DSCI.Spec.ApplicationsNamespace + "-serverless-serving-gateways", + rr.DSCI.Spec.ApplicationsNamespace + "-kserve-external-authz", + } + + for _, ftName := range ftNames { + obj := &featuresv1.FeatureTracker{} + err := rr.Client.Get(ctx, client.ObjectKey{Name: ftName}, obj) + switch { + case k8serr.IsNotFound(err): + continue + case err != nil: + return fmt.Errorf("error while retrieving FeatureTracker %s: %w", ftName, err) + } + + if err := resources.RemoveOwnerReferences(ctx, rr.Client, obj, isLegacyOwnerRef); err != nil { + return err + } + } + + return nil +} + func configureServerless(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { k, ok := rr.Instance.(*componentApi.Kserve) if !ok { diff --git a/controllers/components/kserve/kserve_support.go b/controllers/components/kserve/kserve_support.go index 2f12314db33..7fad7f6ad22 100644 --- a/controllers/components/kserve/kserve_support.go +++ b/controllers/components/kserve/kserve_support.go @@ -22,6 +22,7 @@ import ( dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" featuresv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" odhtypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" @@ -266,3 +267,14 @@ func ownedViaFT(cli client.Client) handler.MapFunc { return []reconcile.Request{} } } + +func isLegacyOwnerRef(or metav1.OwnerReference) bool { + switch { + case or.APIVersion == gvk.DataScienceCluster.GroupVersion().String() && or.Kind == gvk.DataScienceCluster.Kind: + return true + case or.APIVersion == gvk.DSCInitialization.GroupVersion().String() && or.Kind == gvk.DSCInitialization.Kind: + return true + default: + return false + } +} diff --git a/pkg/cluster/gvk/gvk.go b/pkg/cluster/gvk/gvk.go index 240e4408e38..be654aaddd9 100644 --- a/pkg/cluster/gvk/gvk.go +++ b/pkg/cluster/gvk/gvk.go @@ -8,6 +8,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" + featuresv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" serviceApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/services/v1alpha1" ) @@ -28,6 +29,11 @@ var ( Version: "v1", Kind: "DSCInitialization", } + FeatureTracker = schema.GroupVersionKind{ + Group: featuresv1.GroupVersion.Group, + Version: featuresv1.GroupVersion.Version, + Kind: "FeatureTracker", + } Deployment = schema.GroupVersionKind{ Group: appsv1.SchemeGroupVersion.Group, diff --git a/pkg/cluster/meta.go b/pkg/cluster/meta.go index 659aa8112df..1fb576ff21f 100644 --- a/pkg/cluster/meta.go +++ b/pkg/cluster/meta.go @@ -37,6 +37,12 @@ func OwnedBy(owner metav1.Object, scheme *runtime.Scheme) MetaOptions { } } +func ControllerBy(owner metav1.Object, scheme *runtime.Scheme) MetaOptions { + return func(obj metav1.Object) error { + return controllerutil.SetControllerReference(owner, obj, scheme) + } +} + func WithLabels(labels ...string) MetaOptions { return func(obj metav1.Object) error { labelsMap, err := extractKeyValues(labels) diff --git a/pkg/controller/actions/deploy/action_deploy.go b/pkg/controller/actions/deploy/action_deploy.go index bb419f30f5c..a8614f0fa4c 100644 --- a/pkg/controller/actions/deploy/action_deploy.go +++ b/pkg/controller/actions/deploy/action_deploy.go @@ -134,7 +134,7 @@ func (a *Action) run(ctx context.Context, rr *odhTypes.ReconciliationRequest) er default: // Remove the DSC and DSCI owner reference if set, This is required during the // transition from the old to the new operator. - if err := removeOwnerReferences(ctx, rr.Client, current, isLegacyOwnerRef); err != nil { + if err := resources.RemoveOwnerReferences(ctx, rr.Client, current, isLegacyOwnerRef); err != nil { return err } diff --git a/pkg/controller/actions/deploy/action_deploy_support.go b/pkg/controller/actions/deploy/action_deploy_support.go index f6799a03af3..433b5f6473a 100644 --- a/pkg/controller/actions/deploy/action_deploy_support.go +++ b/pkg/controller/actions/deploy/action_deploy_support.go @@ -1,11 +1,7 @@ package deploy import ( - "context" - "fmt" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" ) @@ -20,54 +16,3 @@ func isLegacyOwnerRef(or metav1.OwnerReference) bool { return false } } - -// removeOwnerReferences removes all owner references from a Kubernetes object that match the provided predicate. -// -// This function iterates through the OwnerReferences of the given object, filters out those that satisfy -// the predicate, and updates the object in the cluster using the provided client. -// -// Parameters: -// - ctx: The context for the request, which can carry deadlines, cancellation signals, and other request-scoped values. -// - cli: A controller-runtime client used to update the Kubernetes object. -// - obj: The Kubernetes object whose OwnerReferences are to be filtered. It must implement client.Object. -// - predicate: A function that takes an OwnerReference and returns true if the reference should be removed. -// -// Returns: -// - An error if the update operation fails, otherwise nil. -func removeOwnerReferences( - ctx context.Context, - cli client.Client, - obj client.Object, - predicate func(reference metav1.OwnerReference) bool, -) error { - oldRefs := obj.GetOwnerReferences() - if len(oldRefs) == 0 { - return nil - } - - newRefs := oldRefs[:0] - for _, ref := range oldRefs { - if !predicate(ref) { - newRefs = append(newRefs, ref) - } - } - - if len(newRefs) == len(oldRefs) { - return nil - } - - obj.SetOwnerReferences(newRefs) - - // Update the object in the cluster - if err := cli.Update(ctx, obj); err != nil { - return fmt.Errorf( - "failed to remove owner references from object %s/%s with gvk %s: %w", - obj.GetNamespace(), - obj.GetName(), - obj.GetObjectKind().GroupVersionKind(), - err, - ) - } - - return nil -} diff --git a/pkg/controller/actions/deploy/action_deploy_support_test.go b/pkg/controller/actions/deploy/action_deploy_support_test.go index 01396aa3a22..297c0e16113 100644 --- a/pkg/controller/actions/deploy/action_deploy_support_test.go +++ b/pkg/controller/actions/deploy/action_deploy_support_test.go @@ -2,30 +2,12 @@ package deploy import ( - "context" - "path/filepath" "testing" - "github.com/onsi/gomega/gstruct" "github.com/onsi/gomega/types" - "github.com/rs/xid" - appsv1 "k8s.io/api/apps/v1" - corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/envtest" - componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" - dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" - dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" - odhCli "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client" - "github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil" . "github.com/onsi/gomega" ) @@ -97,93 +79,3 @@ func TestIsLegacyOwnerRef(t *testing.T) { }) } } - -func TestRemoveOwnerRef(t *testing.T) { - g := NewWithT(t) - s := runtime.NewScheme() - - ctx := context.Background() - ns := xid.New().String() - - utilruntime.Must(corev1.AddToScheme(s)) - utilruntime.Must(appsv1.AddToScheme(s)) - utilruntime.Must(apiextensionsv1.AddToScheme(s)) - utilruntime.Must(componentApi.AddToScheme(s)) - utilruntime.Must(dsciv1.AddToScheme(s)) - utilruntime.Must(dscv1.AddToScheme(s)) - utilruntime.Must(rbacv1.AddToScheme(s)) - - projectDir, err := envtestutil.FindProjectRoot() - g.Expect(err).NotTo(HaveOccurred()) - - envTest := &envtest.Environment{ - CRDInstallOptions: envtest.CRDInstallOptions{ - Scheme: s, - Paths: []string{ - filepath.Join(projectDir, "config", "crd", "bases"), - }, - ErrorIfPathMissing: true, - CleanUpAfterUse: false, - }, - } - - t.Cleanup(func() { - _ = envTest.Stop() - }) - - cfg, err := envTest.Start() - g.Expect(err).NotTo(HaveOccurred()) - - envTestClient, err := client.New(cfg, client.Options{Scheme: s}) - g.Expect(err).NotTo(HaveOccurred()) - - cli, err := odhCli.NewFromConfig(cfg, envTestClient) - g.Expect(err).NotTo(HaveOccurred()) - - err = cli.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}) - g.Expect(err).ToNot(HaveOccurred()) - - cm1 := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm1", Namespace: ns}} - cm1.SetGroupVersionKind(gvk.ConfigMap) - - err = cli.Create(ctx, cm1) - g.Expect(err).ToNot(HaveOccurred()) - - cm2 := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm2", Namespace: ns}} - cm2.SetGroupVersionKind(gvk.ConfigMap) - - err = cli.Create(ctx, cm2) - g.Expect(err).ToNot(HaveOccurred()) - - // Create a ConfigMap with OwnerReferences - configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "test-configmap", Namespace: ns}} - - err = controllerutil.SetOwnerReference(cm1, configMap, s) - g.Expect(err).ToNot(HaveOccurred()) - err = controllerutil.SetOwnerReference(cm2, configMap, s) - g.Expect(err).ToNot(HaveOccurred()) - - err = cli.Create(ctx, configMap) - g.Expect(err).ToNot(HaveOccurred()) - - predicate := func(ref metav1.OwnerReference) bool { - return ref.Name == cm1.Name - } - - err = removeOwnerReferences(ctx, cli, configMap, predicate) - g.Expect(err).ToNot(HaveOccurred()) - - updatedConfigMap := &corev1.ConfigMap{} - err = cli.Get(ctx, client.ObjectKeyFromObject(configMap), updatedConfigMap) - g.Expect(err).ToNot(HaveOccurred()) - - g.Expect(updatedConfigMap.GetOwnerReferences()).Should(And( - HaveLen(1), - HaveEach(gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{ - "Name": Equal(cm2.Name), - "APIVersion": Equal(gvk.ConfigMap.GroupVersion().String()), - "Kind": Equal(gvk.ConfigMap.Kind), - "UID": Equal(cm2.UID), - })), - )) -} diff --git a/pkg/controller/reconciler/reconciler.go b/pkg/controller/reconciler/reconciler.go index 565738e3a7b..8ac0e6f9a23 100644 --- a/pkg/controller/reconciler/reconciler.go +++ b/pkg/controller/reconciler/reconciler.go @@ -13,6 +13,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -26,6 +27,10 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" ) +const ( + finalizerName = "platform.opendatahub.io/finalizer" +) + // Reconciler provides generic reconciliation functionality for ODH objects. type Reconciler struct { Client *odhClient.Client @@ -112,7 +117,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if err := r.delete(ctx, res); err != nil { return ctrl.Result{}, err } + if err := r.removeFinalizer(ctx, res); err != nil { + return ctrl.Result{}, err + } } else { + if err := r.addFinalizer(ctx, res); err != nil { + return ctrl.Result{}, err + } + if err := r.apply(ctx, res); err != nil { return ctrl.Result{}, err } @@ -121,6 +133,36 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, nil } +func (r *Reconciler) addFinalizer(ctx context.Context, res common.PlatformObject) error { + if len(r.Finalizer) == 0 { + return nil + } + + if !controllerutil.AddFinalizer(res, finalizerName) { + return nil + } + + err := r.Client.Update(ctx, res) + if err != nil { + return fmt.Errorf("failure adding finalizer %s: %w", finalizerName, err) + } + + return nil +} + +func (r *Reconciler) removeFinalizer(ctx context.Context, res common.PlatformObject) error { + if !controllerutil.RemoveFinalizer(res, finalizerName) { + return nil + } + + err := r.Client.Update(ctx, res) + if err != nil { + return fmt.Errorf("failure removing finalizer %s: %w", finalizerName, err) + } + + return nil +} + func (r *Reconciler) delete(ctx context.Context, res common.PlatformObject) error { l := log.FromContext(ctx) l.Info("delete") diff --git a/pkg/feature/builder.go b/pkg/feature/builder.go index 54fadcabbec..83b7b14e862 100644 --- a/pkg/feature/builder.go +++ b/pkg/feature/builder.go @@ -19,6 +19,7 @@ type featureBuilder struct { managed bool source featurev1.Source owner metav1.Object + controller bool targetNs string builders []partialBuilder @@ -95,6 +96,12 @@ func (fb *featureBuilder) OwnedBy(object metav1.Object) *featureBuilder { return fb } +func (fb *featureBuilder) Controller(controller bool) *featureBuilder { + fb.controller = controller + + return fb +} + // Managed marks the feature as managed by the operator. This effectively marks all resources which are part of this feature // as those that should be updated on operator reconcile. // Managed marks the feature as managed by the operator. @@ -193,12 +200,13 @@ func (fb *featureBuilder) Create() (*Feature, error) { } f := &Feature{ - Name: fb.featureName, - Managed: fb.managed, - Enabled: alwaysEnabled, - Log: log.Log.WithName("features").WithValues("feature", fb.featureName), - source: &fb.source, - owner: fb.owner, + Name: fb.featureName, + Managed: fb.managed, + Enabled: alwaysEnabled, + Log: log.Log.WithName("features").WithValues("feature", fb.featureName), + source: &fb.source, + owner: fb.owner, + controller: fb.controller, } for i := range fb.builders { diff --git a/pkg/feature/feature.go b/pkg/feature/feature.go index 33e569a0fd5..4d16c094751 100644 --- a/pkg/feature/feature.go +++ b/pkg/feature/feature.go @@ -7,6 +7,7 @@ import ( "github.com/go-logr/logr" "github.com/hashicorp/go-multierror" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" @@ -43,9 +44,10 @@ type Feature struct { Log logr.Logger - tracker *featurev1.FeatureTracker - source *featurev1.Source - owner metav1.Object + tracker *featurev1.FeatureTracker + source *featurev1.Source + owner metav1.Object + controller bool data map[string]any @@ -168,8 +170,22 @@ func OwnedBy(f *Feature) cluster.MetaOptions { return cluster.WithOwnerReference(f.AsOwnerReference()) } +func ControlledBy(f *Feature) cluster.MetaOptions { + or := f.AsOwnerReference() + or.Controller = ptr.To[bool](true) + or.BlockOwnerDeletion = ptr.To[bool](true) + return cluster.WithOwnerReference(or) +} + func DefaultMetaOptions(f *Feature) []cluster.MetaOptions { - resourceMeta := []cluster.MetaOptions{OwnedBy(f)} + resourceMeta := make([]cluster.MetaOptions, 0, 1) + + if f.controller { + resourceMeta = append(resourceMeta, ControlledBy(f)) + } else { + resourceMeta = append(resourceMeta, OwnedBy(f)) + } + if f.Managed { resourceMeta = append(resourceMeta, func(obj metav1.Object) error { objAnnotations := obj.GetAnnotations() diff --git a/pkg/feature/feature_tracker_handler.go b/pkg/feature/feature_tracker_handler.go index 6ba728a11d6..6a8949b9308 100644 --- a/pkg/feature/feature_tracker_handler.go +++ b/pkg/feature/feature_tracker_handler.go @@ -38,20 +38,34 @@ func createFeatureTracker(ctx context.Context, cli client.Client, f *Feature) er if k8serr.IsNotFound(errGet) { tracker = featurev1.NewFeatureTracker(f.Name, f.TargetNamespace) - tracker.Spec = featurev1.FeatureTrackerSpec{ - Source: *f.source, - AppNamespace: f.TargetNamespace, + } + + tracker.Spec = featurev1.FeatureTrackerSpec{ + Source: *f.source, + AppNamespace: f.TargetNamespace, + } + + if f.owner != nil { + var ownerRef cluster.MetaOptions + if f.controller { + ownerRef = cluster.ControllerBy(f.owner, cli.Scheme()) + } else { + ownerRef = cluster.OwnedBy(f.owner, cli.Scheme()) } - if f.owner != nil { - ownerRef := cluster.OwnedBy(f.owner, cli.Scheme()) - if errMetaOpts := cluster.ApplyMetaOptions(tracker, ownerRef); errMetaOpts != nil { - return fmt.Errorf("failed adding owner to FeatureTracker %s: %w", tracker.Name, errMetaOpts) - } + + if errMetaOpts := cluster.ApplyMetaOptions(tracker, ownerRef); errMetaOpts != nil { + return fmt.Errorf("failed adding owner to FeatureTracker %s: %w", tracker.Name, errMetaOpts) } + } + if k8serr.IsNotFound(errGet) { if errCreate := cli.Create(ctx, tracker); errCreate != nil { return fmt.Errorf("failed creating FeatureTracker %s: %w", tracker.Name, errCreate) } + } else { + if errUpdate := cli.Update(ctx, tracker); errUpdate != nil { + return fmt.Errorf("failed updating FeatureTracker %s: %w", tracker.Name, errUpdate) + } } if errGVK := ensureGVKSet(tracker, cli.Scheme()); errGVK != nil { diff --git a/pkg/feature/handler.go b/pkg/feature/handler.go index 890a51a1fcc..6e71d5e91df 100644 --- a/pkg/feature/handler.go +++ b/pkg/feature/handler.go @@ -30,6 +30,7 @@ var _ featuresHandler = (*FeaturesHandler)(nil) type FeaturesHandler struct { source featurev1.Source owner metav1.Object + controller bool targetNamespace string features []*Feature featuresProviders []FeaturesProvider @@ -47,6 +48,7 @@ func (fh *FeaturesHandler) Add(builders ...*featureBuilder) error { feature, err := fb. TargetNamespace(fh.targetNamespace). OwnedBy(fh.owner). + Controller(fh.controller). Source(fh.source). Create() multiErr = multierror.Append(multiErr, err) @@ -112,6 +114,7 @@ func ClusterFeaturesHandler(dsci *dsciv1.DSCInitialization, def ...FeaturesProvi func ComponentFeaturesHandler(owner metav1.Object, componentName, targetNamespace string, def ...FeaturesProvider) *FeaturesHandler { return &FeaturesHandler{ owner: owner, + controller: true, targetNamespace: targetNamespace, source: featurev1.Source{Type: featurev1.ComponentType, Name: componentName}, featuresProviders: def, diff --git a/pkg/resources/resources.go b/pkg/resources/resources.go index f5de7cf39a3..72ddd91622d 100644 --- a/pkg/resources/resources.go +++ b/pkg/resources/resources.go @@ -2,6 +2,7 @@ package resources import ( "bytes" + "context" "crypto/sha256" "encoding/base64" "errors" @@ -13,6 +14,7 @@ import ( routev1 "github.com/openshift/api/route/v1" "gopkg.in/yaml.v3" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -327,3 +329,54 @@ func NamespacedNameFromObject(obj client.Object) types.NamespacedName { Name: obj.GetName(), } } + +// RemoveOwnerReferences removes all owner references from a Kubernetes object that match the provided predicate. +// +// This function iterates through the OwnerReferences of the given object, filters out those that satisfy +// the predicate, and updates the object in the cluster using the provided client. +// +// Parameters: +// - ctx: The context for the request, which can carry deadlines, cancellation signals, and other request-scoped values. +// - cli: A controller-runtime client used to update the Kubernetes object. +// - obj: The Kubernetes object whose OwnerReferences are to be filtered. It must implement client.Object. +// - predicate: A function that takes an OwnerReference and returns true if the reference should be removed. +// +// Returns: +// - An error if the update operation fails, otherwise nil. +func RemoveOwnerReferences( + ctx context.Context, + cli client.Client, + obj client.Object, + predicate func(reference metav1.OwnerReference) bool, +) error { + oldRefs := obj.GetOwnerReferences() + if len(oldRefs) == 0 { + return nil + } + + newRefs := oldRefs[:0] + for _, ref := range oldRefs { + if !predicate(ref) { + newRefs = append(newRefs, ref) + } + } + + if len(newRefs) == len(oldRefs) { + return nil + } + + obj.SetOwnerReferences(newRefs) + + // Update the object in the cluster + if err := cli.Update(ctx, obj); err != nil { + return fmt.Errorf( + "failed to remove owner references from object %s/%s with gvk %s: %w", + obj.GetNamespace(), + obj.GetName(), + obj.GetObjectKind().GroupVersionKind(), + err, + ) + } + + return nil +} diff --git a/pkg/resources/resources_test.go b/pkg/resources/resources_test.go index 88d4d589539..11289e4b199 100644 --- a/pkg/resources/resources_test.go +++ b/pkg/resources/resources_test.go @@ -1,16 +1,32 @@ package resources_test import ( + "context" "errors" + "path/filepath" "testing" + "github.com/onsi/gomega/gstruct" + "github.com/rs/xid" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/envtest" + + componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" + dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" + dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" + odhCli "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" + "github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil" . "github.com/onsi/gomega" ) @@ -136,3 +152,93 @@ func TestEnsureGroupVersionKind(t *testing.T) { g.Expect(err.Error()).To(ContainSubstring("failed to get GVK")) }) } + +func TestRemoveOwnerRef(t *testing.T) { + g := NewWithT(t) + s := runtime.NewScheme() + + ctx := context.Background() + ns := xid.New().String() + + utilruntime.Must(corev1.AddToScheme(s)) + utilruntime.Must(appsv1.AddToScheme(s)) + utilruntime.Must(apiextensionsv1.AddToScheme(s)) + utilruntime.Must(componentApi.AddToScheme(s)) + utilruntime.Must(dsciv1.AddToScheme(s)) + utilruntime.Must(dscv1.AddToScheme(s)) + utilruntime.Must(rbacv1.AddToScheme(s)) + + projectDir, err := envtestutil.FindProjectRoot() + g.Expect(err).NotTo(HaveOccurred()) + + envTest := &envtest.Environment{ + CRDInstallOptions: envtest.CRDInstallOptions{ + Scheme: s, + Paths: []string{ + filepath.Join(projectDir, "config", "crd", "bases"), + }, + ErrorIfPathMissing: true, + CleanUpAfterUse: false, + }, + } + + t.Cleanup(func() { + _ = envTest.Stop() + }) + + cfg, err := envTest.Start() + g.Expect(err).NotTo(HaveOccurred()) + + envTestClient, err := client.New(cfg, client.Options{Scheme: s}) + g.Expect(err).NotTo(HaveOccurred()) + + cli, err := odhCli.NewFromConfig(cfg, envTestClient) + g.Expect(err).NotTo(HaveOccurred()) + + err = cli.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}) + g.Expect(err).ToNot(HaveOccurred()) + + cm1 := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm1", Namespace: ns}} + cm1.SetGroupVersionKind(gvk.ConfigMap) + + err = cli.Create(ctx, cm1) + g.Expect(err).ToNot(HaveOccurred()) + + cm2 := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm2", Namespace: ns}} + cm2.SetGroupVersionKind(gvk.ConfigMap) + + err = cli.Create(ctx, cm2) + g.Expect(err).ToNot(HaveOccurred()) + + // Create a ConfigMap with OwnerReferences + configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "test-configmap", Namespace: ns}} + + err = controllerutil.SetOwnerReference(cm1, configMap, s) + g.Expect(err).ToNot(HaveOccurred()) + err = controllerutil.SetOwnerReference(cm2, configMap, s) + g.Expect(err).ToNot(HaveOccurred()) + + err = cli.Create(ctx, configMap) + g.Expect(err).ToNot(HaveOccurred()) + + predicate := func(ref metav1.OwnerReference) bool { + return ref.Name == cm1.Name + } + + err = resources.RemoveOwnerReferences(ctx, cli, configMap, predicate) + g.Expect(err).ToNot(HaveOccurred()) + + updatedConfigMap := &corev1.ConfigMap{} + err = cli.Get(ctx, client.ObjectKeyFromObject(configMap), updatedConfigMap) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(updatedConfigMap.GetOwnerReferences()).Should(And( + HaveLen(1), + HaveEach(gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{ + "Name": Equal(cm2.Name), + "APIVersion": Equal(gvk.ConfigMap.GroupVersion().String()), + "Kind": Equal(gvk.ConfigMap.Kind), + "UID": Equal(cm2.UID), + })), + )) +} diff --git a/tests/e2e/kserve_test.go b/tests/e2e/kserve_test.go index 715cc6e81f9..aa12f85a4d1 100644 --- a/tests/e2e/kserve_test.go +++ b/tests/e2e/kserve_test.go @@ -2,16 +2,21 @@ package e2e_test import ( "encoding/json" + "errors" "fmt" "testing" + "github.com/rs/xid" "github.com/stretchr/testify/require" k8serr "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" + featuresv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/modelcontroller" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" @@ -31,13 +36,12 @@ func kserveTestSuite(t *testing.T) { ComponentTestCtx: ct, } - // TODO: removed once we know what's left on the cluster that's causing the tests - // to fail because of "existing KNativeServing resource was found" err = componentCtx.setUpServerless(t) require.NoError(t, err) t.Run("Validate component enabled", componentCtx.ValidateComponentEnabled) t.Run("Validate component spec", componentCtx.validateSpec) + t.Run("Validate FeatureTrackers", componentCtx.validateFeatureTrackers) t.Run("Validate model controller", componentCtx.validateModelControllerInstance) t.Run("Validate operands have OwnerReferences", componentCtx.ValidateOperandsOwnerReferences) t.Run("Validate default certs", componentCtx.validateDefaultCertsAvailable) @@ -51,6 +55,8 @@ type KserveTestCtx struct { //nolint:thelper func (c *KserveTestCtx) setUpServerless(t *testing.T) error { + // TODO: removed once we know what's left on the cluster that's causing the tests + // to fail because of "existing KNativeServing resource was found" ksl := unstructured.UnstructuredList{} ksl.SetGroupVersionKind(gvk.KnativeServing) @@ -75,6 +81,24 @@ func (c *KserveTestCtx) setUpServerless(t *testing.T) error { } } + ft := &featuresv1.FeatureTracker{} + ft.SetName(c.ApplicationNamespace + "-serverless-serving-deployment") + + if _, err := controllerutil.CreateOrUpdate(c.Context(), c.Client(), ft, func() error { + dsc, err := c.GetDSC() + if err != nil { + return err + } + if err := controllerutil.SetOwnerReference(dsc, ft, c.Client().Scheme()); err != nil { + return err + } + ft.Spec.Source.Name = xid.New().String() + + return nil + }); err != nil { + return errors.New("error creating pre-existing FeatureTracker") + } + return nil } @@ -96,6 +120,49 @@ func (c *KserveTestCtx) validateSpec(t *testing.T) { )) } +func (c *KserveTestCtx) validateFeatureTrackers(t *testing.T) { + g := c.NewWithT(t) + ftName := types.NamespacedName{Name: c.ApplicationNamespace + "-serverless-serving-deployment"} + + g.Get(gvk.FeatureTracker, ftName).Eventually().Should(And( + jq.Match(`(.metadata.ownerReferences | length) == 1`), + jq.Match(`.metadata.ownerReferences[0].apiVersion == "%s"`, gvk.Kserve.GroupVersion().String()), + jq.Match(`.metadata.ownerReferences[0].kind == "%s"`, gvk.Kserve.Kind), + jq.Match(`.metadata.ownerReferences[0].blockOwnerDeletion == true`), + jq.Match(`.metadata.ownerReferences[0].controller == true`), + )) + + dsc, err := c.GetDSC() + g.Expect(err).NotTo(HaveOccurred()) + + g.Update( + gvk.FeatureTracker, + ftName, + func(obj *unstructured.Unstructured) error { + if err := controllerutil.SetOwnerReference(dsc, obj, c.Client().Scheme()); err != nil { + return err + } + + // trigger reconciliation as spec changes + if err = unstructured.SetNestedField(obj.Object, xid.New().String(), "spec", "source", "name"); err != nil { + return err + } + + return nil + }, + ).Eventually().Should(And( + jq.Match(`(.metadata.ownerReferences | length) == 2`), + )) + + g.Get(gvk.FeatureTracker, ftName).Eventually().Should(And( + jq.Match(`(.metadata.ownerReferences | length) == 1`), + jq.Match(`.metadata.ownerReferences[0].apiVersion == "%s"`, gvk.Kserve.GroupVersion().String()), + jq.Match(`.metadata.ownerReferences[0].kind == "%s"`, gvk.Kserve.Kind), + jq.Match(`.metadata.ownerReferences[0].blockOwnerDeletion == true`), + jq.Match(`.metadata.ownerReferences[0].controller == true`), + )) +} + func (c *KserveTestCtx) validateModelControllerInstance(t *testing.T) { g := c.NewWithT(t)