diff --git a/internal/controller/node_controller.go b/internal/controller/node_controller.go index 2d1b1c12..3a93b460 100644 --- a/internal/controller/node_controller.go +++ b/internal/controller/node_controller.go @@ -152,12 +152,13 @@ func (r *RuleReadinessController) processNodeAgainstAllRules(ctx context.Context "rule", rule.Name, "ruleResourceVersion", rule.ResourceVersion) - if err := r.evaluateRuleForNode(ctx, rule, node); err != nil { - log.Error(err, "Failed to evaluate rule for node", + evalErr := r.evaluateRuleForNode(ctx, rule, node) + if evalErr != nil { + log.Error(evalErr, "Failed to evaluate rule for node", "node", node.Name, "rule", rule.Name) // Continue with other rules even if one fails - r.recordNodeFailure(rule, node.Name, "EvaluationError", err.Error()) - errs = append(errs, err) + r.recordNodeFailure(rule, node.Name, "EvaluationError", evalErr.Error()) + errs = append(errs, evalErr) metrics.Failures.WithLabelValues(rule.Name, string(metrics.FailureReasonEvaluationError)).Inc() } @@ -201,16 +202,23 @@ func (r *RuleReadinessController) processNodeAgainstAllRules(ctx context.Context ) } - // handle status.FailedNodes for this node + // Rebuild FailedNodes for this node. Drop any existing entry from + // what the API server currently holds. var updatedFailedNodes []readinessv1alpha1.NodeFailure for _, failure := range latestRule.Status.FailedNodes { if failure.NodeName != node.Name { updatedFailedNodes = append(updatedFailedNodes, failure) } } - for _, failure := range rule.Status.FailedNodes { - if failure.NodeName == node.Name { - updatedFailedNodes = append(updatedFailedNodes, failure) + // Re-add this node's failure entry only when evaluation just + // failed. On success we leave it absent so that a stale error + // recorded by a previous transient failure is cleared. + if evalErr != nil { + for _, failure := range rule.Status.FailedNodes { + if failure.NodeName == node.Name { + updatedFailedNodes = append(updatedFailedNodes, failure) + break + } } } latestRule.Status.FailedNodes = updatedFailedNodes diff --git a/internal/controller/node_controller_test.go b/internal/controller/node_controller_test.go index aace5818..80714d45 100644 --- a/internal/controller/node_controller_test.go +++ b/internal/controller/node_controller_test.go @@ -1298,5 +1298,91 @@ var _ = Describe("Node Controller", func() { Expect(after).To(BeNumerically(">", before), "metrics.Failures{rule, EvaluationError} must increment when the node reconciler hits an evaluation error") }) + + // Regression test for: processNodeAgainstAllRules permanently leaks + // transient evaluation errors into Status.FailedNodes + // https://github.com/kubernetes-sigs/node-readiness-controller/issues/376 + It("should clear a stale FailedNodes entry when the node subsequently evaluates successfully", func() { + Expect(nodereadinessiov1alpha1.AddToScheme(testScheme)).To(Succeed()) + + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stale-failed-nodes-node", + }, + Spec: corev1.NodeSpec{ + Taints: []corev1.Taint{ + {Key: "readiness.k8s.io/stale-test", Effect: corev1.TaintEffectNoSchedule}, + }, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + // Condition is already True: evaluation will succeed and remove the taint. + {Type: "StaleTestCondition", Status: corev1.ConditionTrue}, + }, + }, + } + rule := &nodereadinessiov1alpha1.NodeReadinessRule{ + ObjectMeta: metav1.ObjectMeta{Name: "stale-failed-nodes-rule"}, + Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{ + NodeSelector: metav1.LabelSelector{}, // matches all nodes + Conditions: []nodereadinessiov1alpha1.ConditionRequirement{ + {Type: "StaleTestCondition", RequiredStatus: corev1.ConditionTrue}, + }, + Taint: corev1.Taint{ + Key: "readiness.k8s.io/stale-test", + Effect: corev1.TaintEffectNoSchedule, + }, + EnforcementMode: nodereadinessiov1alpha1.EnforcementModeContinuous, + }, + } + + fc := fakeclient.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(node, rule). + // WithStatusSubresource ensures Status().Patch() roundtrips + // through the fake so that the updated FailedNodes is readable. + WithStatusSubresource(rule). + Build() + + controller := &RuleReadinessController{ + Client: fc, + Scheme: testScheme, + clientset: fake.NewSimpleClientset(), + ruleCache: map[string]*nodereadinessiov1alpha1.NodeReadinessRule{rule.Name: rule}, + EventRecorder: events.NewFakeRecorder(10), + } + + // Seed a stale FailedNodes entry to simulate a previous transient error. + seededRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + Expect(fc.Get(ctx, types.NamespacedName{Name: rule.Name}, seededRule)).To(Succeed()) + seededRule.Status.FailedNodes = []nodereadinessiov1alpha1.NodeFailure{ + { + NodeName: node.Name, + Reason: "EvaluationError", + Message: "simulated transient error from a previous reconcile", + LastEvaluationTime: metav1.Now(), + }, + } + Expect(fc.Status().Update(ctx, seededRule)).To(Succeed()) + + // Refresh the cached rule so it carries the seeded stale failure. + cachedRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + Expect(fc.Get(ctx, types.NamespacedName{Name: rule.Name}, cachedRule)).To(Succeed()) + controller.ruleCache[rule.Name] = cachedRule + + // Run reconcile. The node satisfies the condition so + // evaluateRuleForNode will succeed. + err := controller.processNodeAgainstAllRules(ctx, node) + Expect(err).NotTo(HaveOccurred()) + + // The stale entry must be gone: a successful evaluation should + // clear any prior FailedNodes record for that node. + updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + Expect(fc.Get(ctx, types.NamespacedName{Name: rule.Name}, updatedRule)).To(Succeed()) + for _, f := range updatedRule.Status.FailedNodes { + Expect(f.NodeName).NotTo(Equal(node.Name), + "stale FailedNodes entry must be cleared after successful evaluation") + } + }) }) })