From 94c57aa842e762291668169eb120bf5cfcbea125 Mon Sep 17 00:00:00 2001 From: adrianc Date: Mon, 5 Feb 2024 19:41:49 +0200 Subject: [PATCH 1/2] Change sriov device plugin owner ref Use default SriovOperatorConfig object instead of default SriovNetworkNodePolicy. This change is required as subsequent changes will drop the creation of the default policy. Signed-off-by: adrianc --- controllers/helper.go | 21 ++++++++++++------- .../sriovnetworknodepolicy_controller.go | 2 +- controllers/sriovoperatorconfig_controller.go | 2 +- .../sriovoperatorconfig_controller_test.go | 20 ++++++++++++++++++ 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/controllers/helper.go b/controllers/helper.go index b15c11d8b..f341f5f73 100644 --- a/controllers/helper.go +++ b/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 diff --git a/controllers/sriovnetworknodepolicy_controller.go b/controllers/sriovnetworknodepolicy_controller.go index 16c1b7449..fcc62dec2 100644 --- a/controllers/sriovnetworknodepolicy_controller.go +++ b/controllers/sriovnetworknodepolicy_controller.go @@ -157,7 +157,7 @@ func (r *SriovNetworkNodePolicyReconciler) Reconcile(ctx context.Context, req ct 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 } diff --git a/controllers/sriovoperatorconfig_controller.go b/controllers/sriovoperatorconfig_controller.go index 556bbf44d..c4eeec339 100644 --- a/controllers/sriovoperatorconfig_controller.go +++ b/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 } diff --git a/controllers/sriovoperatorconfig_controller_test.go b/controllers/sriovoperatorconfig_controller_test.go index 1db6f51c8..bbb777e45 100644 --- a/controllers/sriovoperatorconfig_controller_test.go +++ b/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() { From 5b1bdda1a6786b580c272c62de2e03d37022f078 Mon Sep 17 00:00:00 2001 From: adrianc Date: Tue, 6 Feb 2024 18:17:32 +0200 Subject: [PATCH 2/2] Change OwnerReference of SriovNetworkNodeState change SriovNetworkNodeState OwnerReference to point to the "default" SriovOperatorConfig as the current owner (default SriovNetworkNodePolicy) is going to be removed in subsequent commit Signed-off-by: adrianc --- .../sriovnetworknodepolicy_controller.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/controllers/sriovnetworknodepolicy_controller.go b/controllers/sriovnetworknodepolicy_controller.go index fcc62dec2..0b59097ff 100644 --- a/controllers/sriovnetworknodepolicy_controller.go +++ b/controllers/sriovnetworknodepolicy_controller.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "reflect" "sort" "strings" "time" @@ -149,7 +150,7 @@ 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 @@ -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 }