From f0d088bc1d2f2a57f5d5aa3946582a402aaf453d Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 30 Apr 2026 02:37:01 -0400 Subject: [PATCH] test: cover the legacy -v migration with go and bats tests Adds plugins/storage/migrate_test.go exercising migrateApp and MigrateLegacyMounts: single deploy phase, cross-phase grouping into one attachment, :ro and free-form volume options preserved on the attachment, idempotency on a second pass, the per-app flag-file fast path that skips already-migrated apps, and the name-collision refusal that aborts before draining. tests/unit/storage.bats gains an end-to-end case that stages a -v line via docker-options:add, runs storage:migrate, and asserts the synthesized colon form shows up in storage:list, the legacy- entry shows up in storage:list-entries, and the original -v line is gone from docker-options:report on both phases. To make that test (and operators with restored backups) practical, storage gains a storage:migrate |--all subcommand backed by a new MigrateApp helper that skips the per-app flag file so a re-run actually re-scans. --- plugins/storage/Makefile | 2 +- plugins/storage/commands_entries.go | 38 +++ plugins/storage/migrate.go | 15 + plugins/storage/migrate_test.go | 317 ++++++++++++++++++ plugins/storage/src/commands/commands.go | 1 + .../storage/src/subcommands/subcommands.go | 5 + plugins/storage/subcommands.go | 1 + tests/unit/storage.bats | 68 ++++ 8 files changed, 446 insertions(+), 1 deletion(-) create mode 100644 plugins/storage/migrate_test.go diff --git a/plugins/storage/Makefile b/plugins/storage/Makefile index 520b4e9a6..11829d5ec 100644 --- a/plugins/storage/Makefile +++ b/plugins/storage/Makefile @@ -1,5 +1,5 @@ GOARCH ?= amd64 -SUBCOMMANDS = subcommands/default subcommands/create subcommands/destroy subcommands/ensure-directory subcommands/exec subcommands/info subcommands/list subcommands/list-entries subcommands/mount subcommands/report subcommands/set subcommands/unmount subcommands/wait +SUBCOMMANDS = subcommands/default subcommands/create subcommands/destroy subcommands/ensure-directory subcommands/exec subcommands/info subcommands/list subcommands/list-entries subcommands/migrate subcommands/mount subcommands/report subcommands/set subcommands/unmount subcommands/wait TRIGGERS = triggers/install triggers/storage-list triggers/storage-app-mounts triggers/docker-args-deploy triggers/docker-args-run triggers/post-delete triggers/post-app-clone-setup triggers/post-app-rename-setup BUILD = commands subcommands triggers PLUGIN_NAME = storage diff --git a/plugins/storage/commands_entries.go b/plugins/storage/commands_entries.go index ada6f68e3..a79d1cdff 100644 --- a/plugins/storage/commands_entries.go +++ b/plugins/storage/commands_entries.go @@ -323,6 +323,44 @@ func stdinModes() (interactive bool, tty bool) { return interactive, tty } +// CommandMigrate re-runs the legacy `-v` → entry/attachment migration +// for a single app or every app on the install. Useful when an operator +// has restored an app from a backup that predates the install-time +// migration, or when they've added `-v` lines via docker-options:add +// after the per-app flag was set. +func CommandMigrate(appName string, all bool) error { + if all && appName != "" { + return errors.New("storage:migrate accepts either an app name or --all, not both") + } + if !all && appName == "" { + return errors.New("storage:migrate requires an app name (or --all)") + } + if all { + apps, err := common.DokkuApps() + if err != nil { + if errors.Is(err, common.NoAppsExist) { + return nil + } + return err + } + for _, app := range apps { + if err := MigrateApp(app); err != nil { + return err + } + } + common.LogInfo1(fmt.Sprintf("Re-migrated %d app(s)", len(apps))) + return nil + } + if err := common.VerifyAppName(appName); err != nil { + return err + } + if err := MigrateApp(appName); err != nil { + return err + } + common.LogInfo1(fmt.Sprintf("Re-migrated %s", appName)) + return nil +} + // CommandWait blocks until a k3s storage entry's PVC is bound. func CommandWait(name string) error { if !EntryExists(name) { diff --git a/plugins/storage/migrate.go b/plugins/storage/migrate.go index 3dba56eb0..62fae8d8c 100644 --- a/plugins/storage/migrate.go +++ b/plugins/storage/migrate.go @@ -25,6 +25,21 @@ func migrationFlagFile(appName string) string { return filepath.Join(migrationFlagDir(), appName) } +// MigrateApp re-runs the legacy migration for a single app, ignoring +// the per-app flag file so an operator can force-rescan an app whose +// docker-options state changed after the bulk install-time pass. +// Used by `dokku storage:migrate `. +func MigrateApp(appName string) error { + if err := os.MkdirAll(migrationFlagDir(), 0755); err != nil { + return fmt.Errorf("unable to create migration flag dir: %w", err) + } + _ = os.Remove(migrationFlagFile(appName)) + if err := migrateApp(appName); err != nil { + return fmt.Errorf("storage migration failed for app %q: %w", appName, err) + } + return nil +} + // MigrateLegacyMounts walks every app and converts its legacy `-v` // docker-options entries into named storage entries plus attachments. // Idempotent: per-app flag files short-circuit re-runs. diff --git a/plugins/storage/migrate_test.go b/plugins/storage/migrate_test.go new file mode 100644 index 000000000..41188e246 --- /dev/null +++ b/plugins/storage/migrate_test.go @@ -0,0 +1,317 @@ +package storage + +import ( + "os" + "os/user" + "path/filepath" + "sort" + "testing" + + "github.com/dokku/dokku/plugins/common" + dockeroptions "github.com/dokku/dokku/plugins/docker-options" +) + +// setupMigrationEnv points DOKKU_LIB_ROOT and DOKKU_ROOT at temp dirs and +// tells the permission helpers to chown to the current user (a no-op), +// so the test works without root and without the dokku system user. +// Mirrors the helper in docker-options/migration_test.go. +func setupMigrationEnv(t *testing.T) (libRoot string, dokkuRoot string) { + t.Helper() + libRoot = t.TempDir() + dokkuRoot = t.TempDir() + + t.Setenv("DOKKU_LIB_ROOT", libRoot) + t.Setenv("DOKKU_ROOT", dokkuRoot) + t.Setenv("PLUGIN_PATH", filepath.Join(libRoot, "plugins")) + + current, err := user.Current() + if err != nil { + t.Fatalf("user.Current: %v", err) + } + group, err := user.LookupGroupId(current.Gid) + if err != nil { + t.Fatalf("user.LookupGroupId: %v", err) + } + t.Setenv("DOKKU_SYSTEM_USER", current.Username) + t.Setenv("DOKKU_SYSTEM_GROUP", group.Name) + + // MigrateLegacyMounts creates the flag dir before iterating apps; + // migrateApp (the per-app helper) assumes it already exists. Pre- + // create it so tests that call migrateApp directly don't trip. + if err := os.MkdirAll(migrationFlagDir(), 0755); err != nil { + t.Fatalf("mkdir migration flag dir: %v", err) + } + + return libRoot, dokkuRoot +} + +// stageApp registers the named app under DOKKU_ROOT so DokkuApps can +// see it, and seeds its docker-options property files for the requested +// phases. Each map entry is a phase ("deploy", "run", "build") whose +// value is the full property-list contents (one option per line). +func stageApp(t *testing.T, dokkuRoot string, app string, options map[string][]string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(dokkuRoot, app), 0755); err != nil { + t.Fatalf("mkdir app: %v", err) + } + for phase, lines := range options { + if err := dockeroptions.AddDockerOptionToPhases(app, []string{phase}, ""); err != nil { + t.Fatalf("seed property file: %v", err) + } + // Replace whatever AddDockerOptionToPhases left with the precise + // list the test wants, since it appends rather than overwriting. + key := "_default_." + phase + if err := common.PropertyListWrite("docker-options", app, key, lines); err != nil { + t.Fatalf("PropertyListWrite: %v", err) + } + } +} + +func phaseOptions(t *testing.T, app string, phase string) []string { + t.Helper() + got, err := dockeroptions.GetDockerOptionsForPhase(app, phase) + if err != nil { + t.Fatalf("GetDockerOptionsForPhase %s/%s: %v", app, phase, err) + } + return got +} + +func TestMigrateAppSinglePhase(t *testing.T) { + _, dokkuRoot := setupMigrationEnv(t) + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /var/log:/log", "--restart=on-failure:5"}, + }) + + if err := migrateApp("alpha"); err != nil { + t.Fatalf("migrateApp: %v", err) + } + + entry := LegacyMountToEntry("/var/log:/log") + if !EntryExists(entry.Name) { + t.Fatalf("expected entry %s on disk", entry.Name) + } + loaded, err := LoadEntry(entry.Name) + if err != nil { + t.Fatalf("LoadEntry: %v", err) + } + if loaded.HostPath != "/var/log" { + t.Errorf("entry HostPath = %s, want /var/log", loaded.HostPath) + } + if loaded.Scheduler != SchedulerDockerLocal { + t.Errorf("entry Scheduler = %s, want %s", loaded.Scheduler, SchedulerDockerLocal) + } + + attachments, err := LoadAttachments("alpha") + if err != nil { + t.Fatalf("LoadAttachments: %v", err) + } + if len(attachments) != 1 { + t.Fatalf("expected 1 attachment, got %d", len(attachments)) + } + att := attachments[0] + if att.EntryName != entry.Name || att.ContainerPath != "/log" { + t.Errorf("attachment = %+v, want entry=%s container=/log", att, entry.Name) + } + if len(att.Phases) != 1 || att.Phases[0] != PhaseDeploy { + t.Errorf("attachment phases = %v, want [deploy]", att.Phases) + } + + // docker-options drained the -v line but kept everything else. + deployAfter := phaseOptions(t, "alpha", "deploy") + if !equalSorted(deployAfter, []string{"--restart=on-failure:5"}) { + t.Errorf("deploy options after migration = %v, want [--restart=on-failure:5]", deployAfter) + } + + // Migration flag file is in place. + if _, err := os.Stat(migrationFlagFile("alpha")); err != nil { + t.Errorf("migration flag missing: %v", err) + } +} + +func TestMigrateAppCrossPhaseGroupsIntoOneAttachment(t *testing.T) { + _, dokkuRoot := setupMigrationEnv(t) + // Same -v in both phases groups into one attachment with phases = [deploy, run]. + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /data:/d"}, + "run": {"-v /data:/d"}, + }) + + if err := migrateApp("alpha"); err != nil { + t.Fatalf("migrateApp: %v", err) + } + + attachments, err := LoadAttachments("alpha") + if err != nil { + t.Fatalf("LoadAttachments: %v", err) + } + if len(attachments) != 1 { + t.Fatalf("expected 1 attachment, got %d", len(attachments)) + } + got := attachments[0].Phases + sort.Strings(got) + if !equalSorted(got, []string{PhaseDeploy, PhaseRun}) { + t.Errorf("phases = %v, want [deploy run]", got) + } + + // Drain affected both phases. + if got := phaseOptions(t, "alpha", "deploy"); len(got) != 0 { + t.Errorf("deploy options after migration = %v, want []", got) + } + if got := phaseOptions(t, "alpha", "run"); len(got) != 0 { + t.Errorf("run options after migration = %v, want []", got) + } +} + +func TestMigrateAppPreservesVolumeOptions(t *testing.T) { + _, dokkuRoot := setupMigrationEnv(t) + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /ro:/r:ro", "-v /label:/l:Z"}, + }) + + if err := migrateApp("alpha"); err != nil { + t.Fatalf("migrateApp: %v", err) + } + + atts, err := LoadAttachments("alpha") + if err != nil { + t.Fatalf("LoadAttachments: %v", err) + } + byPath := map[string]*Attachment{} + for _, a := range atts { + byPath[a.ContainerPath] = a + } + + if a := byPath["/r"]; a == nil { + t.Fatalf("missing /r attachment") + } else if !a.Readonly || a.VolumeOptions != "" { + t.Errorf("/r attachment = readonly=%v opts=%q, want readonly=true opts=\"\"", a.Readonly, a.VolumeOptions) + } + + if a := byPath["/l"]; a == nil { + t.Fatalf("missing /l attachment") + } else if a.Readonly || a.VolumeOptions != "Z" { + t.Errorf("/l attachment = readonly=%v opts=%q, want readonly=false opts=\"Z\"", a.Readonly, a.VolumeOptions) + } +} + +func TestMigrateAppIdempotent(t *testing.T) { + _, dokkuRoot := setupMigrationEnv(t) + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /var/log:/log"}, + }) + + if err := migrateApp("alpha"); err != nil { + t.Fatalf("first migrateApp: %v", err) + } + + // Re-stage the same -v line by hand to simulate someone re-adding + // the legacy form via docker-options:add after migration. The + // migration is gated by the per-app flag file at the + // MigrateLegacyMounts level, but migrateApp itself should still be + // safe to call directly: a second run produces no duplicates. + if err := common.PropertyListWrite("docker-options", "alpha", "_default_.deploy", []string{"-v /var/log:/log"}); err != nil { + t.Fatalf("re-stage: %v", err) + } + if err := migrateApp("alpha"); err != nil { + t.Fatalf("second migrateApp: %v", err) + } + + atts, err := LoadAttachments("alpha") + if err != nil { + t.Fatalf("LoadAttachments: %v", err) + } + if len(atts) != 1 { + t.Errorf("expected 1 attachment after rerun, got %d", len(atts)) + } +} + +func TestMigrateLegacyMountsBulkAndFlagFastPath(t *testing.T) { + _, dokkuRoot := setupMigrationEnv(t) + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /alpha-data:/data"}, + }) + stageApp(t, dokkuRoot, "beta", map[string][]string{ + "deploy": {"-v /beta-data:/data"}, + "run": {"-v /beta-data:/data"}, + }) + + if err := MigrateLegacyMounts(); err != nil { + t.Fatalf("MigrateLegacyMounts: %v", err) + } + + for _, app := range []string{"alpha", "beta"} { + if _, err := os.Stat(migrationFlagFile(app)); err != nil { + t.Errorf("flag file missing for %s: %v", app, err) + } + atts, err := LoadAttachments(app) + if err != nil { + t.Fatalf("LoadAttachments %s: %v", app, err) + } + if len(atts) != 1 { + t.Errorf("%s: expected 1 attachment, got %d", app, len(atts)) + } + } + + // Re-stage a -v on alpha and rerun MigrateLegacyMounts. The flag + // file short-circuits per-app migration, so the new line stays in + // docker-options. + if err := common.PropertyListWrite("docker-options", "alpha", "_default_.deploy", []string{"-v /sneaky:/x"}); err != nil { + t.Fatalf("re-stage alpha: %v", err) + } + if err := MigrateLegacyMounts(); err != nil { + t.Fatalf("MigrateLegacyMounts (rerun): %v", err) + } + got := phaseOptions(t, "alpha", "deploy") + if !equalSorted(got, []string{"-v /sneaky:/x"}) { + t.Errorf("expected sneaky -v line untouched after second pass, got %v", got) + } + atts, err := LoadAttachments("alpha") + if err != nil { + t.Fatalf("LoadAttachments alpha rerun: %v", err) + } + if len(atts) != 1 { + t.Errorf("expected 1 attachment after rerun, got %d", len(atts)) + } +} + +func TestMigrateAppRefusesEntryNameCollision(t *testing.T) { + libRoot, dokkuRoot := setupMigrationEnv(t) + _ = libRoot + + // Plant a colliding entry at the legacy- name with conflicting + // fields so migration's "fields match" check trips. + collide := LegacyMountToEntry("/var/log:/log") + collide.HostPath = "/something/else" + if err := SaveEntry(collide); err != nil { + t.Fatalf("seed colliding entry: %v", err) + } + + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /var/log:/log"}, + }) + + if err := migrateApp("alpha"); err == nil { + t.Fatalf("expected migrateApp to error on entry name collision") + } + + // Drain did not happen. + if got := phaseOptions(t, "alpha", "deploy"); len(got) != 1 || got[0] != "-v /var/log:/log" { + t.Errorf("expected legacy line preserved on collision, got %v", got) + } +} + +func equalSorted(a, b []string) bool { + if len(a) != len(b) { + return false + } + ax := append([]string(nil), a...) + bx := append([]string(nil), b...) + sort.Strings(ax) + sort.Strings(bx) + for i := range ax { + if ax[i] != bx[i] { + return false + } + } + return true +} diff --git a/plugins/storage/src/commands/commands.go b/plugins/storage/src/commands/commands.go index 68d4edca3..a026f0709 100644 --- a/plugins/storage/src/commands/commands.go +++ b/plugins/storage/src/commands/commands.go @@ -25,6 +25,7 @@ Additional commands:` storage:info [--format text|json], Show details for one storage entry storage:list [--format text|json], List bind mounts for app's container(s) (host:container) storage:list-entries [--scheduler s] [--format text|json], List registered storage entries + storage:migrate [|--all], Re-run the legacy -v to attachment migration for an app storage:mount , Create a new bind mount storage:report [] [], Displays a storage report for one or more apps storage:set [flags], Update a storage entry in place diff --git a/plugins/storage/src/subcommands/subcommands.go b/plugins/storage/src/subcommands/subcommands.go index 6af83275d..3a5bb8d6b 100644 --- a/plugins/storage/src/subcommands/subcommands.go +++ b/plugins/storage/src/subcommands/subcommands.go @@ -131,6 +131,11 @@ func main() { AsUser: *asUser, Args: cmd, }) + case "migrate": + args := flag.NewFlagSet("storage:migrate", flag.ExitOnError) + all := args.Bool("all", false, "--all: re-migrate every app on this install") + args.Parse(os.Args[2:]) + err = storage.CommandMigrate(args.Arg(0), *all) case "wait": args := flag.NewFlagSet("storage:wait", flag.ExitOnError) args.Parse(os.Args[2:]) diff --git a/plugins/storage/subcommands.go b/plugins/storage/subcommands.go index f8b857bf7..2e92125ad 100644 --- a/plugins/storage/subcommands.go +++ b/plugins/storage/subcommands.go @@ -26,6 +26,7 @@ Additional commands:` storage:info [--format text|json], Show details for one storage entry storage:list [--format text|json], List bind mounts for app's container(s) (host:container) storage:list-entries [--scheduler s] [--format text|json], List registered storage entries + storage:migrate [|--all], Re-run the legacy -v to attachment migration for an app storage:mount , Create a new bind mount storage:report [] [], Displays a storage report for one or more apps storage:set [flags], Update a storage entry in place diff --git a/tests/unit/storage.bats b/tests/unit/storage.bats index ad48240ed..83af1fc59 100644 --- a/tests/unit/storage.bats +++ b/tests/unit/storage.bats @@ -322,6 +322,74 @@ teardown() { assert_success } +@test "(storage) storage:migrate converts legacy docker-options -v lines into attachments" { + # Stage a legacy `-v` line directly via docker-options:add. This + # bypasses the new storage:mount path so the line lives in + # docker-options without a corresponding attachment, simulating an + # app upgraded from a pre-PR Dokku version. + run /bin/bash -c "dokku docker-options:add $TEST_APP deploy,run \"-v /tmp/legacy-mount:/legacy\"" + echo "output: $output" + echo "status: $status" + assert_success + + # Before migration, storage:list shows nothing (attachment-only) and + # docker-options has the -v line. + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '. | length'" + assert_success + assert_output "0" + + run /bin/bash -c "dokku docker-options:report $TEST_APP --docker-options-deploy" + assert_success + assert_output_contains "-v /tmp/legacy-mount:/legacy" + + # Run the migration for this app. + run /bin/bash -c "dokku storage:migrate $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + # storage:list now surfaces the synthesized colon form. + run /bin/bash -c "dokku --quiet storage:list $TEST_APP" + assert_success + assert_output "/tmp/legacy-mount:/legacy" + + # The synthesized entry shows up under the legacy- prefix. + run /bin/bash -c "dokku storage:list-entries --format json | jq -r '.[].name' | grep '^legacy-' | head -1" + echo "output: $output" + echo "status: $status" + assert_success + + # docker-options no longer holds the -v line on either phase. + run /bin/bash -c "dokku docker-options:report $TEST_APP --docker-options-deploy" + assert_success + assert_output_not_contains "-v /tmp/legacy-mount:/legacy" + + run /bin/bash -c "dokku docker-options:report $TEST_APP --docker-options-run" + assert_success + assert_output_not_contains "-v /tmp/legacy-mount:/legacy" + + # Idempotency: re-running storage:migrate is a no-op (still one + # attachment, no errors). + run /bin/bash -c "dokku storage:migrate $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '. | length'" + assert_success + assert_output "1" + + # Cleanup: unmount + destroy the synthesized legacy entry. We need + # the entry name from list-entries since legacy- is content-derived. + legacy_name=$(dokku storage:list-entries --format json | jq -r '.[] | select(.name | startswith("legacy-")) | .name' | head -1) + if [[ -n "$legacy_name" ]]; then + run /bin/bash -c "dokku storage:unmount $TEST_APP $legacy_name --container-dir /legacy" + assert_success + run /bin/bash -c "dokku storage:destroy $legacy_name" + assert_success + fi +} + @test "(storage) storage:ensure-directory emits deprecation warning" { run /bin/bash -c "dokku storage:ensure-directory $TEST_APP 2>&1" echo "output: $output"