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

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

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?

Good push — checked, and it's still needed, but not for the reason I originally reached for. The annotation merge itself is fine (map, merges cleanly). What the lock actually guards is the hasTaintBySpec check right before the patch: without the resourceVersion precondition, a taint added between that check and the Patch call goes undetected, and we'd mark bootstrap complete on a node that still carries the taint. There's already a regression test for exactly this ("should not mark bootstrap completed when the rule taints concurrently"), and it predates this PR — this lock isn't new, this PR only touched variable naming here. Added a comment inline explaining this so it's not re-litigated later.

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.

Yes, applies to finalizers too, same root cause. Confirmed and covered: ensureFinalizer's add and reconcileDelete's finalizer removal both now go through client.MergeFromWithOptimisticLock (same pattern as addTaintBySpec/removeTaintBySpec). All four call sites (rule status, finalizer add, finalizer remove, cleanupDeletedNodes) share patchRuleStatusWithOptimisticLock or the equivalent inline pattern, so nothing is left on the plain-MergeFrom path.

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.

Fair, trimmed both comments (nodeStatusDelta and applyNodeStatusDelta) to describe what they do rather than the #341 story. Thanks for the go.dev link, wasn't following that convention closely enough here.

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.

Valid concern, and you're right that we weren't watching for it. Added node_readiness_status_patch_conflicts_total (labels: resource, operation), incremented on every 409 from an optimistic-locked patch — rule status, rule finalizer add/remove, node taint add/remove, and the node bootstrap annotation patch. Should give a direct signal instead of inferring it from reconcile latency. Happy to dig into the scale-test numbers with @vitorfloriano once this metric is out there.

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.

ajaysundark · 2026-08-22T20:03:51Z

xref - #320 (comment)

@rawadhossain can we run an experiment with this patch and #320 API conflicts metric? That could be the baseline for #345, on how NRE helps with status handling.

cc @Karthik-K-N

Tighten the nodeStatusDelta/applyNodeStatusDelta doc comments per review feedback to be self-describing instead of narrating the issue they fix. Add node_readiness_status_patch_conflicts_total, incremented whenever an optimistic-locked patch (rule status, rule finalizer add/remove, node taint add/remove, node bootstrap annotation) hits a 409, so the 409 rate this optimistic locking can introduce is observable instead of only showing up indirectly as reconcile latency. Document why markBootstrapCompleted still needs its optimistic lock even though the patch itself is annotation-only and merges cleanly: the lock guards the hasTaintBySpec check preceding it, not the annotation merge, which the "should not mark bootstrap completed when the rule taints concurrently" regression test already covers.

bhuvan-somisetty · 2026-08-22T20:42:07Z

Thanks for the thorough review. Pushed a follow-up commit addressing the open threads: trimmed the nodeStatusDelta/applyNodeStatusDelta doc comments to be self-describing, added node_readiness_status_patch_conflicts_total to make the 409 rate from optimistic locking observable (rule status, rule finalizer add/remove, node taint add/remove, bootstrap annotation), confirmed and documented why markBootstrapCompleted still needs its lock (guards the hasTaintBySpec check, not the annotation merge - existing regression test covers it), and confirmed finalizer add/remove both go through the same optimistic-lock pattern as rule status now. Left the nodeStatusDelta merge-by-name approach itself as-is since you want to look deeper there.

ajaysundark · 2026-08-22T21:01:31Z

+	// StatusPatchConflicts tracks optimistic-lock conflicts on status/annotation patches.
+	// A rising rate here means concurrent writers are contending for the same object; it may
+	// show up as increased reconcile latency before it shows up as visible errors.
+	StatusPatchConflicts = prometheus.NewCounterVec(


@bhuvan-somisetty Could we leave the new metrics from this PR scope to #320 and #288 to handle this holistically?

cc @rawadhossain

Makes sense, dropped it — pulled StatusPatchConflicts and its call sites back out. Will pick this up again in #320/#288 instead.

Deferred to kubernetes-sigs#320/kubernetes-sigs#288 per review feedback so conflict metrics get handled holistically instead of piecemeal here.

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

This was referenced Aug 21, 2026

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

Draft

fix: record conflict failures and fix missing status patch metric #320

Open

ajaysundark reviewed Aug 22, 2026

View reviewed changes

address review: drop StatusPatchConflicts metric from this PR's scope

e1713aa

Deferred to kubernetes-sigs#320/kubernetes-sigs#288 per review feedback so conflict metrics get handled holistically instead of piecemeal here.


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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaysundark commented Aug 22, 2026

Uh oh!

bhuvan-somisetty commented Aug 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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