fix: use optimistic locking on rule status patches

[bhuvan-somisetty]

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.

Related

Fixes #341

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]

✅ Deploy Preview for node-readiness-controller canceled.

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

[kubernetes-prow]

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

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]

@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]

/ok-to-test

[ajaysundark]

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

[ajaysundark]

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

[bhuvan-somisetty]

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]

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.

[bhuvan-somisetty]

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]

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.

[bhuvan-somisetty]

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]

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

[bhuvan-somisetty]

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]

I left some comments, could you ptal?
Thanks for iterating on this.

[ajaysundark]

Suggested change

	// The optimistic lock here isn't guarding the annotation merge itself (that's map-valued
	// and merges cleanly against concurrent writers, e.g. Kubelet). It guards the
	// hasTaintBySpec check above: without it, a taint added between that check and the Patch
	// below would go undetected, and we'd mark bootstrap complete on a node that still carries
	// the taint. See the "should not mark bootstrap completed when the rule taints concurrently"
	// test for the regression this prevents.
	// The optimistic lock here protects from a race-condition adding a taint between hasTaintBySpec
	// check and mark completed annotation patch from concurrent reconciliations.

[ajaysundark]

can we add unit-tests for these?

[ajaysundark]

Suggested change

	// processAllNodesForRule processes all nodes when a rule changes. It mutates rule.Status in place
	// (as before) and additionally returns a nodeStatusDelta describing exactly which nodes' status
	// this sweep changed, so updateRuleStatus can merge those changes into the latest stored status
	// instead of replacing NodeEvaluations/FailedNodes wholesale.
	// processAllNodesForRule processes all nodes when a rule changes. It mutates rule.Status in place
	// and additionally returns a nodeStatusDelta describing exactly which nodes' status are changed.
	// so updateRuleStatus can merge those changes into the latest stored status
	// instead of replacing NodeEvaluations/FailedNodes wholesale.

[ajaysundark]

Suggested change

	// patchRuleStatusWithOptimisticLock fetches the latest NodeReadinessRule, lets mutate apply status
	// changes to it, and patches the result back with an optimistic-locked JSON merge patch. mutate
	// should return false if it made no changes, to skip an unnecessary Patch call.
	//
	// We use client.MergeFromWithOptimisticLock here for the same reason addTaintBySpec/
	// removeTaintBySpec do (see node_controller.go): a JSON merge patch replaces slice fields
	// (NodeEvaluations, AppliedNodes, FailedNodes) wholesale rather than merging them, so without a
	// resourceVersion precondition retry.RetryOnConflict can never observe a genuine conflict and a
	// concurrent status write from the other reconciler (RuleReconciler and NodeReconciler both patch
	// NodeReadinessRule.Status independently) can be silently overwritten.
	// patchRuleStatusWithOptimisticLock fetches the latest NodeReadinessRule, and apply mutate status
	// changes to it. It then patches the result to API with an optimistic-locked JSON merge patch. mutate
	// should return false if it made no changes, to skip an unnecessary Patch call.
	//
	// We use client.MergeFromWithOptimisticLock here for a JSON merge patch replaces slice fields
	// (NodeEvaluations, AppliedNodes, FailedNodes) wholesale rather than merging them, so without a
	// resourceVersion precondition retry.RetryOnConflict can never observe a genuine conflict and a
	// concurrent status write from the other reconciler (RuleReconciler and NodeReconciler both patch
	// NodeReadinessRule.Status independently) can be silently overwritten.

[ajaysundark]

should updateRuleStatus skip patch when delta is empty?

[ajaysundark]

/ok-to-test

[bhuvan-somisetty]

Updated comments and docstrings per suggestions, added unit tests for applyNodeStatusDelta, and updated updateRuleStatus to return early when there are no status changes. Thanks for the review!

[kubernetes-prow]

@bhuvan-somisetty: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-node-readiness-controller-lint	89bc5b4	link	true	/test pull-node-readiness-controller-lint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[DsThakurRawat]

took this for a run locally since it has been open a while and main has moved under it.

the headline, because it is the thing that goes stale: i merged current upstream/main
(6723e00) into your branch head (89bc5b4) and it is clean. zero conflicts across the
three files that both touched, go build ./... and go vet ./... clean, and the full
suite green on the merged tree (internal/controller 16.3s, plus reporter, metrics and
webhook packages). so the rebase is not going to be the hard part.

on the premise, i wanted to see the two behaviours rather than trust the docs, so i put a
throwaway envtest package on the branch and made two writers race a real API server:

plain client.MergeFrom from a stale copy
  writer A held rv=199, object had moved to rv=203
  patch err  = <nil>
  isConflict = false
  -> patch SUCCEEDED from a stale copy

client.MergeFromWithOptimisticLock from a stale copy
  writer A held rv=207, object had moved to rv=208
  patch err  = Operation cannot be fulfilled on nodes "lockproof-opt":
               the object has been modified; please apply your changes to the latest
               version and try again
  isConflict = true

so the retry.RetryOnConflict wrappers really were unreachable, exactly as your
description says.

the slice half is the part i found more interesting, because on a map field the unlocked
patch is harmless. i seeded a node with taint a, let writer B append taint b, then had
a stale writer A write back [a, c]:

writer A snapshot taints: [a]
after writer B          : [a b]
after writer A          : [a c]
-> writer B's taint "b" was silently discarded. no error, no conflict.

which is why the optimistic lock on its own would not have been enough and the per node
delta is doing real work.

three things i checked because they are where this kind of change usually goes wrong, and
all three hold:

• applyNodeStatusDelta runs inside the patchRuleStatusWithOptimisticLock closure, so it
  re-applies against the freshly fetched object on every retry attempt rather than against
  the snapshot. that is the bit that would quietly undo the fix if it were outside.
• the nodeStatusDelta docstring says AppliedNodes, ObservedGeneration and
  DryRunResults have a single writer and are safe to overwrite. grepping for writes to
  those three, they only ever appear in nodereadinessrule_controller.go, never in
  node_controller.go, so excluding them is right.
• switching from wholesale replace to merge normally means entries for deleted nodes stop
  being pruned. cleanupDeletedNodes still re-filters against the fresh object inside its
  own closure, so that path is unaffected.

one thing i went looking for and did not find, worth recording so nobody else spends the
time: #381 landed after your merge base and adds clearNodeFailure, which mutates the
stale in memory rule rather than the fresh one. that looked like it would reintroduce
the same lost update through a new door. it does not. on the merged tree the node path
rebuilds FailedNodes inside the locked closure keeping other nodes' entries and
re-appending only this node's, so the clear is expressed per node and lands correctly. it
is correct but indirect, since clearNodeFailure's only observable job is to make that
append loop find nothing.

which leads to the one suggestion i have, and it only becomes visible once both changes
sit on the same branch. after the merge there are two per node merge implementations: your
applyNodeStatusDelta in helper.go, and the inline evaluation and FailedNodes rebuild
in processNodeAgainstAllRules. they do the same thing with different code. folding the
node path onto applyNodeStatusDelta with a single node delta would leave one merge rule
to reason about instead of two, and would mean the sort order is applied consistently on
both paths. entirely your call whether that belongs here or in a follow up, since this PR
is already doing enough.

happy to share the envtest package if it is useful as a regression test, though it is
proving a client-go property rather than anything about this repo, so it probably does not
belong in the tree.