networks: Use owner annotation instead of finalizers

Using finalizers to remove dependant objects might
lock a user if the operator is not running.

Using the owner reference to garbage collect dangling
NetworkAttachmentDefinition might clean the above use case.

Signed-off-by: Andrea Panattoni <[email protected]>

Author: zeeke

api/v1/helper.go

@@ -19,6 +19,7 @@ import (
 	uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	intstrutil "k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/kubernetes"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
@@ -28,7 +29,7 @@ import (
 
 const (
 	LASTNETWORKNAMESPACE        = "operator.sriovnetwork.openshift.io/last-network-namespace"
-	NETATTDEFFINALIZERNAME      = "netattdef.finalizers.sriovnetwork.openshift.io"
+	NETATTDEFFINALIZERNAME      = "netattdef.finalizers.sriovnetwork.openshift.io" // Deprecated. Keeping this to migrate old cluster through upgrades
 	POOLCONFIGFINALIZERNAME     = "poolconfig.finalizers.sriovnetwork.openshift.io"
 	OPERATORCONFIGFINALIZERNAME = "operatorconfig.finalizers.sriovnetwork.openshift.io"
 	ESwithModeLegacy            = "legacy"
@@ -651,7 +652,7 @@ func (cr *SriovIBNetwork) RenderNetAttDef() (*uns.Unstructured, error) {
 	} else {
 		data.Data["SriovNetworkNamespace"] = cr.Spec.NetworkNamespace
 	}
-	data.Data["Owner"] = cr.GroupVersionKind().GroupKind().String() + "/" + cr.GetNamespace() + "/" + cr.Name
+	data.Data["Owner"] = OwnerRefToString(cr)
 	data.Data["SriovCniResourceName"] = os.Getenv("RESOURCE_PREFIX") + "/" + cr.Spec.ResourceName
 
 	data.Data["StateConfigured"] = true
@@ -720,7 +721,7 @@ func (cr *SriovNetwork) RenderNetAttDef() (*uns.Unstructured, error) {
 	} else {
 		data.Data["SriovNetworkNamespace"] = cr.Spec.NetworkNamespace
 	}
-	data.Data["Owner"] = cr.GroupVersionKind().GroupKind().String() + "/" + cr.GetNamespace() + "/" + cr.Name
+	data.Data["Owner"] = OwnerRefToString(cr)
 	data.Data["SriovCniResourceName"] = os.Getenv("RESOURCE_PREFIX") + "/" + cr.Spec.ResourceName
 	data.Data["SriovCniVlan"] = cr.Spec.Vlan
 
@@ -839,7 +840,7 @@ func (cr *OVSNetwork) RenderNetAttDef() (*uns.Unstructured, error) {
 	} else {
 		data.Data["NetworkNamespace"] = cr.Spec.NetworkNamespace
 	}
-	data.Data["Owner"] = cr.GroupVersionKind().GroupKind().String() + "/" + cr.GetNamespace() + "/" + cr.Name
+	data.Data["Owner"] = OwnerRefToString(cr)
 	data.Data["CniResourceName"] = os.Getenv("RESOURCE_PREFIX") + "/" + cr.Spec.ResourceName
 
 	if cr.Spec.Capabilities == "" {
@@ -997,3 +998,27 @@ func (s *SriovNetworkNodeState) ResetKeepUntilTime() bool {
 	s.SetAnnotations(annotations)
 	return true
 }
+
+func OwnerRefToString(cr client.Object) string {
+	return cr.GetObjectKind().GroupVersionKind().GroupKind().String() + "/" + cr.GetNamespace() + "/" + cr.GetName()
+}
+
+func StringToOwnerRef(ownerRef string) (client.Object, string, string, error) {
+	chunks := strings.Split(ownerRef, "/")
+	if len(chunks) != 3 {
+		return nil, "", "", fmt.Errorf("invalid ownerRef %s", ownerRef)
+	}
+	groupKind, namespace, name := chunks[0], chunks[1], chunks[2]
+	switch groupKind {
+	case "SriovNetwork.sriovnetwork.openshift.io":
+		return &SriovNetwork{}, namespace, name, nil
+
+	case "SriovIBNetwork.sriovnetwork.openshift.io":
+		return &SriovIBNetwork{}, namespace, name, nil
+
+	case "OVSNetwork.sriovnetwork.openshift.io":
+		return &OVSNetwork{}, namespace, name, nil
+	}
+
+	return nil, "", "", fmt.Errorf("unknown ownerRef groupKind %s", groupKind)
+}

controllers/generic_network_controller.go

@@ -90,7 +90,12 @@ func (r *genericNetworkReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		// Request object not found, could have been deleted after reconcile request.
 		// Owned objects are automatically garbage collected. For additional cleanup logic use finalizers.
 		// Return and don't requeue
-		return reconcile.Result{}, nil
+		return reconcile.Result{}, r.garbageCollect(ctx)
+	}
+
+	err = r.cleanOldFinalizers(ctx, instance)
+	if err != nil {
+		return reconcile.Result{}, err
 	}
 
 	if instance.NetworkNamespace() != "" && instance.GetNamespace() != vars.Namespace {
@@ -104,20 +109,6 @@ func (r *genericNetworkReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return reconcile.Result{}, nil
 	}
 
-	// examine DeletionTimestamp to determine if object is under deletion
-	if instance.GetDeletionTimestamp().IsZero() {
-		// The object is not being deleted, so if it does not have our finalizer,
-		// then lets add the finalizer and update the object. This is equivalent
-		// registering our finalizer.
-		err = r.updateFinalizers(ctx, instance)
-		if err != nil {
-			return reconcile.Result{}, err
-		}
-	} else {
-		// The object is being deleted
-		err = r.cleanResourcesAndFinalizers(ctx, instance)
-		return reconcile.Result{}, err
-	}
 	raw, err := instance.RenderNetAttDef()
 	if err != nil {
 		return reconcile.Result{}, err
@@ -250,24 +241,6 @@ func (r *genericNetworkReconciler) namespaceHandlerCreate(ctx context.Context, e
 	})
 }
 
-// deleteNetAttDef deletes the generated net-att-def CR
-func (r *genericNetworkReconciler) deleteNetAttDef(ctx context.Context, cr NetworkCRInstance) error {
-	// Fetch the NetworkAttachmentDefinition instance
-	namespace := cr.NetworkNamespace()
-	if namespace == "" {
-		namespace = cr.GetNamespace()
-	}
-	instance := &netattdefv1.NetworkAttachmentDefinition{ObjectMeta: metav1.ObjectMeta{Name: cr.GetName(), Namespace: namespace}}
-	err := r.Delete(ctx, instance)
-	if err != nil {
-		if errors.IsNotFound(err) {
-			return nil
-		}
-		return err
-	}
-	return nil
-}
-
 // fetchObject tries to fectch the request object in its own namespace. If it is not found, then tries
 // to fetch from the operator's namespace
 func (r *genericNetworkReconciler) fetchObject(ctx context.Context, req ctrl.Request) (NetworkCRInstance, error) {
@@ -296,43 +269,66 @@ func (r *genericNetworkReconciler) fetchObject(ctx context.Context, req ctrl.Req
 	return nil, nil
 }
 
-func (r *genericNetworkReconciler) updateFinalizers(ctx context.Context, instance NetworkCRInstance) error {
-	if instance.GetNamespace() != vars.Namespace {
-		// If the resource is in a namespace different than the operator one, then the NetworkAttachmentDefinition will
-		// be created in the same namespace and its deletion can be handled by OwnerReferences. There is no need for finalizers
-		return nil
+// garbageCollect searches all NetworkAttachmentDefinition objects that has a `sriovnetwork.openshift.io/owner`
+// annotation pointing to non existing objects
+func (r *genericNetworkReconciler) garbageCollect(ctx context.Context) error {
+	logger := log.Log.WithName("garbageCollect")
+
+	netAttachDefs := &netattdefv1.NetworkAttachmentDefinitionList{}
+	err := r.Client.List(ctx, netAttachDefs)
+	if err != nil {
+		return fmt.Errorf("garbageCollect: failed to list NetworkAttachmentDefinition: %w", err)
 	}
 
-	instanceFinalizers := instance.GetFinalizers()
-	if !sriovnetworkv1.StringInArray(sriovnetworkv1.NETATTDEFFINALIZERNAME, instanceFinalizers) {
-		instance.SetFinalizers(append(instanceFinalizers, sriovnetworkv1.NETATTDEFFINALIZERNAME))
-		if err := r.Update(ctx, instance); err != nil {
-			return err
+	for _, netAttDef := range netAttachDefs.Items {
+		owner, ok := netAttDef.GetAnnotations()[consts.OwnerAnnotation]
+		if !ok {
+			continue
+		}
+
+		obj, namespace, name, err := sriovnetworkv1.StringToOwnerRef(owner)
+		if err != nil {
+			logger.Error(err, "bad value for `sriovnetwork.openshift.io/owner`", "owner", owner)
+			continue
+		}
+
+		err = r.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, obj)
+		if err != nil {
+			if !errors.IsNotFound(err) {
+				logger.Error(err, "failed to get owner object", "owner", owner)
+				continue
+			}
+
+			logger.Info("owner object not found. deleting NetworkAttachmentDefinition", "obj", netAttDef)
+
+			err := r.Delete(ctx, &netAttDef)
+			if err != nil {
+				logger.Error(err, "can't delete NetworkAttachmentDefinition", "obj", netAttDef)
+			}
 		}
 	}
 
 	return nil
 }
 
-func (r *genericNetworkReconciler) cleanResourcesAndFinalizers(ctx context.Context, instance NetworkCRInstance) error {
+func (r *genericNetworkReconciler) cleanOldFinalizers(ctx context.Context, instance NetworkCRInstance) error {
 	instanceFinalizers := instance.GetFinalizers()
 
-	if sriovnetworkv1.StringInArray(sriovnetworkv1.NETATTDEFFINALIZERNAME, instanceFinalizers) {
-		// our finalizer is present, so lets handle any external dependency
-		log.FromContext(ctx).Info("delete NetworkAttachmentDefinition CR", "Namespace", instance.NetworkNamespace(), "Name", instance.GetName())
-		if err := r.deleteNetAttDef(ctx, instance); err != nil {
-			// if fail to delete the external dependency here, return with error
-			// so that it can be retried
+	if !sriovnetworkv1.StringInArray(sriovnetworkv1.NETATTDEFFINALIZERNAME, instanceFinalizers) {
+		return nil
+	}
+
+	// remove our finalizer from the list and update it.
+	newFinalizers, found := sriovnetworkv1.RemoveString(sriovnetworkv1.NETATTDEFFINALIZERNAME, instanceFinalizers)
+	if found {
+		logger := log.Log.WithName("cleanFinalizers")
+
+		instance.SetFinalizers(newFinalizers)
+		logger.Info("Updating network instance to remove finalizer `netattdef.finalizers.sriovnetwork.openshift.io`", "obj", instance)
+		if err := r.Update(ctx, instance); err != nil {
 			return err
 		}
-		// remove our finalizer from the list and update it.
-		newFinalizers, found := sriovnetworkv1.RemoveString(sriovnetworkv1.NETATTDEFFINALIZERNAME, instanceFinalizers)
-		if found {
-			instance.SetFinalizers(newFinalizers)
-			if err := r.Update(ctx, instance); err != nil {
-				return err
-			}
-		}
 	}
+
 	return nil
 }

controllers/sriovnetwork_controller_test.go

@@ -381,6 +381,30 @@ var _ = Describe("SriovNetwork Controller", Ordered, func() {
 				Expect(netAttDef.ObjectMeta.OwnerReferences).To(ContainElement(expectedOwnerReference))
 			})
 		})
+
+		Context("Migration upgrades", func() {
+			It("should remove finalizer in existing SriovNetworks", func() {
+				cr := sriovnetworkv1.SriovNetwork{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "sriovnet-old",
+						Namespace: testNamespace,
+						Finalizers: []string{
+							sriovnetworkv1.NETATTDEFFINALIZERNAME,
+						},
+					},
+				}
+
+				err := k8sClient.Create(ctx, &cr)
+				Expect(err).NotTo(HaveOccurred())
+
+				Eventually(func(g Gomega) {
+					err := k8sClient.Get(ctx, types.NamespacedName{Namespace: cr.Namespace, Name: cr.Name}, &cr)
+					g.Expect(err).ToNot(HaveOccurred())
+					g.Expect(cr.Finalizers).To(BeEmpty())
+				}).WithTimeout(time.Second).Should(Succeed())
+			})
+		})
+
 	})
 })