Chnages to handle Node taint mutation only through the node controller

[Karthik-K-N]

Description

Its is just a PoC

Currently we have NodeReadinessRule(NRR) controller and Node controller, which both acts on the node, Though the system is expected to be eventual consistent two controllers acting on Node may cause some issues to enduser.

With this PoC changes are made such that only the Node controller is authorative to mutate Node object that is adding/removing taint, where the the NRR controller takes care of building cache, managing finalizers and cleaning up.

Related Issue

Type of Change

Testing

Checklist

• make test passes
• make lint passes

Does this PR introduce a user-facing change?

Doc #(issue)

[kubernetes-prow]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[netlify]

✅ Deploy Preview for node-readiness-controller ready!

Name	Link
🔨 Latest commit	8fe2497
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a7aed5e7075f90008251f7f
😎 Deploy Preview	https://deploy-preview-389--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]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Karthik-K-N
Once this PR has been reviewed and has the lgtm label, please assign tallclair 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]

PR needs rebase.

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.

[DsThakurRawat]

had a look at this since it came out of #382, and the single writer split does address the race there. with taint mutation living in the node controller there's no second writer to lose the annotation update against.

few things came up while testing the branch locally.

observedGeneration now marks "seen" rather than "applied". the rule reconciler sets rule.Status.ObservedGeneration = rule.Generation and patches it before the node reconciler has touched anything, so observedGeneration == generation can be true while every matching node is still untainted. on main it was set after the evaluation loop had run. probably fine for a PoC, but it's an API visible change, and anything polling that field as "this rule is applied" would go green early.

appliedNodes doesn't shrink when a node is deleted. the full recompute used to live in processAllNodesForRule, which loses its only caller here, so Status.AppliedNodes is now only maintained incrementally in processNodeAgainstAllRules and nothing prunes it. a deleted node keeps its entry. though you mentioned on #382 that NRE should own per node rule results, so if appliedNodes is on its way out with that work then this probably isn't worth fixing here.

the delete path still writes taints from the rule reconciler. reconcileDelete calls cleanupTaintsForRule, which goes straight to removeTaintBySpec. so the sole writer invariant holds everywhere except rule deletion. not sure whether that exception is deliberate for now.

also processAllNodesForRule is unused outside tests after this.

happy to share the envtest cases if they're useful.

[ajaysundark]

I see it could be cleaner and would avoid patch fixes like #343. But I'm skeptical of multiple major refactors.. Concurrent update risks we are seeing are mainly node status handling (taint management is optimistically locked), so I want to wait for #345, and revisit this if we still want to go down this route. Wdyt?

[Karthik-K-N]

I see it could be cleaner and would avoid patch fixes like #343. But I'm skeptical of multiple major refactors.. Concurrent update risks we are seeing are mainly node status handling (taint management is optimistically locked), so I want to wait for #345, and revisit this if we still want to go down this route. Wdyt?

Sure, we can do that.

[Karthik-K-N]

had a look at this since it came out of #382, and the single writer split does address the race there. with taint mutation living in the node controller there's no second writer to lose the annotation update against.

few things came up while testing the branch locally.

observedGeneration now marks "seen" rather than "applied". the rule reconciler sets rule.Status.ObservedGeneration = rule.Generation and patches it before the node reconciler has touched anything, so observedGeneration == generation can be true while every matching node is still untainted. on main it was set after the evaluation loop had run. probably fine for a PoC, but it's an API visible change, and anything polling that field as "this rule is applied" would go green early.

appliedNodes doesn't shrink when a node is deleted. the full recompute used to live in processAllNodesForRule, which loses its only caller here, so Status.AppliedNodes is now only maintained incrementally in processNodeAgainstAllRules and nothing prunes it. a deleted node keeps its entry. though you mentioned on #382 that NRE should own per node rule results, so if appliedNodes is on its way out with that work then this probably isn't worth fixing here.

the delete path still writes taints from the rule reconciler. reconcileDelete calls cleanupTaintsForRule, which goes straight to removeTaintBySpec. so the sole writer invariant holds everywhere except rule deletion. not sure whether that exception is deliberate for now.

also processAllNodesForRule is unused outside tests after this.

happy to share the envtest cases if they're useful.

Hey, Thanks for looking at this. As its just a PoC so I haven't fixed most of the things, but based on the feedback will work in future. Thanks