fix: stop reporting adoption for taints the controller applied itself

[tejassinghbhati]

Description

NodeReconciler evaluates rules from the in-memory cache, and the cached copy carries whatever status the rule had the last time RuleReconciler ran. Node evaluations persisted since then are not in it, and because the rule spec is immutable GenerationChangedPredicate means that reconcile rarely fires again, so the snapshot can stay stale for the life of the process.

evaluateRuleForNode decides adoption from that status, so for a node the snapshot never contained isFirstEvaluation is always true and the adoption branch fires on every pass. The result is a TaintAdopted event and an Adopting pre-existing taint log line every time an unready, already tainted node reconciles, for a taint the controller applied itself.

The fix refreshes NodeEvaluations from the live object before evaluating. That read goes through the controller-runtime cache, so it is not an extra API call, and the persist block a few lines below already does the same Get. It deliberately touches only NodeEvaluations and leaves FailedNodes alone, so it does not overlap with #381 or #209.

Related Issue

Fixes #383

Type of Change

/kind bug

Testing

Two specs added under when a node is re-evaluated while still unready:

The first applies the taint, confirms the evaluation is persisted, then reconciles three more times and asserts no further TaintAdopted events. It fails on main:

INFO  Adopting pre-existing taint  {"node": "adopt-events-node", ...}
INFO  Adopting pre-existing taint  {"node": "adopt-events-node", ...}
INFO  Adopting pre-existing taint  {"node": "adopt-events-node", ...}
[FAILED] the rule already owns this taint, so no further TaintAdopted events should be emitted

The second puts the taint on the node before the rule ever evaluates it and asserts exactly one TaintAdopted event, so the feature still works. That one passes both with and without the fix, which is what shows the change is narrow rather than just switching adoption off.

Full controller suite passes, 64 of 64 specs, up from 62 before these two. internal/webhook and cmd/readiness-condition-reporter pass too. Run against envtest with Kubernetes 1.36.2 binaries. golangci-lint v2.12.1 reports 0 issues.

Checklist

• make test passes
• make lint passes

Does this PR introduce a user-facing change?

Fixed repeated TaintAdopted events being recorded for taints the controller applied itself. The event is now emitted only when a taint was already on the node before the rule took ownership of it.

[netlify]

✅ Deploy Preview for node-readiness-controller canceled.

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

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tejassinghbhati
Once this PR has been reviewed and has the lgtm label, please assign dchen1107 for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[kubernetes-prow]

Hi @tejassinghbhati. 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.

[ajaysundark]

Thanks for the PR @tejassinghbhati.

K8s events are by design best-effort, I dont think this is required to refresh rule cache (which can be a large object) when a node adoption need to be verified.

[tejassinghbhati]

Fair enough. The Get deep copies the whole rule status, so on a big rule that cost per node is not worth it just to quiet down an event. Thanks for looking.