From 25a291e9e17ffbe260c02afea0a8f8c56fb13093 Mon Sep 17 00:00:00 2001 From: William Zhao Date: Tue, 29 Aug 2023 17:27:35 -0400 Subject: [PATCH 1/3] Move Config Daemon Node Selector tests from Pending to Running Signed-off-by: William Zhao --- controllers/sriovoperatorconfig_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/sriovoperatorconfig_controller_test.go b/controllers/sriovoperatorconfig_controller_test.go index ffef04c34..86be5ccce 100644 --- a/controllers/sriovoperatorconfig_controller_test.go +++ b/controllers/sriovoperatorconfig_controller_test.go @@ -151,7 +151,7 @@ var _ = Describe("Operator", func() { Expect(err).NotTo(HaveOccurred()) }) - PIt("should be able to update the node selector of sriov-network-config-daemon", func() { + It("should be able to update the node selector of sriov-network-config-daemon", func() { By("specify the configDaemonNodeSelector") config := &sriovnetworkv1.SriovOperatorConfig{} err := util.WaitForNamespacedObject(config, k8sClient, testNamespace, "default", interval, timeout) From bf30003a3d9db7431864b7c8b2ad0a4b2335808b Mon Sep 17 00:00:00 2001 From: William Zhao Date: Tue, 29 Aug 2023 17:59:56 -0400 Subject: [PATCH 2/3] Add test for multiple updates to the node selector of the config daemon It was found that updating the node selector may not be applied. This test ensures that we catch any problems with the node selector being modified. Signed-off-by: William Zhao --- .../sriovoperatorconfig_controller_test.go | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/controllers/sriovoperatorconfig_controller_test.go b/controllers/sriovoperatorconfig_controller_test.go index 86be5ccce..c925324b9 100644 --- a/controllers/sriovoperatorconfig_controller_test.go +++ b/controllers/sriovoperatorconfig_controller_test.go @@ -171,5 +171,27 @@ var _ = Describe("Operator", func() { }, timeout*10, interval).Should(Equal(config.Spec.ConfigDaemonNodeSelector)) }) + It("should be able to do multiple updates to the node selector of sriov-network-config-daemon", func() { + By("changing the configDaemonNodeSelector") + config := &sriovnetworkv1.SriovOperatorConfig{} + err := util.WaitForNamespacedObject(config, k8sClient, testNamespace, "default", interval, timeout) + Expect(err).NotTo(HaveOccurred()) + config.Spec.ConfigDaemonNodeSelector = map[string]string{"labelA": "", "labelB": "", "labelC": ""} + err = k8sClient.Update(goctx.TODO(), config) + Expect(err).NotTo(HaveOccurred()) + config.Spec.ConfigDaemonNodeSelector = map[string]string{"labelA": "", "labelB": ""} + err = k8sClient.Update(goctx.TODO(), config) + Expect(err).NotTo(HaveOccurred()) + + daemonSet := &appsv1.DaemonSet{} + Eventually(func() map[string]string { + err := k8sClient.Get(goctx.TODO(), types.NamespacedName{Name: "sriov-network-config-daemon", Namespace: testNamespace}, daemonSet) + if err != nil { + return nil + } + return daemonSet.Spec.Template.Spec.NodeSelector + }, timeout*10, interval).Should(Equal(config.Spec.ConfigDaemonNodeSelector)) + }) + }) }) From 5c1b0afbf468034085047d5faf4130dbb6829b23 Mon Sep 17 00:00:00 2001 From: William Zhao Date: Tue, 1 Aug 2023 16:15:37 -0400 Subject: [PATCH 3/3] Fix Config Daemon node selector When node selectors are added then removed via "configDaemonNodeSelector" via the sriovoperatorconfigs CRD, certain combinations will not trigger an update due to the ordering of arguments into "DeepDerivative" from the reflect library. This is an example combination of setting "configDaemonNodeSelector" that would make it such that the sriov-config-daemon daemon set's nodeSelector to become out of sync with the original "DeepDerivative" argument order: Step 1) Create 3 labels in node selector: configDaemonNodeSelector = {"labelA": "", "labelB": "", "labelC": ""} config-daemon DS NodeSelector = {"labelA": "", "labelB": "", "labelC": ""} Step 2) Remove 1 label in node selector without making changes to the other labels: configDaemonNodeSelector = {"labelA": "", "labelB": ""} config-daemon DS NodeSelector = {"labelA": "", "labelB": "", "labelC": ""} (Out of Sync) "DeepDerivative" assumes that the left argument is the original (v1) and the right argument is the updated (v2). For maps it only checks if v1 > v2 in length: case reflect.Map: ... if v1.Len() > v2.Len() { return false } ... This commit just reverts changes from commit: "661a65b8e1aee6339037948732f75d06ceb91611" Use DeepDerivative to compare the kube object content Such that we react to changes to node selectors properly. Signed-off-by: William Zhao --- pkg/apply/apply.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/apply/apply.go b/pkg/apply/apply.go index afebdf405..f5560e993 100644 --- a/pkg/apply/apply.go +++ b/pkg/apply/apply.go @@ -90,11 +90,11 @@ func ApplyObject(ctx context.Context, client k8sclient.Client, obj *uns.Unstruct if err := MergeObjectForUpdate(existing, obj); err != nil { return errors.Wrapf(err, "could not merge object %s with existing", objDesc) } - if !equality.Semantic.DeepDerivative(obj, existing) { + if !equality.Semantic.DeepEqual(existing, obj) { if err := client.Update(ctx, obj); err != nil { return errors.Wrapf(err, "could not update object %s", objDesc) } else { - log.Printf("update was successful") + log.Printf("update was successful %s", objDesc) } }