diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 4c5645d..8ecdd8d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -304,6 +304,19 @@ against an already-migrated store. while the server is still running, and again at cutover: the answer does not change between them, and only one of the two points can refuse without having already cost an outage. +- What is *running* is a property of a container's image, not of anything + on this host. Preflight's first check used to run `--version` on + `--binary`, which a container-only host does not have - so the first + check failed and nothing downstream ever ran, including every container + check. A host that happens to have a stray binary is worse than one that + does not: it answers confidently with a version nothing is running. The + source version is now read by running the image the container is on, by + ID rather than by the tag it was started from, with the same command + §4.3 uses for the target image so the two cannot disagree about what a + Stalwart image reports. +- For the same reason there is no old binary to move aside on a container. + Its equivalent is already guaranteed: the old container is renamed and + the old image is never pruned. - What a container *inherits* from its image is not what it *overrides*, and only the override is the operator's. `docker inspect` reports `User`, `Cmd` and `Entrypoint` either way - a container off the official diff --git a/cmd/stalwart-migrate/run.go b/cmd/stalwart-migrate/run.go index bb4eca8..70ecfa0 100644 --- a/cmd/stalwart-migrate/run.go +++ b/cmd/stalwart-migrate/run.go @@ -292,6 +292,17 @@ func runRun(args []string) (err error) { fmt.Println("\n--- preserve the old binary ---") if _, err := store.RunStep(rs, checkpoint.PhaseBackup, "preserve-binary", func() (checkpoint.StepOutcome, error) { + // A container has no binary on this host to move aside, and its + // equivalent is already guaranteed elsewhere: cutover renames the + // old container rather than removing it and never prunes the old + // image, which together are what an operator starts again by hand + // (§4.5). Renaming a stray /usr/local/bin/stalwart here would + // preserve something nothing was running. + if isContainer { + return checkpoint.StepOutcome{Detail: fmt.Sprintf( + "container deployment: nothing to preserve on this host. The old image (%s) is never pruned and the old "+ + "container is renamed rather than removed at cutover", containerFacts.Image)}, nil + } preserved, err := backup.PreserveBinary(*binaryPath, rs.SourceVersion) if err != nil { return checkpoint.StepOutcome{}, err diff --git a/internal/preflight/checks.go b/internal/preflight/checks.go index bde973e..e7ff8f6 100644 --- a/internal/preflight/checks.go +++ b/internal/preflight/checks.go @@ -96,8 +96,25 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi return outcome, nil } + // Detected once, here, because the very first check needs it: what is + // running is a property of a container's image, not of any binary on + // this host, and a container-only host has no binary to ask. The + // deployment-kind check below reports it; this only decides who to put + // the question to. + kind := DetectDeploymentKind(ctx, c.opts.ContainerName) + versionOutcome, err := runCheck("version", func() (CheckResult, string) { - cur, err := DetectVersion(ctx, c.opts.BinaryPath) + var ( + cur string + err error + source string + ) + if kind == DeploymentDocker { + cur, err = DetectContainerVersion(ctx, c.opts.ContainerName) + source = ", read from the image behind container " + containerNameOr(c.opts.ContainerName) + } else { + cur, err = DetectVersion(ctx, c.opts.BinaryPath) + } if err != nil { return CheckResult{Status: StatusFail, Detail: err.Error()}, "" } @@ -108,7 +125,7 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi Detail: fmt.Sprintf("current version %s is older than the minimum supported %s - upgrade to 0.15.x first", cur, minSupportedSource), }, cur } - return CheckResult{Status: StatusOK, Detail: fmt.Sprintf("current version %s", cur)}, cur + return CheckResult{Status: StatusOK, Detail: fmt.Sprintf("current version %s%s", cur, source)}, cur }) if err != nil { return report, err @@ -193,7 +210,8 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi } deploymentOutcome, err := runCheck("deployment-kind", func() (CheckResult, string) { - kind := DetectDeploymentKind(ctx, c.opts.ContainerName) + // Detected once above, since the version check already needed it. + // Asking twice could report two different answers for one run. // Docker has to fail here rather than later. Cutover refuses this // deployment - recreating a container from a new image is not // swapping a binary and rewriting a unit, and this tool does not diff --git a/internal/preflight/container.go b/internal/preflight/container.go index e53d8cc..198c0a9 100644 --- a/internal/preflight/container.go +++ b/internal/preflight/container.go @@ -436,6 +436,15 @@ func describeOverrides(f ContainerFacts) string { return ", including what it overrides on its image: " + strings.Join(parts, ", ") } +// containerNameOr is the container this tool would act on, defaulted the +// same way every other caller defaults it. +func containerNameOr(name string) string { + if name == "" { + return "stalwart" + } + return name +} + func shortID(id string) string { id = strings.TrimPrefix(id, "sha256:") if len(id) > 12 { diff --git a/internal/preflight/container_test.go b/internal/preflight/container_test.go index 919a41d..c96d8f1 100644 --- a/internal/preflight/container_test.go +++ b/internal/preflight/container_test.go @@ -20,6 +20,15 @@ import ( // it. A container reporting exactly these has overridden nothing, which is // the case the image comparison exists to recognise - `docker inspect` // reports all three either way. +// fakeImageID is the digest inspectDoc reports, and what the fake answers +// `inspect -f {{.Image}}` with. fakeContainerVersion is what running that +// image prints for --version - the same 0.15.5 the fake host binary +// reports, so the docker and binary paths are testing the same migration. +const ( + fakeImageID = "sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + fakeContainerVersion = "stalwart 0.15.5" +) + var ( imageUser = "stalwart" imageEntrypoint = []string{"/usr/local/bin/stalwart"} @@ -47,9 +56,16 @@ func fakeInspectOn(t *testing.T, containerDoc, imgDoc string) { if err := os.WriteFile(img, []byte(imgDoc), 0o644); err != nil { t.Fatal(err) } + // Four questions this tool asks docker: the container's definition, + // the image's defaults, the container's image ID, and what that image + // reports as its version. script := fmt.Sprintf("#!/bin/sh\n"+ "case \"$1 $2\" in \"image inspect\") cat %q ; exit 0 ;; esac\n"+ - "case \"$1\" in inspect) cat %q ;; *) exit 1 ;; esac\n", img, out) + "case \"$1\" in\n"+ + " inspect) if [ \"$2\" = \"-f\" ]; then echo %q; else cat %q; fi ;;\n"+ + " run) echo %q ;;\n"+ + " *) exit 1 ;;\n"+ + "esac\n", img, fakeImageID, out, fakeContainerVersion) if err := os.WriteFile(filepath.Join(dir, "docker"), []byte(script), 0o755); err != nil { t.Fatal(err) } @@ -323,3 +339,41 @@ func TestInspectContainerRefusesWhenTheImageCannotBeRead(t *testing.T) { t.Fatal("want an error when the image's defaults cannot be read") } } + +// A container-only host has no Stalwart binary, so reading the running +// version from --binary failed preflight's very first check and nothing +// downstream ever ran. What is running is a property of the container's +// image. +func TestPreflightReadsTheSourceVersionFromTheContainersImage(t *testing.T) { + report := dockerPreflightOn(t, false, nil) + for _, res := range report.Results { + if res.Name != "version" { + continue + } + if res.Status != StatusOK { + t.Fatalf("version status = %q, want %q: %s", res.Status, StatusOK, res.Detail) + } + if !strings.Contains(res.Detail, "0.15.5") { + t.Errorf("version detail should report the image's version, got %q", res.Detail) + } + if !strings.Contains(res.Detail, "image behind container") { + t.Errorf("version detail should say where it read the version, got %q", res.Detail) + } + return + } + t.Fatalf("no version result in report:\n%s", report.String()) +} + +// Asking the image by ID rather than by the tag the container was started +// from: a moved tag would report a version nothing is running. +func TestDetectContainerVersionAsksTheImageTheContainerIsOn(t *testing.T) { + fakeInspect(t, inspectDoc(t, nil, []Mount{dataVolume("/var/lib/stalwart")})) + + got, err := DetectContainerVersion(context.Background(), "stalwart") + if err != nil { + t.Fatalf("DetectContainerVersion: %v", err) + } + if got != "0.15.5" { + t.Errorf("DetectContainerVersion = %q, want 0.15.5", got) + } +} diff --git a/internal/preflight/version.go b/internal/preflight/version.go index cd7fe60..340f894 100644 --- a/internal/preflight/version.go +++ b/internal/preflight/version.go @@ -77,6 +77,47 @@ func VersionFromOutput(out string) (string, error) { return v.String(), nil } +// DetectContainerVersion reads the running version of a container +// deployment, which is a property of its image rather than of anything on +// this host. +// +// A container-only host has no Stalwart binary to ask, and a host that +// happens to have one is worse than a host that does not: a stray +// /usr/local/bin/stalwart left over from an older install answers +// confidently with a version nothing is running. So this asks the image +// the container is actually on, by ID rather than by the tag it was +// started from. +// +// The command mirrors internal/stage's, including the fallback, because +// the two have to agree about what a Stalwart image reports or the source +// and target versions could be read by different rules. +func DetectContainerVersion(ctx context.Context, containerName string) (string, error) { + if containerName == "" { + containerName = "stalwart" + } + out, err := exec.CommandContext(ctx, "docker", "inspect", "-f", "{{.Image}}", containerName).Output() + if err != nil { + return "", fmt.Errorf("preflight: reading the image behind container %s: %w", containerName, err) + } + imageID := strings.TrimSpace(string(out)) + if imageID == "" { + return "", fmt.Errorf("preflight: container %s reports no image", containerName) + } + + raw, runErr := exec.CommandContext(ctx, "docker", "run", "--rm", imageID, "--version").Output() + if runErr != nil { + raw, runErr = exec.CommandContext(ctx, "docker", "run", "--rm", "--entrypoint", "stalwart", imageID, "--version").Output() + } + if runErr != nil { + return "", fmt.Errorf("preflight: asking image %s for its version: %w", shortID(imageID), runErr) + } + v, err := VersionFromOutput(string(raw)) + if err != nil { + return "", fmt.Errorf("preflight: image %s did not report a version this understands: %w", shortID(imageID), err) + } + return v, nil +} + // DetectVersion runs the installed binary's --version flag and extracts a // semver from its output. func DetectVersion(ctx context.Context, binaryPath string) (string, error) {