From 56152ed7de69d7e101be60526b44681534fde25d Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sat, 29 Aug 2026 17:56:32 -0700 Subject: [PATCH] Ask the container what it is running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preflight's first check ran `--version` on --binary. A container-only host has no such file, so the check failed, and because it is first, nothing downstream ever ran — including every container check that exists to decide whether that container can be migrated at all. The container path was unreachable on exactly the hosts it is for. A host that happens to have a binary is the worse case, not the better one: a stray /usr/local/bin/stalwart from an older install answers confidently with a version nothing is running, and the whole migration plan is derived from that number. The source version now comes from running the image the container is on, by ID rather than by the tag it was started from, using the same command and the same fallback stage already uses for the target image — the two have to agree about what a Stalwart image reports or the source and target could be read by different rules. Verified against a real stalwartlabs/stalwart image, not only the fake. Deployment kind is detected once and shared, rather than asked again by the check that reports it. Two answers for one run is not a thing this should be able to produce. The same reasoning retires preserve-binary on a container: there is nothing on this host to move aside, and the equivalent is already guaranteed, since cutover renames the old container and never prunes the old image. Renaming a stray binary would have preserved something nothing was running. --- ARCHITECTURE.md | 13 +++++++ cmd/stalwart-migrate/run.go | 11 ++++++ internal/preflight/checks.go | 24 ++++++++++-- internal/preflight/container.go | 9 +++++ internal/preflight/container_test.go | 56 +++++++++++++++++++++++++++- internal/preflight/version.go | 41 ++++++++++++++++++++ 6 files changed, 150 insertions(+), 4 deletions(-) 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) {