diff --git a/internal/engine/deploy.go b/internal/engine/deploy.go index 7274aa4..479fdc2 100644 --- a/internal/engine/deploy.go +++ b/internal/engine/deploy.go @@ -75,7 +75,19 @@ func (e *Engine) deployCore(ctx context.Context, releaseID, localStagingDir stri if err != nil { return err } - defer e.ReleaseLock(ctx) + // A non-empty reason keeps the lock and says why. The check below refuses + // both when a job container is running and when the host could not be + // asked, and the lock is kept for the same reason either way: releasing it + // would hand the host to the next mutator over a state this deploy declined + // to proceed against. + holdLockReason := "" + defer func() { + if holdLockReason != "" { + e.warnf("%s", holdLockReason) + return + } + e.ReleaseLock(ctx) + }() if err := e.WriteFence(ctx, releaseID, epoch); err != nil { return err } @@ -105,6 +117,16 @@ func (e *Engine) deployCore(ctx context.Context, releaseID, localStagingDir stri if err := e.requireServingApplicationManifest(ctx, prev); err != nil { return err } + // After preflight, so an unreachable daemon is reported by the check that + // exists for it rather than by a raw `docker ps` failure — and still before + // any workload is rolled or any gate job runs. + if err := e.refuseForeignJobContainers(ctx, releaseID, epoch); err != nil { + holdLockReason = fmt.Sprintf( + "nothing was deployed: %v. The application lock is being kept until this is "+ + "resolved, so nothing else mutates meanwhile; it expires on its own after %s", + err, e.lockTTL()) + return err + } rollbackDebt := false if done == nil { rollbackDebt, err = e.rollbackEffectDebt(ctx, prev) diff --git a/internal/engine/deploy_test.go b/internal/engine/deploy_test.go index c911080..b12e5ef 100644 --- a/internal/engine/deploy_test.go +++ b/internal/engine/deploy_test.go @@ -563,3 +563,52 @@ func TestRollbackReplaysPreviousRelease(t *testing.T) { t.Fatalf("rollback must re-activate previous:\n%s", seq) } } + +// A deploy rolls workloads and runs its own gate jobs. An orphaned job +// container still changing data underneath it is the overlap the lock exists to +// prevent, and the lock does not catch it once its holder is gone. +func TestDeployRefusesWhileAForeignJobContainerRuns(t *testing.T) { + f := happyFake() + inner := f.Dynamic + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + e := New(testConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + err := e.Deploy(context.Background(), "20260101-000000-aaa111", t.TempDir()) + if err == nil || !strings.Contains(err.Error(), "other-op") { + t.Fatalf("deploy = %v, want a refusal naming the foreign operation", err) + } + // Refused before anything is rolled or any gate job runs. + if seq := strings.Join(f.Commands, "\n"); strings.Contains(seq, "--scale web=") { + t.Fatalf("the deploy rolled anyway:\n%s", seq) + } +} + +// A refused deploy keeps the application lock, and has to say so: an operator +// who is told only that the deploy stopped will not know the host is still held. +func TestDeployKeepsAndExplainsTheLockWhenItRefuses(t *testing.T) { + f := happyFake() + inner := f.Dynamic + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + var out bytes.Buffer + e := New(testConfig(), testProject(t), f, Options{Out: &out, Sleep: noSleep}) + if err := e.Deploy(context.Background(), "20260101-000000-aaa111", t.TempDir()); err == nil { + t.Fatal("expected a refusal") + } + for _, c := range f.Commands { + if strings.Contains(c, "rm -f") && strings.Contains(c, "/lock") { + t.Fatalf("the lock was released over a live container:\n%s", c) + } + } + if s := out.String(); !strings.Contains(s, "lock is being kept") { + t.Fatalf("the operator was not told the lock is held:\n%s", s) + } +} diff --git a/internal/engine/job.go b/internal/engine/job.go index 8ffd5de..c0e9240 100644 --- a/internal/engine/job.go +++ b/internal/engine/job.go @@ -49,13 +49,13 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) // while the terminal journal append — which uses the cancelled one — does // not: ownership would be dropped, immediately and silently, over a // container still changing data. - holdLockForLiveContainer := false + // A non-empty reason keeps the lock and says why. Two different situations + // hold it, and telling an operator the wrong one sends them looking for a + // run that never started. + holdLockReason := "" defer func() { - if holdLockForLiveContainer { - e.warnf("operation %s was interrupted while its container is still running; "+ - "keeping the application lock so nothing else mutates alongside it. "+ - "Inspect with `docker ps --filter label=%s=%s`; the lock expires on its own after %s", - operationID, JobOperationLabel, operationID, e.lockTTL()) + if holdLockReason != "" { + e.warnf("%s", holdLockReason) return } e.ReleaseLock(ctx) @@ -86,6 +86,25 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) return operationID, nil, errors.New("job plan is stale: current release runtime changed — re-plan") } + // After the staleness checks, so a stale plan is told it is stale rather + // than told about a container, and before this run creates one of its own. + if err := e.refuseForeignJobContainers(ctx, operationID, epoch); err != nil { + // Keep the lock. Releasing it here would hand the host to the next + // mutator over a container this check has just established is alive — + // the opposite of what refusing is for, and worse than not refusing, + // because the lock reclaimed from the interrupted run would be gone too. + // The sentence around the error asserts nothing about what was found: + // this refuses both when a job container is running and when the host + // could not be asked, and the lock is kept for the same reason either + // way — an unanswered question is not an answer of no. What was + // actually determined travels in the error itself. + holdLockReason = fmt.Sprintf( + "nothing was run: %v. The application lock is being kept until this is "+ + "resolved, so nothing else mutates meanwhile; it expires on its own after %s", + err, e.lockTTL()) + return operationID, nil, err + } + writer := &journal.Writer{ T: e.T, Names: e.names(), DeployID: operationID, Epoch: epoch, Operator: journal.DefaultOperator(), GitSHA: e.Opts.GitSHA, ConfigHash: e.Opts.ConfigHash, @@ -175,7 +194,13 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) if interruptedRun(ctx, runErr) { // Cancelling the client kills at most the wrapper shell; the container // belongs to the daemon and keeps running. - holdLockForLiveContainer = e.jobContainerRunning(operationID) + if e.jobContainerRunning(operationID) { + holdLockReason = fmt.Sprintf( + "operation %s was interrupted while its container is still running; "+ + "keeping the application lock so nothing else mutates alongside it. "+ + "Inspect with `docker ps --filter label=%s=%s`; the lock expires on its own after %s", + operationID, JobOperationLabel, operationID, e.lockTTL()) + } } var result *journal.JobResultEvidence if evidence, ok := e.jobResults[job]; ok { diff --git a/internal/engine/job_containers.go b/internal/engine/job_containers.go new file mode 100644 index 0000000..c45fd93 --- /dev/null +++ b/internal/engine/job_containers.go @@ -0,0 +1,110 @@ +package engine + +import ( + "context" + "fmt" + "strconv" + "strings" +) + +// refuseForeignJobContainers refuses when a one-off job container from another +// operation is still running on the host. +// +// The question "is a job still running" is asked of Docker, not of the journal. +// A journal says what a client managed to write, and the whole failure mode +// here is a client that did not write. An interrupted run that DID record its +// interruption looks finished on paper while its container keeps changing data, +// and a plan re-run appends a second invocation to the same journal — both are +// invisible to any reduction over records, and both are one `docker ps` away. +func (e *Engine) refuseForeignJobContainers(ctx context.Context, currentOperationID string, currentEpoch int) error { + containers, err := e.jobContainers(ctx) + if err != nil { + return err + } + currentEpochLabel := strconv.Itoa(currentEpoch) + for _, c := range containers { + // Operation AND epoch. A sealed job plan is re-runnable and carries one + // operation id for its whole life, and AcquireLock hands the lock + // straight back to a caller presenting the id already written in it. So + // a second run of one plan would reclaim the lock from a live first run + // and then exempt that run's container as its own — two concurrent + // data-changing containers, which is the single thing this prevents. + if c.operation == currentOperationID && c.epoch == currentEpochLabel { + continue + } + if c.operation == currentOperationID { + // A differing epoch says the container belongs to some other + // invocation of this operation, not which one or when — epochs are + // not ordered against each other here — and a missing epoch says + // only that it cannot be placed at all. Neither supports calling it + // an earlier run. + if c.epoch == "" { + return fmt.Errorf( + "a job container of operation %s is running on this host (%.12s) carrying no %s label, "+ + "so it cannot be placed against this run; establish what it did and stop it with "+ + "`docker rm -f %s`", + c.operation, c.id, JobEpochLabel, c.id) + } + return fmt.Errorf( + "another invocation of operation %s (epoch %s, this run is epoch %s) left a job container "+ + "running on this host (%.12s); wait for it to finish, or establish what it did and stop "+ + "it with `docker rm -f %s`", + c.operation, c.epoch, currentEpochLabel, c.id, c.id) + } + if c.operation == "" { + // The label is present but carries no value, so the container + // cannot be attributed. Refuse anyway: an unattributable job + // container is exactly as dangerous as an attributable one. + return fmt.Errorf( + "a job container is running on this host (%.12s) with an empty %s label, so the operation that "+ + "started it cannot be identified; establish what it did and stop it with `docker rm -f %s`", + c.id, JobOperationLabel, c.id) + } + return fmt.Errorf( + "a job container from operation %s is still running on this host (%.12s); "+ + "if that operation is still in progress, wait for it — otherwise establish what it did "+ + "and stop it with `docker rm -f %s`", + c.operation, c.id, c.id) + } + return nil +} + +type jobContainer struct { + id string + operation string + epoch string +} + +// jobContainers lists every running one-off job container, whichever operation +// created it. The label is unvalued in the filter so this finds containers of +// operations this process knows nothing about, which is the point. +func (e *Engine) jobContainers(ctx context.Context) ([]jobContainer, error) { + res, err := e.T.Run(ctx, + "docker ps --filter label="+q(JobOperationLabel)+ + " --format "+q("{{.ID}} {{.Label \""+JobOperationLabel+"\"}} {{.Label \""+JobEpochLabel+"\"}}")) + if err != nil { + return nil, err + } + if res.ExitCode != 0 { + return nil, fmt.Errorf("list running job containers (exit %d): %s", res.ExitCode, strings.TrimSpace(res.Stderr)) + } + var out []jobContainer + for _, line := range strings.Split(res.Stdout, "\n") { + // Each field is cut and trimmed on its own, and Cut yields empty for a + // separator that is not there — so a label docker could not resolve + // parses as empty whether or not its separator survives. What matters is + // that the id parses: a line that yields none is dropped below, and a + // dropped line is a container nobody can see. + id, rest, _ := strings.Cut(strings.TrimSpace(line), " ") + operation, epoch, _ := strings.Cut(rest, " ") + id, operation, epoch = strings.TrimSpace(id), strings.TrimSpace(operation), strings.TrimSpace(epoch) + if id == "" { + continue + } + if !validID.MatchString(id) { + return nil, fmt.Errorf("suspicious container id %q from docker ps — refusing to reuse in a command", id) + } + out = append(out, jobContainer{id: id, operation: operation, epoch: epoch}) + } + return out, nil +} diff --git a/internal/engine/job_containers_test.go b/internal/engine/job_containers_test.go new file mode 100644 index 0000000..a37d8f2 --- /dev/null +++ b/internal/engine/job_containers_test.go @@ -0,0 +1,174 @@ +package engine + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/labstack/onebox/internal/transport" +) + +// jobContainerFake answers the running-container probe. `running` is +// ` ` lines, exactly as the label probe formats them. +func jobContainerFake(running []string) *transport.Fake { + return &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + switch { + case strings.Contains(cmd, "label='ob.operation'"): + return transport.Result{Stdout: strings.Join(running, "\n") + "\n"}, true + } + return transport.Result{}, false + }} +} + +func jobContainerEngine(t *testing.T, f *transport.Fake) *Engine { + t.Helper() + return New(testConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) +} + +func TestRefuseWhileAnotherOperationsJobContainerRuns(t *testing.T) { + f := jobContainerFake([]string{"abc123def456 J1 4"}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J2", 4) + if err == nil { + t.Fatal("a live job container from another operation must refuse") + } + for _, want := range []string{"J1", "abc123def456", "still running"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("refusal missing %q: %v", want, err) + } + } +} + +// This operation's own container is not a reason to refuse itself — a deploy +// runs gate jobs under its own id. +func TestRefuseAllowsThisOperationsOwnContainer(t *testing.T) { + f := jobContainerFake([]string{"abc123def456 J1 4"}) + if err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 4); err != nil { + t.Fatalf("own container refused: %v", err) + } +} + +func TestRefuseCatchesAContainerWithAnEmptyOperationLabel(t *testing.T) { + f := jobContainerFake([]string{"abc123def456 "}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J2", 4) + if err == nil { + t.Fatal("an unattributable job container must refuse") + } + if !strings.Contains(err.Error(), "empty") || !strings.Contains(err.Error(), "abc123def456") { + t.Fatalf("refusal did not name the problem: %v", err) + } +} + +// A sealed job plan carries one operation id for its whole life and is +// re-runnable, and AcquireLock hands the lock straight back to a caller +// presenting the id already written in it. Matching the operation alone would +// let one run exempt the container another invocation of it left behind. +func TestRefuseCatchesAnotherInvocationOfTheSameOperation(t *testing.T) { + f := jobContainerFake([]string{"abc123def456 J1 4"}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 5) + if err == nil { + t.Fatal("an earlier invocation of the same plan must refuse") + } + for _, want := range []string{"another invocation", "J1", "epoch 4", "this run is epoch 5", "abc123def456"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("refusal missing %q: %v", want, err) + } + } +} + +// A container carrying no epoch label cannot be shown to belong to this +// invocation, so it is not exempt from it either. +func TestRefuseDoesNotExemptAContainerWithNoEpoch(t *testing.T) { + f := jobContainerFake([]string{"abc123def456 J1 "}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 4) + if err == nil || !strings.Contains(err.Error(), "carrying no ob.epoch label") { + t.Fatalf("unlabelled epoch = %v, want a refusal saying it cannot be placed", err) + } +} + +// A line with leading whitespace must still yield its container. Cutting on the +// first space without stripping it yields an empty id, and the container is +// dropped in silence — invisible to the check that exists to see it. +func TestRefuseSeesAContainerOnAPaddedLine(t *testing.T) { + f := jobContainerFake([]string{" abc123def456 other-op 2"}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 1) + if err == nil || !strings.Contains(err.Error(), "other-op") { + t.Fatalf("padded line = %v, want the container refused", err) + } +} + +// The parsing has now been wrong twice in ways a single example did not catch — +// once dropping an empty label, once dropping a padded line. This states the +// whole shape of what `docker ps` can hand back, so the next mistake fails here +// rather than in the field, where a dropped line is a container nobody sees. +func TestJobContainerParsing(t *testing.T) { + for _, tc := range []struct { + name string + stdout string + want []jobContainer + }{ + {"nothing running", "", nil}, + {"blank output", "\n\n", nil}, + {"one container", "abc123def456 op-1 4\n", []jobContainer{{"abc123def456", "op-1", "4"}}}, + {"several", "abc123def456 op-1 4\nfed654cba321 op-2 9\n", + []jobContainer{{"abc123def456", "op-1", "4"}, {"fed654cba321", "op-2", "9"}}}, + // A label docker cannot resolve renders empty, and the separators stay. + {"no epoch label", "abc123def456 op-1 \n", []jobContainer{{"abc123def456", "op-1", ""}}}, + {"no operation label", "abc123def456 \n", []jobContainer{{"abc123def456", "", ""}}}, + {"no trailing separators", "abc123def456\n", []jobContainer{{"abc123def456", "", ""}}}, + {"padded line", " abc123def456 op-1 4\n", []jobContainer{{"abc123def456", "op-1", "4"}}}, + {"carriage return", "abc123def456 op-1 4\r\n", []jobContainer{{"abc123def456", "op-1", "4"}}}, + {"no trailing newline", "abc123def456 op-1 4", []jobContainer{{"abc123def456", "op-1", "4"}}}, + } { + t.Run(tc.name, func(t *testing.T) { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + return transport.Result{Stdout: tc.stdout}, true + }} + got, err := jobContainerEngine(t, f).jobContainers(context.Background()) + if err != nil { + t.Fatalf("parse %q: %v", tc.stdout, err) + } + if len(got) != len(tc.want) { + t.Fatalf("parse %q = %+v, want %+v", tc.stdout, got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("parse %q [%d] = %+v, want %+v", tc.stdout, i, got[i], tc.want[i]) + } + } + }) + } +} + +// A daemon that cannot answer must stop the operation, not report an empty host. +func TestJobContainersRefusesAnUnusableAnswer(t *testing.T) { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + return transport.Result{ExitCode: 1, Stderr: "Cannot connect to the Docker daemon"}, true + }} + if _, err := jobContainerEngine(t, f).jobContainers(context.Background()); err == nil { + t.Fatal("a failed docker ps must not read as no containers") + } + bad := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + return transport.Result{Stdout: "not-a-container-id op-1 4\n"}, true + }} + if _, err := jobContainerEngine(t, bad).jobContainers(context.Background()); err == nil { + t.Fatal("output that is not a container id must not be trusted") + } +} + +// Exempting this run's own container must not end the scan. A deploy runs its +// own gate jobs, so its container is routinely listed first — and a foreign one +// behind it is exactly what this exists to catch. +func TestRefuseKeepsScanningPastItsOwnContainer(t *testing.T) { + f := jobContainerFake([]string{ + "aaa111bbb222 op-mine 3", + "ccc333ddd444 op-other 9", + }) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "op-mine", 3) + if err == nil { + t.Fatal("a foreign container behind an exempt one must still refuse") + } + if !strings.Contains(err.Error(), "op-other") || !strings.Contains(err.Error(), "ccc333ddd444") { + t.Fatalf("refusal named the wrong container: %v", err) + } +} diff --git a/internal/engine/job_test.go b/internal/engine/job_test.go index 88cfd4b..67b2ba6 100644 --- a/internal/engine/job_test.go +++ b/internal/engine/job_test.go @@ -11,6 +11,13 @@ import ( ) func manualJobEngine(t *testing.T, target *transport.Fake) *Engine { + t.Helper() + return manualJobEngineTo(t, target, &bytes.Buffer{}) +} + +// manualJobEngineTo is the same engine with its narration captured, for the +// tests that assert what an operator is told. +func manualJobEngineTo(t *testing.T, target *transport.Fake, out *bytes.Buffer) *Engine { t.Helper() config := testConfig() job := config.Workloads["migrate"] @@ -18,7 +25,7 @@ func manualJobEngine(t *testing.T, target *transport.Fake) *Engine { job.DataEffect = "none" config.Workloads["migrate"] = job return New(config, testProject(t), target, Options{ - Out: &bytes.Buffer{}, Sleep: noSleep, + Out: out, Sleep: noSleep, ApprovalDigest: "approval-digest", ApprovalClass: "one_time", ApprovedBy: "operator@example.test", ApprovalSource: "local_cli", }) @@ -172,3 +179,118 @@ func TestInterruptedRunClassifiesTheRunNotTheClient(t *testing.T) { t.Fatal("a job that failed on its own terms is not interrupted") } } + +// The refusal is only worth having if it is actually called. Deleting the call +// site left the unit tests green, so this drives the whole run against a host +// reporting a foreign job container and requires it to stop before the job +// starts. +func TestRunJobRefusesWhileAForeignJobContainerRuns(t *testing.T) { + const runtime = "services:\n migrate:\n image: ghcr.io/x/app@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + target := currentJobFake(runtime) + inner := target.Dynamic + target.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + engine := manualJobEngine(t, target) + _, _, err := engine.RunJobWithJournalID(context.Background(), JobRunRequest{ + OperationID: "op-job-run", Job: "migrate", ExpectedRelease: engineTestPreviousReleaseID, + ExpectedRuntimeDigest: HashBytes([]byte(runtime)), ExpectedDataEffect: "none", + }) + if err == nil || !strings.Contains(err.Error(), "other-op") { + t.Fatalf("run job = %v, want a refusal naming the foreign operation", err) + } + if strings.Contains(strings.Join(target.Commands, "\n"), "ONEBOX_RESULT_FILE=") { + t.Fatalf("the job ran anyway:\n%s", strings.Join(target.Commands, "\n")) + } +} + +// Refusing must not hand the host to the next mutator. Releasing the lock here +// would leave no lock and a live data-changing container — worse than not +// refusing, because the lock this run reclaimed from the interrupted one would +// be gone with it. +func TestRunJobKeepsTheLockWhenItRefuses(t *testing.T) { + const runtime = "services:\n migrate:\n image: ghcr.io/x/app@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + target := currentJobFake(runtime) + inner := target.Dynamic + target.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + engine := manualJobEngine(t, target) + if _, _, err := engine.RunJobWithJournalID(context.Background(), JobRunRequest{ + OperationID: "op-job-run", Job: "migrate", ExpectedRelease: engineTestPreviousReleaseID, + ExpectedRuntimeDigest: HashBytes([]byte(runtime)), ExpectedDataEffect: "none", + }); err == nil { + t.Fatal("expected a refusal") + } + for _, c := range target.Commands { + if strings.Contains(c, "rm -f") && strings.Contains(c, "/lock") { + t.Fatalf("the lock was released over a live container:\n%s", c) + } + } +} + +// Both situations keep the lock, and each has to say which it is. Telling an +// operator their run was interrupted when it never started sends them looking +// for work that does not exist. +func TestRunJobExplainsWhyItKeptTheLock(t *testing.T) { + const runtime = "services:\n migrate:\n image: ghcr.io/x/app@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + target := currentJobFake(runtime) + inner := target.Dynamic + target.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + var out bytes.Buffer + engine := manualJobEngineTo(t, target, &out) + if _, _, err := engine.RunJobWithJournalID(context.Background(), JobRunRequest{ + OperationID: "op-job-run", Job: "migrate", ExpectedRelease: engineTestPreviousReleaseID, + ExpectedRuntimeDigest: HashBytes([]byte(runtime)), ExpectedDataEffect: "none", + }); err == nil { + t.Fatal("expected a refusal") + } + if s := out.String(); !strings.Contains(s, "nothing was run") { + t.Fatalf("refusal did not say the run never started:\n%s", s) + } + if s := out.String(); strings.Contains(s, "was interrupted while its container") { + t.Fatalf("refusal claimed this run was interrupted:\n%s", s) + } +} + +// The refusal also fires when the host cannot be asked, and the lock is kept +// for the same reason: an unanswered question is not an answer of no. The +// narration must not claim a container was found in that case. +func TestRunJobKeepsTheLockWhenItCannotAskTheHost(t *testing.T) { + const runtime = "services:\n migrate:\n image: ghcr.io/x/app@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + target := currentJobFake(runtime) + inner := target.Dynamic + target.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{ExitCode: 1, Stderr: "Cannot connect to the Docker daemon"}, true + } + return inner(cmd) + } + var out bytes.Buffer + engine := manualJobEngineTo(t, target, &out) + if _, _, err := engine.RunJobWithJournalID(context.Background(), JobRunRequest{ + OperationID: "op-job-run", Job: "migrate", ExpectedRelease: engineTestPreviousReleaseID, + ExpectedRuntimeDigest: HashBytes([]byte(runtime)), ExpectedDataEffect: "none", + }); err == nil { + t.Fatal("an unanswerable host must refuse") + } + for _, c := range target.Commands { + if strings.Contains(c, "rm -f") && strings.Contains(c, "/lock") { + t.Fatalf("the lock was released without an answer:\n%s", c) + } + } + if s := out.String(); strings.Contains(s, "alongside that container") { + t.Fatalf("narration claimed a container was found:\n%s", s) + } +} diff --git a/internal/onebox/service_test.go b/internal/onebox/service_test.go index 113c25b..827873e 100644 --- a/internal/onebox/service_test.go +++ b/internal/onebox/service_test.go @@ -81,6 +81,9 @@ func serviceFake() *transport.Fake { return transport.Result{Stdout: "demo\n"}, true case strings.Contains(cmd, "readlink"): return transport.Result{Stdout: "releases/R0\n"}, true + // Ahead of the project-container probe, which also uses --format. + case strings.Contains(cmd, "docker ps --filter label='ob.operation'"): + return transport.Result{Stdout: "\n"}, true case strings.Contains(cmd, "docker ps") && strings.Contains(cmd, "--format"): return transport.Result{Stdout: "S1|web|R0|Up (healthy)\nPG1|database|R0|Up (healthy)\n"}, true case strings.Contains(cmd, "for f in"): @@ -267,6 +270,8 @@ deployment: {order: [web, worker]} return transport.Result{Stdout: project(false)}, true case strings.Contains(command, "cat ") && strings.Contains(command, "compose.yaml"): return transport.Result{Stdout: liveCompose}, true + case strings.Contains(command, "docker ps --filter label='ob.operation'"): + return transport.Result{Stdout: "\n"}, true case strings.Contains(command, "docker ps") && strings.Contains(command, "--format"): return transport.Result{Stdout: "S1|web|R0|Up\nW1|worker|R0|Up\n"}, true default: