diff --git a/cmd/workload/up/cmd.go b/cmd/workload/up/cmd.go index da891c70..b6ea41dd 100644 --- a/cmd/workload/up/cmd.go +++ b/cmd/workload/up/cmd.go @@ -217,9 +217,11 @@ sends the sizing with the rollout, so the new version comes up with it. A workload that is not ready to be deployed onto is dealt with rather than refused. One still starting or stopping is waited out and then re-read, so the -plan is built against where it landed. A stopped one is started and then -reconciled in the same run, which is one command whether the file asks for a -start alone or for a start and a new version. +plan is built against where it landed, and so is one already being rolled onto +a new version, which reports itself running for the whole of the swap. A +stopped one is started and then reconciled in the same run, which is one +command whether the file asks for a start alone or for a start and a new +version. Examples: dr workload up diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index b9c708b5..6b5d0ca8 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -25,6 +25,7 @@ import ( "time" "github.com/datarobot/cli/internal/drapi" + "github.com/datarobot/cli/internal/log" "github.com/datarobot/cli/internal/workload" "github.com/datarobot/cli/internal/workload/ignore" "github.com/datarobot/cli/internal/workload/manifest" @@ -54,6 +55,7 @@ var ( listBuildsFn = workload.ListArtifactBuilds getCredentialFn = workload.GetCredential findCredentialFn = workload.FindCredentialNamed + activeReplacementFn = workload.GetActiveReplacement guardReplacementFn = workload.RefuseActiveReplacement startReplacementFn = workload.StartReplacement waitReplacementFn = workload.WaitForReplacement @@ -327,13 +329,245 @@ func guardRollout(workloadID, consequence string) error { // lookSettled is the live read a plan is built from: the workload as it is, // once it has stopped moving. +// +// Two things can be moving, and they are asked about in this order because the +// rollout is the coarser of them. A swap that lands leaves the workload coming +// up, so waiting the rollout out first and the status second settles both in +// one pass; the other order would return with the workload still provisioning +// and hand deployable a state it has to refuse. Neither wait needs the plan, +// which is the point: a deploy arriving mid-transition is early rather than +// wrong. func lookSettled(workloadID string, opts Options) (Live, error) { found, err := Look(workloadID) if err != nil { return Live{}, err } - return awaitSteady(found, opts) + replaced, previewed, err := awaitReplaced(found, opts) + if err != nil { + return replaced, err + } + + // A preview that has already said a deploy would wait for the rollout says + // nothing further. The settling state it hands back is synthetic — put there + // so an empty plan is not called up to date — and awaitSteady would read it + // as a second transition and print the same sentence about the same wait. + if previewed { + return replaced, nil + } + + return awaitSteady(replaced, opts) +} + +// awaitReplaced waits out a swap somebody else already started, and hands back +// the workload as it is once that swap has landed. +// +// This is the transition awaitSteady structurally cannot see: a workload being +// replaced reports itself running for the whole of the swap, so the only way to +// know is to ask the replacement route. It used to be asked as a refusal, which +// left the same dead end a settling workload used to have — "wait for it to +// settle before starting another" is work the command can do itself. +// +// The re-read afterwards is not a formality. A rollout that completes moves the +// workload onto a different artifact, so a plan built against the state as it +// was would roll a version the platform had already installed, or report drift +// the swap had just closed. +// +// The guards at the apply sites stay refusals and are not made redundant by +// this. A rollout that appears after this read is somebody deploying +// concurrently, and waiting there would apply a plan built against state that +// has since moved. +// +// A dry run never waits, for the reason it never waits on a settling workload: +// blocking a preview for the poll timeout is the opposite of what a preview is +// for. The bool it returns is that case and only that case: true means the +// preview has already said a deploy would wait here, so the caller stops rather +// than letting awaitSteady say it again about the settling state synthesised +// below. A real run never returns true, because it waits instead of previewing. +func awaitReplaced(live Live, opts Options) (Live, bool, error) { + if !replaceable(live) { + return live, false, nil + } + + active, err := activeReplacementFn(live.WorkloadID) + if err != nil { + return live, false, fmt.Errorf( + "cannot tell whether workload %s already has a rollout in progress, so nothing was deployed: %w", + live.WorkloadID, err) + } + + if active == nil { + // The ordinary path, and the only one that says nothing to the user. + // Logged so a --debug transcript shows the route was asked at all: a + // deploy that planned against a stale artifact looks the same here + // whether the answer was "nothing in flight" or the question was never + // put, and those have different causes. + log.Debug("no rollout in flight; planning against the workload as read", + "workload_id", live.WorkloadID) + + return live, false, nil + } + + // A settled record stays readable for a while after the rollout ends, so + // waiting on one would block every deploy for as long as it lingers. It is + // still re-read rather than returned as it stands: a terminal record means a + // swap landed recently, possibly in the window between the read above and + // this one, and the snapshot from before it names the outgoing artifact. + // Planning against that would roll a version the platform had just + // installed, and a --lock run would make the version being rolled off + // permanent, which cannot be undone. + // + // The residual, stated rather than pretended away: a swap whose record is + // 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) { + // The one decision here that is neither waited on nor said out loud, so + // the debug log is the only place it can be seen. It matters after the + // fact: a plan that looks like it rolled the wrong artifact is either + // this re-read having happened or it not having happened. + log.Debug("the rollout already settled; re-reading before planning", + "workload_id", live.WorkloadID, "replacement_id", active.ID, "status", active.Status) + + refreshed, err := Look(live.WorkloadID) + + return refreshed, false, err + } + + report := newReporter(opts.Stderr, opts.Spinner) + + report.say(" %s\n", tui.HintStyle.Render(replacingNote(live.WorkloadID, active))) + + if opts.DryRun { + report.say(" %s\n", tui.HintStyle.Render( + "A deploy would wait for this rollout to finish and plan against where it lands.")) + + // The plan below is computed against a workload the platform is already + // moving, so an empty one is not "up to date": the swap decides what + // differs, and it has not landed. StateSettling is what says so, and it + // is the same answer for the same reason a workload halfway through + // stopping gets. Without it the preview prints "Already up to date" + // directly beneath the note above, contradicting it. + // + // Safe to synthesise because it is confined to the preview. Every state + // 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 + + return live, true, nil + } + + if opts.Detach { + // --detach is about not waiting for the deploy to serve. This wait is + // before the deploy: what to apply cannot be known until the swap has + // landed. Saying so beats blocking in silence. + report.say(" %s\n", tui.HintStyle.Render( + "Waiting for it to land before planning; --detach applies to the deploy.")) + } + + var settled *workload.Replacement + + err = report.run("Waiting for the rollout already in progress", func() error { + // Seeded with the record just read, which is what lets the wait tell a + // rollout the platform settled before the first poll landed from one + // that was never there at all. + replacement, waitErr := waitReplacementFn( + live.WorkloadID, active, opts.PollInterval, opts.PollTimeout, nil) + settled = replacement + + return waitErr + }) + if err != nil { + if failure := replacedFailed(live, settled, err); failure != nil { + return live, false, failure + } + + report.say(" %s\n", tui.WarnStyle.Render(fmt.Sprintf( + "⚠ That rollout ended as %s, so the workload is still running the version it was.", + settled.Status))) + } + + refreshed, err := Look(live.WorkloadID) + + return refreshed, false, err +} + +// replaceable says whether there is a workload for the replacement route to +// answer about. +// +// A workload the platform does not have cannot be being replaced, and the route +// answers the same 404 for "no such workload" as it does for "nothing in +// flight", so asking about one would be a round trip whose answer could not be +// read either way. A terminated workload is refused by deployable regardless, +// so the question is only cost there too. +func replaceable(live Live) bool { + if live.WorkloadID == "" { + return false + } + + switch live.State { + case StateUnbound, StateMissing, StateTerminated: + return false + + case StateStopped, StateSettling, StateRunning, StateErrored: + return true + + default: + return true + } +} + +// replacingNote says what is in flight, carrying only the fields the platform +// filled in. +// +// The artifact is one of them because a settings-only rollout moves the +// workload onto the artifact it is already running and the platform returns no +// candidate for it, so naming one unconditionally would print an empty field +// mid-sentence. +// +// It is said before the wait, not after. Without a terminal there is no spinner +// and a phase prints nothing until it ends, so a CI log would otherwise show +// nothing at all for as long as the swap takes. +func replacingNote(workloadID string, active *workload.Replacement) string { + note := "Workload " + workloadID + " is being replaced" + + if active.ArtifactID != "" { + note += " onto artifact " + active.ArtifactID + } + + if active.Status != "" { + note += ", status " + active.Status + } + + return note + "." +} + +// replacedFailed is the verdict on a wait that did not come back clean, and +// returns nil for the one case the run carries on from. +// +// A rollout that ends failed never promotes: the version that was serving is +// still serving, so the workload is deployable and this deploy is the natural +// remedy. Refusing there would hand back the same "run it again" dead end this +// wait exists to remove, so the caller notes what happened and plans against +// what the failure left behind. +// +// Everything else is a wait that told us nothing — a timeout or a failed poll — +// and names where the rollout got to, which is what decides whether to wait +// longer or go and look at the platform. +func replacedFailed(live Live, settled *workload.Replacement, err error) error { + if settled != nil && workload.IsFailedReplacementStatus(settled.Status) { + return nil + } + + where := "did not finish rolling out" + if settled != nil && settled.Status != "" { + where = "was still " + settled.Status + } + + return fmt.Errorf( + "the rollout of workload %s %s, so nothing was deployed; check 'dr workload status %s': %w", + live.WorkloadID, where, live.WorkloadID, err) } // awaitSteady waits out a workload that is still moving, and hands back what diff --git a/internal/workload/up/run_test.go b/internal/workload/up/run_test.go index 87fc65aa..617c50b2 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -206,10 +206,12 @@ type fakes struct { // checkEndpoint is the one GET a deploy ends with. checkEndpoint func(string) (int, error) - // The roll track: refuse to queue a second swap, start one, follow it. - guard func(string) error - replace func(string, string, json.RawMessage) (*workload.Replacement, error) - waitReplace func(string, *workload.Replacement, time.Duration, time.Duration, + // The roll track: read what is already in flight, refuse to queue a second + // swap, start one, follow it. + activeReplacement func(string) (*workload.Replacement, error) + guard func(string) error + replace func(string, string, json.RawMessage) (*workload.Replacement, error) + waitReplace func(string, *workload.Replacement, time.Duration, time.Duration, func(*workload.Replacement)) (*workload.Replacement, error) // settings is the in-place path: a change that moved only the sizing. @@ -324,7 +326,12 @@ func install(t *testing.T, f fakes) { swap(t, &listBuildsFn, f.builds) // Nothing stands in the way of a rollout unless a test says so, because - // the quiet answer is the one every other roll test wants. + // the quiet answer is the one every other roll test wants. That covers the + // pre-plan read as well as the guards: a test wiring neither is saying its + // workload has no swap in flight. + force(t, &activeReplacementFn, func(string) (*workload.Replacement, error) { return nil, nil }) + swap(t, &activeReplacementFn, f.activeReplacement) + force(t, &guardReplacementFn, func(string) error { return nil }) swap(t, &guardReplacementFn, f.guard) swap(t, &startReplacementFn, f.replace) @@ -558,7 +565,13 @@ func TestRun_LockWithNothingToDoStillLocks(t *testing.T) { // that artifact. Locking cannot be undone, so a lost race would leave the // outgoing version permanent and the rollout unable to complete. deployable // cannot catch it: the workload reports itself running for the whole of a swap. -func TestRun_LockWithNothingToDoWaitsForARolloutInFlight(t *testing.T) { +// +// This is the backstop rather than the ordinary answer. A swap that was already +// under way when the workload was read is waited out before the plan is built, +// so what reaches this guard is one that started after that read, which is +// somebody else deploying concurrently. Refusing is the only safe verdict +// there: waiting would take a one-way lock on a plan that is already stale. +func TestRun_LockWithNothingToDoRefusesARolloutThatStartedLate(t *testing.T) { install(t, fakes{ workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, artifactD: func(string) (workload.Document, error) { return draftArtifact(t), nil }, @@ -1187,6 +1200,421 @@ func TestRun_SettlingIntoErroredIsNotReportedAsUpToDate(t *testing.T) { } } +// A rollout already in flight used to be refused outright, which is the same +// dead end a settling workload was: "wait for it to settle, then deploy" is +// work the command can do itself. It is the one transition awaitSteady cannot +// see, because the workload reports itself running for the whole of a swap. +// +// The re-read is the point. Planning against the state as it was would roll a +// version onto a workload the platform is already moving, so the fixture +// carries drift on the first read and none on the second: the swap that was in +// flight is what closed the gap, and the deploy has nothing left to do. +func TestRun_RolloutInFlightIsWaitedOutAndThenDeployed(t *testing.T) { + var ( + artifacts int + waitedFor string + seeded *workload.Replacement + ) + + active := &workload.Replacement{ + ID: "rep-1", WorkloadID: "68b0c1d2e3f4a5b6c7d8e9f0", + ArtifactID: "68a0000000000000000000a2", Status: "switching", + } + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { + artifacts++ + + d := doc(t, liveArtifactJSON) + if artifacts == 1 { + // Drift only the first read sees: a port the file does not name. + group := d["spec"].(map[string]any)["containerGroups"].([]any)[0].(map[string]any) + group["containers"].([]any)[0].(map[string]any)["port"] = float64(8001) + } + + return d, nil + }, + activeReplacement: func(string) (*workload.Replacement, error) { return active, nil }, + waitReplace: func(id string, started *workload.Replacement, _, _ time.Duration, + _ func(*workload.Replacement), + ) (*workload.Replacement, error) { + waitedFor, seeded = id, started + + return &workload.Replacement{ID: "rep-1", Status: workload.ReplacementStatusCompleted}, nil + }, + replace: func(string, string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("the swap already in flight is what closed the gap; nothing was left to roll") + + return nil, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, stderr, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err) + + assert.Equal(t, "68b0c1d2e3f4a5b6c7d8e9f0", waitedFor) + assert.Same(t, active, seeded, + "the record just read is the seed, so the wait can tell a rollout that finished early "+ + "from one that was never there") + assert.Equal(t, 2, artifacts, "the workload is re-read, because a swap lands somewhere new") + assert.Equal(t, ActionUnchanged, result.Action, + "the plan is built against where the swap landed, and the swap is what closed the gap") + + assert.Contains(t, stderr, "is being replaced") + assert.Contains(t, stderr, "68a0000000000000000000a2", "the note names what is being rolled on") + assert.Contains(t, stderr, "switching") + assert.Contains(t, stderr, "Waiting for the rollout already in progress") +} + +// A rollout that ends failed never promotes, so the version that was serving is +// still serving and the workload is deployable. Refusing here would strand the +// user on the same "run it again" dead end; the run says what it saw and plans +// against the state the failure left behind. +func TestRun_RolloutThatEndsFailedIsNotedAndTheRunContinues(t *testing.T) { + var started string + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return stoppedWorkload(t), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(id string, _ *workload.Replacement, _, _ time.Duration, + _ func(*workload.Replacement), + ) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: workload.ReplacementStatusFailed}, + errors.New("replacement for workload " + id + " ended with status failed") + }, + start: func(id string) (*workload.WorkloadOperationResponse, error) { + started = id + + return &workload.WorkloadOperationResponse{WorkloadID: id}, nil + }, + wait: func(id string, _ workload.Serving, _, _ time.Duration, _ func(*workload.Workload)) (*workload.Workload, error) { + return running(id), nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, stderr, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err, "a rollout somebody else lost is not this deploy's failure") + + assert.Equal(t, "68b0c1d2e3f4a5b6c7d8e9f0", started, "the run went on to do what the file asked") + assert.Equal(t, ActionStarted, result.Action) + assert.Contains(t, stderr, "ended as failed") + assert.Contains(t, stderr, "still running the version it was") +} + +// A rollout still going when the wait gave up has not been deployed onto. The +// message names where it got to, because "still switching after 30m" is what +// decides whether to wait longer or go and look at the platform. +func TestRun_RolloutThatNeverLandsStopsBeforeMutating(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return stoppedWorkload(t), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(id string, _ *workload.Replacement, _, _ time.Duration, + _ func(*workload.Replacement), + ) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, + errors.New("timeout waiting for replacement on workload " + id + " after 30m0s") + }, + start: func(string) (*workload.WorkloadOperationResponse, error) { + t.Fatal("a run that never got a settled state must not have changed anything") + + return nil, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, _, err := runIn(t, bound, Options{NonInteractive: true}) + require.Error(t, err) + assert.Contains(t, err.Error(), "was still switching") + assert.Contains(t, err.Error(), "dr workload status 68b0c1d2e3f4a5b6c7d8e9f0") + + // The binding survives the failure, for the same reason it does when a + // workload never settles: losing the id is how a deploy becomes unfindable. + assert.Equal(t, "68b0c1d2e3f4a5b6c7d8e9f0", result.WorkloadID) +} + +// A read that cannot answer is not a rollout. The two want different words: one +// is a state to wait out, the other is a question that did not get asked. +func TestRun_UnreadableRolloutStateStopsTheRun(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return stoppedWorkload(t), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return nil, errors.New("500 Internal Server Error") + }, + start: func(string) (*workload.WorkloadOperationResponse, error) { + t.Fatal("a run that could not read the rollout state must not have changed anything") + + return nil, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, _, err := runIn(t, bound, Options{NonInteractive: true}) + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot tell whether") +} + +// A preview must not block for the poll timeout. It changes nothing, so the +// honest answer is the plan as things stand plus a note that a deploy would +// wait: the same bargain a settling workload gets. +func TestRun_DryRunWithARolloutInFlightDoesNotWait(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(string, *workload.Replacement, time.Duration, time.Duration, + func(*workload.Replacement), + ) (*workload.Replacement, error) { + t.Fatal("a preview must not block for the poll timeout") + + return nil, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, stderr, err := runIn(t, bound, Options{NonInteractive: true, DryRun: true}) + require.NoError(t, err) + + assert.Contains(t, stderr, "is being replaced") + assert.Contains(t, stderr, "A deploy would wait for this rollout to finish") + + // One wait pending, one sentence about it. The settling state this preview + // leaves behind is synthetic — awaitReplaced put it there so an empty plan + // is not called up to date — so awaitSteady must not read it as a second + // transition and print its own near-identical hint underneath. + assert.Equal(t, 1, strings.Count(stderr, "plan against where it lands")) +} + +// --detach is about not waiting for the deploy to serve, and this wait comes +// before the deploy: what to apply cannot be known until the swap lands. +// Blocking is right, blocking in silence is not. +func TestRun_DetachedRunSaysWhyItIsWaitingForTheRollout(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(string, *workload.Replacement, time.Duration, time.Duration, + func(*workload.Replacement), + ) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: workload.ReplacementStatusCompleted}, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, stderr, err := runIn(t, bound, Options{NonInteractive: true, Detach: true}) + require.NoError(t, err) + assert.Contains(t, stderr, "--detach applies to the deploy") +} + +// The counterpart to the backstop above, and the reason the wait is placed +// before the plan rather than at the apply sites. A `--lock` run whose swap was +// already under way when the workload was read waits it out and locks whatever +// the swap left serving, instead of refusing a run whose only fault was +// arriving early. Getting this order wrong is not cosmetic: locking is one-way, +// so a lock taken against the pre-swap read would make the outgoing version +// permanent. +func TestRun_LockWaitsOutARolloutThatWasAlreadyInFlight(t *testing.T) { + var ( + locked string + artifacts int + ) + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { + artifacts++ + + return draftArtifact(t), nil + }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: "switching"}, nil + }, + waitReplace: func(string, *workload.Replacement, time.Duration, time.Duration, + func(*workload.Replacement), + ) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", Status: workload.ReplacementStatusCompleted}, nil + }, + lock: func(artifactID string) (*workload.Artifact, error) { + assert.Equal(t, 2, artifacts, + "the lock is one-way, so it lands on what the swap left serving, not on the pre-swap read") + + locked = artifactID + + return &workload.Artifact{ID: artifactID, Status: workload.ArtifactStatusLocked}, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, stderr, err := runIn(t, bound, Options{NonInteractive: true, Lock: true}) + require.NoError(t, err) + + assert.NotEmpty(t, locked) + assert.True(t, result.Locked) + assert.Contains(t, stderr, "Waiting for the rollout already in progress") +} + +// The narrow race the pre-plan read opens, and the reason a terminal record is +// re-read rather than returned as it stands. The swap lands between the +// workload read and the replacement read, so the route answers "completed" and +// there is nothing to wait for — but the snapshot in hand is from before the +// swap and names the artifact being rolled off. +// +// --lock is where that costs something that cannot be taken back: the lock +// lands on result.ArtifactID, which is seeded from that snapshot, so a run +// missing the re-read makes the outgoing version permanent. +func TestRun_SwapThatLandsBeforeTheRolloutReadIsStillReRead(t *testing.T) { + const ( + outgoing = "68a0000000000000000000a1" + incoming = "68a0000000000000000000b2" + ) + + var ( + looks int + locked string + ) + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { + looks++ + + d := doc(t, liveWorkloadJSON) + if looks > 1 { + // The swap has landed by the time anything re-reads. + d["artifactId"] = incoming + } + + return d, nil + }, + artifactD: func(id string) (workload.Document, error) { + d := draftArtifact(t) + d["id"] = id + + return d, nil + }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ + ID: "rep-1", ArtifactID: incoming, Status: workload.ReplacementStatusCompleted, + }, nil + }, + waitReplace: func(string, *workload.Replacement, time.Duration, time.Duration, + func(*workload.Replacement), + ) (*workload.Replacement, error) { + t.Fatal("a settled record is nothing to wait for") + + return nil, nil + }, + lock: func(artifactID string) (*workload.Artifact, error) { + locked = artifactID + + return &workload.Artifact{ID: artifactID, Status: workload.ArtifactStatusLocked}, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, _, err := runIn(t, bound, Options{NonInteractive: true, Lock: true}) + require.NoError(t, err) + + assert.Equal(t, 2, looks, "a terminal record means a swap landed, so the snapshot is re-read") + assert.Equal(t, incoming, locked, + "locking is one-way, so it must land on what the swap installed, never on the version rolled off") + assert.Equal(t, incoming, result.ArtifactID) +} + +// A record that says nothing is in flight is no evidence a swap just happened, +// so the quiet path stays one workload read. Without this the fix above would +// double the read on every deploy to close a window it cannot see anyway. +func TestRun_NoRolloutRecordCostsNoSecondRead(t *testing.T) { + var looks int + + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { + looks++ + + return doc(t, liveWorkloadJSON), nil + }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, _, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err) + assert.Equal(t, 1, looks, "nothing in flight is nothing to re-read") +} + +// A preview during a swap must not print the one verdict it cannot support. The +// plan is computed against a workload the platform is already moving, so an +// empty one says the swap has not landed yet, not that there is nothing to do — +// and "Already up to date" directly beneath "a deploy would wait" is a preview +// contradicting itself. +// +// The fixture matches the live state field for field, which is what makes the +// plan empty and is exactly the shape that used to print the wrong answer. +func TestRun_DryRunDuringARolloutDoesNotClaimUpToDate(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + activeReplacement: func(string) (*workload.Replacement, error) { + return &workload.Replacement{ID: "rep-1", ArtifactID: "68a0…b2", Status: "promoting"}, nil + }, + }) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, stderr, err := runIn(t, bound, Options{NonInteractive: true, DryRun: true}) + require.NoError(t, err) + + assert.NotContains(t, stderr, "Already up to date", + "the swap decides what differs, and it has not landed") + assert.Contains(t, stderr, "still settling") + assert.Contains(t, stderr, "plan against where it lands") + assert.Equal(t, "settling", result.Status, + "the envelope reports the state, and a workload mid-swap is not settled") +} + +// A manifest with nothing to resolve has no workload to ask about, and the +// replacement route answers the same 404 for "no such workload" as it does for +// "nothing in flight". Asking would be a round trip whose answer cannot be +// read either way. +func TestRun_ARunWithNoLiveWorkloadNeverAsksAboutARollout(t *testing.T) { + install(t, fakes{ + activeReplacement: func(string) (*workload.Replacement, error) { + t.Fatal("there is no workload to be replaced") + + return nil, nil + }, + create: func(any) (*workload.Workload, error) { return running("wl-new"), nil }, + wait: func(id string, _ workload.Serving, _, _ time.Duration, _ func(*workload.Workload)) (*workload.Workload, error) { + return running(id), nil + }, + }) + + result, _, err := runIn(t, unboundImageManifest, Options{NonInteractive: true}) + require.NoError(t, err) + assert.Equal(t, ActionCreated, result.Action) +} + // TestRun_MissingWorkloadIsRecreated is the reported bug: a workload deleted // out from under the manifest used to leave the user with a binding only a // hand edit could clear. It is drift, so the deploy recreates and rebinds. diff --git a/internal/workload/up/state.go b/internal/workload/up/state.go index 5624ef8a..2e4d7524 100644 --- a/internal/workload/up/state.go +++ b/internal/workload/up/state.go @@ -66,8 +66,16 @@ const ( // to be running. StateStopped - // StateSettling is a workload still moving under its own power. `up` - // waits for it rather than acting on a state that is about to change. + // StateSettling is a workload still moving. `up` waits for it rather than + // acting on a state that is about to change. + // + // Mostly that is a workload moving under its own power, which is what + // stateFor reads off the status. A swap already in flight is the other + // case and cannot be read that way, because a workload being replaced + // reports itself running throughout; awaitReplaced asks the replacement + // route and reduces it to this state on the one path that does not wait, + // so a preview says what a deploy would do rather than calling a workload + // mid-transition up to date. StateSettling // StateRunning is the ordinary case: reconcile against it.