fix: isolate rule status cache per rule-reconcile workers

[bhuvan-somisetty]

Description

This PR resolves data races and memory leaks in the controller when --node-concurrent-reconciles is set to > 1:

1. Rule Cache Data Race Fix: Updated RuleReadinessController.getApplicableRulesForNode to return rule.DeepCopy() pointers. Previously, returning pointers straight from r.ruleCache caused concurrent NodeReconciler worker threads to mutate rule.Status.NodeEvaluations and rule.Status.FailedNodes on the shared in-memory pointer without synchronization, resulting in Go data races and memory corruption under concurrent node reconciliation load.
   2. Prometheus Metric Leak Fix: Added metrics.EvaluationDuration.DeleteLabelValues(rule.Name) to reconcileDelete in nodereadinessrule_controller.go to ensure histogram metric series are unregistered when a rule CR is deleted.
2. Unit Tests: Added TestGetApplicableRulesForNode_DeepCopy in helper_unit_test.go to verify that getApplicableRulesForNode returns isolated, deep-copied rule instances.

Related Issue

Fixes #331

Type of Change

/kind bug

Testing

• Unit testing via go test -v ./internal/controller/...
• Verified deep-copy isolation unit test passes without race conditions.

Checklist

• make test passes
• make lint passes

Does this PR introduce a user-facing change?

NONE

[netlify]

✅ Deploy Preview for node-readiness-controller canceled.

Name	Link
🔨 Latest commit	d58c938
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a6d9beb3bf01e0008f13dda

[kubernetes-prow]

Welcome @bhuvan-somisetty!

It looks like this is your first PR to kubernetes-sigs/node-readiness-controller 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/node-readiness-controller has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[kubernetes-prow]

Hi @bhuvan-somisetty. 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.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

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.

[bhuvan-somisetty]

/cc @ajaysundark @Priyankasaggu11929 @vitorfloriano @Karthik-K-N @AvineshTripathi

Hey folks! Whenever you get a chance, could you please take a look at this PR when convenient? It addresses the data race and metric memory leak under concurrent node reconciliations described in #331. Thanks a lot!

[ajaysundark]

Thanks for your PR.

Note, this is duplicate to #330 and #209

[ajaysundark]

#209 however lacks the unit-tests, so could obsolete the former to take this one.

Could you evaluate the fixes and also address the lint failures and update?

[ajaysundark]

/uncc @Priyankasaggu11929 @vitorfloriano @Karthik-K-N @AvineshTripathi @dchen1107 @SergeyKanzhelev
/assign @bhuvan-somisetty

please reassign to me once you updated the PR.

[bhuvan-somisetty]

Thanks for the review, @ajaysundark!

I rebased onto latest main and noticed #330 (the metric cleanup fix) had already been merged, so I dropped that hunk from this PR to avoid the duplicate — this PR now only carries the ruleCache deep-copy fix plus the unit test that #209 was missing.

Also fixed the lint failure: the test file had a gofmt formatting issue (extra blank line) and a usetesting nit (swapped context.Background() for t.Context()). Verified locally with go build ./..., go vet ./..., go test ./internal/controller/..., and golangci-lint run — all clean now.

Reassigning to you as requested. Let me know if you'd like anything else adjusted.

[bhuvan-somisetty]

/assign @ajaysundark

[ajaysundark]

/ok-to-test

[ajaysundark]

@bhuvan-somisetty Can you review #209 and evaluate the recordFailure patch you noted on your issue as well - this could be a separate followup PR.

[ajaysundark]

/retitle fix: isolate rule status cache per rule-reconcile workers

[ajaysundark]

/lgtm

[ajaysundark]

Thanks!

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajaysundark, bhuvan-somisetty

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

[bhuvan-somisetty]

Thanks @ajaysundark!

Took a look at #209 and dug into the recordFailure/stale FailedNodes behavior on top of what's now merged here. Turns out it's a real, still-open bug, but only on one of the two code paths:

• nodereadinessrule_controller.go's processAllNodesForRule (the RuleReconciler path) already clears FailedNodes inline on success, so it's fine as-is.
• node_controller.go's processNodeAgainstAllRules (the NodeReconciler path) does not — it only records a failure on error and never clears it on success. Since rule there is now a deep-copied snapshot from ruleCache, a stale failure entry from an earlier reconcile keeps getting carried forward on every subsequent successful evaluation, so a recovered node stays stuck in status.failedNodes indefinitely.

Opened #333 as the separate follow-up you suggested, scoped to just that fix (added a clearNodeFailure helper mirroring recordNodeFailure, plus unit tests). Verified locally with go build, go vet, go test ./internal/controller/..., and golangci-lint run — all clean.