fix(operator): never resume a disabled NodeWright's suspended Jobs - #422
fix(operator): never resume a disabled NodeWright's suspended Jobs#422ayuskauskas wants to merge 8 commits into
Conversation
Item 5a of #411, confirmed against the code. resumeSuspendedJobs is called from the reconcile loop for any Skyhook that is not paused, and the only IsDisabled guard in that flow lives in processSkyhooksPerNode, a different function further down. So clearing the pause annotation and setting disable in one edit un-suspended every Job pause had suspended: disabling a paused NodeWright restarted it. That makes disable strictly weaker than pause for in-flight work, which is the opposite of how it reads — docs/cli.md offered it as "disable completely", directly under the pause example. Disable still does not stop work already running; the design doc is explicit that it never claimed to. The fix is only that it must not RESTART work pause stopped. Re-enabling resumes them. The guard is inside resumeSuspendedJobs rather than at the call site so it holds for every caller, and because that is where the existing specs already exercise this behaviour — a call-site guard would have been untestable without standing up a whole reconcile, and this controller has no full-Reconcile spec to model one on. docs/cli.md now states the distinction rather than implying disable is the bigger hammer. Written as prose rather than a third stacked blockquote, which MD028 rejects. Spec verified to fail against the unguarded version. Refs #411 (item 5a). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
github-actions
Bot
added
doc
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI and documentation distinguish Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@docs/cli.md`:
- Around line 514-522: Align the disable contract in the documentation and
NewDisableCmd: state that disable prevents new work while allowing an in-flight
stage to finish, rather than stopping all processing. Explicitly document that
removing disable does not resume jobs while the pause condition remains set,
consistent with SkyhookReconciler.Reconcile checking IsPaused() before
resumeSuspendedJobs. Clarify that pause is required to stop currently running
work.
🪄 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: b6dafc53-f403-4fec-9f4f-20fc50466c5e
📒 Files selected for processing (3)
docs/cli.mdoperator/internal/controller/skyhook_controller.gooperator/internal/controller/swap_test.go
Coverage Report for CI Build 31654161533Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 78.988%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
CodeRabbit review on #422; all three points were right. My replacement for "disable completely" was "stop it being processed at all", which oversells it just as much. It is now "prevent new work being scheduled". NewDisableCmd still said "Disable a NodeWright completely" and "the operator will completely stop processing". Updating docs/cli.md without it left the CLI's own --help contradicting the page, which the repo requires to move together. Its long help now states that a stage already under way runs to completion, and that disable never restarts what pause stopped. The re-enable sentence was wrong, not merely vague. Reconcile hits `if skyhook.IsPaused() { ...; continue }` before resumeSuspendedJobs, so while pause is set the resume never runs regardless of disable. "Re-enabling resumes them" implied enable alone was enough; both annotations have to be cleared. The command's short help keeps the word "Disable" because lifecycle_test.go asserts each Short contains its verb — a convention worth conforming to rather than loosening the test for. Refs #411 (item 5a). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
There was a problem hiding this comment.
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 `@docs/cli.md`:
- Line 516: Update the pause guidance in the CLI documentation to qualify its
immediate-stop guarantee by operator version, reflecting that older operators
and pre-upgrade pods may finish the current run before pausing. Keep the
emergency-stop recommendation consistent with this qualification, while
preserving the existing annotation-order behavior.
In `@operator/cmd/cli/app/lifecycle.go`:
- Around line 211-219: Update the help text in NewPauseCmd and NewEnableCmd,
plus the corresponding docs/CLI wording, to match the disable contract: pause
may suspend the current stage for Job-based operators, and enable resumes
processing only when the pause annotation is also cleared. Keep the statements
about already-running work and suspended stages consistent across all three
descriptions.
🪄 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: 678871f1-2540-4047-843a-11c554ab567d
📒 Files selected for processing (2)
docs/cli.mdoperator/cmd/cli/app/lifecycle.go
…ommodations Item 2 of #411, resolved as a docs fix now that the epic's open question has an answer: the rename and the Jobs migration ship together. The Upgrade section specified four in-place accommodations so Jobs and raw pods could run side by side for a minor release — legacy-aware completion, existence gating that ORs in raw pods, a legacy sweep in validation, and direct deletion of legacy erroring pods on config update. None were built. That was not an oversight; they were superseded by legacyMigrationHold, which takes a stricter line: it runs first in Reconcile and requeues while any pre-rename Skyhook is still rolling out, so the two execution models never overlap rather than being taught to coexist. Shipping the two together is what makes that work, and the section now says so. Had the rename landed in an earlier release, the preceding operator would already be nodewright-native, no legacy Skyhook objects would exist, the hold would never fire, and its raw pods would carry labels the legacy sweep does not select — and the accommodations really would have been required. Also records the one case the hold does not cover: it treats a paused legacy Skyhook as not-in-flight so migration does not force an unpause, but pre-Jobs pause never stopped a running pod. Unpausing such a Skyhook on the new operator before its pod finishes puts a Job alongside it on the same host copyDir. Narrow, idempotent in practice via the agent's flag files, and now written down rather than discovered later. ValidateRunningPackages claimed a legacy raw-pod sweep ran alongside its Job checks. It never did; the comment now says why it walks Jobs only. Refs #411 (item 2). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
There was a problem hiding this comment.
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 `@docs/designs/2026-07-10-package-execution-as-jobs.md`:
- Around line 230-234: Update legacyMigrationHold to account for active raw pods
belonging to paused or disabled legacy Skyhooks, preventing NodeWright Jobs from
starting on their hosts until those pods terminate; alternatively block
NodeWright scheduling through the equivalent safeguard. Add a regression test
covering upgrade, paused legacy execution, and subsequent unpause, and revise
the documented non-overlap guarantee only if the overlap is intentionally
retained.
🪄 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: be8553a3-bfe2-47c0-8651-5de93f8574ac
📒 Files selected for processing (2)
docs/designs/2026-07-10-package-execution-as-jobs.mdoperator/internal/controller/skyhook_controller.go
…e overlap safe CodeRabbit review on #422. Three findings; a fourth comment was it confirming the previous round rather than a new issue. NewPauseCmd still said pause "will not interrupt any currently running operations" — the pre-Jobs behaviour. NewEnableCmd said the operator "will resume normal processing after this command", which is untrue while pause is still set. Fixing disable's help last round and leaving its two siblings stale was the same miss the previous review caught, one command over. Both now describe what the Jobs path actually does, pause version-qualified because its stop-strength depends on the operator generation. docs/cli.md asserted pause halts running work and then, fourteen lines later, said older operators let the current run finish. Now version-qualified inline instead of relying on the reader following a pointer. The design doc's paused-legacy note leaned on the agent's flag files as if they made the overlap safe. They do not: they make re-execution idempotent, they are not a lock, and they neither serialize two executors nor order concurrent writes into the shared copyDir. The same overstatement is being corrected on #421. The note now says the overlap is unguarded while the window is open, and names what closing it would take — teaching the in-flight gate to count legacy raw pods. Refs #411 (items 2 and 5a). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
|
Three findings addressed in a91634b (the fourth comment was you confirming the previous round, not a new issue). Synchronize the pause and enable help — correct, and it's the same miss you caught last round, one command over. Qualify the pause guarantee by version — right. I'd written "halts work already running" with a "(see the version note below)" pointer, and fourteen lines later the note contradicts the sentence. Relying on the reader following a pointer is not the same as being accurate. Now qualified inline. The paused-legacy overlap — you're right about the safety claim, and I've fixed that: I leaned on the agent's flag files as though they made the overlap safe. They don't. They make re-execution idempotent, they are not a lock, and they neither serialize two executors nor order concurrent writes into the shared On closing it rather than documenting it — that's a maintainer decision that has already been made, and I'm recording it rather than overriding it. The epic owner confirmed the rename and the Jobs migration ship together, and chose to document this window rather than close it. I flagged the fix at the time and have named it again in the note: teach the in-flight gate to count legacy raw pods, so a Job is not created beside one. It's one extra list call in For the record, two independent reviewers have now landed on this same point, which I've said on #411. If a maintainer wants it closed before merge I'll do it — but I'm not going to quietly reverse an explicit decision inside a docs commit.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/designs/2026-07-10-package-execution-as-jobs.md (1)
230-234:⚠️ Potential issue | 🟠 MajorDo not leave the legacy execution overlap unguarded.
The migration hold excludes paused and disabled legacy Skyhooks, but pre-Jobs pause does not stop a running raw pod. If the new operator unpauses or enables that Skyhook before the raw pod exits, a Job can run beside the raw pod on the same host and write to the same
copyDir.Make the hold wait for active legacy raw pods regardless of pause or disable state, or block Job scheduling until those pods terminate. Add a regression test for the upgrade and unpause/enable sequence.
🤖 Prompt for 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. In `@docs/designs/2026-07-10-package-execution-as-jobs.md` around lines 230 - 234, The legacy migration hold must also account for active raw pods belonging to paused or disabled legacy Skyhooks, preventing NodeWright Jobs from starting beside them on the same host and copyDir. Update legacyMigrationHold and its in-flight detection to wait until those pods terminate, while preserving existing handling for rollout, rollback, and deletion states. Add a regression test covering upgrade followed by unpause or enable before the legacy raw pod exits.
♻️ Duplicate comments (1)
docs/cli.md (1)
514-516:⚠️ Potential issue | 🟠 MajorKeep the emergency-stop guidance version-qualified.
Line 514 states that older operators only block new scheduling. Line 516 then says that
pausestops everything immediately. This can cause operators to expect a running stage to stop on an older operator.Replace the final sentence with guidance that qualifies immediate suspension to Job-based operators.
🤖 Prompt for 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. In `@docs/cli.md` around lines 514 - 516, Update the final emergency-stop sentence in the pause/disable documentation to qualify immediate suspension as applying only to operators that run package stages as Jobs. Preserve the existing distinction that older operators merely block new scheduling, and avoid implying that pause can stop an already-running stage on those versions.
🤖 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/cmd/cli/app/lifecycle.go`:
- Around line 243-245: Update the enable help text near the NodeWright
scheduling description to replace the incomplete phrase “Stages that pause
suspended resume” with “Stages suspended by pause resume,” preserving the
surrounding explanation and behavior.
---
Outside diff comments:
In `@docs/designs/2026-07-10-package-execution-as-jobs.md`:
- Around line 230-234: The legacy migration hold must also account for active
raw pods belonging to paused or disabled legacy Skyhooks, preventing NodeWright
Jobs from starting beside them on the same host and copyDir. Update
legacyMigrationHold and its in-flight detection to wait until those pods
terminate, while preserving existing handling for rollout, rollback, and
deletion states. Add a regression test covering upgrade followed by unpause or
enable before the legacy raw pod exits.
---
Duplicate comments:
In `@docs/cli.md`:
- Around line 514-516: Update the final emergency-stop sentence in the
pause/disable documentation to qualify immediate suspension as applying only to
operators that run package stages as Jobs. Preserve the existing distinction
that older operators merely block new scheduling, and avoid implying that pause
can stop an already-running stage on those versions.
🪄 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: 8f8fc435-e397-47dc-b8d0-762e8e44f7a1
📒 Files selected for processing (3)
docs/cli.mddocs/designs/2026-07-10-package-execution-as-jobs.mdoperator/cmd/cli/app/lifecycle.go
Closes the paused-legacy overlap by contract rather than in code, per the epic owner's call and after two reviewers raised it. The migration guide told users that paused and disabled Skyhooks are fine to leave as-is and need no unpausing before upgrade. That is still true for the migration itself — they migrate in that state and do not roll out. What it did not say is that pause on the pre-rename operator only blocked new scheduling and never stopped a pod already running, so a Skyhook paused shortly before the upgrade can still have one, and unpausing while it runs lets the new operator start a stage Job beside it: the in-flight gate counts Jobs, not raw pods, and both executors share one package directory on the host with nothing ordering their writes. That sequence is now documented as unsupported, with the check to run first (list the legacy-labelled pods and wait for it to be empty) and the reassurance that leaving it paused is safe indefinitely, since the new operator suspends and skips it and creates no Jobs at all. The design doc records why this is contract rather than code: guarding it would mean teaching the in-flight gate to count legacy raw pods, and the reason the rename and the Jobs migration ship together is precisely so the two execution models never have to coexist. One narrow, opt-in sequence did not justify carrying legacy-pod awareness into the gate for a release. Refs #411 (item 2). Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
|
Closed in 19b06b1 — by contract, not in code. Maintainer's call, recorded here so the reasoning isn't lost.
So the guide now carries an explicit warning: unpausing a migrated NodeWright while its pre-upgrade package pods are still running is unsupported. With the check to run first — kubectl get pods -n skyhook -l skyhook.nvidia.com/name=<skyhook-name>— wait for it to come back empty. And the reassurance that matters operationally: leaving it paused is safe indefinitely. The new operator suspends and skips a paused NodeWright and creates no Jobs at all, so there is no pressure to unpause quickly. Those pods finish on their own and the legacy sweep clears them once The design doc records why this is contract rather than a guard: closing it in code means teaching the in-flight gate to count legacy raw pods, and the reason the rename and the Jobs migration ship together is precisely so the two execution models never have to coexist. Carrying legacy-pod awareness into the gate for one narrow, opt-in sequence — for a single release — wasn't judged worth it. To be straight about what this does and doesn't do: it removes the unwitting path. A user following the guide won't hit it. A user who ignores the warning still can, and the outcome is two executors sharing one host directory with nothing ordering their writes. That's the trade being accepted, and it's now written down in both the guide and the design doc rather than living in a PR thread.
|
There was a problem hiding this comment.
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 `@docs/nodewright-migration.md`:
- Around line 233-241: Update docs/nodewright-migration.md lines 233-241 to
prohibit both unpausing and enabling a migrated NodeWright until all pre-upgrade
package pods are gone, while preserving the existing safety guidance. Update
docs/designs/2026-07-10-package-execution-as-jobs.md line 234 to include
enabling disabled legacy Skyhooks in the unsupported paused-pod overlap
sequence.
🪄 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: 6e9ab481-c9d4-4c65-b645-f3438c8a3b4e
📒 Files selected for processing (2)
docs/designs/2026-07-10-package-execution-as-jobs.mddocs/nodewright-migration.md
"Stages that pause suspended resume" reads as a fragment. Say "stages suspended by pause" so the condition is unambiguous. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
… pod The overlap warning named only the pause/unpause sequence, but the migration hold treats a paused OR disabled legacy Skyhook as not-in-flight, and pre-Jobs disable no more stopped a running pod than pause did: processSkyhooksPerNode skips a disabled Skyhook without touching its pods. So enabling a disabled Skyhook over a live raw pod puts a Job beside it exactly as unpausing does, and both transitions need the same pre-check. Also closes the blockquote before "The runtime hold only waits on Skyhooks..." — the warning had swallowed the first three words of the sentence that follows it, leaving the paragraph starting mid-clause. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
…bs-migration/411-disable-no-resume Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com> # Conflicts: # operator/internal/controller/skyhook_controller.go
3 participants
Refs #411 (item 5a). Does not close it. Part of #223, based on
feature/package-as-jobs.Confirmed
#411 filed this as "needs triage — one reviewer's position, confirm against the code." It holds.
resumeSuspendedJobsis called from the reconcile loop for any Skyhook that is not paused (skyhook_controller.go:463). The onlyIsDisabled()guard anywhere in that flow is at:556, insideprocessSkyhooksPerNode— a different function further down, which only stops new work being scheduled.So this sequence restarted a NodeWright the user had just switched off:
pause→ every unfinished Job getsspec.suspend: true, the running stage stopsIsPaused()is false →resumeSuspendedJobsruns → suspend cleared on everything, stages resumeNet effect: disable was strictly weaker than pause for in-flight work.
docs/cli.mdoffered it as "disable completely", directly under the pause example, which is the opposite of what it did.What this does not change
Disable still does not stop work already running. The design doc is explicit — "
disableis unchanged: it skips a Skyhook from processing but has never claimed to halt in-flight work." A stage already under way runs to completion, same as before. The only change is that disable no longer restarts work that pause stopped. Re-enabling resumes them.Where the guard went, and why
Inside
resumeSuspendedJobs, not at the call site.resumeSuspendedJobsdirectly; a call-site guard would need a fullSkyhookReconciler.Reconcilespec, and this controller has none to model on — inventing that pattern to cover a one-line condition is a bad trade, and a test that re-implements the guard to check the guard proves nothing.I also considered a pause-style
continuefor disabled in the loop. Rejected: that skipsIntrospectSkyhook/SaveNodesAndSkyhook, and a disabled NodeWright still needs its status derived as the in-flight stages it is permitted to finish actually finish. It would have traded this bug for a status-staleness bug.Docs
docs/cli.mdnow states the distinction instead of implying disable is the bigger hammer, including that the two compose safely in either order. Written as prose rather than a third stacked blockquote — MD028 rejects adjacent blockquotes separated by a blank line.Testing
New spec: "does not resume a disabled Skyhook's suspended Jobs", with a fixture that asserts it genuinely reads as disabled before relying on it.
Verified load-bearing — removing the guard fails exactly that spec and nothing else:
make unit-testsgreen,golangci-lint0 issues,markdownlint-cli20 issues.Closes #423 — the item-2 docs fix (
f8194ea4,19b06b1c) landed here, so that issue is fully covered by this PR.