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	2c09114
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a97c78189d5020008a02d4c

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

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

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

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!

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

[ajaysundark]

Left some comments, ptal!

This is close, thanks for iterating on this PR.

[ajaysundark]

Actually can we handle skip rule-patch here, instead of checking whether each of the field changed within the mutate function, so all write calls get this check?

Suggested change

	if !mutate(latestRule) {
	return nil
	}
	mutate(latestRule)
	if apiequality.Semantic.DeepEqual(stored.Status, latestRule.Status) {
	return nil
	}

[ajaysundark]

nit: if you no longer need the return value of mutate, this can be void. This could simplify the mutate functions as they only need to transform, dont have to check each of the fields and return false/true.

[ajaysundark]

this can be reverted to previous commit state, by checking the semantic comparison in the patch helper instead.

[ajaysundark]

@bhuvan-somisetty Could you also check the failing lint?

[ajaysundark]

folding the node path onto applyNodeStatusDelta with a single node delta would leave one merge rule
the sort order is applied consistently on both paths

@DsThakurRawat Thanks for taking a review pass on this. I saw the sort-order. But dont follow your single node-delta suggestion. Could you clarify?

happy to share the envtest package if it is useful as a regression test

are you suggesting an e2e test for concurrent rule update scenario? Feel free send a PR if you want us to take a look.

[ajaysundark]

/ok-to-test

[bhuvan-somisetty]

Addressed all suggestions: updated patchRuleStatusWithOptimisticLock to check apiequality.Semantic.DeepEqual to skip unchanged status patches across all callers, simplified the mutate functions to void return, and added node name sorting in NodeReconciler. Thanks!

[rawadhossain]

/retest

[bhuvan-somisetty]

/retest

[DsThakurRawat]

taking both questions in order.

1. the single node-delta suggestion, and why it is not just tidying.

applyNodeStatusDelta already takes a map keyed by node name, so one node is just a
one-entry map. the suggestion is that the inline block in processNodeAgainstAllRules
builds a nodeStatusDelta for this one node and calls it, rather than open coding
find-or-append a second time.

what makes it worth doing is that the two implementations do not agree today. the inline
block starts from a zero value:

currEval := readinessv1alpha1.NodeEvaluation{}
for _, eval := range rule.Status.NodeEvaluations {
    if eval.NodeName == node.Name { currEval = eval; break }
}

then writes currEval into latestRule whether or not the loop found anything.
evaluateRuleForNode returns early on any taint add or remove failure, before
updateNodeEvaluationStatus runs, so in that case the cached rule holds no evaluation for
the node and the zero value is what gets persisted.

on your head eb5a35e merged with current main 4c6b9d2: seeded status with a healthy
evaluation for probe-node and one for other-node, made the node patch fail, called
processNodeAgainstAllRules.

BEFORE: probe-node has a real evaluation, other-node has one too
AFTER : NodeEvaluations count = 2
  [0] nodeName=""           taintStatus=""        conditionResults=0
  [1] nodeName="other-node" taintStatus="Present" conditionResults=0

probe-node's evaluation is gone, replaced by an entry with an empty nodeName.
other-node survives, so merge-by-name is doing its job. it is only the node being
reconciled that gets clobbered, and with the sort you just added the empty name sorts to
index 0.

2. the delta path does not have this, which is the actual argument.

same starting status, same failure expressed the way processAllNodesForRule expresses it,
a failure entry and no evaluation entry:

AFTER delta path: NodeEvaluations count = 2
  [0] nodeName="other-node" taintStatus="Present" conditionResults=0
  [1] nodeName="probe-node" taintStatus="Absent"  conditionResults=1
FailedNodes count = 1
  [0] nodeName="probe-node" reason="EvaluationError"

the evaluation survives and the failure is recorded next to it. applyNodeStatusDelta is
guarded on len(delta.evaluations) > 0, and processAllNodesForRule only puts an entry in
the map when it found one, so "nothing evaluated this sweep" means "leave what is there".
folding the node path onto it fixes the above as a side effect. the full suite is green on
that merged tree, internal/controller included, so nothing currently catches it.

3. on the envtest package, no, i was not proposing an e2e test.

what i had proves a client-go property, merge patch versus optimistic lock. that is true of
any controller and does not belong in this repo, which is why i hedged it rather than
offering it outright.

4. there is a real gap next to it though.

every concurrency test in internal/controller today is either the controller-runtime fake
client with an interceptor scripting a single conflict, or single threaded. none run two
writers concurrently against a real API server. the lost update this PR fixes is a genuine
race and a scripted conflict does not reproduce it. the test worth having is two goroutines
on envtest driving a rule-level and a node-level status write at the same rule, asserting
neither side's entry is lost, plus the empty nodeName case above pinned as a regression.

happy to send that as a PR, either onto this branch or as a follow up once this merges.
tell me which shape you want and i will put it up.

5. unrelated, the lint @bhuvan-somisetty is chasing.

make lint reproduces here as a single gofmt issue at
internal/controller/helper_unit_test.go:238: the map value comments in the new
applyNodeStatusDelta test need realigning, and there is a trailing blank line at end of
file. gofmt -w internal/controller/helper_unit_test.go clears it.

[bhuvan-somisetty]

Good call on using applyNodeStatusDelta in processNodeAgainstAllRules! Refactored processNodeAgainstAllRules to construct a nodeStatusDelta for the target node and pass it to applyNodeStatusDelta, avoiding duplicate logic and preventing zero-value evaluation entries when evaluation fails. Thanks!

[bhuvan-somisetty]

Updated NodeReconciler (processNodeAgainstAllRules) to handle evaluation success and failure distinctly when building nodeStatusDelta:

• On evaluation success: delta.failures[node.Name] = nil to clear any stale failure from latestRule.Status.FailedNodes, and delta.evaluations[node.Name] is set to the fresh evaluation.
• On evaluation failure: delta.failures[node.Name] is set to the recorded failure, and delta.evaluations[node.Name] is omitted so stale evaluations aren't persisted.

Also added unit tests for these cases in helper_unit_test.go. Thanks for catching this!

[ajaysundark]

/lgtm

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajaysundark, bhuvan-somisetty

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ajaysundark]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment