fix: use optimistic locking on rule status patches by bhuvan-somisetty · Pull Request #343 · kubernetes-sigs/node-readiness-controller

bhuvan-somisetty · 2026-08-03T10:11:43Z

Description

RuleReconciler and NodeReconciler both patch NodeReadinessRule.Status concurrently, but every status/finalizer patch used a plain client.MergeFrom with no resourceVersion precondition, wrapped in retry.RetryOnConflict. A JSON merge patch only carries that precondition when MergeFromWithOptimisticLock is used, so without it the API server never returns a conflict and the retry wrapper never actually retries. This is the same bug #180 fixed for node taint patches (addTaintBySpec/removeTaintBySpec), just left open on the rule-status side.

The worst instance was updateRuleStatus: it replaced NodeEvaluations/FailedNodes wholesale from a snapshot computed at the start of a RuleReconciler sweep, so it could silently discard a concurrent NodeReconciler per-node update for a node outside that sweep's snapshot. Fixed by having processAllNodesForRule return a delta of exactly the per-node changes it made, and merging that delta by node name instead of overwriting the whole slice.

Also added the missing optimistic lock to ensureFinalizer, the finalizer removal in reconcileDelete, cleanupDeletedNodes, and markBootstrapCompleted's node annotation patch, matching the pattern already used by addTaintBySpec/removeTaintBySpec.

Type of Change

/kind bug

Testing

go build ./...
go vet ./...
go test ./internal/controller/... (63/63 specs pass; the only failure locally is envtest's Windows-only teardown limitation, unrelated to this change)
Added two regression tests: one proving a NodeReconciler-written evaluation for a node outside the RuleReconciler sweep survives updateRuleStatus, and one proving updateRuleStatus actually retries (and doesn't lose data) on a genuine conflict.

Checklist

make test passes
make lint passes

Does this PR introduce a user-facing change?

Fixed a bug where concurrent status writes from RuleReconciler and NodeReconciler to the same NodeReadinessRule could silently overwrite each other, since the retry-on-conflict wrapper around these patches never actually detected a conflict.

netlify · 2026-08-03T10:11:49Z

✅ Deploy Preview for node-readiness-controller ready!

Name	Link
🔨 Latest commit	`b6eab97`
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a82e3349d513300083e9f15
😎 Deploy Preview	https://deploy-preview-343--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 · 2026-08-03T10:11:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bhuvan-somisetty
Once this PR has been reviewed and has the lgtm label, please assign haircommander 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 · 2026-08-03T10:11:53Z

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.

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.

bhuvan-somisetty · 2026-08-03T10:20:29Z

@ajaysundark fixed the commit message (had a couple of bare #NNN references prow flagged as invalid). Should be clear now, ready whenever you get a chance to take a look.

ajaysundark · 2026-08-07T13:11:56Z

/ok-to-test

ajaysundark

Thanks for the PR. Did you confirm if all the patches are required, please write e2e tests if possible.

ajaysundark · 2026-08-10T02:27:06Z


 		node.Annotations[annotationKey] = bootstrapAnnotationValue(ruleName)
-		if err := r.Patch(ctx, node, patch); err != nil {
+		if err := r.Patch(ctx, node, client.MergeFromWithOptions(stored, client.MergeFromWithOptimisticLock{})); err != nil {


It seems like annotations can merge cleanly (because it is a map?) so optimistic locking here is actually a bad idea, as can compete with Kubelet patches as well..

Recently, we had to add optimistic lock for annotation patch to fix a race condition with remove-taint. This PR had previously suggested that approach, but we initially thought it wasn't necessary. Thank you for pointing this out.

Given it's already handled does this change here for node annotations still necessary?

ajaysundark · 2026-08-10T02:27:06Z

+
+		stored := latest.DeepCopy()
+		controllerutil.RemoveFinalizer(latest, finalizerName)
+		return r.Patch(ctx, latest, client.MergeFromWithOptions(stored, client.MergeFromWithOptimisticLock{}))


Does this issue apply for finalizers as well? Or did you verify only for rule.status. confirm all the patches are required.

bhuvan-somisetty · 2026-08-10T10:53:37Z

Good catch, both of you. Reverted the node annotation patch back to a plain client.MergeFrom — you're right that it merges cleanly as a map, so the optimistic lock there was just going to fight with kubelet's own patches for no reason.

Kept the lock on the two finalizer patches (add + remove) though, since finalizers is a []string and a JSON merge patch replaces list fields wholesale rather than merging them — without the resourceVersion precondition a concurrent write there could get silently dropped. Left a comment at each site explaining why the two cases are handled differently so it's not ambiguous next time.

Also added two unit tests simulating concurrent RuleReconciler/NodeReconciler status writes to make sure the merge-by-node-name logic actually survives a real conflict and retry, not just the happy path.

RuleReconciler and NodeReconciler both patch NodeReadinessRule.Status concurrently, but every status/finalizer patch used a plain client.MergeFrom with no resourceVersion precondition, wrapped in retry.RetryOnConflict. Since a JSON merge patch never carries that precondition unless MergeFromWithOptimisticLock is used, the API server never returns a conflict and the retry wrapper never actually retries. Worse, updateRuleStatus replaced NodeEvaluations/FailedNodes wholesale from a snapshot computed at the start of a RuleReconciler sweep, so it could silently discard a concurrent NodeReconciler per-node update for a node outside that sweep. Fix this by having processAllNodesForRule return a delta of exactly the per-node changes it made, and merging that delta by node name instead of overwriting the whole slice. Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>

bhuvan-somisetty · 2026-08-17T10:35:07Z

Rebased onto latest main branch and resolved all merge conflicts. All tests are passing cleanly.

ajaysundark

Thanks for the PR. I left some comments. Will have a second deeper look later.

ajaysundark · 2026-08-21T03:12:19Z

+// applyNodeStatusDelta merges delta into rule's NodeEvaluations/FailedNodes, replacing only the
+// entries for nodes present in delta and leaving every other node's entry untouched.
+//
+// This is the crux of fixing the lost-update bug described in #341: a naive full-slice
+// replacement of NodeEvaluations/FailedNodes (computed from a nodeList snapshot taken at the
+// start of a RuleReconciler sweep) would silently discard any per-node status update written
+// concurrently by NodeReconciler for a node this particular sweep didn't touch. Merging by node
+// name instead means each writer only ever overwrites the entries it just recomputed.


Can we keep these comments concise? A function doc comment could self describe what functionality it is exposing. Context specific to this issue doesn't belong there, unless it carries long-term maintenance information.

https://go.dev/doc/comment#func is a good ref for comments.

ajaysundark · 2026-08-21T03:16:51Z

-
-			patch := client.MergeFrom(latestRule.DeepCopy())
-
+		err := r.patchRuleStatusWithOptimisticLock(ctx, rule.Name, func(latestRule *readinessv1alpha1.NodeReadinessRule) bool {


I expect this will spike 409s at API for our scale test.

cc @vitorfloriano and I dont think we monitor API conflicts..it may reflect in reconcile latency though, not sure.

ajaysundark · 2026-08-21T03:45:37Z


 	log.Info("Processing all nodes for rule", "rule", rule.Name, "totalNodes", len(nodeList.Items))

+	delta := nodeStatusDelta{


I like this idea in theory, but want to look deeper into the details and risks of it. Will spend more time on this and add more comments.

kubernetes-prow Bot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. kind/bug Categorizes issue or PR as related to a bug. labels Aug 3, 2026

kubernetes-prow Bot requested review from haircommander and tallclair August 3, 2026 10:11

kubernetes-prow Bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 3, 2026

kubernetes-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 3, 2026

bhuvan-somisetty force-pushed the fix-status-patch-optimistic-locking branch from fc312d0 to 68f2b6d Compare August 3, 2026 10:17

kubernetes-prow Bot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Aug 3, 2026

kubernetes-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2026

bhuvan-somisetty force-pushed the fix-status-patch-optimistic-locking branch from 68f2b6d to cb75223 Compare August 5, 2026 06:26

kubernetes-prow Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 5, 2026

kubernetes-prow Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 7, 2026

ajaysundark reviewed Aug 10, 2026

View reviewed changes

tejassinghbhati mentioned this pull request Aug 10, 2026

[BUG] TaintAdopted events are emitted repeatedly for taints the controller applied itself #383

Closed

bhuvan-somisetty force-pushed the fix-status-patch-optimistic-locking branch from dea3fbc to dec6907 Compare August 10, 2026 11:45

kubernetes-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2026

DsThakurRawat mentioned this pull request Aug 10, 2026

docs: add observability design #344

Open

2 tasks

ajaysundark mentioned this pull request Aug 11, 2026

[BUG] bootstrap-only: grandfathering marks bootstrap completed concurrently with taint addition, permanently orphaning the taint #382

Closed

kubernetes-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2026

bhuvan-somisetty force-pushed the fix-status-patch-optimistic-locking branch from dec6907 to fd82d60 Compare August 14, 2026 16:43

kubernetes-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2026

kubernetes-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2026

bhuvan-somisetty force-pushed the fix-status-patch-optimistic-locking branch from fd82d60 to b6eab97 Compare August 17, 2026 10:32

kubernetes-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2026

ajaysundark reviewed Aug 21, 2026

View reviewed changes

ajaysundark mentioned this pull request Aug 21, 2026

Chnages to handle Node taint mutation only through the node controller #389

Draft

2 tasks


		patch := client.MergeFrom(latestRule.DeepCopy())

		err := r.patchRuleStatusWithOptimisticLock(ctx, rule.Name, func(latestRule *readinessv1alpha1.NodeReadinessRule) bool {


		log.Info("Processing all nodes for rule", "rule", rule.Name, "totalNodes", len(nodeList.Items))

		delta := nodeStatusDelta{

Uh oh!

Conversation

bhuvan-somisetty commented Aug 3, 2026

Description

Related

Type of Change

Testing

Checklist

Does this PR introduce a user-facing change?

Uh oh!

netlify Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for node-readiness-controller ready!

Uh oh!

kubernetes-prow Bot commented Aug 3, 2026

Uh oh!

kubernetes-prow Bot commented Aug 3, 2026

Uh oh!

bhuvan-somisetty commented Aug 3, 2026

Uh oh!

ajaysundark commented Aug 7, 2026

Uh oh!

ajaysundark left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaysundark Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaysundark Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

ajaysundark Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

ajaysundark Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

bhuvan-somisetty commented Aug 10, 2026

Uh oh!

bhuvan-somisetty commented Aug 17, 2026

Uh oh!

ajaysundark left a comment

Choose a reason for hiding this comment

Uh oh!

ajaysundark Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

ajaysundark Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

ajaysundark Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

netlify Bot commented Aug 3, 2026 •

edited

Loading

ajaysundark left a comment •

edited

Loading

ajaysundark Aug 10, 2026 •

edited

Loading