diff --git a/cmd/stalwart-migrate/rehearse.go b/cmd/stalwart-migrate/rehearse.go index b2a69a3..e27297e 100644 --- a/cmd/stalwart-migrate/rehearse.go +++ b/cmd/stalwart-migrate/rehearse.go @@ -113,7 +113,7 @@ func runRehearse(args []string) (err error) { BinaryPath: *binaryPath, ConfigPath: *configPath, DataDir: *dataDir, ContainerName: *containerName, AdminURL: *adminURL, AdminUser: *adminUser, AdminPassword: *adminPassword, TargetVersion: *targetVersion, MinFreeMultiple: *minFree, HTTPClient: httpClient, - CLIPath: *stalwartCLI, PythonPath: *pythonPath, ToolCheckAdvisory: true, + CLIPath: *stalwartCLI, PythonPath: *pythonPath, ToolCheckAdvisory: true, DeploymentCheckAdvisory: true, }) pfReport, err := checker.Run(ctx, store, rs) fmt.Print(pfReport.String()) diff --git a/cmd/stalwart-migrate/run.go b/cmd/stalwart-migrate/run.go index 67a7803..503f475 100644 --- a/cmd/stalwart-migrate/run.go +++ b/cmd/stalwart-migrate/run.go @@ -253,6 +253,41 @@ func runRun(args []string) (err error) { } fmt.Println(controller.Target(), "stopped") + // Mail is down from here, and every return below is a return with it + // still down. Registered after the stop rather than before, so it only + // ever restarts something this tool actually stopped. + // + // It does not pretend to have recovered the migration: a run that + // aborted midway is still part-migrated and still needs --resume or the + // operator's recovery point. What it prevents is the narrower and worse + // outcome of the tool exiting on an error it could see coming while the + // server it stopped stays stopped. + defer func() { + if err == nil { + return + } + // The run's context may already be cancelled - that can be why we + // are here - and a cancelled context cannot start anything. + restartCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 3*time.Minute) + defer cancel() + active, activeErr := controller.Active(restartCtx) + if activeErr == nil && active { + return + } + fmt.Println("\n--- this run failed with", controller.Target(), "stopped: starting it again ---") + if startErr := controller.Start(restartCtx); startErr != nil { + fmt.Printf("could not start %s: %v\n", controller.Target(), startErr) + fmt.Println("MAIL IS STILL DOWN - start it by hand before anything else.") + return + } + if waitErr := service.WaitFor(restartCtx, controller, true, 2*time.Minute); waitErr != nil { + fmt.Printf("%s was asked to start but did not come up: %v\n", controller.Target(), waitErr) + fmt.Println("MAIL IS STILL DOWN - check it by hand before anything else.") + return + } + fmt.Println(controller.Target(), "is running again. The migration itself did not complete - see the error below.") + }() + if p.CrossesMajorBoundary { fmt.Println("\n--- convert ---") if _, err := store.RunStep(rs, checkpoint.PhaseStage, "convert-settings", func() (checkpoint.StepOutcome, error) { diff --git a/internal/preflight/checks.go b/internal/preflight/checks.go index 3bfd780..a228322 100644 --- a/internal/preflight/checks.go +++ b/internal/preflight/checks.go @@ -44,7 +44,15 @@ type Options struct { // about to be used and a missing one means stopping a mail server to // find out. ToolCheckAdvisory bool - HTTPClient *http.Client + // DeploymentCheckAdvisory downgrades the deployment-kind check from + // blocking to advisory, for the same reason as ToolCheckAdvisory. + // `rehearse` sets it: it never stops the service or cuts over, so a + // deployment this tool cannot cut over is still worth rehearsing + // against - the reconnaissance is exactly what tells an operator what + // the manual path involves. `run` leaves it false, because there the + // alternative is finding out after mail is already down. + DeploymentCheckAdvisory bool + HTTPClient *http.Client } // Checker runs the preflight checks described in ARCHITECTURE.md ยง4.1. @@ -186,6 +194,20 @@ 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) + // 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 + // automate it - but cutover runs after the service has been + // stopped. Refusing there means refusing with mail already down, + // which is how a migration attempt turned into an outage. + if kind == DeploymentDocker && !c.opts.DeploymentCheckAdvisory { + return CheckResult{ + Status: StatusFail, + Detail: "detected deployment kind: docker - this tool cannot cut over a container. " + + "Migrating one means pulling the new image and recreating the container, which has to be done by hand; " + + "`rehearse` still works and will tell you what the migration involves", + }, string(kind) + } status := StatusOK if kind == DeploymentUnknown { status = StatusWarn diff --git a/internal/preflight/checks_test.go b/internal/preflight/checks_test.go index e623b0c..233f65f 100644 --- a/internal/preflight/checks_test.go +++ b/internal/preflight/checks_test.go @@ -450,3 +450,95 @@ func TestAdminAccountKindWithoutASnapshot(t *testing.T) { t.Fatalf("status = %q, want a warning when there was nothing to look in", res.Status) } } + +// withFakeDocker puts a `docker` on PATH that answers `inspect` successfully, +// so DetectDeploymentKind sees a container without one being involved. +// +// It skips rather than fails if this host has a real stalwart systemd unit: +// detection checks those paths first and would win, and a test that depends +// on the host *not* having Stalwart installed is a test that fails for a +// reason unrelated to what it is checking. +func withFakeDocker(t *testing.T) { + t.Helper() + for _, p := range systemdUnitPaths { + if _, err := os.Stat(p); err == nil { + t.Skipf("host has %s, which detection prefers over docker", p) + } + } + dir := t.TempDir() + script := "#!/bin/sh\ncase \"$1\" in inspect) exit 0 ;; esac\nexit 1\n" + if err := os.WriteFile(filepath.Join(dir, "docker"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +// dockerPreflight runs a minimal but real preflight against a fake 0.15.5 +// install with a fake docker on PATH. +func dockerPreflight(t *testing.T, advisory bool) Report { + t.Helper() + withFakeDocker(t) + + counterPath := filepath.Join(t.TempDir(), "invocations") + binaryPath := writeFakeBinary(t, "0.15.5", counterPath) + configPath := filepath.Join(t.TempDir(), "config.toml") + if err := os.WriteFile(configPath, []byte("[server]\nhostname = \"mail.example.com\"\n"), 0o644); err != nil { + t.Fatal(err) + } + withFakeGithub(t, func(w http.ResponseWriter, r *http.Request) { + json.NewEncoder(w).Encode(Release{TagName: "v0.16.14"}) + }) + store := checkpoint.NewStore(t.TempDir()) + rs, err := store.Create("", "latest") + if err != nil { + t.Fatal(err) + } + // ToolCheckAdvisory is on in both cases so the deployment flag is the + // only thing that varies: whether stalwart-cli happens to be installed + // on the machine running the tests is not what this is testing. + report, err := New(Options{ + BinaryPath: binaryPath, ConfigPath: configPath, DataDir: t.TempDir(), + TargetVersion: "latest", ToolCheckAdvisory: true, DeploymentCheckAdvisory: advisory, + }).Run(context.Background(), store, rs) + if err != nil { + t.Fatalf("Run: %v", err) + } + return report +} + +// A container has to be refused here, in preflight, and not later. Cutover +// already refuses it -- but cutover runs after the service has been stopped, +// so refusing there refuses with mail down, which turned an attempted +// migration into an outage. +func TestPreflightBlocksADockerDeployment(t *testing.T) { + report := dockerPreflight(t, false) + if !report.Blocking() { + t.Fatalf("expected a blocking report for a docker deployment, got:\n%s", report.String()) + } + var found bool + for _, res := range report.Results { + if res.Name == "deployment-kind" { + found = true + if res.Status != StatusFail { + t.Errorf("deployment-kind status = %q, want %q", res.Status, StatusFail) + } + if !strings.Contains(res.Detail, "docker") { + t.Errorf("deployment-kind detail does not mention docker: %q", res.Detail) + } + } + } + if !found { + t.Fatalf("no deployment-kind result in report:\n%s", report.String()) + } +} + +// rehearse is read-only: it never stops anything and never cuts over, so it +// has to keep working against a container. Telling an operator what the +// migration involves is most useful precisely when the tool cannot do it for +// them. +func TestRehearseStillRunsAgainstADockerDeployment(t *testing.T) { + report := dockerPreflight(t, true) + if report.Blocking() { + t.Fatalf("advisory mode should not block on docker, got:\n%s", report.String()) + } +}