Merge pull request #14 from LINUXexpert-org/container-source-version

Ask the container what it is running

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

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

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

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 {

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)
+	}
+}

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) {