Improve Kserve's FeatureTraker handing (opendatahub-io#1562)

* 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 - Make it possible to set FT's generated resources OwnerReference as Controller reference so the kubernetes garbage collector can block FT deletion till the resources have been deleted - Add an Kserver reconciler action to remove legacy ownership on DSCI/DSC is any and related e2e tests * Update RBACs * Update RBACs * Fix findings * Update pkg/cluster/meta.go Co-authored-by: Gerard Ryan <[email protected]> * Fix renaming * Remove unrelated methods --------- Co-authored-by: Gerard Ryan <[email protected]> (cherry picked from commit bd00c9b)
zdtsw-forking · Feb 3, 2025 · 27dc833 · 27dc833
1 parent aa8e640
commit 27dc833
Show file tree

Hide file tree

Showing 19 changed files with 409 additions and 195 deletions.
diff --git a/bundle/manifests/rhods-operator.clusterserviceversion.yaml b/bundle/manifests/rhods-operator.clusterserviceversion.yaml
@@ -852,6 +852,14 @@ spec:
           - patch
           - update
           - watch
+        - apiGroups:
+          - features.opendatahub.io
+          resources:
+          - featuretrackers/finalizers
+          verbs:
+          - get
+          - patch
+          - update
         - apiGroups:
           - features.opendatahub.io
           resources:
@@ -1074,6 +1082,14 @@ spec:
           - knativeservings
           verbs:
           - '*'
+        - apiGroups:
+          - operator.knative.dev
+          resources:
+          - knativeservings/finalizers
+          verbs:
+          - get
+          - patch
+          - update
         - apiGroups:
           - operator.openshift.io
           resources:

diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -508,6 +508,14 @@ rules:
   - patch
   - update
   - watch
+- apiGroups:
+  - features.opendatahub.io
+  resources:
+  - featuretrackers/finalizers
+  verbs:
+  - get
+  - patch
+  - update
 - apiGroups:
   - features.opendatahub.io
   resources:
@@ -730,6 +738,14 @@ rules:
   - knativeservings
   verbs:
   - '*'
+- apiGroups:
+  - operator.knative.dev
+  resources:
+  - knativeservings/finalizers
+  verbs:
+  - get
+  - patch
+  - update
 - apiGroups:
   - operator.openshift.io
   resources:

diff --git 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"
@@ -69,17 +68,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{}).
@@ -162,6 +151,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
@@ -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
@@ -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,7 @@ func ownedViaFT(cli client.Client) handler.MapFunc {
 		return []reconcile.Request{}
 	}
 }
+
+func isLegacyOwnerRef(or metav1.OwnerReference) bool {
+	return or.APIVersion == gvk.DataScienceCluster.GroupVersion().String() && or.Kind == gvk.DataScienceCluster.Kind
+}
diff --git a/controllers/datasciencecluster/kubebuilder_rbac.go b/controllers/datasciencecluster/kubebuilder_rbac.go
@@ -239,6 +239,7 @@ package datasciencecluster
 /* Serverless prerequisite */
 // +kubebuilder:rbac:groups="networking.istio.io",resources=gateways,verbs=*
 // +kubebuilder:rbac:groups="operator.knative.dev",resources=knativeservings,verbs=*
+// +kubebuilder:rbac:groups="operator.knative.dev",resources=knativeservings/finalizers,verbs=update;patch;get
 // +kubebuilder:rbac:groups="config.openshift.io",resources=ingresses,verbs=get
 
 // WB

diff --git a/controllers/dscinitialization/kubebuilder_rbac.go b/controllers/dscinitialization/kubebuilder_rbac.go
@@ -5,6 +5,7 @@ package dscinitialization
 // +kubebuilder:rbac:groups="dscinitialization.opendatahub.io",resources=dscinitializations,verbs=get;list;watch;create;update;patch;delete;deletecollection
 // +kubebuilder:rbac:groups="features.opendatahub.io",resources=featuretrackers,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups="features.opendatahub.io",resources=featuretrackers/status,verbs=get;update;patch;delete
+// +kubebuilder:rbac:groups="features.opendatahub.io",resources=featuretrackers/finalizers,verbs=update;patch;get
 
 /* Auth */
 // +kubebuilder:rbac:groups="config.openshift.io",resources=authentications,verbs=get;watch;list

diff --git 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
@@ -37,6 +37,12 @@ func OwnedBy(owner metav1.Object, scheme *runtime.Scheme) MetaOptions {
 	}
 }
 
+func ControlledBy(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
@@ -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
@@ -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
-}