[RAPTOR-19530] feat(workload): wait out a rollout in flight instead of refusing - #853
[RAPTOR-19530] feat(workload): wait out a rollout in flight instead of refusing#853cdevent wants to merge 3 commits into
Conversation
|
🎫 Jira: |
|
🚀 Smoke tests triggered! Running on Linux and Windows... |
|
✅ All smoke tests passed! ✅ Linux: success |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 43650a6. Configure here.
| // waiting on one would block every deploy for as long as it lingers. | ||
| if active == nil || workload.IsTerminalReplacementStatus(active.Status) { | ||
| return live, nil | ||
| } |
There was a problem hiding this comment.
Completed swap skips the re-read
Medium Severity
awaitReplaced returns the original Look when the replacement route already shows a terminal record, so a swap that finishes between those two reads is never re-fetched. The plan is then built against the pre-swap artifact, awaitSteady is skipped because that snapshot still says running, and --lock can make the outgoing version permanent.
Reviewed by Cursor Bugbot for commit 43650a6. Configure here.
Code OwnershipWorkload Cli
Review requested from the teams above. Labels will be removed automatically upon approval. |
| // that decides the shape of a plan already groups settling with running | ||
| // (see actsOnState and creates), so only the verdict moves; and a real | ||
| // run never arrives here with it, because it waits and re-reads instead. | ||
| live.State = StateSettling |
There was a problem hiding this comment.
Dry-run with a rollout in flight prints the same hint twice.
When --dry-run hits an in-flight rollout, awaitReplaced (line 369 above) prints:
A deploy would wait for this rollout to finish and plan against where it lands.
...then sets live.State = StateSettling and returns. Back in lookSettled, awaitSteady(replaced, opts) runs next, sees live.State == StateSettling, and — since opts.DryRun is also true — prints its own near-identical hint right after:
A deploy would wait for this to finish and plan against where it lands.
Confirmed live by running the TestRun_DryRunWithARolloutInFlightDoesNotWait scenario and dumping stderr — both lines appear back to back:
Workload 68b0c1d2e3f4a5b6c7d8e9f0 is being replaced, status switching.
A deploy would wait for this rollout to finish and plan against where it lands.
A deploy would wait for this to finish and plan against where it lands.
None of the new tests catch this because they only assert.Contains, never checking the message isn't duplicated. Worth either short-circuiting lookSettled after the dry-run synthetic-state assignment, or having awaitSteady skip its own hint when the settling state came from awaitReplaced rather than a real read.
| live.WorkloadID, err) | ||
| } | ||
|
|
||
| if active == nil { |
There was a problem hiding this comment.
Nit: nothing marks the quiet path (no rollout in flight) in the debug log.
version.go in this same package logs decisions the user wouldn't see otherwise (e.g. log.Debug("artifact repository refused; creating the version in a new one", ...) at line 220) so a --debug run can explain why something took the path it did. awaitReplaced never touches internal/log at all — this branch is the ordinary, silent case (nothing in flight, proceed straight to awaitSteady), so probably fine as-is, but worth a log.Debug("no replacement in flight", "workload_id", live.WorkloadID) if a support engineer ever needs to confirm from a --debug transcript that this route was even checked.
| // collected inside that same window reads as nil above and is not re-read. | ||
| // Closing it means re-reading on every deploy, which doubles the workload | ||
| // GET on the quiet path to catch a window narrower than the one this covers. | ||
| if workload.IsTerminalReplacementStatus(active.Status) { |
There was a problem hiding this comment.
This branch is the one truly silent decision in the new code — worth a debug log.
When the replacement record read here is already terminal, the function discards live and does a second, unlogged Look() instead of waiting — the exact narrow race the PR description calls out (a swap landing between the workload read and the replacement read). Nothing is said to the user (no report.say) and nothing goes to internal/log either, so this decision is invisible even under --debug. Compare to version.go's fellBack handling, which is deliberately "said out loud rather than only to the debug log" for the same reason: "nobody goes looking for the cause of that after a run that printed nothing but check marks." Something like:
log.Debug("replacement already settled; re-reading before planning",
"workload_id", live.WorkloadID, "status", active.Status)would make this traceable if a plan ever looks like it rolled the wrong artifact.
|
LGTM, would suggest adding the debug logs, defer to @wojtekwdr and @adamalpi for final approval |
…r the silent paths Review on #853. A --dry-run onto a workload with a rollout in flight said the same thing twice. awaitReplaced printed its note, then reduced the workload to StateSettling so the empty plan below would not be called up to date -- and awaitSteady, seeing that state under --dry-run, printed its own near-identical hint underneath: A deploy would wait for this rollout to finish and plan against where it lands. A deploy would wait for this to finish and plan against where it lands. That state is synthetic, put there for the verdict rather than read off the platform, so awaitSteady had nothing left to say about it. awaitReplaced now reports that it previewed, and lookSettled stops there. A real run never returns that flag: it waits and re-reads instead of previewing. The existing test only asserted the hint was present, which two copies satisfy. It now counts them. Two decisions in the new code were invisible even under --debug, and both are ones you would go looking for after the fact: - nothing in flight, plan against the workload as read -- the ordinary path, and the only one that prints nothing at all. Without the line, a deploy that planned against a stale artifact looks the same whether the route answered "nothing" or was never asked. - the rollout had already settled, so re-read rather than wait -- the narrow race between the workload read and the replacement read. A plan that looks like it rolled the wrong artifact is either this re-read having happened or not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
43637cb to
6b7fde1
Compare
7855afc to
6b7fde1
Compare
…f refusing `dr workload up` already waits out a workload that is still moving under its own power (RAPTOR-19686). It did not wait out the one transition that check structurally cannot see: a workload being replaced reports itself running for the whole of the swap, so a rollout in flight reached guardRollout and was refused with "wait for it to settle before starting another" — the same dead end a settling workload used to have, and work the command can do itself. lookSettled now asks the replacement route before it builds the plan. A swap already under way is named on stderr, waited out, and the workload re-read, so the plan is built against where the swap landed rather than against state the platform was already moving. The rollout is waited first and the status second because a swap that lands leaves the workload coming up: one pass settles both. A rollout that ends failed never promotes, so the version that was serving is still serving and the workload is deployable. The run says what it saw and plans against what the failure left behind rather than handing back the same "run it again" refusal. A timeout or an unreadable route stops the run and names where the rollout got to. The guards at the apply sites stay refusals. A rollout that appears after the pre-plan read is somebody deploying concurrently, and waiting there would apply a plan built against state that has since moved — for --lock, it would take a one-way lock on the outgoing version. --dry-run says a deploy would wait and changes nothing; --detach explains that this wait precedes the deploy, as it already does for a settling workload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…review up to date Two review findings on the pre-plan rollout wait, both confirmed. A terminal replacement record was returned as-is, so a swap that landed in the window between the workload read and the replacement read was never re-fetched. The plan was then built against the artifact being rolled off, awaitSteady was skipped because that snapshot still said running, and --lock made the outgoing version permanent: lock() locks result.ArtifactID, which is seeded from exactly that snapshot, and locking cannot be undone. A terminal record now re-reads. A record that says nothing is in flight still costs no second read, since it is no evidence a swap just happened. The residual is stated in the code rather than pretended away: a swap whose record is collected inside that same window reads as nil and is not re-read. Closing it would mean re-reading on every deploy, doubling the workload GET on the quiet path to catch a window narrower than the one this covers. A --dry-run during a swap left the state as running, so an empty plan printed "Already up to date" directly beneath the note saying a deploy would wait and plan against where the swap lands — a preview contradicting itself. The plan is computed against a workload the platform is already moving, so an empty one says the swap has not landed, not that there is nothing to do. The preview now reduces that to StateSettling, which is the same answer a workload halfway through stopping already gets, and settledVerdict says so. Synthesising the state is confined to the preview: every state that decides the shape of a plan already groups settling with running (actsOnState, creates), so only the verdict moves, and a real run never arrives there because it waits and re-reads instead. StateSettling's doc records that it now covers both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r the silent paths Review on #853. A --dry-run onto a workload with a rollout in flight said the same thing twice. awaitReplaced printed its note, then reduced the workload to StateSettling so the empty plan below would not be called up to date -- and awaitSteady, seeing that state under --dry-run, printed its own near-identical hint underneath: A deploy would wait for this rollout to finish and plan against where it lands. A deploy would wait for this to finish and plan against where it lands. That state is synthetic, put there for the verdict rather than read off the platform, so awaitSteady had nothing left to say about it. awaitReplaced now reports that it previewed, and lookSettled stops there. A real run never returns that flag: it waits and re-reads instead of previewing. The existing test only asserted the hint was present, which two copies satisfy. It now counts them. Two decisions in the new code were invisible even under --debug, and both are ones you would go looking for after the fact: - nothing in flight, plan against the workload as read -- the ordinary path, and the only one that prints nothing at all. Without the line, a deploy that planned against a stale artifact looks the same whether the route answered "nothing" or was never asked. - the rollout had already settled, so re-read rather than wait -- the narrow race between the workload read and the replacement read. A plan that looks like it rolled the wrong artifact is either this re-read having happened or not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6b7fde1 to
70c110d
Compare
|
took a look through this myself, nothing new to add. the earlier comments (both bugbot ones and aj's three) are all addressed in the follow-up commits and each has a test. gtg from me. seconding aj on deferring final approval. @datarobot-oss/workload-cli this is ready for your review. |
3 participants
RATIONALE
RAPTOR-19530 — "if a workload is still settling when I run
dr wl upit should wait around and tell me it is doing so rather than erroring out."Most of that shipped in RAPTOR-19686 (#836):
awaitSteadywaits out a workload whose status is transitional (submitted,provisioning,launching,stopping,unknown) before the plan is built.What was left is the one transition that check structurally cannot see, and it is the most common one. A workload being replaced reports itself
runningfor the whole of the swap, so a rollout in flight never reachedawaitSteady; it reachedguardRolloutand was refused:That is literally the erroring-out-and-telling-you-to-wait the ticket asks us to stop doing.
CHANGES
lookSettledasks the replacement route before it builds the plan (internal/workload/up/run.go). NewawaitReplacednames the swap on stderr, waits it out, and re-reads the workload:Ordering: rollout first, status second. A swap that lands leaves the workload coming up, so one pass settles both; the other order would return with the workload still
provisioningand handdeployablea state it has to refuse.The re-read is the point, not a formality — a completed swap moves the workload onto a different artifact, so a plan built against the pre-swap state would roll a version the platform had already installed, or report drift the swap had just closed.
A rollout that ends
failedis noted, and the run continues. A failed rollout never promotes, so the version that was serving is still serving and the workload is deployable. Refusing there would hand back the same "run it again" dead end this wait exists to remove:A timeout or an unreadable route stops the run and names where the rollout got to (
"the rollout of workload … was still switching, so nothing was deployed; check 'dr workload status …'"), because "still switching after 30m" is what decides whether to wait longer or go and look at the platform.The guards at the apply sites stay refusals. A rollout that appears after the pre-plan read is somebody deploying concurrently, and waiting there would apply a plan built against state that has since moved — for
--lock, it would take a one-way lock on the outgoing version. All fiveguardRolloutsites are unchanged.Flag behaviour mirrors the settling wait:
--dry-runsays a deploy would wait and changes nothing;--detachexplains that this wait precedes the deploy.Not asked at all when there is no workload for the route to answer about (unbound, missing, terminated) — the replacement route returns the same 404 for "no such workload" as for "nothing in flight", so the answer could not be read either way.
Help text for
dr workload upupdated to cover the new case.TESTING
task lintclean across all three GOOS targets;task testgreen (141 packages).Eight new tests in
internal/workload/up/run_test.go, all verified red before the change:RolloutInFlightIsWaitedOutAndThenDeployedRolloutThatEndsFailedIsNotedAndTheRunContinuesRolloutThatNeverLandsStopsBeforeMutatingUnreadableRolloutStateStopsTheRun"cannot tell whether"— a read that failed is not a rolloutDryRunWithARolloutInFlightDoesNotWaitDetachedRunSaysWhyItIsWaitingForTheRollout--detach applies to the deployARunWithNoLiveWorkloadNeverAsksAboutARolloutLockWaitsOutARolloutThatWasAlreadyInFlightTestRun_LockWithNothingToDoWaitsForARolloutInFlightrenamed to…RefusesARolloutThatStartedLate— its behaviour is unchanged, but it now documents the late guard as the backstop rather than the ordinary answer.Verified live on staging
Same workload, back to back, on
https://staging.datarobot.com.--detachreturns the moment the swap is requested, which leaves a rollout in flight deterministically — no two-shell race needed.Before —
mainbinary, deploying while a rollout was in flight:After — this branch, same situation:
--dry-runduring a rollout returned immediately (checked under a 60s timeout), said what it saw and changed nothing:Probe workload and all four draft artifacts deleted afterwards;
dr workload listanddr artifact listare clean.NOTES
The live run turned up a fact worth recording. The two in-flight statuses the platform actually returned were
initializingandpromoting. Neither appears in the documented replacement vocabulary —internal/workload/replacement.gonames onlycompleted,failedanderrored, with a comment noting the non-terminal vocabulary "is not documented anywhere". Both read correctly as in-flight only becauseIsTerminalReplacementStatustreats an unrecognised status as still in progress. That stance is now load-bearing for this feature, and the two observed names are evidence for the open question already flagged in that file's comment.Cost: one extra
GET /workloads/{id}/replacement/per deploy onto an existing workload. Against a deploy measured in minutes, it replaces a class of failure that costs a whole re-run.Note
Medium Risk
Changes the pre-plan deploy path and artifact/lock timing on an existing workload, though apply guards and broad test coverage limit blast radius.
Overview
dr workload upno longer errors when another rollout is already swapping the workload. Before building the plan,lookSettlednow callsawaitReplaced, which checks the replacement API (workloads stayrunningduring a swap, so status-only waits cannot see it), prints what is in flight, waits for completion when appropriate, and re-reads live state so planning targets the post-swap artifact.Rollout wait runs before the existing settling wait.
--dry-runand--detachbehave like the settling path: preview does not block; detach still waits pre-plan but explains why. A rollout that ends failed is warned and the run continues; timeouts and unreadable replacement state still abort without mutating. Apply-timeguardRolloutrefusals are unchanged for concurrent swaps that start after the pre-plan read (including the late--lockrace). Help text documents the new behavior; tests cover wait, re-read, lock ordering, and flag behavior.Reviewed by Cursor Bugbot for commit 43650a6. Configure here.