From ac116be0bad9089a56fb325e03513232ba93aa58 Mon Sep 17 00:00:00 2001 From: oraz Date: Thu, 29 Feb 2024 08:42:22 +0200 Subject: [PATCH 1/3] Separate nm status initalization from update Separation of initMaintenanceStatus logic to only nm status initialization and update is done only afterwards. It is mostly needed for UTs of the initMaintenanceStatus function --- controllers/nodemaintenance_controller.go | 10 ++++++---- controllers/nodemaintenance_controller_test.go | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/controllers/nodemaintenance_controller.go b/controllers/nodemaintenance_controller.go index 668f1c8e..600d8892 100644 --- a/controllers/nodemaintenance_controller.go +++ b/controllers/nodemaintenance_controller.go @@ -148,8 +148,11 @@ func (r *NodeMaintenanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ return emptyResult, nil } - err = initMaintenanceStatus(ctx, nm, drainer, r.Client) - if err != nil { + if err = initMaintenanceStatus(ctx, nm, drainer); err != nil { + r.logger.Error(err, "Failed to initalize NodeMaintenance status") + return r.onReconcileError(ctx, nm, drainer, err) + } + if err = r.Client.Status().Update(ctx, nm); err != nil { r.logger.Error(err, "Failed to update NodeMaintenance with \"Running\" status") return r.onReconcileError(ctx, nm, drainer, err) } @@ -390,7 +393,7 @@ func (r *NodeMaintenanceReconciler) fetchNode(ctx context.Context, drainer *drai return node, nil } -func initMaintenanceStatus(ctx context.Context, nm *v1beta1.NodeMaintenance, drainer *drain.Helper, r client.Client) error { +func initMaintenanceStatus(ctx context.Context, nm *v1beta1.NodeMaintenance, drainer *drain.Helper) error { if nm.Status.Phase == "" { nm.Status.Phase = v1beta1.MaintenanceRunning setLastUpdate(nm) @@ -412,7 +415,6 @@ func initMaintenanceStatus(ctx context.Context, nm *v1beta1.NodeMaintenance, dra return err } nm.Status.TotalPods = len(podlist.Items) - err = r.Status().Update(ctx, nm) return err } return nil diff --git a/controllers/nodemaintenance_controller_test.go b/controllers/nodemaintenance_controller_test.go index 5c747ba2..e78d2e36 100644 --- a/controllers/nodemaintenance_controller_test.go +++ b/controllers/nodemaintenance_controller_test.go @@ -66,7 +66,7 @@ var _ = Describe("Node Maintenance", func() { }) When("Status was initalized", func() { It("should be set for running with 2 pods to drain", func() { - Expect(initMaintenanceStatus(ctx, nm, drainer, r.Client)).To(HaveOccurred()) + Expect(initMaintenanceStatus(ctx, nm, drainer)).To(Succeed()) // status was initialized but the function will fail on updating the CR status, since we don't create a nm CR here Expect(nm.Status.Phase).To(Equal(v1beta1.MaintenanceRunning)) Expect(len(nm.Status.PendingPods)).To(Equal(2)) @@ -78,7 +78,7 @@ var _ = Describe("Node Maintenance", func() { }) When("Owner ref was set", func() { It("should be set properly", func() { - Expect(initMaintenanceStatus(ctx, nm, drainer, r.Client)).To(HaveOccurred()) + Expect(initMaintenanceStatus(ctx, nm, drainer)).To(Succeed()) // status was initialized but the function will fail on updating the CR status, since we don't create a nm CR here By("Setting owner ref for a modified nm CR") node := &corev1.Node{} @@ -101,7 +101,7 @@ var _ = Describe("Node Maintenance", func() { It("Should not modify the CR after initalization", func() { nmCopy := nm.DeepCopy() nmCopy.Status.Phase = v1beta1.MaintenanceFailed - Expect(initMaintenanceStatus(ctx, nmCopy, drainer, r.Client)).To(Succeed()) + Expect(initMaintenanceStatus(ctx, nmCopy, drainer)).To(Succeed()) // status was not initialized thus the function succeeds Expect(nmCopy.Status.Phase).To(Equal(v1beta1.MaintenanceFailed)) Expect(len(nmCopy.Status.PendingPods)).To(Equal(0)) From b84b512a31a1dd5b23ffa945dfddce115a7b6cdc Mon Sep 17 00:00:00 2001 From: oraz Date: Fri, 1 Mar 2024 08:08:23 +0200 Subject: [PATCH 2/3] Return a bool for whether we should update the CR status InitMaintenanceStatus initializes the nm CR status only at the first time, when phase is empty, and it returns a bool whether we should update the CR status and an error if it can't be initialized. Without this change an update to CR will happen at the beginning of every reconcile call --- controllers/nodemaintenance_controller.go | 23 +++++++++++-------- .../nodemaintenance_controller_test.go | 6 ++--- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/controllers/nodemaintenance_controller.go b/controllers/nodemaintenance_controller.go index 600d8892..e06368e8 100644 --- a/controllers/nodemaintenance_controller.go +++ b/controllers/nodemaintenance_controller.go @@ -148,13 +148,16 @@ func (r *NodeMaintenanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ return emptyResult, nil } - if err = initMaintenanceStatus(ctx, nm, drainer); err != nil { + needUpdate, err := initMaintenanceStatus(ctx, nm, drainer) + if err != nil { r.logger.Error(err, "Failed to initalize NodeMaintenance status") return r.onReconcileError(ctx, nm, drainer, err) } - if err = r.Client.Status().Update(ctx, nm); err != nil { - r.logger.Error(err, "Failed to update NodeMaintenance with \"Running\" status") - return r.onReconcileError(ctx, nm, drainer, err) + if needUpdate { + if err = r.Client.Status().Update(ctx, nm); err != nil { + r.logger.Error(err, "Failed to update NodeMaintenance with \"Running\" status") + return r.onReconcileError(ctx, nm, drainer, err) + } } nodeName := nm.Spec.NodeName @@ -393,13 +396,15 @@ func (r *NodeMaintenanceReconciler) fetchNode(ctx context.Context, drainer *drai return node, nil } -func initMaintenanceStatus(ctx context.Context, nm *v1beta1.NodeMaintenance, drainer *drain.Helper) error { +// initMaintenanceStatus initializes the nm CR status only at the first time, when phase is empty, +// It returns a bool whether we should update the CR status and an error if it can't be initialized +func initMaintenanceStatus(ctx context.Context, nm *v1beta1.NodeMaintenance, drainer *drain.Helper) (bool, error) { if nm.Status.Phase == "" { nm.Status.Phase = v1beta1.MaintenanceRunning setLastUpdate(nm) pendingList, errlist := drainer.GetPodsForDeletion(nm.Spec.NodeName) if errlist != nil { - return fmt.Errorf("failed to get pods for eviction while initializing status") + return false, fmt.Errorf("failed to get pods for eviction while initializing status") } if pendingList != nil { nm.Status.PendingPods = GetPodNameList(pendingList.Pods()) @@ -412,12 +417,12 @@ func initMaintenanceStatus(ctx context.Context, nm *v1beta1.NodeMaintenance, dra FieldSelector: fields.SelectorFromSet(fields.Set{"spec.nodeName": nm.Spec.NodeName}).String(), }) if err != nil { - return err + return false, err } nm.Status.TotalPods = len(podlist.Items) - return err + return true, nil } - return nil + return false, nil } func (r *NodeMaintenanceReconciler) onReconcileErrorWithRequeue(ctx context.Context, nm *v1beta1.NodeMaintenance, drainer *drain.Helper, err error, duration *time.Duration) (ctrl.Result, error) { diff --git a/controllers/nodemaintenance_controller_test.go b/controllers/nodemaintenance_controller_test.go index e78d2e36..899e7d29 100644 --- a/controllers/nodemaintenance_controller_test.go +++ b/controllers/nodemaintenance_controller_test.go @@ -66,7 +66,7 @@ var _ = Describe("Node Maintenance", func() { }) When("Status was initalized", func() { It("should be set for running with 2 pods to drain", func() { - Expect(initMaintenanceStatus(ctx, nm, drainer)).To(Succeed()) + Expect(initMaintenanceStatus(ctx, nm, drainer)).Error().NotTo(HaveOccurred()) // status was initialized but the function will fail on updating the CR status, since we don't create a nm CR here Expect(nm.Status.Phase).To(Equal(v1beta1.MaintenanceRunning)) Expect(len(nm.Status.PendingPods)).To(Equal(2)) @@ -78,7 +78,7 @@ var _ = Describe("Node Maintenance", func() { }) When("Owner ref was set", func() { It("should be set properly", func() { - Expect(initMaintenanceStatus(ctx, nm, drainer)).To(Succeed()) + Expect(initMaintenanceStatus(ctx, nm, drainer)).Error().NotTo(HaveOccurred()) // status was initialized but the function will fail on updating the CR status, since we don't create a nm CR here By("Setting owner ref for a modified nm CR") node := &corev1.Node{} @@ -101,7 +101,7 @@ var _ = Describe("Node Maintenance", func() { It("Should not modify the CR after initalization", func() { nmCopy := nm.DeepCopy() nmCopy.Status.Phase = v1beta1.MaintenanceFailed - Expect(initMaintenanceStatus(ctx, nmCopy, drainer)).To(Succeed()) + Expect(initMaintenanceStatus(ctx, nmCopy, drainer)).Error().NotTo(HaveOccurred()) // status was not initialized thus the function succeeds Expect(nmCopy.Status.Phase).To(Equal(v1beta1.MaintenanceFailed)) Expect(len(nmCopy.Status.PendingPods)).To(Equal(0)) From 404ac4ed02f5a4ac825a84ff4687567dd494cb94 Mon Sep 17 00:00:00 2001 From: oraz Date: Mon, 4 Mar 2024 15:15:09 +0200 Subject: [PATCH 3/3] Check both return values of initMaintenanceStatus Only checking the error is not good enough to verify whether the function can set/initalize the stauts --- controllers/nodemaintenance_controller_test.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/controllers/nodemaintenance_controller_test.go b/controllers/nodemaintenance_controller_test.go index 899e7d29..f31b86ad 100644 --- a/controllers/nodemaintenance_controller_test.go +++ b/controllers/nodemaintenance_controller_test.go @@ -66,8 +66,9 @@ var _ = Describe("Node Maintenance", func() { }) When("Status was initalized", func() { It("should be set for running with 2 pods to drain", func() { - Expect(initMaintenanceStatus(ctx, nm, drainer)).Error().NotTo(HaveOccurred()) - // status was initialized but the function will fail on updating the CR status, since we don't create a nm CR here + toUpdate, err := initMaintenanceStatus(ctx, nm, drainer) + Expect(toUpdate).To(BeTrue()) + Expect(err).NotTo(HaveOccurred()) Expect(nm.Status.Phase).To(Equal(v1beta1.MaintenanceRunning)) Expect(len(nm.Status.PendingPods)).To(Equal(2)) Expect(nm.Status.EvictionPods).To(Equal(2)) @@ -78,8 +79,9 @@ var _ = Describe("Node Maintenance", func() { }) When("Owner ref was set", func() { It("should be set properly", func() { - Expect(initMaintenanceStatus(ctx, nm, drainer)).Error().NotTo(HaveOccurred()) - // status was initialized but the function will fail on updating the CR status, since we don't create a nm CR here + toUpdate, err := initMaintenanceStatus(ctx, nm, drainer) + Expect(toUpdate).To(BeTrue()) + Expect(err).NotTo(HaveOccurred()) By("Setting owner ref for a modified nm CR") node := &corev1.Node{} Expect(k8sClient.Get(ctx, client.ObjectKey{Name: taintedNodeName}, node)).To(Succeed()) @@ -101,8 +103,9 @@ var _ = Describe("Node Maintenance", func() { It("Should not modify the CR after initalization", func() { nmCopy := nm.DeepCopy() nmCopy.Status.Phase = v1beta1.MaintenanceFailed - Expect(initMaintenanceStatus(ctx, nmCopy, drainer)).Error().NotTo(HaveOccurred()) - // status was not initialized thus the function succeeds + toUpdate, err := initMaintenanceStatus(ctx, nmCopy, drainer) + Expect(toUpdate).To(BeFalse()) + Expect(err).NotTo(HaveOccurred()) Expect(nmCopy.Status.Phase).To(Equal(v1beta1.MaintenanceFailed)) Expect(len(nmCopy.Status.PendingPods)).To(Equal(0)) Expect(nmCopy.Status.EvictionPods).To(Equal(0))