Clear FailedNodes on successful node reconciliation

[Ayush4958]

Description

This fixes a state leak in the NodeReconciler where transient evaluation errors permanently leaked into rule.Status.FailedNodes.

When a transient error resolves and the node successfully evaluates on the next retry, the controller was forgetting to clear the old failure from the cached rule object. As a result, the status patch actively re-merged the stale error back into the API server

This fix adds the missing else block to processNodeAgainstAllRules to clear the failure upon success, matching the correct logic already present in the RuleReconciler's processAllNodesForRule. It also adds a unit test to prevent future regressions.

Related Issue

Fixes #376

Type of Change

/kind bug

Testing

Added a regression test in node_controller_test.go ("should clear FailedNodes entry when node evaluation succeeds after a prior transient failure") which simulates a transient failure followed by a successful evaluation, asserting that FailedNodes drops to empty.

Checklist

• make test passes
• make lint passes

Does this PR introduce a user-facing change?

Fixed a bug where transient errors during node reconciliation caused nodes to become permanently stuck in the `FailedNodes` status of a `NodeReadinessRule`.

[netlify]

✅ Deploy Preview for node-readiness-controller ready!

Name	Link
🔨 Latest commit	d2f37c3
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a85b2011b88c90008c7b53f
😎 Deploy Preview	https://deploy-preview-381--node-readiness-controller.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[kubernetes-prow]

Hi @Ayush4958. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[Ayush4958]

@ajaysundark @Karthik-K-N
PR is ready for review pls take time to review it

thanks ...

[rawadhossain]

@Ayush4958 I think the Fixes 379 reference is incorrect here. It seems unrelated to the PR you raised.

[Ayush4958]

@Ayush4958 I think the Fixes 379 reference is incorrect here. It seems unrelated to the PR you raised.

yh, my bad, pressed wrong number key

[DsThakurRawat]

Ran this branch against the repro from #376. The fix in node_controller.go works, and latestRule persisted to the fake client clears FailedNodes as expected.

One quick heads-up on the unit test in node_controller_test.go: go test ./internal/... trips on line 1214:

FailedNodes must be cleared after successful evaluation
Expected <[]v1alpha1.NodeFailure | len:1>: [{NodeName: "recovery-node", ...}] to be empty

That happens because getApplicableRulesForNode returns rule.DeepCopy(). processNodeAgainstAllRules mutates the copy and persists it to the API server, so the in-memory rule pointer from test setup never gets modified in place.

The API server assertion at line 1220 (latestRule.Status.FailedNodes being empty) already proves the fix works. Dropping the assertion on rule at line 1214 (or asserting on latestRule only) should get the suite green when prow tests run.

[Ayush4958]

thanks for it and u were right about memory pointer and
I had dropped that assertion and It passes the test

[ajaysundark]

Thanks for the PR!

Seems duplicate of #209 and #333. I'll check the status of either of them first and see if can be picked up.

[ajaysundark]

Can you extract clearNodeFailure as a helper instead? ref #333.

[ajaysundark]

It'd be good to give credits to previous authors with "Co-authored-by"

[Ayush4958]

Can you extract clearNodeFailure as a helper instead? ref #333.

Done! I had extracted clearNodeFailure into a helper & also included the co-authored-by

[ajaysundark]

/ok-to-test

[ajaysundark]

Thanks for the patch!

/lgtm
/approve

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajaysundark, Ayush4958

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ajaysundark]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment