[BUG] NodeReconciler permanently leaks transient errors into Status.FailedNodes

[Ayush4958]

What happened?

When NodeReconciler encounters an error evaluating a node, it records it in rule.Status.FailedNodes. If the error resolves on the next reconcile and evaluation succeeds, the controller fails to clear the old failure because it lacks an else block to handle success.

The stale failure is read from the local cache and actively merged back into the API server, permanently trapping the healthy node in the FailedNodes array. It only clears if a manual rule edit forces the RuleReconciler to run.

Steps to Reproduce

1. Trigger a transient error during node evaluation
2. Observe the node is added to rule.Status.FailedNodes.
3. Resolve the error. The controller retries and evaluation succeeds.
4. Check the rule status. The node operates normally but is permanently stuck in FailedNodes.

Expected Behavior

processNodeAgainstAllRules must include an else block to explicitly clear the node from rule.Status.FailedNodes upon successful evaluation, mirroring the correct logic found in processAllNodesForRule.

Controller Version / Image Tag

main

Kubernetes Version

Client Version v1.30.0

Controller Logs

No response

Additional Details

This issue is entirely distinct from PR #222 and PR #237.
Those PRs fixed the retry mechanism by ensuring that transient errors are properly returned to the reconciler so controller runtime can requeue them.

While those PRs ensured the controller retries on failure, they did not fix the controller forgetting to clean up Status.FailedNodes upon success.

[Ayush4958]

/assign

[Ayush4958]

Hi @ajaysundark , @Karthik-K-N
I want to work on this but before that
a green flag from team would be best

[DsThakurRawat]

Reproduced this against main at d74dabd. A node that satisfies its rule but carries a stale entry in rule.Status.FailedNodes still has that entry after a successful evaluation, so your read of it is right.

The bit I'd add to your report is why it persists rather than merely failing to clear. The status patch in processNodeAgainstAllRules rebuilds the list in two passes:

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)
	}
}
latestRule.Status.FailedNodes = updatedFailedNodes

The first loop drops this node's entry from what the API server currently has. The second one puts it back from the cached rule, which on a successful evaluation still holds the old failure because nothing cleared it. So each reconcile actively re-asserts the stale entry instead of just forgetting to remove it, which matches what you saw about it surviving until a rule edit.

On whether this is already covered, I don't think it is, and the history is a little more specific than being distinct from #222 and #237. #215 described this same case in passing, "a node that previously failed but later succeeds can still have a stale entry in failedNodes". It was closed on May 15 by #216, but that commit (2239ccef) only touched nodereadinessrule_controller.go. The clearing branch landed in processAllNodesForRule and the NodeReconciler path never got the equivalent. So this looks like the unfixed half of #215 rather than a new regression, which is probably the useful framing for whoever triages it.

One thing to watch if you do send a PR: #217 is open and assigned, and it restructures this same patch block to skip the empty NodeEvaluation on the failure path. Might be worth a word with @sahitya-chandra so the two don't collide.

I tried the else you describe, mirroring the branch in processAllNodesForRule, and it does clear the entry. go test ./internal/... stays green with it, same as without. getApplicableRulesForNode hands back rule.DeepCopy(), so mutating rule.Status on that path doesn't touch the shared cache.

[Ayush4958]

@DsThakurRawat , Thanks for reproducing this and providing the historical context.

when u mention that go test ./internal/... stays green with or without this fix, it means the test suite is entirely blind to state leak. IG #216 missed this one because there was no test

I will open a PR to add the missing else block & also include a unit test in node_controller_test.go which explicitly simulates a transient failure followed by a successful retry, asserting that FailedNodes drops to zero so this doesn't regress again.

Noted on #217, Since that restructures NodeEvaluations and this fix only touches FailedNodes, the logic shouldn't collide. I'll ping @sahitya-chandra if merge conflict comes up in that patch

[tejassinghbhati]

PR #430 addresses this. The fix gates the re-append of the cached FailedNodes entry on evalErr != nil, mirroring the clearing logic already present in processAllNodesForRule. A regression test using a fake client with WithStatusSubresource is included. @Ayush4958 — flagging so we don't end up with two PRs for the same issue.

[Ayush4958]

hi @tejassinghbhati ,

I already have a PR #381 going on which is under review, just pushed some final change requested by maintainer
Since, it's already have reviewing back and forth, let's consolidate efforts there to avoid duplicate work. I'd love your review on my PR if you want to review it

thanks ....

[tejassinghbhati]

Sure @Ayush4958 I'll take a look at that.
Thanks for pointing it out man.