Merge pull request #621 from adrianchiris/set-owner-ref-to-config

[Remove Default Objs  2/4] Set OwnerReference to `default` SriovOperatorConfig

controllers/helper.go

@@ -82,7 +82,6 @@ func syncPluginDaemonObjs(ctx context.Context,
 	client k8sclient.Client,
 	scheme *runtime.Scheme,
 	dc *sriovnetworkv1.SriovOperatorConfig,
-	dp *sriovnetworkv1.SriovNetworkNodePolicy,
 	pl *sriovnetworkv1.SriovNetworkNodePolicyList) error {
 	logger := log.Log.WithName("syncPluginDaemonObjs")
 	logger.V(1).Info("Start to sync sriov daemons objects")
@@ -129,7 +128,7 @@ func syncPluginDaemonObjs(ctx context.Context,
 				return err
 			}
 		}
-		err = syncDsObject(ctx, client, scheme, dp, pl, obj)
+		err = syncDsObject(ctx, client, scheme, dc, pl, obj)
 		if err != nil {
 			logger.Error(err, "Couldn't sync SR-IoV daemons objects")
 			return err
@@ -146,13 +145,13 @@ func deleteK8sResource(ctx context.Context, client k8sclient.Client, in *uns.Uns
 	return nil
 }
 
-func syncDsObject(ctx context.Context, client k8sclient.Client, scheme *runtime.Scheme, dp *sriovnetworkv1.SriovNetworkNodePolicy, pl *sriovnetworkv1.SriovNetworkNodePolicyList, obj *uns.Unstructured) error {
+func syncDsObject(ctx context.Context, client k8sclient.Client, scheme *runtime.Scheme, dc *sriovnetworkv1.SriovOperatorConfig, pl *sriovnetworkv1.SriovNetworkNodePolicyList, obj *uns.Unstructured) error {
 	logger := log.Log.WithName("syncDsObject")
 	kind := obj.GetKind()
 	logger.V(1).Info("Start to sync Objects", "Kind", kind)
 	switch kind {
 	case constants.ServiceAccount, constants.Role, constants.RoleBinding:
-		if err := controllerutil.SetControllerReference(dp, obj, scheme); err != nil {
+		if err := controllerutil.SetControllerReference(dc, obj, scheme); err != nil {
 			return err
 		}
 		if err := apply.ApplyObject(ctx, client, obj); err != nil {
@@ -166,7 +165,7 @@ func syncDsObject(ctx context.Context, client k8sclient.Client, scheme *runtime.
 			logger.Error(err, "Fail to convert to DaemonSet")
 			return err
 		}
-		err = syncDaemonSet(ctx, client, scheme, dp, pl, ds)
+		err = syncDaemonSet(ctx, client, scheme, dc, pl, ds)
 		if err != nil {
 			logger.Error(err, "Fail to sync DaemonSet", "Namespace", ds.Namespace, "Name", ds.Name)
 			return err
@@ -175,7 +174,7 @@ func syncDsObject(ctx context.Context, client k8sclient.Client, scheme *runtime.
 	return nil
 }
 
-func syncDaemonSet(ctx context.Context, client k8sclient.Client, scheme *runtime.Scheme, cr *sriovnetworkv1.SriovNetworkNodePolicy, pl *sriovnetworkv1.SriovNetworkNodePolicyList, in *appsv1.DaemonSet) error {
+func syncDaemonSet(ctx context.Context, client k8sclient.Client, scheme *runtime.Scheme, dc *sriovnetworkv1.SriovOperatorConfig, pl *sriovnetworkv1.SriovNetworkNodePolicyList, in *appsv1.DaemonSet) error {
 	logger := log.Log.WithName("syncDaemonSet")
 	logger.V(1).Info("Start to sync DaemonSet", "Namespace", in.Namespace, "Name", in.Name)
 	var err error
@@ -185,7 +184,7 @@ func syncDaemonSet(ctx context.Context, client k8sclient.Client, scheme *runtime
 			return err
 		}
 	}
-	if err = controllerutil.SetControllerReference(cr, in, scheme); err != nil {
+	if err = controllerutil.SetControllerReference(dc, in, scheme); err != nil {
 		return err
 	}
 	ds := &appsv1.DaemonSet{}
@@ -207,7 +206,13 @@ func syncDaemonSet(ctx context.Context, client k8sclient.Client, scheme *runtime
 		// DeepDerivative checks for changes only comparing non-zero fields in the source struct.
 		// This skips default values added by the api server.
 		// References in https://github.com/kubernetes-sigs/kubebuilder/issues/592#issuecomment-625738183
-		if equality.Semantic.DeepDerivative(in.Spec, ds.Spec) {
+
+		// Note(Adrianc): we check Equality of OwnerReference as we changed sriov-device-plugin owner ref
+		// from SriovNetworkNodePolicy to SriovOperatorConfig, hence even if there is no change in spec,
+		// we need to update the obj's owner reference.
+
+		if equality.Semantic.DeepEqual(in.OwnerReferences, ds.OwnerReferences) &&
+			equality.Semantic.DeepDerivative(in.Spec, ds.Spec) {
 			// DeepDerivative has issue detecting nodeAffinity change
 			// https://bugzilla.redhat.com/show_bug.cgi?id=1914066
 			// DeepDerivative doesn't detect changes in containers args section

controllers/sriovnetworknodepolicy_controller.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"sort"
 	"strings"
 	"time"
@@ -149,15 +150,15 @@ func (r *SriovNetworkNodePolicyReconciler) Reconcile(ctx context.Context, req ct
 	// Sort the policies with priority, higher priority ones is applied later
 	sort.Sort(sriovnetworkv1.ByPriority(policyList.Items))
 	// Sync SriovNetworkNodeState objects
-	if err = r.syncAllSriovNetworkNodeStates(ctx, defaultPolicy, policyList, nodeList); err != nil {
+	if err = r.syncAllSriovNetworkNodeStates(ctx, defaultOpConf, policyList, nodeList); err != nil {
 		return reconcile.Result{}, err
 	}
 	// Sync Sriov device plugin ConfigMap object
 	if err = r.syncDevicePluginConfigMap(ctx, defaultOpConf, policyList, nodeList); err != nil {
 		return reconcile.Result{}, err
 	}
 	// Render and sync Daemon objects
-	if err = syncPluginDaemonObjs(ctx, r.Client, r.Scheme, defaultOpConf, defaultPolicy, policyList); err != nil {
+	if err = syncPluginDaemonObjs(ctx, r.Client, r.Scheme, defaultOpConf, policyList); err != nil {
 		return reconcile.Result{}, err
 	}
 
@@ -255,7 +256,7 @@ func (r *SriovNetworkNodePolicyReconciler) syncDevicePluginConfigMap(ctx context
 	return nil
 }
 
-func (r *SriovNetworkNodePolicyReconciler) syncAllSriovNetworkNodeStates(ctx context.Context, np *sriovnetworkv1.SriovNetworkNodePolicy, npl *sriovnetworkv1.SriovNetworkNodePolicyList, nl *corev1.NodeList) error {
+func (r *SriovNetworkNodePolicyReconciler) syncAllSriovNetworkNodeStates(ctx context.Context, dc *sriovnetworkv1.SriovOperatorConfig, npl *sriovnetworkv1.SriovNetworkNodePolicyList, nl *corev1.NodeList) error {
 	logger := log.Log.WithName("syncAllSriovNetworkNodeStates")
 	logger.V(1).Info("Start to sync all SriovNetworkNodeState custom resource")
 	found := &corev1.ConfigMap{}
@@ -269,7 +270,7 @@ func (r *SriovNetworkNodePolicyReconciler) syncAllSriovNetworkNodeStates(ctx con
 		ns.Namespace = vars.Namespace
 		j, _ := json.Marshal(ns)
 		logger.V(2).Info("SriovNetworkNodeState CR", "content", j)
-		if err := r.syncSriovNetworkNodeState(ctx, np, npl, ns, &node, utils.HashConfigMap(found)); err != nil {
+		if err := r.syncSriovNetworkNodeState(ctx, dc, npl, ns, &node, utils.HashConfigMap(found)); err != nil {
 			logger.Error(err, "Fail to sync", "SriovNetworkNodeState", ns.Name)
 			return err
 		}
@@ -303,11 +304,11 @@ func (r *SriovNetworkNodePolicyReconciler) syncAllSriovNetworkNodeStates(ctx con
 	return nil
 }
 
-func (r *SriovNetworkNodePolicyReconciler) syncSriovNetworkNodeState(ctx context.Context, np *sriovnetworkv1.SriovNetworkNodePolicy, npl *sriovnetworkv1.SriovNetworkNodePolicyList, ns *sriovnetworkv1.SriovNetworkNodeState, node *corev1.Node, cksum string) error {
+func (r *SriovNetworkNodePolicyReconciler) syncSriovNetworkNodeState(ctx context.Context, dc *sriovnetworkv1.SriovOperatorConfig, npl *sriovnetworkv1.SriovNetworkNodePolicyList, ns *sriovnetworkv1.SriovNetworkNodeState, node *corev1.Node, cksum string) error {
 	logger := log.Log.WithName("syncSriovNetworkNodeState")
 	logger.V(1).Info("Start to sync SriovNetworkNodeState", "Name", ns.Name, "cksum", cksum)
 
-	if err := controllerutil.SetControllerReference(np, ns, r.Scheme); err != nil {
+	if err := controllerutil.SetControllerReference(dc, ns, r.Scheme); err != nil {
 		return err
 	}
 	found := &sriovnetworkv1.SriovNetworkNodeState{}
@@ -334,6 +335,7 @@ func (r *SriovNetworkNodePolicyReconciler) syncSriovNetworkNodeState(ctx context
 		logger.V(1).Info("SriovNetworkNodeState already exists, updating")
 		newVersion := found.DeepCopy()
 		newVersion.Spec = ns.Spec
+		newVersion.OwnerReferences = ns.OwnerReferences
 
 		// Previous Policy Priority(ppp) records the priority of previous evaluated policy in node policy list.
 		// Since node policy list is already sorted with priority number, comparing current priority with ppp shall
@@ -359,7 +361,11 @@ func (r *SriovNetworkNodePolicyReconciler) syncSriovNetworkNodeState(ctx context
 			}
 		}
 		newVersion.Spec.DpConfigVersion = cksum
-		if equality.Semantic.DeepEqual(newVersion.Spec, found.Spec) {
+		// Note(adrianc): we check same ownerReferences since SriovNetworkNodeState
+		// was owned by a default SriovNetworkNodePolicy. if we encounter a descripancy
+		// we need to update.
+		if reflect.DeepEqual(newVersion.OwnerReferences, found.OwnerReferences) &&
+			equality.Semantic.DeepEqual(newVersion.Spec, found.Spec) {
 			logger.V(1).Info("SriovNetworkNodeState did not change, not updating")
 			return nil
 		}

controllers/sriovoperatorconfig_controller.go

@@ -120,7 +120,7 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl.
 		return reconcile.Result{}, err
 	}
 
-	if err = syncPluginDaemonObjs(ctx, r.Client, r.Scheme, defaultConfig, defaultPolicy, policyList); err != nil {
+	if err = syncPluginDaemonObjs(ctx, r.Client, r.Scheme, defaultConfig, policyList); err != nil {
 		return reconcile.Result{}, err
 	}
 

controllers/sriovoperatorconfig_controller_test.go

@@ -14,6 +14,7 @@ import (
 	. "github.com/onsi/gomega"
 
 	sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1"
+	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
 	constants "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
 	mock_platforms "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/platforms/mock"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/platforms/openshift"
@@ -56,6 +57,21 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
 			Expect(err).ToNot(HaveOccurred())
 		})
 
+		otherPolicy := &sriovnetworkv1.SriovNetworkNodePolicy{}
+		otherPolicy.SetNamespace(testNamespace)
+		otherPolicy.SetName("other-policy")
+		otherPolicy.Spec = sriovnetworkv1.SriovNetworkNodePolicySpec{
+			NumVfs:       5,
+			NodeSelector: map[string]string{"foo": "bar"},
+			NicSelector:  sriovnetworkv1.SriovNetworkNicSelector{},
+			Priority:     20,
+		}
+		Expect(k8sClient.Create(context.Background(), otherPolicy)).ToNot(HaveOccurred())
+		DeferCleanup(func() {
+			err := k8sClient.Delete(context.Background(), otherPolicy)
+			Expect(err).ToNot(HaveOccurred())
+		})
+
 		// setup controller manager
 		By("Setup controller manager")
 		k8sManager, err := setupK8sManagerForTest()
@@ -125,10 +141,14 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
 				daemonSet := &appsv1.DaemonSet{}
 				err := util.WaitForNamespacedObject(daemonSet, k8sClient, testNamespace, dsName, util.RetryInterval, util.APITimeout)
 				Expect(err).NotTo(HaveOccurred())
+				Expect(daemonSet.OwnerReferences).To(HaveLen(1))
+				Expect(daemonSet.OwnerReferences[0].Kind).To(Equal("SriovOperatorConfig"))
+				Expect(daemonSet.OwnerReferences[0].Name).To(Equal(consts.DefaultConfigName))
 			},
 			Entry("operator-webhook", "operator-webhook"),
 			Entry("network-resources-injector", "network-resources-injector"),
 			Entry("sriov-network-config-daemon", "sriov-network-config-daemon"),
+			Entry("sriov-device-plugin", "sriov-device-plugin"),
 		)
 
 		It("should be able to turn network-resources-injector on/off", func() {