fix(operator): merge node-state deltas instead of restamping a stale snapshot by ayuskauskas · Pull Request #413 · NVIDIA/nodewright

ayuskauskas · 2026-08-06T01:54:33Z

Refs #411 (item 1). Does not close it — the other items there are still open. Part of #223, based on feature/package-as-jobs.

The bug

nodewright.nvidia.com/nodeState_<name> is one annotation whose value is a single JSON document covering every package on the node. Three controllers write it, each with its own workqueue:

Writer	Queue	How it wrote
Heavy pass (`SaveNodesAndSkyhook`)	NodeWright CR	whole value, from a snapshot, unlocked
`JobReconciler`	Job	delta, optimistic-locked + retry
`PodReconciler`	Pod	delta, optimistic-locked + retry

Before the Job and Pod watches became their own controllers they rode the heavy pass's queue via the pod--- prefix at MaxConcurrentReconciles: 1, so pod-driven writes could not interleave with the pass. Splitting them removed that guarantee without replacing it. This is a regression introduced by this branch, not a bug on main — on main the node has exactly one operator writer at a time.

The heavy pass builds its result from a snapshot taken at cluster-state build time. If JobReconcile records package A complete while the pass is working on package B, the pass then writes its whole snapshot-derived value and reverts A to in_progress.

The trigger is the ordinary path, not an exotic interleaving: non-Serial is the default, RunNext returns every DAG-ready package, so a multi-package Skyhook routinely has several packages in flight on one node. The window is the whole pass body, and the pass requeues every 2s while incomplete. Single-package Skyhooks are largely immune — the clobber needs the pass and the Job to be touching two different entries.

What it costs

Not a stall. Nothing re-records the completion (the Job is already marked state-recorded), but shouldDeleteFinishedJob detects exactly this divergence — a processed, Complete Job whose node-state entry still sits at that stage, not complete — and tears the Job down, so the next pass recreates it and the stage runs a second time. Recovery is a couple of 2s passes.

The cost is that second run:

a redundant Job + pod cycle per lost completion;
the node holds its interruption-budget slot for that extra cycle — the budget is what paces the rollout, so on a tight budget this is directly rollout wall-clock;
node status and conditions flap in_progress ↔ complete, with the events and metrics behind them.

Two things it is not: a re-run interrupt does not reboot again (the agent flag-files interrupts per SKYHOOK_RESOURCE_ID on the host root), and nothing is re-evicted (DrainNode short-circuits on IsDrained while the node stays cordoned throughout).

But that idempotence is narrower than it looks, and it is what the blast radius actually rests on. check_flag_file runs the step anyway — flag present or not — for three whole modes:

Stage	Re-run behaviour
`apply`, `post-interrupt`	skipped — flag-gated, `Idempotence.Auto` is the default
`interrupt`	skipped — separate per-`SKYHOOK_RESOURCE_ID` flag
`config`	re-executed on the host
`upgrade`	re-executed on the host
`uninstall`	re-executed on the host
any step with `idempotence: disabled`	re-executed on the host

So a clobbered config: complete is not a no-op pod cycle — it re-runs the package's config step, which for most packages rewrites config and restarts a service. config is a normal stage in the ordinary apply -> config sequence, so this is not a corner case.

Convergence is also probabilistic rather than bounded: nothing counts re-runs, and each re-run is itself a blob write (ApplyPackage upserts in_progress), so a busy multi-package node gets fresh exposure to the same race on every cycle.

The net of it is that the operator's safety on this path currently rests on the agent's idempotence rather than on its own state machine — and that guarantee is opt-out, by mode and by a per-step knob any package author can set.

Why a lock alone was not the fix

This was the tempting one-liner, and it does not work. The value is computed long before the write, so an optimistic lock just serializes a stale value into place. To fix it with locking alone the lock would have to span read → write, which for the heavy pass is an entire whole-world reconcile.

It is also what the earlier attempt did: #382 records the measured cost (0 → 156 conflicts across CI runs, since reverted). Those conflicts were pure loss, because each retry re-sent the same stale whole value.

The fix

Derive the pass's delta — which package entries it actually changed — by diffing its starting snapshot against its result, then apply only that on top of whatever the annotation holds at write time, under an optimistic lock with retry. Entries the pass never touched keep whatever another writer put there, so a retry converges instead of clobbering.

Only that one value is re-derived. Everything else the pass changed stays an ordinary strategic-merge diff against the pass's own snapshot, exactly as before this PR. That is what keeps the patch from mentioning any label, annotation, taint or cordon this pass did not touch — a key present in neither the snapshot nor the pass's object cannot appear in the diff at all.

Two details worth review attention:

The diff base stays the snapshot; only the resourceVersion comes from the fresh read. The base does two jobs — left-hand side of the diff, and source of the optimistic-lock precondition — and MergeFromWithOptimisticLock reads the version from the base, so the two can be served by different objects. spec.unschedulable and spec.taints then need no special handling at all: unchanged by the pass means absent from the diff.
SkyhookReconciler gains the uncached reader that NewJobReconciler already takes. Without it RetryOnConflict re-reads through the informer, rebuilds the same resourceVersion precondition that just lost, and burns every attempt. Same split, and same reason, as patchNodeState. It is also what makes the first read safe to serve from cache: a stale cached read merges the delta onto a stale value, but its precondition is stale too, so it 409s and the retry re-reads from the apiserver.

An earlier revision of this PR rebuilt the whole patch target from the fresh read and hand-replayed each metadata surface onto it. That was dropped: the premise (that a snapshot-based diff would emit deletions for keys another writer added) does not hold, and the hand-rolled replay it forced is where the only two clobbers in this PR came from. 122 lines went with it.

Scope

Deliberately just the lost update. Not in scope:

JobReconciler: key the workqueue by node for per-node serial, cross-node parallel reconciles #382 (node-keyed JobReconciler workqueue). Complementary — this makes that safe, but does not do it.
Heavy pass can delete foreign node taints (atomic list restamped from a stale snapshot) #447 — the heavy pass can delete a foreign taint when it does change a taint (spec.taints is listType=atomic, so the whole list is replaced from the snapshot). Pre-existing on main, unchanged here.
Node state is one multi-writer JSON blob; key it per package so strategic merge handles concurrency #448 — the structural fix: key node state per package so strategic merge handles the concurrency natively and this PR's delta machinery can be deleted. Needs a node-state migration.
Folding PodReconciler into JobReconciler. Every pod failure that matters increments job.Status.Failed and so already produces a Job event, which would remove a third writer entirely. Worth doing, bigger change, and it needs a story for CrashLoopBackOff under restartPolicy: OnFailure (interrupt Jobs), where Job status does not change.

Testing

make unit-tests passes (504 specs across 8 suites), golangci-lint 0 issues.

The regression test — "does not revert a completion recorded after the pass took its snapshot" — drives the real saveNodeChanges path against a client whose stored node has diverged from the pass's snapshot. I verified it fails against the old overwrite behaviour before keeping it, by temporarily restoring the restamp and confirming the failure, so it is not vacuous.

That same spec now also pins the foreign-metadata guarantees at the real seam: the stored node carries a label, a second NodeWright's nodeState_* key, an autoscaler taint and a cordon that this pass never saw, and all four must survive the write. Verified to fail when the diff base is swapped back to the fresh read.

Also covered: delta computation (changed / added / removed), preserving a concurrent completion, preserving another NodeWright's key, applying a removal over a changed current value, the conflict retry and its uncached re-read, the missing-snapshot error, and the node-deleted-mid-retry path.

Not covered: real concurrent execution. The race is exercised by construction (a diverged stored object), not by running two controllers at once, which would be flaky.

…snapshot The heavy pass, JobReconciler and PodReconciler are three controllers with three workqueues, all writing nodewright.nvidia.com/nodeState_<name> — one annotation whose value is a single JSON document covering every package. Before the Job and Pod watches became their own controllers they rode the heavy pass's queue at MaxConcurrentReconciles 1 and could not interleave; splitting them removed that guarantee without replacing it. The heavy pass builds its result from a snapshot taken at cluster-state build time and then patches the whole value unconditionally. A completion recorded by JobReconcile in between is reverted to in_progress, and nothing re-records it because the Job is already marked state-recorded: the stage recovers only by being torn down and re-run. Locking the write would not have fixed it. The value is computed long before the write, so a lock just serializes a stale value into place. Instead derive the pass's delta by diffing its starting snapshot against its result, and apply only that on top of whatever the annotation holds at write time, under an optimistic lock with retry. Entries the pass never touched keep whatever another writer put there. The patch target is rebuilt from the freshly read node rather than the pass's own object, deliberately: that object predates any concurrent write, so diffing against it would emit deletions for keys another writer added since the snapshot — another NodeWright's nodeState_* among them. SkyhookReconciler gains the uncached reader NewJobReconciler already takes. Without it RetryOnConflict re-reads through the informer, rebuilds the same resourceVersion precondition that just lost, and burns every attempt — which is how the earlier optimistic-locking attempt produced a conflict storm. The regression test was verified to fail against the old overwrite behaviour before being kept. Refs #411 (item 1). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>

coderabbitai · 2026-08-06T01:58:18Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

The reconciler now uses an uncached Kubernetes reader for conflict retries. Node persistence computes heavy-pass deltas and applies them to freshly read nodes instead of overwriting whole snapshots. Optimistic-lock conflicts trigger retries, while concurrent node-state entries and selected metadata changes are preserved. Reconciler construction and tests now pass the additional reader. New tests cover delta merging, stale snapshots, metadata replay, no-op behavior, and concurrent completion persistence.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: rice-riley, lockwobr

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main fix: merging node-state deltas instead of overwriting state with a stale snapshot.
Description check	✅ Passed	The description directly explains the lost-update bug, the optimistic-locking fix, scope, and test coverage.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch jobs-migration/411-nodestate-lost-update

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@operator/internal/controller/node_state_merge_test.go`:
- Around line 210-286: Extend the saveNodeChanges tests with coverage for both
missing paths: use interceptor.NewClient to make the first Patch return
apierrors.NewConflict, then verify the retry reads via the uncached reader and
the merged state is eventually persisted; also add an untracked-node test
calling saveNodeChanges with original == nil and assert the node is actually
updated, exposing and fixing the empty-diff handling in saveNodeChanges.

In `@operator/internal/controller/skyhook_controller.go`:
- Around line 1352-1359: Wrap every propagated error returned by saveNodeChanges
and readNodeForSave with contextual fmt.Errorf messages using the %w verb,
including errors from parseNodeState and the patch/save operations. Preserve the
underlying errors so retry.RetryOnConflict can continue detecting conflicts
through apierrors.IsConflict, and eliminate all bare err returns in these
functions.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 2b4a015b-c73a-4973-bebc-4d0f761f7173

📥 Commits

Reviewing files that changed from the base of the PR and between a3dd68c and 077ad94.

📒 Files selected for processing (8)

docs/designs/2026-07-10-package-execution-as-jobs.md
operator/cmd/manager/main.go
operator/internal/controller/node_state_merge_test.go
operator/internal/controller/skyhook_controller.go
operator/internal/controller/skyhook_controller_test.go
operator/internal/controller/suite_test.go
operator/internal/controller/swap_test.go
operator/internal/controller/workload_migration_test.go

coveralls · 2026-08-06T02:15:17Z

Coverage Report for CI Build 31653882834

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Warning

No base build found for commit 5dd9536 on feature/package-as-jobs.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 78.974%

Details

Patch coverage: 12 uncovered changes across 2 files (97 of 109 lines covered, 88.99%).

Uncovered Changes

File	Changed	Covered	%
operator/internal/controller/skyhook_controller.go	100	90	90.0%
operator/internal/wrapper/node.go	8	6	75.0%
Total (3 files)	109	97	88.99%

Coverage Regressions

Requires a base build to compare against. How to fix this →

Coverage Stats


Relevant Lines:	13612
Covered Lines:	10750
Line Coverage:	78.97%
Coverage Strength:	8.17 hits per line

💛 - Coveralls

…e's blast radius Review of #413 found that rebuilding the patch target from the freshly read node — the step that makes the node-state merge work — quietly turned two other fields into whole-value restamps, and left the wrapper answering from a cache the merge had invalidated. spec.taints and spec.unschedulable were assigned from the pass outright on the grounds that this controller owns them. It does not: the cluster autoscaler, node-problem-detector and a human with kubectl all write taints, and hasSkyhookCordon exists precisely because two Skyhooks can hold a cordon on one node. With the fresh read as the patch base, an untouched field became an explicit write — taints are listType=atomic so the list is replaced, and Unschedulable is omitempty so false diffs as an explicit null. A pass that never touched either could delete an autoscaler taint or uncordon a node another Skyhook was draining. Both now replay only what the pass changed. skyhookNode caches a parsed copy of node state and State() serves it whenever it is non-nil, so replacing the embedded Node with the merged one left IsComplete, NextStage and UpdateCondition reading the pre-merge map. That published a NotReady/Incomplete condition for a node that had just completed and dropped its MarkComplete event. The wrapper gains InvalidateStateCache, matching the nodeState = nil precedent Reset and CleanupSCRMetadata already use, and the merge calls it. The node condition patch was still based on the pass's build-time snapshot while being applied to the post-merge object. That re-sent whatever the kubelet had changed in .status and carried the snapshot's resourceVersion into the patch body, where the apiserver reads it as a precondition — so any concurrent write turned it into a 409, and that error gates both the Skyhook's UpdateCondition and its status patch. It is now based on the post-merge object, so the diff is exactly the conditions UpdateCondition touches. The untracked-node branch returned StrategicMergeFrom against a copy of the object itself, which is an empty patch: a broken invariant would have silently dropped the write. BuildState tracks and adds a node in the same step so it is unreachable; it now says so. Also drops the duplicated node reader in favour of the existing readNodeForPatch, which is a free function for exactly this reason. Both new spec-field regressions and the cache-coherence assertion were verified to fail against the unfixed code before being kept. Refs #411 (item 1). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>

ayuskauskas · 2026-08-06T02:39:23Z

Ran an adversarial review against this. It found a blocker I introduced, plus three should-fixes — all valid, all addressed in ff2ca87.

Blocker: the fresh-read patch base silently made `spec` a whole-value restamp

applyPassChanges assigned spec.taints and spec.unschedulable from the pass outright, with a comment claiming this controller owns them. It does not — the cluster autoscaler, node-problem-detector and a human with kubectl taint all write taints, and hasSkyhookCordon exists precisely because two Skyhooks can hold a cordon on one node.

The reason this was a regression rather than a pre-existing hazard is the same change that fixes the annotation: once the patch base is the fresh read, any field where fresh and the snapshot disagree becomes an explicit patch operation. Taints is listType=atomic (strategic merge replaces the whole list) and Unschedulable is omitempty (false diffs as an explicit null). The old code diffed against the snapshot, so untouched fields were simply absent from the patch body.

Concrete failure inside one reconcile: Skyhooks are processed sequentially from one BuildState snapshot. A cordons node N and writes. B's pass — its own copy of N, Unschedulable: false — conflicts, retries uncached, sees the cordon, and writes unschedulable: null anyway. N is uncordoned mid-drain, and cordon_A survives in the annotations so nothing looks wrong.

Both now replay only what the pass actually changed, taints matched by key+effect.

Should-fixes

Wrapper cache staleness — the one I flagged to the reviewer as a suspicion, and it was real. State() serves the cached parsed map whenever non-nil and never re-parses, so *node.GetNode() = *target left IsComplete/NextStage/UpdateCondition reading the pre-merge map. That publishes NotReady/Incomplete for a node that just completed and drops its MarkComplete event. Added InvalidateStateCache, matching the nodeState = nil precedent Reset and CleanupSCRMetadata already use.

Condition patch base — still built from the build-time snapshot while applied to the post-merge object. It re-sent whatever the kubelet changed in .status, and carried the snapshot's resourceVersion into the patch body where the apiserver reads it as a precondition. So any concurrent write — exactly the contention this PR exists for — turned it into a 409, and that error gates both the Skyhook's UpdateCondition and its status patch. Now based on the post-merge object.

Untracked-node branch — StrategicMergeFrom against a copy of the object itself is an empty patch, so a broken invariant would have silently dropped the write. BuildState tracks and adds a node in the same step, so it's unreachable; it now returns an error saying so.

Also dropped the duplicated node reader for the existing readNodeForPatch, which is a free function for that reason.

Verification

Two new spec-field regressions and a cache-coherence assertion. I sabotaged both fixes and confirmed 3 specs fail, then restored:

[FAIL] saveNodeChanges does not revert a completion recorded after the pass took its snapshot
[FAIL] applyPassChanges leaves a cordon and taints the pass never touched alone
[FAIL] applyPassChanges applies the pass's own cordon and taint changes without disturbing others

make unit-tests green, golangci-lint 0 issues.

Still open, and worth a human's call: no test forces a conflict, so RetryOnConflict and the uncached branch remain uncovered. That gap already exists for patchNodeState, so this inherits rather than creates it — but it's the least-tested part of the change.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 1470-1477: The taint merge loop around taintID must replay
pass-owned edits after concurrent deletion: when a modified taint identity is
absent from fresh, append it if it was not in original or its value differs from
the original taint. Preserve skipping identities still present in fresh, and add
a regression test covering an original-to-modified value change followed by
deletion from fresh.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 3c109772-43f2-4cdc-9279-90ca07a739f6

📥 Commits

Reviewing files that changed from the base of the PR and between 077ad94 and ff2ca87.

⛔ Files ignored due to path filters (2)

operator/api/nodewright/v1alpha1/zz_generated.deepcopy.go is excluded by !**/zz_generated*.go
operator/api/v1alpha1/zz_generated.deepcopy.go is excluded by !**/zz_generated*.go

📒 Files selected for processing (11)

operator/config/crd/bases/nodewright.nvidia.com_deploymentpolicies.yaml
operator/config/crd/bases/nodewright.nvidia.com_nodewrights.yaml
operator/config/crd/bases/skyhook.nvidia.com_deploymentpolicies.yaml
operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml
operator/config/rbac/role.yaml
operator/config/webhook/manifests.yaml
operator/internal/controller/node_state_merge_test.go
operator/internal/controller/skyhook_controller.go
operator/internal/wrapper/mock/SkyhookNode.go
operator/internal/wrapper/mock/SkyhookNodeOnly.go
operator/internal/wrapper/node.go

💤 Files with no reviewable changes (6)

operator/config/crd/bases/skyhook.nvidia.com_deploymentpolicies.yaml
operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml
operator/config/crd/bases/nodewright.nvidia.com_deploymentpolicies.yaml
operator/config/crd/bases/nodewright.nvidia.com_nodewrights.yaml
operator/config/rbac/role.yaml
operator/config/webhook/manifests.yaml

These were the last untested part of the node-state merge, called out on the PR: nothing in the suite forced a conflict, so neither RetryOnConflict nor the uncached branch of readNodeForPatch ever ran. The optimistic lock is only worth anything if a conflict is retried AND the retry re-derives against a fresh read, and neither half was being checked. The first test forces a conflict on the first patch via an interceptor client (the pattern job_controller_test.go already uses) and supplies the uncached reader with a node the cached client does not have: a completion only the apiserver knows about. It then asserts the patch was retried, that exactly one uncached read happened (attempt 0 stays cached), and that the retry merged its delta onto what the uncached read returned rather than onto the stale cache. That last assertion is what makes the uncached branch load-bearing rather than an optimisation nobody would notice losing. The second covers the node being deleted between attempts, which returns nil rather than an error: a node that went away mid-pass has no state to resurrect. Both were verified against sabotaged code before being kept. Disabling the uncached branch fails both; replacing RetryOnConflict with a single call fails both. Refs #411 (item 1). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>

ayuskauskas · 2026-08-06T13:28:35Z

Closed the coverage gap I flagged, in 115d1a4. RetryOnConflict and the uncached branch of readNodeForPatch are no longer dead weight.

The retry test forces a conflict on the first patch with an interceptor client (the pattern job_controller_test.go:453 already uses), and hands the uncached reader a node the cached client does not have — a completion only the apiserver knows about. It then asserts three things:

the patch was retried rather than surfaced
exactly one uncached read happened, proving attempt 0 stayed on the cache and only the retry went to the apiserver
the retry merged its delta onto what the uncached read returned

That third assertion is the one that matters. Without it the uncached branch is an optimisation nobody would notice losing; with it, reading from the stale cache on retry produces a wrong final state and fails.

The second test covers the node being deleted between attempts — readNodeForPatch returns nil, nil on NotFound and saveNodeChanges stops without error, since a node that went away mid-pass has no state to resurrect.

Verified load-bearing, same method as the earlier fixes — sabotage, confirm failure, restore:

Sabotage	Result
`readNodeForPatch` never uses the uncached reader	both specs fail
`RetryOnConflict` replaced with a single call	both specs fail

make unit-tests green (297 controller specs), golangci-lint 0 issues.

That was the last thing I had listed as untested on this PR.

CodeRabbit review on #413. Three findings; one was already closed by the adversarial-review pass, two were not. Wrap the propagated errors in saveNodeChanges. Five bare `return err` values from a function that reads, merges and patches, which the coding guidelines forbid outright. The patch error is wrapped with %w deliberately: RetryOnConflict decides via apierrors.IsConflict, which unwraps, so the retry keeps working — and the conflict-retry spec added in 115d1a4 fails if that ever stops being true, which is how this was checked rather than assumed. Replay a pass-owned taint edit that a concurrent delete removed. If original and modified hold the same taint identity with different values and the fresh read no longer has it, applyTaintChanges dropped the pass's edit: the second loop only appended identities absent from original. It now appends when the pass added the taint OR changed the value of one it inherited, while an identity the pass left exactly as it found it still yields to the concurrent deletion. This one is latent rather than live: Taint() early-returns when the key is already present and RemoveTaint() only removes, so the operator has no path that edits a taint's value in place and the scenario is unreachable today. Pinned with specs on both sides of the rule so it stays closed if such a path ever appears. Also adds the untracked-node spec CodeRabbit asked for, inverted to match what that branch now does. Its suggestion — assert the node still changed — was right for the original code, which diffed the object against a copy of itself and silently dropped the write. The branch returns an error now, so the spec asserts that instead: a broken invariant must not look like success. The retry-loop half of that same finding was already covered by 115d1a4. Refs #411 (item 1). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>

ayuskauskas · 2026-08-06T13:41:15Z

Went through all three CodeRabbit comments. Two were on 077ad94, before the adversarial-review pass, so the question was which had already been closed by it. Answer: one had, two hadn't. All resolved now in c4b0cfb.

#	Finding	Already fixed by our review?
1	Coverage for the retry loop	Yes — `115d1a4`
1	Coverage for the untracked-node branch	Defect yes, test no → added
2	Wrap the propagated errors	No → fixed
3	Taint edit lost after a concurrent delete	No, and it post-dates the fix → fixed

#2 — error wrapping. Five bare return err from a function that reads, merges and patches. Guidelines forbid it outright. Wrapped, patch error with %w. CodeRabbit's note that RetryOnConflict unwraps via apierrors.IsConflict is the risk worth naming — and I didn't have to take it on faith, because the conflict-retry spec from 115d1a4 fails if wrapping ever breaks conflict detection. It still passes, so the retry still works.

#3 — taint edit lost. Correct reading of the code: the second loop only appended identities absent from original, so an identity present in both original and modified with a changed value, deleted concurrently, lost the pass's edit.

Worth being precise about severity though — this is latent, not live. Taint() early-returns when the key is already present (node.go:467-471) and RemoveTaint() only removes, so the operator has no code path that edits a taint's value in place; the scenario is unreachable today. Fixed anyway since it's two lines, with specs on both sides of the rule: a pass-owned edit survives a concurrent delete, and a taint the pass never touched still yields to one. Getting only the first half right would resurrect taints the operator has no business restoring.

#1 — untracked branch. The suggestion was to assert the node still changed. That was right for the original code, which diffed the object against a copy of itself — an empty patch that silently dropped the write. The adversarial review caught that and the branch now returns an error, so I inverted the spec: it asserts the error, because the one thing a broken invariant must not do is look like success.

All new specs sabotage-verified. Disabling the taint edit-replay fails exactly the one spec that covers it. make unit-tests green, golangci-lint 0 issues.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 1478-1484: Update the taint comparison in the result-building
logic around originalByID so inherited taints are compared by value using
apiequality.Semantic.DeepEqual rather than struct inequality; retain
resurrection for missing identities or genuinely changed taints. Add a
regression test covering unchanged taints with equal non-nil
corev1.Taint.TimeAdded values to ensure concurrent deletion is not undone.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: f36ad85a-53d9-4cd8-ae2d-29931afdcd63

📥 Commits

Reviewing files that changed from the base of the PR and between 115d1a4 and c4b0cfb.

📒 Files selected for processing (2)

operator/internal/controller/node_state_merge_test.go
operator/internal/controller/skyhook_controller.go

CodeRabbit review on #413, and a real bug in the taint fix from c4b0cfb. corev1.Taint carries TimeAdded *metav1.Time, so `originalTaint != taint` compares pointer identity, not the instant. original and modified are separate DeepCopies and DeepCopy allocates a fresh *metav1.Time, so every taint that carries TimeAdded compared as edited even when the pass never touched it — and the edit-replay branch then resurrected taints another writer had concurrently deleted. That is the exact failure mode the surrounding code exists to prevent. Key and Effect are the taint's identity, so Value is the only thing an edit can change; comparing it sidesteps the pointer entirely. TimeAdded is set by the system rather than this operator, so it has no business driving a replay decision either way. The existing specs missed it because every fixture left TimeAdded nil, where two nil pointers compare equal. The new one sets it, and fails against the struct compare. Refs #411 (item 1). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>

ayuskauskas · 2026-08-06T13:53:47Z

Correct, and this was a real bug I introduced in c4b0cfb — fixed in 75b3834.

corev1.Taint carries TimeAdded *metav1.Time, so originalTaint != taint compares pointer identity, not the instant. original and modified are separate DeepCopies and DeepCopy allocates a fresh *metav1.Time, so any taint carrying TimeAdded compared as edited even when the pass never touched it — and the edit-replay branch then resurrected taints another writer had concurrently deleted.

That is precisely the failure mode this function exists to prevent, so the fix I added yesterday for the latent case had reintroduced the live one in a narrower form.

Now compares Value. Key and Effect are the taint's identity, so Value is the only thing an edit can change, and TimeAdded is set by the system rather than this operator — it has no business driving a replay decision either way.

Why the existing specs missed it: every fixture left TimeAdded nil, where two nil pointers compare equal. The new spec sets it, and fails against the struct compare:

[FAIL] applyPassChanges lets a concurrent delete stand for an untouched taint that carries TimeAdded

Worth noting for the record: I had the earlier reviewer explicitly verify PackageStatus was comparable with != before relying on it there, and it is — four strings, two string-typed enums, an int32. I then reached for the same idiom on corev1.Taint without re-checking, and that one has a pointer. Same idiom, different struct, opposite answer.

make unit-tests green, golangci-lint 0 issues.

cli-e2e has failed on every commit since ff2ca87, consistently, on cli-deployment-policy-reset: the rollout stalls at progressPercent 50 with status waiting and an empty batchState. This is the cause. ff2ca87 added InvalidateStateCache, which set skyhookNode.nodeState to nil so the next State() would re-parse the merged annotation. State() does re-parse when the cache is nil — but IsComplete, NextStage, GetComplete and PackageStatus read node.nodeState DIRECTLY and never go through State(). A nil cache therefore reads as "no package has any state": a node that had just completed reported incomplete, its MarkComplete event never fired, RemoveNodePriority was never called, and skyhook.IsComplete stayed false, so the DeploymentPolicy batch never advanced past the first node. NodeState.Upsert makes it worse still — it allocates a fresh map over a nil one, so the next write on that wrapper would have dropped every other package's entry. Replaced with ReloadState, which re-parses into the cache rather than leaving it nil. That is what the review actually asked for; nilling it was my substitution and it was wrong. The unit test missed it because it asserted through State(), the one accessor that tolerates a nil cache by re-parsing. It now also asserts through PackageStatus, which reads the cache directly, and fails against the nil-cache version. Refs #411 (item 1). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>

…ed value Review found the justification for rebuilding the patch target from the freshly read node does not hold. A two-way strategic merge diff is computed from the snapshot and the pass's object alone, so a key present in neither -- another NodeWright's nodeState_*, a foreign label, an autoscaler taint -- cannot appear in the patch at all. The emitted body for a pass that edits one annotation and two labels is exactly: {"metadata":{"annotations":{"...nodeState_a":"..."},"labels":{"added":"1","drop":null}}} Rebuilding from the fresh read is what turned every untouched field into an explicit write, and applyPassChanges/applyMapChanges/applyTaintChanges existed only to undo that. The taint half did not fully succeed: its first loop restamped the pass's value over a taint another writer had edited, and dropped TimeAdded that the apiserver had stamped on a NoExecute taint. Both were reachable by a pass that changed no taints at all. So keep the pass's snapshot as the diff base and splice the fresh resourceVersion into it for the lock. The base serves two roles -- left-hand side of the diff, and source of the optimistic-lock precondition -- and MergeFromWithOptimisticLock reads the version from the base, so the two can be served by different objects. Only the contended annotation value is re-derived. spec.unschedulable and spec.taints then need no handling whatsoever: unchanged by the pass means absent from the diff. That removes 122 lines and both taint regressions with them. Clobbering a foreign taint when the pass DOES change a taint is pre-existing on main and is tracked in #447; the structural fix that would delete the delta machinery outright is #448. Also guards the merge on the pass having kept the annotation. Reset() deletes it outright and means it, so re-merging would resurrect the key it just wiped -- a divergence from main that the fresh-read version shared. The foreign-metadata assertions move off applyPassChanges unit specs onto the real saveNodeChanges path, where a stored node carries a foreign label, a second NodeWright's state key, an autoscaler taint and a cordon that this pass never saw. Verified to fail with the base swapped back to the fresh read. Refs #411 (item 1). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>

Two gaps found auditing what the node-state work actually proves. The passKeptState guard shipped without a spec. Reset() deletes the annotation outright, and a delta of pure removals would otherwise be re-marshalled and written back as "{}" -- resurrecting the key the pass just wiped and leaving a node that reads as tracked-with-no-packages rather than untracked. Verified to fail with the guard forced true. shouldDeleteFinishedJob had four cases and none of them was the one the lost update actually produces. Absent-entry was covered (a reset or an uninstall) but not an entry reverted to (this stage, in_progress), which is the regression shape: a completion clobbered back by the pass. That predicate is the reason a lost update self-heals rather than stalling, at the cost of re-running the stage, so it is worth pinning as its own case rather than inferring it from the absent-entry one. Refs #411 (item 1). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>

…t into jobs-migration/411-nodestate-lost-update Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com> # Conflicts: # operator/internal/controller/swap_test.go

ayuskauskas requested review from lockwobr and rice-riley as code owners August 6, 2026 01:54

github-actions Bot assigned ayuskauskas Aug 6, 2026

github-actions Bot added doc Documentation change (PR path label; doc issues use the Documentation type) component/operator Skyhook operator (controller-manager) component/ci CI workflows, GitHub Actions, and repo tooling labels Aug 6, 2026

coderabbitai Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread operator/internal/controller/node_state_merge_test.go

Comment thread operator/internal/controller/skyhook_controller.go

coderabbitai Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread operator/internal/controller/skyhook_controller.go Outdated

coderabbitai Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread operator/internal/controller/skyhook_controller.go Outdated

ayuskauskas added 2 commits August 12, 2026 13:44

lockwobr approved these changes Aug 13, 2026

View reviewed changes

Merge branch 'feature/package-as-jobs' of github.com:NVIDIA/nodewrigh…

1490ed1

…t into jobs-migration/411-nodestate-lost-update Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com> # Conflicts: # operator/internal/controller/swap_test.go

lockwobr merged commit d68da3e into feature/package-as-jobs Aug 13, 2026
28 checks passed

lockwobr deleted the jobs-migration/411-nodestate-lost-update branch August 13, 2026 00:23

coderabbitai Bot mentioned this pull request Aug 13, 2026

docs(operator): clarify that autoTaintNewNodes means never-touched, and is one-way #460

Merged

Navigation Menu

Uh oh!

fix(operator): merge node-state deltas instead of restamping a stale snapshot - #413

fix(operator): merge node-state deltas instead of restamping a stale snapshot#413
lockwobr merged 9 commits into
feature/package-as-jobsfrom
jobs-migration/411-nodestate-lost-update

ayuskauskas commented Aug 6, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading

Reviews paused

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

coveralls commented Aug 6, 2026 •

edited

Loading

Uh oh!

ayuskauskas commented Aug 6, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

ayuskauskas commented Aug 6, 2026

Uh oh!

ayuskauskas commented Aug 6, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

ayuskauskas commented Aug 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

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

Uh oh!

The bug

What it costs

Why a lock alone was not the fix

The fix

Scope

Testing

Uh oh!

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

Uh oh!

Reviews paused

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Coverage Report for CI Build 31653882834

Coverage: 78.974%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

ayuskauskas commented Aug 6, 2026

Blocker: the fresh-read patch base silently made spec a whole-value restamp

Should-fixes

Verification

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ayuskauskas commented Aug 6, 2026

Uh oh!

ayuskauskas commented Aug 6, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ayuskauskas commented Aug 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ayuskauskas commented Aug 6, 2026 •

edited

Loading

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading

coveralls commented Aug 6, 2026 •

edited

Loading

Blocker: the fresh-read patch base silently made `spec` a whole-value restamp