From 2725c4b17e1a1753c3671568f81ada55a946aba6 Mon Sep 17 00:00:00 2001 From: Rawad Hossain Date: Mon, 4 May 2026 21:01:24 +0600 Subject: [PATCH 1/2] clean up failedNodes for deleted nodes Signed-off-by: Rawad Hossain --- .../nodereadinessrule_controller.go | 21 +++++- .../nodereadinessrule_controller_test.go | 72 ++++++++++++++++++- 2 files changed, 90 insertions(+), 3 deletions(-) diff --git a/internal/controller/nodereadinessrule_controller.go b/internal/controller/nodereadinessrule_controller.go index 99347e9..3261667 100644 --- a/internal/controller/nodereadinessrule_controller.go +++ b/internal/controller/nodereadinessrule_controller.go @@ -222,7 +222,15 @@ func (r *RuleReadinessController) cleanupDeletedNodes(ctx context.Context, rule } } - if len(newNodeEvaluations) == len(rule.Status.NodeEvaluations) { + var newFailedNodes []readinessv1alpha1.NodeFailure + for _, failure := range rule.Status.FailedNodes { + if existingNodes[failure.NodeName] { + newFailedNodes = append(newFailedNodes, failure) + } + } + + if len(newNodeEvaluations) == len(rule.Status.NodeEvaluations) && + len(newFailedNodes) == len(rule.Status.FailedNodes) { log.V(4).Info("No deleted nodes to clean up", "rule", rule.Name) return nil } @@ -246,12 +254,21 @@ func (r *RuleReadinessController) cleanupDeletedNodes(ctx context.Context, rule } } - if len(freshNodeEvaluations) == len(fresh.Status.NodeEvaluations) { + var freshFailedNodes []readinessv1alpha1.NodeFailure + for _, failure := range fresh.Status.FailedNodes { + if existingNodes[failure.NodeName] { + freshFailedNodes = append(freshFailedNodes, failure) + } + } + + if len(freshNodeEvaluations) == len(fresh.Status.NodeEvaluations) && + len(freshFailedNodes) == len(fresh.Status.FailedNodes) { return nil } patch := client.MergeFrom(fresh.DeepCopy()) fresh.Status.NodeEvaluations = freshNodeEvaluations + fresh.Status.FailedNodes = freshFailedNodes return r.Status().Patch(ctx, fresh, patch) }) } diff --git a/internal/controller/nodereadinessrule_controller_test.go b/internal/controller/nodereadinessrule_controller_test.go index 7e1f5d1..e5d3fe0 100644 --- a/internal/controller/nodereadinessrule_controller_test.go +++ b/internal/controller/nodereadinessrule_controller_test.go @@ -908,7 +908,18 @@ var _ = Describe("NodeReadinessRule Controller", func() { }) AfterEach(func() { - Expect(k8sClient.Delete(ctx, rule)).To(Succeed()) + updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + if err := k8sClient.Get(ctx, types.NamespacedName{Name: "delete-node-rule"}, updatedRule); err == nil { + updatedRule.Finalizers = nil + _ = k8sClient.Update(ctx, updatedRule) + _ = k8sClient.Delete(ctx, updatedRule) + } + + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{Name: "delete-node-rule"}, &nodereadinessiov1alpha1.NodeReadinessRule{}) + return apierrors.IsNotFound(err) + }, time.Second*10).Should(BeTrue()) + // node1 is already deleted in the test _ = k8sClient.Delete(ctx, node2) }) @@ -955,6 +966,65 @@ var _ = Describe("NodeReadinessRule Controller", func() { return false }, time.Second*5).Should(BeTrue()) }) + + It("removes failedNodes entries for deleted nodes", func() { + _, err := ruleReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "delete-node-rule"}}) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func() int { + updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + _ = k8sClient.Get(ctx, types.NamespacedName{Name: "delete-node-rule"}, updatedRule) + return len(updatedRule.Status.NodeEvaluations) + }, time.Second*5).Should(Equal(2)) + + seededRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "delete-node-rule"}, seededRule)).To(Succeed()) + statusPatch := client.MergeFrom(seededRule.DeepCopy()) + seededRule.Status.FailedNodes = append(seededRule.Status.FailedNodes, nodereadinessiov1alpha1.NodeFailure{ + NodeName: "node1", + Reason: "EvaluationError", + Message: "test failure", + LastEvaluationTime: metav1.Now(), + }) + Expect(k8sClient.Status().Patch(ctx, seededRule, statusPatch)).To(Succeed()) + + Eventually(func() bool { + updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + _ = k8sClient.Get(ctx, types.NamespacedName{Name: "delete-node-rule"}, updatedRule) + for _, f := range updatedRule.Status.FailedNodes { + if f.NodeName == "node1" { + return true + } + } + return false + }, time.Second*5).Should(BeTrue()) + verifyRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "delete-node-rule"}, verifyRule)).To(Succeed()) + for _, f := range verifyRule.Status.FailedNodes { + Expect(f.NodeName).NotTo(Equal("node2"), "setup should not add a failure for node2") + } + + Expect(k8sClient.Delete(ctx, node1)).To(Succeed()) + + _, err = ruleReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "delete-node-rule"}}) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func() bool { + updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + _ = k8sClient.Get(ctx, types.NamespacedName{Name: "delete-node-rule"}, updatedRule) + for _, f := range updatedRule.Status.FailedNodes { + if f.NodeName == "node1" { + return false + } + } + return true + }, time.Second*5).Should(BeTrue()) + patchedRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "delete-node-rule"}, patchedRule)).To(Succeed()) + for _, f := range patchedRule.Status.FailedNodes { + Expect(f.NodeName).NotTo(Equal("node2"), "cleanup should not introduce a failure for node2") + } + }) }) Context("when a rule's nodeSelector is modified", func() { From f5e0b15cc49f2deb3b9bc11804167f2511e561c3 Mon Sep 17 00:00:00 2001 From: Rawad Hossain Date: Tue, 5 May 2026 23:06:54 +0600 Subject: [PATCH 2/2] utility function for filtering logic Signed-off-by: Rawad Hossain --- internal/controller/helper.go | 24 +++++++++++++ .../nodereadinessrule_controller.go | 36 ++++++------------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/internal/controller/helper.go b/internal/controller/helper.go index 7765fcf..931c787 100644 --- a/internal/controller/helper.go +++ b/internal/controller/helper.go @@ -21,6 +21,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + readinessv1alpha1 "sigs.k8s.io/node-readiness-controller/api/v1alpha1" ) // nodeSelectorChanged checks if nodeSelector has changed. @@ -117,6 +118,29 @@ func taintsEqual(a, b []corev1.Taint) bool { return true } +// filters nodeEvaluations and failedNodes to keep only existing nodes. +func filterStatusForExistingNodes( + existingNodes map[string]bool, + nodeEvaluations []readinessv1alpha1.NodeEvaluation, + failedNodes []readinessv1alpha1.NodeFailure, +) ([]readinessv1alpha1.NodeEvaluation, []readinessv1alpha1.NodeFailure) { + filteredEvaluations := make([]readinessv1alpha1.NodeEvaluation, 0, len(nodeEvaluations)) + for _, evaluation := range nodeEvaluations { + if existingNodes[evaluation.NodeName] { + filteredEvaluations = append(filteredEvaluations, evaluation) + } + } + + filteredFailedNodes := make([]readinessv1alpha1.NodeFailure, 0, len(failedNodes)) + for _, failure := range failedNodes { + if existingNodes[failure.NodeName] { + filteredFailedNodes = append(filteredFailedNodes, failure) + } + } + + return filteredEvaluations, filteredFailedNodes +} + // labelsEqual checks if two label maps are equal. func labelsEqual(a, b map[string]string) bool { if len(a) != len(b) { diff --git a/internal/controller/nodereadinessrule_controller.go b/internal/controller/nodereadinessrule_controller.go index 3261667..b3bd9d0 100644 --- a/internal/controller/nodereadinessrule_controller.go +++ b/internal/controller/nodereadinessrule_controller.go @@ -215,19 +215,11 @@ func (r *RuleReadinessController) cleanupDeletedNodes(ctx context.Context, rule } // Filter out deleted nodes - var newNodeEvaluations []readinessv1alpha1.NodeEvaluation - for _, evaluation := range rule.Status.NodeEvaluations { - if existingNodes[evaluation.NodeName] { - newNodeEvaluations = append(newNodeEvaluations, evaluation) - } - } - - var newFailedNodes []readinessv1alpha1.NodeFailure - for _, failure := range rule.Status.FailedNodes { - if existingNodes[failure.NodeName] { - newFailedNodes = append(newFailedNodes, failure) - } - } + newNodeEvaluations, newFailedNodes := filterStatusForExistingNodes( + existingNodes, + rule.Status.NodeEvaluations, + rule.Status.FailedNodes, + ) if len(newNodeEvaluations) == len(rule.Status.NodeEvaluations) && len(newFailedNodes) == len(rule.Status.FailedNodes) { @@ -247,19 +239,11 @@ func (r *RuleReadinessController) cleanupDeletedNodes(ctx context.Context, rule return err } - var freshNodeEvaluations []readinessv1alpha1.NodeEvaluation - for _, evaluation := range fresh.Status.NodeEvaluations { - if existingNodes[evaluation.NodeName] { - freshNodeEvaluations = append(freshNodeEvaluations, evaluation) - } - } - - var freshFailedNodes []readinessv1alpha1.NodeFailure - for _, failure := range fresh.Status.FailedNodes { - if existingNodes[failure.NodeName] { - freshFailedNodes = append(freshFailedNodes, failure) - } - } + freshNodeEvaluations, freshFailedNodes := filterStatusForExistingNodes( + existingNodes, + fresh.Status.NodeEvaluations, + fresh.Status.FailedNodes, + ) if len(freshNodeEvaluations) == len(fresh.Status.NodeEvaluations) && len(freshFailedNodes) == len(fresh.Status.FailedNodes) {