fix: clean up taints on rule deletion for nodes that stopped matching the selector

[tejassinghbhati]

Description

cleanupTaintsForRule only considered Nodes that currently match the rule's spec.nodeSelector:

for _, node := range nodeList.Items {
	if !r.ruleAppliesTo(ctx, rule, &node) {
		continue
	}

A Node that the rule had tainted, and that was later relabelled out of the selector, was skipped during cleanup. reconcileDelete then removed the finalizer and the rule object was deleted, leaving the taint behind with no owner. Nothing in the system references it any more, so no future reconcile can remove it and an operator has to clear it by hand. For a NoSchedule taint the Node stops accepting new workloads indefinitely; for NoExecute it also keeps evicting Pods without a toleration, on a Node that no rule governs.

Cleanup now also covers Nodes recorded in the rule's own status, through status.nodeEvaluations and status.appliedNodes.

Two design points worth calling out, since both came up while looking at the related issues:

Why using status is acceptable here. The review discussion on #339 concluded that leaning on rule.Status to decide what to act on is fragile, which is a large part of why #342 is still open. That objection is about steady state reconciliation, where the controller repeatedly consults status it wrote itself. The deletion path is different: it runs once, it is terminal, and the rule is being torn down, so the set of Nodes the rule ever managed is exactly the set that needs cleaning. There is no ongoing loop for a stale entry to feed back into.

Why not just remove the taint everywhere. The validating webhook rejects rules sharing a taint key and effect only when their selectors overlap, so two rules may legitimately share readiness.k8s.io/x:NoSchedule with disjoint selectors. Unconditionally stripping the key on deletion would pull the taint off Nodes owned by the surviving sibling rule. Scoping to this rule's status avoids that, because a Node managed solely by the sibling never appears in this rule's status. The second test added here locks that behaviour in.

This is deliberately scoped to the deletion path and does not attempt to resolve #342, which needs the harder "used to match, no longer does" detection during normal reconciliation. I filed this separately because the deletion case is strictly worse: while the rule still exists the taint is at least recoverable by a future fix, whereas after deletion it is permanent.

Related Issue

Fixes #347

Type of Change

/kind bug

Testing

Two specs added to the existing when a rule is deleted context in internal/controller/nodereadinessrule_controller_test.go:

1. should remove taints from nodes that no longer match the selector reproduces the bug. The Node is managed by the rule, relabelled out of the selector, and the rule is deleted. The spec asserts the rule object is fully gone (so nothing could clean up later) and that the taint is not left behind.
2. should not remove a taint from a node the rule never managed guards the fix against over-removal, using a Node outside the selector that carries the same taint key and effect.

Verified the first test genuinely catches the regression by stashing the source change and re-running against unmodified main:

[FAILED] Timed out after 10.000s.
Rule deletion should not leave an orphaned taint on a node that stopped matching the selector

With the fix applied, the full controller suite passes: 64 of 64 specs, up from 62 before these two were added. internal/webhook and cmd/readiness-condition-reporter pass as well. 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 rule deletion leaving a permanently orphaned taint on nodes that had stopped matching the rule's nodeSelector while the rule was still active.

[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

[netlify]

✅ Deploy Preview for node-readiness-controller canceled.

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: tejassinghbhati / name: tejassinghbhati (ab96593)

[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.

[Karthik-K-N]

Lets discuss about the scope of the issue in corresponding issue.

[tejassinghbhati]

Sure, that makes sense. I have written up the options and the trade offs over on #347 so the discussion stays in one place.

Happy to hold this until there is a direction, and equally happy to close it if we land on leaving the behaviour as it is and documenting it instead. Either way I will follow up with the docs change so the outcome is written down somewhere an operator will actually find it.

[tejassinghbhati]

Closing this. Agreed on #347 that it is a duplicate of #235, so this should be redone against that issue once there is agreement on the approach.

The branch is still on my fork if it is worth reviving. The change scopes deletion cleanup to nodes recorded in the rule status, plus two envtest specs, one reproducing the stranded taint and one checking a node the rule never managed keeps its taint.