From b612285392508918bf99e9e159ff1f56813a3288 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 21 Sep 2026 18:08:42 -0400 Subject: [PATCH] fix: honor storage attachment process types at deploy time An attachment's process type was recorded, reported and used to scope `storage:mount --replace` and `storage:unmount --all`, but nothing read it when the volume was actually mounted, so `storage:mount --container-dir --process-type web` bound the volume into every process rather than only `web`. Both schedulers now scope a mount to the process type it was declared for, and containers that belong to no process type - a `dokku run` one-off, an `app.json` deploy task, a k3s cron job - receive default-scoped mounts only. A container path can now be claimed by a single mount per process type, and `storage:migrate` drains process-scoped `-v` lines out of `docker-options` rather than only the default-scoped ones. --- docs/advanced-usage/persistent-storage.md | 36 +++- docs/deployment/schedulers/k3s.md | 2 + docs/development/plugin-triggers.md | 2 +- plugins/scheduler-k3s/chart.go | 20 +- plugins/scheduler-k3s/storage.go | 47 ++++- plugins/scheduler-k3s/storage_test.go | 115 +++++++++-- plugins/storage/.gitignore | 3 + plugins/storage/Makefile | 2 +- plugins/storage/attachment.go | 74 ++++++- plugins/storage/attachment_test.go | 122 ++++++++++- plugins/storage/migrate.go | 112 +++++++--- plugins/storage/migrate_test.go | 163 +++++++++++++-- plugins/storage/mounts_test.go | 60 ++++++ plugins/storage/report.go | 6 +- plugins/storage/src/triggers/triggers.go | 16 +- plugins/storage/subcommands.go | 32 +-- plugins/storage/triggers.go | 48 ++++- plugins/storage/triggers_test.go | 80 +++++++- tests/unit/storage-1.bats | 10 +- tests/unit/storage-4.bats | 239 ++++++++++++++++++++++ 20 files changed, 1075 insertions(+), 114 deletions(-) create mode 100644 tests/unit/storage-4.bats diff --git a/docs/advanced-usage/persistent-storage.md b/docs/advanced-usage/persistent-storage.md index 3e17ade6b..a3488bc99 100644 --- a/docs/advanced-usage/persistent-storage.md +++ b/docs/advanced-usage/persistent-storage.md @@ -247,7 +247,7 @@ dokku storage:mount node-js-app /var/lib/dokku/data/storage/node-js-app:/app/sto dokku storage:mount node-js-app some-docker-volume:/app/storage ``` -In the first example, Dokku will then mount the shared contents of `/var/lib/dokku/data/storage/node-js-app` to `/app/storage` inside the container. The mount point is *not* relative to your app's working directory, and is instead relative to the root (`/`) of the container. Mounts are only available for containers created via `run` and by the deploy process, and not during the build process. In addition, the host path is never auto-created by either Dokku or Docker, and should be an explicit path, not one relative to the current working directory. +In the first example, Dokku will then mount the shared contents of `/var/lib/dokku/data/storage/node-js-app` to `/app/storage` inside the container. The mount point is *not* relative to your app's working directory, and is instead relative to the root (`/`) of the container. Mounts are only available for containers created via `run` and by the deploy process, and not during the build process. A mount is bound into every process type unless `--process-type` narrows it - see [scoping a mount to a process type](#scoping-a-mount-to-a-process-type). In addition, the host path is never auto-created by either Dokku or Docker, and should be an explicit path, not one relative to the current working directory. > If the `/storage` path within the container had pre-existing content, the container files will be over-written. This may be an issue for users that create assets at build time but then mount a directory at the same place during runtime. Files are not merged. @@ -268,6 +268,40 @@ Once persistent storage is mounted, the app requires a restart. See the [process dokku ps:restart app-name ``` +#### Scoping a mount to a process type + +> [!IMPORTANT] +> New as of 0.38.28 + +By default a mount is bound into every one of an app's containers. `--process-type` narrows it to a single Procfile process type, which is how a worker gets a scratch volume the web process has no business seeing: + +```shell +dokku storage:create node-js-scratch +dokku storage:mount node-js-app node-js-scratch --container-dir /scratch --process-type worker +``` + +The magic `_default_` process type is the unscoped case, and it is what a mount gets when `--process-type` is omitted. Default-scoped mounts are bound into every process; a mount scoped to a named process type is bound only into that one. + +Some containers belong to no process type at all - a `dokku run` one-off, the ephemeral containers that run `app.json` deploy tasks, and k3s cron jobs. Those receive default-scoped mounts only, since there is no process type for a named scope to match. On k3s this applies to the deploy phase; `--phase run` attachments are not mounted there at all. + +A container path can be claimed by only one mount per process type, so `storage:mount` rejects a container path that an overlapping scope already holds. The default scope overlaps every named one: + +```shell +dokku storage:create node-js-data +dokku storage:create node-js-cache + +dokku storage:mount node-js-app node-js-data --container-dir /app/storage +# fails: the default-scoped mount above also applies to web +dokku storage:mount node-js-app node-js-cache --container-dir /app/storage --process-type web +``` + +Two *named* scopes never apply to the same container, so they may each bind a different entry at the same container path: + +```shell +dokku storage:mount node-js-app node-js-data --container-dir /app/storage --process-type web +dokku storage:mount node-js-app node-js-cache --container-dir /app/storage --process-type worker +``` + #### Replacing the entire mount set > [!IMPORTANT] diff --git a/docs/deployment/schedulers/k3s.md b/docs/deployment/schedulers/k3s.md index ce7802a67..b9aae61a0 100644 --- a/docs/deployment/schedulers/k3s.md +++ b/docs/deployment/schedulers/k3s.md @@ -1158,6 +1158,8 @@ git push dokku master For a hostPath-backed PV (no StorageClass), pass `` as the second positional argument and omit `--storage-class-name`. The plugin renders both the PV and the PVC into the entry's helm release. The `--reclaim-policy` flag (`Retain` or `Delete`) controls whether the underlying PV survives `storage:destroy`. Annotations and labels set via `storage:annotations:set` and `storage:labels:set` propagate to both the PVC and the PV so backup tools (Velero, Longhorn snapshots) can find them. +`storage:mount --process-type ` scopes a volume to a single process type, so only that deployment's pods mount the PVC; a mount left in the default `_default_` scope is mounted by every deployment. Cron jobs are keyed by a generated cron ID rather than a Procfile process type, so they mount default-scoped volumes only. Only deploy-phase attachments are mounted on k3s - `--phase run` has no effect here. + The legacy `storage:mount :` colon form is rejected on k3s apps; create a named entry instead. See [Persistent Storage](/docs/advanced-usage/persistent-storage.md) for the full command reference. ### Chart upgrade callbacks diff --git a/docs/development/plugin-triggers.md b/docs/development/plugin-triggers.md index 5be733618..76e2f1543 100644 --- a/docs/development/plugin-triggers.md +++ b/docs/development/plugin-triggers.md @@ -875,7 +875,7 @@ APP="$1"; IMAGE_SOURCE_TYPE="$2" ### `docker-args-process-deploy` -- Description: emits docker arguments scoped to a specific Procfile process type. The `docker-options` plugin implements this trigger to surface options registered via `docker-options:add --process `. `$PROC_TYPE` may be empty (or set to the magic `_default_` value) to signify default-scope docker deploy options. +- Description: emits docker arguments scoped to a specific Procfile process type. The `docker-options` plugin implements this trigger to surface options registered via `docker-options:add --process `, and the `storage` plugin implements it to emit the `-v` flags for mounts made with `storage:mount --process-type `. `$PROC_TYPE` may be empty (or set to the magic `_default_` value) to signify default-scope docker deploy options. Note that the two plugins split the work differently: `docker-options` emits only its process-specific scope here and its default scope through `docker-args-deploy`, while `storage` emits both scopes here and does not implement `docker-args-deploy` at all. - Invoked by: `dokku deploy` - Arguments: `$APP $IMAGE_SOURCE_TYPE $IMAGE_TAG [$PROC_TYPE $CONTAINER_INDEX]` - Example: diff --git a/plugins/scheduler-k3s/chart.go b/plugins/scheduler-k3s/chart.go index 414f46265..b254b57d3 100644 --- a/plugins/scheduler-k3s/chart.go +++ b/plugins/scheduler-k3s/chart.go @@ -16,6 +16,7 @@ import ( "github.com/dokku/dokku/plugins/config" "github.com/dokku/dokku/plugins/cron" "github.com/dokku/dokku/plugins/registry" + "github.com/dokku/dokku/plugins/storage" "github.com/kballard/go-shellquote" ) @@ -335,9 +336,12 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti } } - processVolumes := []ProcessVolume{} + // Volumes every workload in the app receives, whichever process type it + // belongs to. Storage attachments are layered on top per process type + // inside the loops below. + baseVolumes := []ProcessVolume{} if shmSize := getComputedShmSize(appName); shmSize != "" { - processVolumes = append(processVolumes, ProcessVolume{ + baseVolumes = append(baseVolumes, ProcessVolume{ Name: "shmem", MountPath: "/dev/shm", EmptyDir: &ProcessVolumeEmptyDir{ @@ -351,11 +355,9 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti if err != nil { return cleanup(fmt.Errorf("error loading storage app mounts: %w", err)) } - deployVolumes, err := ToProcessVolumes(deployMountPairs) - if err != nil { + if err := ValidateMountPairs(deployMountPairs); err != nil { return cleanup(err) } - processVolumes = append(processVolumes, deployVolumes...) for processType, processCount := range processes { healthchecks, ok := appJSON.Healthchecks[processType] @@ -411,7 +413,7 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti ProcessType: ProcessType_Worker, Replicas: int32(processCount), Resources: processResources, - Volumes: processVolumes, + Volumes: processVolumesFor(baseVolumes, deployMountPairs, processType), } if processType == "web" { @@ -589,7 +591,11 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti ProcessType: ProcessType_Cron, Replicas: 1, Resources: processResources, - Volumes: processVolumes, + // A cron ID is a hash of the app name, command and schedule, so it + // is not something an operator can name in --process-type and it + // churns whenever the task changes. Cron jobs therefore sit with + // `dokku run` and app.json tasks: default-scoped mounts only. + Volumes: processVolumesFor(baseVolumes, deployMountPairs, storage.DefaultProcessType), } values.Processes[cronTask.ID] = processValues } diff --git a/plugins/scheduler-k3s/storage.go b/plugins/scheduler-k3s/storage.go index fc7b39969..61f08f447 100644 --- a/plugins/scheduler-k3s/storage.go +++ b/plugins/scheduler-k3s/storage.go @@ -275,18 +275,37 @@ func LoadAppMounts(appName string, phase string) ([]AppMountPair, error) { return pairs, nil } -// ToProcessVolumes converts each AppMountPair into a ProcessVolume. K3s -// app deployments reference the PVC by name; the PVC itself is owned by -// the storage entry's separate helm release. Any docker-local entries -// found here are an error - they cannot be mounted on a k3s app. -func ToProcessVolumes(pairs []AppMountPair) ([]ProcessVolume, error) { - volumes := []ProcessVolume{} +// ValidateMountPairs rejects any pair whose entry belongs to another +// scheduler. Kept separate from ToProcessVolumes so the chart builder can run +// it once over the whole set: folding it into the per-process conversion would +// make it reachable only for apps that have at least one scaled process or +// cron task, and an app scaled to zero would stop reporting the mismatch. +func ValidateMountPairs(pairs []AppMountPair) error { for _, pair := range pairs { if pair.Entry == nil || pair.Attachment == nil { continue } if pair.Entry.Scheduler != storage.SchedulerK3s { - return nil, fmt.Errorf("storage entry %q is scheduler=%s but is mounted on a k3s app; recreate it with --scheduler k3s", pair.Entry.Name, pair.Entry.Scheduler) + return fmt.Errorf("storage entry %q is scheduler=%s but is mounted on a k3s app; recreate it with --scheduler k3s", pair.Entry.Name, pair.Entry.Scheduler) + } + } + return nil +} + +// ToProcessVolumes converts the AppMountPairs that apply to processType into +// ProcessVolumes. K3s app deployments reference the PVC by name; the PVC +// itself is owned by the storage entry's separate helm release. +// +// An empty processType names a workload that belongs to no process type - a +// cron job - and so receives default-scoped attachments only. +func ToProcessVolumes(pairs []AppMountPair, processType string) []ProcessVolume { + volumes := []ProcessVolume{} + for _, pair := range pairs { + if pair.Entry == nil || pair.Attachment == nil { + continue + } + if !pair.Attachment.AppliesToProcessType(processType) { + continue } volumes = append(volumes, ProcessVolume{ Name: pair.Entry.Name, @@ -298,7 +317,19 @@ func ToProcessVolumes(pairs []AppMountPair) ([]ProcessVolume, error) { }, }) } - return volumes, nil + return volumes +} + +// processVolumesFor returns the volumes one process type's pods receive: the +// volumes every workload in the app gets, plus the storage attachments scoped +// to that process type. The slice is freshly allocated rather than appended +// onto base, so one workload's volumes can never alias another's. +func processVolumesFor(base []ProcessVolume, pairs []AppMountPair, processType string) []ProcessVolume { + scoped := ToProcessVolumes(pairs, processType) + volumes := make([]ProcessVolume, 0, len(base)+len(scoped)) + volumes = append(volumes, base...) + volumes = append(volumes, scoped...) + return volumes } // asK8sVolumeMount is a small helper used by tests / other callers that diff --git a/plugins/scheduler-k3s/storage_test.go b/plugins/scheduler-k3s/storage_test.go index aa2bb5d4f..a43d607eb 100644 --- a/plugins/scheduler-k3s/storage_test.go +++ b/plugins/scheduler-k3s/storage_test.go @@ -6,7 +6,7 @@ import ( "github.com/dokku/dokku/plugins/storage" ) -func TestToProcessVolumesRejectsDockerLocalEntry(t *testing.T) { +func TestValidateMountPairsRejectsDockerLocalEntry(t *testing.T) { pairs := []AppMountPair{ { Entry: &storage.Entry{ @@ -19,7 +19,7 @@ func TestToProcessVolumesRejectsDockerLocalEntry(t *testing.T) { }, } - _, err := ToProcessVolumes(pairs) + err := ValidateMountPairs(pairs) if err == nil { t.Fatal("expected error for docker-local entry, got nil") } @@ -29,7 +29,7 @@ func TestToProcessVolumesRejectsDockerLocalEntry(t *testing.T) { } } -func TestToProcessVolumesRejectsNonK3sEntry(t *testing.T) { +func TestValidateMountPairsRejectsNonK3sEntry(t *testing.T) { pairs := []AppMountPair{ { Entry: &storage.Entry{ @@ -42,7 +42,7 @@ func TestToProcessVolumesRejectsNonK3sEntry(t *testing.T) { }, } - _, err := ToProcessVolumes(pairs) + err := ValidateMountPairs(pairs) if err == nil { t.Fatal("expected error for non-k3s entry, got nil") } @@ -52,6 +52,28 @@ func TestToProcessVolumesRejectsNonK3sEntry(t *testing.T) { } } +// TestValidateMountPairsRejectsOutOfScopeEntry pins the reason validation is a +// separate pass: a mismatched entry scoped to a process type the caller is not +// currently building still has to fail the deploy. +func TestValidateMountPairsRejectsOutOfScopeEntry(t *testing.T) { + pairs := []AppMountPair{ + { + Entry: &storage.Entry{ + Name: "demo-data", + Scheduler: storage.SchedulerDockerLocal, + }, + Attachment: &storage.Attachment{ + ContainerPath: "/data", + ProcessType: "worker", + }, + }, + } + + if err := ValidateMountPairs(pairs); err == nil { + t.Fatal("expected error for docker-local entry scoped to another process, got nil") + } +} + func TestToProcessVolumesAcceptsK3sEntry(t *testing.T) { pairs := []AppMountPair{ { @@ -67,10 +89,7 @@ func TestToProcessVolumesAcceptsK3sEntry(t *testing.T) { }, } - volumes, err := ToProcessVolumes(pairs) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + volumes := ToProcessVolumes(pairs, "web") if len(volumes) != 1 { t.Fatalf("expected 1 volume, got %d", len(volumes)) } @@ -99,11 +118,81 @@ func TestToProcessVolumesSkipsNilPairs(t *testing.T) { {Entry: nil, Attachment: &storage.Attachment{ContainerPath: "/data"}}, } - volumes, err := ToProcessVolumes(pairs) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if len(volumes) != 0 { + if volumes := ToProcessVolumes(pairs, "web"); len(volumes) != 0 { t.Fatalf("expected 0 volumes, got %d", len(volumes)) } } + +// TestToProcessVolumesScopesToProcessType is the k3s half of #9059: a mount +// scoped to a named process type reaches that process and no other, while a +// default-scoped mount reaches all of them. +func TestToProcessVolumesScopesToProcessType(t *testing.T) { + pairs := []AppMountPair{ + { + Entry: &storage.Entry{Name: "shared", Scheduler: storage.SchedulerK3s}, + Attachment: &storage.Attachment{ContainerPath: "/shared", ProcessType: storage.DefaultProcessType}, + }, + { + Entry: &storage.Entry{Name: "web-only", Scheduler: storage.SchedulerK3s}, + Attachment: &storage.Attachment{ContainerPath: "/web", ProcessType: "web"}, + }, + { + Entry: &storage.Entry{Name: "legacy", Scheduler: storage.SchedulerK3s}, + Attachment: &storage.Attachment{ContainerPath: "/legacy"}, + }, + } + + for _, tc := range []struct { + processType string + expected []string + }{ + {processType: "web", expected: []string{"shared", "web-only", "legacy"}}, + {processType: "worker", expected: []string{"shared", "legacy"}}, + {processType: "", expected: []string{"shared", "legacy"}}, + } { + volumes := ToProcessVolumes(pairs, tc.processType) + names := []string{} + for _, volume := range volumes { + names = append(names, volume.Name) + } + if len(names) != len(tc.expected) { + t.Fatalf("process type %q: expected %v, got %v", tc.processType, tc.expected, names) + } + for i, name := range tc.expected { + if names[i] != name { + t.Fatalf("process type %q: expected %v, got %v", tc.processType, tc.expected, names) + } + } + } +} + +// TestProcessVolumesForDoesNotAliasBase pins that each process type gets its +// own slice: appending onto a shared backing array would leak one process's +// storage mounts into another's pod spec. +func TestProcessVolumesForDoesNotAliasBase(t *testing.T) { + base := make([]ProcessVolume, 0, 8) + base = append(base, ProcessVolume{Name: "shmem", MountPath: "/dev/shm"}) + + pairs := []AppMountPair{ + { + Entry: &storage.Entry{Name: "web-only", Scheduler: storage.SchedulerK3s}, + Attachment: &storage.Attachment{ContainerPath: "/web", ProcessType: "web"}, + }, + } + + web := processVolumesFor(base, pairs, "web") + worker := processVolumesFor(base, pairs, "worker") + + if len(web) != 2 { + t.Fatalf("expected web to get 2 volumes, got %d", len(web)) + } + if len(worker) != 1 { + t.Fatalf("expected worker to get 1 volume, got %d", len(worker)) + } + if worker[0].Name != "shmem" { + t.Fatalf("expected worker to keep only the base volume, got %s", worker[0].Name) + } + if len(base) != 1 { + t.Fatalf("expected base to be left untouched, got %d volumes", len(base)) + } +} diff --git a/plugins/storage/.gitignore b/plugins/storage/.gitignore index d3939e0d4..dce8215ae 100644 --- a/plugins/storage/.gitignore +++ b/plugins/storage/.gitignore @@ -1,4 +1,7 @@ /commands +/docker-args-deploy +/docker-args-process-deploy +/docker-args-run /subcommands/* /triggers/* /triggers diff --git a/plugins/storage/Makefile b/plugins/storage/Makefile index b430b0882..1b2e6b558 100644 --- a/plugins/storage/Makefile +++ b/plugins/storage/Makefile @@ -1,6 +1,6 @@ GOARCH ?= amd64 SUBCOMMANDS = subcommands/default subcommands/annotations:set subcommands/annotations:report subcommands/create subcommands/destroy subcommands/ensure-directory subcommands/exec subcommands/info subcommands/labels:set subcommands/labels:report 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 +TRIGGERS = triggers/install triggers/storage-list triggers/storage-app-mounts triggers/docker-args-process-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/attachment.go b/plugins/storage/attachment.go index 510428885..3fde0f01d 100644 --- a/plugins/storage/attachment.go +++ b/plugins/storage/attachment.go @@ -66,6 +66,38 @@ func (a *Attachment) Validate() error { return nil } +// EffectiveProcessType returns the process type an attachment is scoped to, +// treating an unset value as the default scope so attachments written before +// the field existed still sort into a scope. +func (a *Attachment) EffectiveProcessType() string { + if a == nil || a.ProcessType == "" { + return DefaultProcessType + } + + return a.ProcessType +} + +// AppliesToProcessType reports whether an attachment is mounted into the +// container being built for the given process type. The default scope applies +// to every process; a named scope applies only to itself. An empty processType +// names a container that belongs to no process at all - a `dokku run` one-off, +// an app.json deploy task, a k3s cron job - and so sees the default scope only. +func (a *Attachment) AppliesToProcessType(processType string) bool { + if processType == "" { + processType = DefaultProcessType + } + + scope := a.EffectiveProcessType() + return scope == DefaultProcessType || scope == processType +} + +// scopesOverlap reports whether two process-type scopes can both apply to one +// container. The default scope overlaps every named scope; two named scopes +// overlap only when they are equal. +func scopesOverlap(first string, second string) bool { + return first == DefaultProcessType || second == DefaultProcessType || first == second +} + // LoadAttachments returns every attachment registered against an app. func LoadAttachments(appName string) ([]*Attachment, error) { lines, err := common.PropertyListGet(PluginName, appName, AttachmentsProperty) @@ -101,8 +133,29 @@ func SaveAttachments(appName string, attachments []*Attachment) error { return common.PropertyListWrite(PluginName, appName, AttachmentsProperty, lines) } +// ensureContainerPathFree rejects a candidate whose container path is already +// claimed by an attachment in an overlapping scope. Two attachments bound at +// one container path in overlapping scopes emit two -v flags for the same +// target, leaving which volume the process actually sees up to Docker. +func ensureContainerPathFree(appName string, attachments []*Attachment, candidate *Attachment) error { + for _, existing := range attachments { + if existing.ContainerPath != candidate.ContainerPath { + continue + } + if !scopesOverlap(existing.EffectiveProcessType(), candidate.EffectiveProcessType()) { + continue + } + + return fmt.Errorf("Container path %s on app %s is already mounted by storage entry %s for process type %s", + candidate.ContainerPath, appName, existing.EntryName, existing.EffectiveProcessType()) + } + + return nil +} + // AddAttachment appends an attachment, rejecting duplicates of the same -// (entry_name, container_path, process_type) tuple. +// (entry_name, container_path, process_type) tuple as well as container paths +// already claimed by an attachment in an overlapping scope. func AddAttachment(appName string, attachment *Attachment) error { if err := attachment.Validate(); err != nil { return err @@ -116,12 +169,16 @@ func AddAttachment(appName string, attachment *Attachment) error { for _, existing := range attachments { if existing.EntryName == attachment.EntryName && existing.ContainerPath == attachment.ContainerPath && - existing.ProcessType == attachment.ProcessType { + existing.EffectiveProcessType() == attachment.EffectiveProcessType() { return fmt.Errorf("storage entry %q is already mounted at %q for process type %q on app %q", - attachment.EntryName, attachment.ContainerPath, attachment.ProcessType, appName) + attachment.EntryName, attachment.ContainerPath, attachment.EffectiveProcessType(), appName) } } + if err := ensureContainerPathFree(appName, attachments, attachment); err != nil { + return err + } + attachments = append(attachments, attachment) return SaveAttachments(appName, attachments) } @@ -136,6 +193,9 @@ func AddAttachment(appName string, attachment *Attachment) error { // false Readonly clears any previously-set value, mirroring the operator's // "set the flags as I just typed them" intent and the same idempotent // contract storage:create offers for entries. +// +// A new attachment is rejected when its container path is already claimed by +// an attachment in an overlapping scope. func UpsertAttachment(appName string, attachment *Attachment) (bool, error) { if err := attachment.Validate(); err != nil { return false, err @@ -149,7 +209,7 @@ func UpsertAttachment(appName string, attachment *Attachment) (bool, error) { for _, existing := range attachments { if existing.EntryName == attachment.EntryName && existing.ContainerPath == attachment.ContainerPath && - existing.ProcessType == attachment.ProcessType { + existing.EffectiveProcessType() == attachment.EffectiveProcessType() { existing.Phases = attachment.Phases existing.Subpath = attachment.Subpath existing.Readonly = attachment.Readonly @@ -159,6 +219,12 @@ func UpsertAttachment(appName string, attachment *Attachment) (bool, error) { } } + // Only the append branch needs the overlap check: rewriting an existing + // tuple in place would otherwise trip over the attachment it is updating. + if err := ensureContainerPathFree(appName, attachments, attachment); err != nil { + return false, err + } + attachments = append(attachments, attachment) return true, SaveAttachments(appName, attachments) } diff --git a/plugins/storage/attachment_test.go b/plugins/storage/attachment_test.go index d43f7ec53..5f7906937 100644 --- a/plugins/storage/attachment_test.go +++ b/plugins/storage/attachment_test.go @@ -302,19 +302,135 @@ func TestUpsertAttachmentDistinctIdentity(t *testing.T) { Expect(err).NotTo(HaveOccurred()) Expect(created).To(BeTrue()) - // Different process type on the same (entry, container_path) is also distinct. + // Two named scopes never apply to the same container, so they can each + // claim one container path. created, err = UpsertAttachment("demo", &Attachment{ EntryName: "demo-data", - ContainerPath: "/data", + ContainerPath: "/scoped", Phases: []string{PhaseDeploy}, ProcessType: "web", }) Expect(err).NotTo(HaveOccurred()) Expect(created).To(BeTrue()) + created, err = UpsertAttachment("demo", &Attachment{ + EntryName: "demo-data", + ContainerPath: "/scoped", + Phases: []string{PhaseDeploy}, + ProcessType: "worker", + }) + Expect(err).NotTo(HaveOccurred()) + Expect(created).To(BeTrue()) + attachments, err := LoadAttachments("demo") Expect(err).NotTo(HaveOccurred()) - Expect(attachments).To(HaveLen(3)) + Expect(attachments).To(HaveLen(4)) +} + +// TestUpsertAttachmentRejectsOverlappingContainerPath covers the conflict the +// process-type scoping makes reachable: the default scope also applies to +// "web", so both attachments would emit a -v flag for /data on the same +// container and Docker would decide which one wins. +func TestUpsertAttachmentRejectsOverlappingContainerPath(t *testing.T) { + RegisterTestingT(t) + withTempLibRoot(t) + + created, err := UpsertAttachment("demo", &Attachment{ + EntryName: "demo-data", + ContainerPath: "/data", + Phases: []string{PhaseDeploy}, + ProcessType: DefaultProcessType, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(created).To(BeTrue()) + + _, err = UpsertAttachment("demo", &Attachment{ + EntryName: "demo-cache", + ContainerPath: "/data", + Phases: []string{PhaseDeploy}, + ProcessType: "web", + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Container path /data on app demo is already mounted by storage entry demo-data for process type _default_")) + + // The rejected write leaves the stored set exactly as it was. + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + + // The same collision the other way round: a default-scoped mount cannot + // claim a path a named scope already holds. + _, err = UpsertAttachment("demo", &Attachment{ + EntryName: "demo-cache", + ContainerPath: "/cache", + Phases: []string{PhaseDeploy}, + ProcessType: "worker", + }) + Expect(err).NotTo(HaveOccurred()) + + _, err = UpsertAttachment("demo", &Attachment{ + EntryName: "demo-logs", + ContainerPath: "/cache", + Phases: []string{PhaseDeploy}, + ProcessType: DefaultProcessType, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("for process type worker")) +} + +// TestAttachmentAppliesToProcessType pins the scoping rule every deploy-time +// caller shares: the default scope reaches every process, a named scope reaches +// only itself, and a container with no process type of its own - a `dokku run` +// one-off, an app.json deploy task, a k3s cron job - sees the default scope. +func TestAttachmentAppliesToProcessType(t *testing.T) { + RegisterTestingT(t) + + defaultScoped := &Attachment{ProcessType: DefaultProcessType} + legacyScoped := &Attachment{} + webScoped := &Attachment{ProcessType: "web"} + + Expect(defaultScoped.EffectiveProcessType()).To(Equal(DefaultProcessType)) + Expect(legacyScoped.EffectiveProcessType()).To(Equal(DefaultProcessType)) + Expect(webScoped.EffectiveProcessType()).To(Equal("web")) + + for _, processType := range []string{"web", "worker", "", DefaultProcessType} { + Expect(defaultScoped.AppliesToProcessType(processType)).To(BeTrue(), "default scope, process type %q", processType) + Expect(legacyScoped.AppliesToProcessType(processType)).To(BeTrue(), "legacy scope, process type %q", processType) + } + + Expect(webScoped.AppliesToProcessType("web")).To(BeTrue()) + Expect(webScoped.AppliesToProcessType("worker")).To(BeFalse()) + Expect(webScoped.AppliesToProcessType("")).To(BeFalse()) + Expect(webScoped.AppliesToProcessType(DefaultProcessType)).To(BeFalse()) +} + +// TestAddAttachmentTreatsLegacyScopeAsDefault covers attachments written before +// the process-type field existed: an empty value and an explicit _default_ name +// the same scope, so the pair is a duplicate rather than two mounts. +func TestAddAttachmentTreatsLegacyScopeAsDefault(t *testing.T) { + RegisterTestingT(t) + root := withTempLibRoot(t) + + writeAttachmentsFile(t, root, "demo", []*Attachment{ + { + EntryName: "demo-data", + ContainerPath: "/data", + Phases: []string{PhaseDeploy}, + }, + }) + + err := AddAttachment("demo", &Attachment{ + EntryName: "demo-data", + ContainerPath: "/data", + Phases: []string{PhaseDeploy}, + ProcessType: DefaultProcessType, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("is already mounted at")) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) } func TestUpsertAttachmentValidationFailureLeavesStoreUntouched(t *testing.T) { diff --git a/plugins/storage/migrate.go b/plugins/storage/migrate.go index 20c31909e..b72d557b7 100644 --- a/plugins/storage/migrate.go +++ b/plugins/storage/migrate.go @@ -19,6 +19,12 @@ import ( // filesystem flag file at `data/storage-registry/migrations/`. const MigratedProperty = "legacy-mounts-migrated" +// MigratedPropertyVersion is the value MigratedProperty carries once an app +// has been through the current migration. Earlier releases wrote "true" and +// drained only the default process-type scope, so an app still holding that +// value is rescanned once to pick up its process-scoped `-v` lines. +const MigratedPropertyVersion = "2" + // migrationFlagDir / migrationFlagFile remain for one release cycle so // the upgrade-cycle helper convertLegacyMigrationFlag can drain any // leftover flag files into the new property. @@ -43,7 +49,7 @@ func MigrateApp(appName string) error { if err := convertLegacyMigrationFlag(appName); err != nil { return err } - if err := migrateApp(appName); err != nil { + if err := migrateApp(appName, true); err != nil { return fmt.Errorf("storage migration failed for app %q: %w", appName, err) } return nil @@ -66,10 +72,7 @@ func MigrateLegacyMounts() error { if err := convertLegacyMigrationFlag(app); err != nil { return err } - if common.PropertyExists(PluginName, app, MigratedProperty) { - continue - } - if err := migrateApp(app); err != nil { + if err := migrateApp(app, false); err != nil { return fmt.Errorf("storage migration failed for app %q: %w", app, err) } } @@ -97,18 +100,57 @@ func convertLegacyMigrationFlag(appName string) error { return os.Remove(path) } -// migrateApp performs the per-app migration. Order is intentional: write -// the entry, write the attachment, drain the docker-options line. Any -// step failing aborts before drain, so the original behavior is -// preserved on partial failure. -func migrateApp(appName string) error { - deployLines, err := dockeroptions.GetDockerOptionsForPhase(appName, PhaseDeploy) +// migrateApp performs the per-app migration across every process-type scope +// docker-options holds options for. The default scope is migrated first: the +// per-scope drain skips a named-scope mount that the default scope already +// covers, which only works once the default scope's attachments exist. +// +// force bypasses the version gate, which is what `dokku storage:migrate ` +// wants - it exists so an operator can rescan an app whose docker-options +// state changed after the bulk install-time pass. +func migrateApp(appName string, force bool) error { + migrated := common.PropertyGet(PluginName, appName, MigratedProperty) + if !force && migrated == MigratedPropertyVersion { + return nil + } + + processTypes := []string{DefaultProcessType} + named, err := dockeroptions.ListProcessTypesWithOptions(appName) if err != nil { return err } - runLines, err := dockeroptions.GetDockerOptionsForPhase(appName, PhaseRun) + processTypes = append(processTypes, named...) + + drained := false + for _, processType := range processTypes { + scopeDrained, err := migrateProcessScope(appName, processType) + if err != nil { + return err + } + drained = drained || scopeDrained + } + + // An app that never had legacy state keeps no marker at all, so + // `storage:report` (and future tooling) can still tell it apart from one + // that did and was drained. An app already carrying an older marker keeps + // one, advanced to the current version so it stops being rescanned. + if !drained && migrated == "" { + return nil + } + + return common.PropertyWrite(PluginName, appName, MigratedProperty, MigratedPropertyVersion) +} + +// migrateProcessScope drains one process-type scope's legacy `-v` lines into +// attachments. Reports whether anything was drained. +func migrateProcessScope(appName string, processType string) (bool, error) { + deployLines, err := dockeroptions.GetDockerOptionsForProcessPhase(appName, processType, PhaseDeploy) if err != nil { - return err + return false, err + } + runLines, err := dockeroptions.GetDockerOptionsForProcessPhase(appName, processType, PhaseRun) + if err != nil { + return false, err } deployMounts := filterMountLines(deployLines) @@ -130,22 +172,19 @@ func migrateApp(appName string) error { sort.Strings(mounts) if len(mounts) == 0 { - // No legacy `-v` lines for this app. Leave the marker unset so - // `storage:report` (and future tooling) distinguishes apps that - // never had legacy state from apps that did and were drained. - return nil + return false, nil } for _, mount := range mounts { - if err := migrateMount(appName, mount, phaseMap[mount]); err != nil { - return err + if err := migrateMount(appName, processType, mount, phaseMap[mount]); err != nil { + return false, err } } - return common.PropertyWrite(PluginName, appName, MigratedProperty, "true") + return true, nil } -func migrateMount(appName string, mount string, phases []string) error { +func migrateMount(appName string, processType string, mount string, phases []string) error { parsed := ParseMountPath(mount) entry := LegacyMountToEntry(mount) @@ -173,7 +212,7 @@ func migrateMount(appName string, mount string, phases []string) error { EntryName: entry.Name, ContainerPath: containerPath, Phases: phases, - ProcessType: DefaultProcessType, + ProcessType: processType, Readonly: parsed.Readonly, VolumeOptions: parsed.VolumeOptions, } @@ -182,7 +221,11 @@ func migrateMount(appName string, mount string, phases []string) error { if err != nil { return err } - if !attachmentExists(existing, attachment) { + // A named scope whose mount the default scope already covers needs no + // attachment of its own - the default one reaches that process anyway, and + // recording both would manufacture two -v flags for one container path. + // The docker-options line is still drained so it stops being emitted twice. + if !attachmentExists(existing, attachment) && !coveredByDefaultScope(existing, attachment) { existing = append(existing, attachment) if err := SaveAttachments(appName, existing); err != nil { return err @@ -193,12 +236,31 @@ func migrateMount(appName string, mount string, phases []string) error { for _, phase := range phases { mountedPhases = append(mountedPhases, phase) } - if err := dockeroptions.RemoveDockerOptionFromPhases(appName, mountedPhases, fmt.Sprintf("-v %s", mount)); err != nil { + if err := dockeroptions.RemoveDockerOptionFromProcessPhases(appName, []string{processType}, mountedPhases, fmt.Sprintf("-v %s", mount)); err != nil { return err } return nil } +// coveredByDefaultScope reports whether a named-scope candidate binds the same +// entry at the same container path as an attachment already in the default +// scope. Always false for a default-scope candidate, which is its own cover. +func coveredByDefaultScope(attachments []*Attachment, candidate *Attachment) bool { + if candidate.EffectiveProcessType() == DefaultProcessType { + return false + } + + for _, existing := range attachments { + if existing.EffectiveProcessType() != DefaultProcessType { + continue + } + if existing.EntryName == candidate.EntryName && existing.ContainerPath == candidate.ContainerPath { + return true + } + } + return false +} + func filterMountLines(lines []string) []string { out := []string{} for _, line := range lines { @@ -222,7 +284,7 @@ func attachmentExists(attachments []*Attachment, candidate *Attachment) bool { for _, existing := range attachments { if existing.EntryName == candidate.EntryName && existing.ContainerPath == candidate.ContainerPath && - existing.ProcessType == candidate.ProcessType { + existing.EffectiveProcessType() == candidate.EffectiveProcessType() { return true } } diff --git a/plugins/storage/migrate_test.go b/plugins/storage/migrate_test.go index 484615f64..a25a36bf0 100644 --- a/plugins/storage/migrate_test.go +++ b/plugins/storage/migrate_test.go @@ -72,7 +72,7 @@ func TestMigrateAppSinglePhase(t *testing.T) { "deploy": {"-v /var/log:/log", "--restart=on-failure:5"}, }) - if err := migrateApp("alpha"); err != nil { + if err := migrateApp("alpha", false); err != nil { t.Fatalf("migrateApp: %v", err) } @@ -126,7 +126,7 @@ func TestMigrateAppCrossPhaseGroupsIntoOneAttachment(t *testing.T) { "run": {"-v /data:/d"}, }) - if err := migrateApp("alpha"); err != nil { + if err := migrateApp("alpha", false); err != nil { t.Fatalf("migrateApp: %v", err) } @@ -158,7 +158,7 @@ func TestMigrateAppPreservesVolumeOptions(t *testing.T) { "deploy": {"-v /ro:/r:ro", "-v /label:/l:Z"}, }) - if err := migrateApp("alpha"); err != nil { + if err := migrateApp("alpha", false); err != nil { t.Fatalf("migrateApp: %v", err) } @@ -190,7 +190,7 @@ func TestMigrateAppIdempotent(t *testing.T) { "deploy": {"-v /var/log:/log"}, }) - if err := migrateApp("alpha"); err != nil { + if err := migrateApp("alpha", false); err != nil { t.Fatalf("first migrateApp: %v", err) } @@ -202,7 +202,7 @@ func TestMigrateAppIdempotent(t *testing.T) { 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 { + if err := migrateApp("alpha", false); err != nil { t.Fatalf("second migrateApp: %v", err) } @@ -280,7 +280,7 @@ func TestMigrateAppRefusesEntryNameCollision(t *testing.T) { "deploy": {"-v /var/log:/log"}, }) - if err := migrateApp("alpha"); err == nil { + if err := migrateApp("alpha", false); err == nil { t.Fatalf("expected migrateApp to error on entry name collision") } @@ -301,7 +301,7 @@ func TestMigrateAppChownsLegacyEntryFiles(t *testing.T) { "deploy": {"-v /var/log:/log"}, }) - if err := migrateApp("alpha"); err != nil { + if err := migrateApp("alpha", false); err != nil { t.Fatalf("migrateApp: %v", err) } @@ -334,7 +334,7 @@ func TestMigrateAppNoMountsDoesNotMarkMigrated(t *testing.T) { "deploy": {"--restart=on-failure:5"}, }) - if err := migrateApp("alpha"); err != nil { + if err := migrateApp("alpha", false); err != nil { t.Fatalf("migrateApp: %v", err) } @@ -370,25 +370,28 @@ func TestMigrateLegacyMountsConvertsLegacyFlagFile(t *testing.T) { t.Fatalf("MigrateLegacyMounts: %v", err) } + // The flag file converts to the old "true" value, which the version gate + // then treats as stale and rescans, leaving the marker at the current + // version rather than at the value the flag file carried. got := common.PropertyGetDefault(PluginName, "alpha", MigratedProperty, "") - if got != "true" { - t.Errorf("legacy-mounts-migrated = %q, want %q", got, "true") + if got != MigratedPropertyVersion { + t.Errorf("legacy-mounts-migrated = %q, want %q", got, MigratedPropertyVersion) } if _, err := os.Stat(flagPath); !os.IsNotExist(err) { t.Errorf("expected flag file gone, got err=%v", err) } } -// TestMigrateLegacyMountsRespectsExistingProperty confirms that the -// property is the gate: an app with a `-v` line is skipped when the -// property is already set, leaving the legacy line in docker-options. -func TestMigrateLegacyMountsRespectsExistingProperty(t *testing.T) { +// TestMigrateLegacyMountsRespectsCurrentMarker confirms that the marker is the +// gate: an app with a `-v` line is skipped when it already carries the current +// version, leaving the legacy line in docker-options. +func TestMigrateLegacyMountsRespectsCurrentMarker(t *testing.T) { _, dokkuRoot := setupMigrationEnv(t) stageApp(t, dokkuRoot, "alpha", map[string][]string{ "deploy": {"-v /var/log:/log"}, }) - if err := common.PropertyWrite(PluginName, "alpha", MigratedProperty, "true"); err != nil { + if err := common.PropertyWrite(PluginName, "alpha", MigratedProperty, MigratedPropertyVersion); err != nil { t.Fatalf("seed property: %v", err) } @@ -405,7 +408,40 @@ func TestMigrateLegacyMountsRespectsExistingProperty(t *testing.T) { t.Fatalf("LoadAttachments: %v", err) } if len(atts) != 0 { - t.Errorf("expected 0 attachments (property gated migration), got %d", len(atts)) + t.Errorf("expected 0 attachments (marker gated migration), got %d", len(atts)) + } +} + +// TestMigrateLegacyMountsRescansStaleMarker is the other half of that gate. +// Releases before process-scoped migration wrote "true" and drained only the +// default scope, so an app still holding that value has to be rescanned once +// rather than skipped forever. +func TestMigrateLegacyMountsRescansStaleMarker(t *testing.T) { + _, dokkuRoot := setupMigrationEnv(t) + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /var/log:/log"}, + }) + + if err := common.PropertyWrite(PluginName, "alpha", MigratedProperty, "true"); err != nil { + t.Fatalf("seed property: %v", err) + } + + if err := MigrateLegacyMounts(); err != nil { + t.Fatalf("MigrateLegacyMounts: %v", err) + } + + if got := phaseOptions(t, "alpha", "deploy"); len(got) != 0 { + t.Errorf("expected -v line drained, got %v", got) + } + atts, err := LoadAttachments("alpha") + if err != nil { + t.Fatalf("LoadAttachments: %v", err) + } + if len(atts) != 1 { + t.Fatalf("expected 1 attachment, got %d", len(atts)) + } + if got := common.PropertyGetDefault(PluginName, "alpha", MigratedProperty, ""); got != MigratedPropertyVersion { + t.Errorf("legacy-mounts-migrated = %q, want %q", got, MigratedPropertyVersion) } } @@ -424,3 +460,98 @@ func equalSorted(a, b []string) bool { } return true } + +// stageProcessOptions seeds a named process-type scope's docker-options +// property file for one phase, the shape `docker-options:add --process worker` +// leaves behind. +func stageProcessOptions(t *testing.T, app string, processType string, phase string, lines []string) { + t.Helper() + key := processType + "." + phase + if err := common.PropertyListWrite("docker-options", app, key, lines); err != nil { + t.Fatalf("PropertyListWrite: %v", err) + } +} + +func processPhaseOptions(t *testing.T, app string, processType string, phase string) []string { + t.Helper() + got, err := dockeroptions.GetDockerOptionsForProcessPhase(app, processType, phase) + if err != nil { + t.Fatalf("GetDockerOptionsForProcessPhase %s/%s/%s: %v", app, processType, phase, err) + } + return got +} + +// TestMigrateAppDrainsProcessScopedMounts is the migration half of #9059. A +// `-v` line registered under `docker-options:add --process worker` used to be +// invisible to storage forever, because the drain only read the default scope. +func TestMigrateAppDrainsProcessScopedMounts(t *testing.T) { + _, dokkuRoot := setupMigrationEnv(t) + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /var/log:/log"}, + }) + stageProcessOptions(t, "alpha", "worker", "deploy", []string{"-v /var/spool:/spool", "--restart=on-failure:5"}) + + if err := migrateApp("alpha", false); err != nil { + t.Fatalf("migrateApp: %v", err) + } + + attachments, err := LoadAttachments("alpha") + if err != nil { + t.Fatalf("LoadAttachments: %v", err) + } + if len(attachments) != 2 { + t.Fatalf("expected 2 attachments, got %d", len(attachments)) + } + + scopes := map[string]string{} + for _, attachment := range attachments { + scopes[attachment.ContainerPath] = attachment.EffectiveProcessType() + } + if scopes["/log"] != DefaultProcessType { + t.Errorf("expected /log in the default scope, got %q", scopes["/log"]) + } + if scopes["/spool"] != "worker" { + t.Errorf("expected /spool scoped to worker, got %q", scopes["/spool"]) + } + + // The drained line goes, the non-mount option in the same scope stays. + if got := processPhaseOptions(t, "alpha", "worker", "deploy"); !equalSorted(got, []string{"--restart=on-failure:5"}) { + t.Errorf("expected only the non-mount option left, got %v", got) + } +} + +// TestMigrateAppSkipsProcessScopeCoveredByDefault covers an app that declared +// the same mount twice, once globally and once for a process. The default +// attachment already reaches that process, so recording the second one would +// emit two -v flags for one container path. +func TestMigrateAppSkipsProcessScopeCoveredByDefault(t *testing.T) { + _, dokkuRoot := setupMigrationEnv(t) + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /var/log:/log"}, + }) + stageProcessOptions(t, "alpha", "worker", "deploy", []string{"-v /var/log:/log"}) + + if err := migrateApp("alpha", false); 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)) + } + if attachments[0].EffectiveProcessType() != DefaultProcessType { + t.Errorf("expected the surviving attachment in the default scope, got %q", attachments[0].EffectiveProcessType()) + } + + // Both lines are still drained, so docker-options stops emitting the + // duplicate alongside the storage attachment. + if got := processPhaseOptions(t, "alpha", "worker", "deploy"); len(got) != 0 { + t.Errorf("expected the worker scope drained, got %v", got) + } + if got := phaseOptions(t, "alpha", "deploy"); len(got) != 0 { + t.Errorf("expected the default scope drained, got %v", got) + } +} diff --git a/plugins/storage/mounts_test.go b/plugins/storage/mounts_test.go index a55a5e337..94eecb0ef 100644 --- a/plugins/storage/mounts_test.go +++ b/plugins/storage/mounts_test.go @@ -178,6 +178,66 @@ func TestCommandMountReplaceReadonlyFlagAppliesToEverySpec(t *testing.T) { Expect(attachments[1].Readonly).To(BeTrue()) } +// TestCommandMountReplaceRejectsPathHeldByAnotherScope covers the scopes a +// --replace call does not replace. The declared set is already unique on +// container path within its own scope, but a path held by a different process +// type would still collide at deploy time. +func TestCommandMountReplaceRejectsPathHeldByAnotherScope(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + NameOrPath: "demo-data", + ContainerDir: "/app/storage", + ProcessType: "web", + })).To(Succeed()) + + err := CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-cache:/app/storage"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Container path /app/storage on app demo is already mounted by storage entry demo-data for process type web")) + + // The rejected call leaves the stored set exactly as it was. + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].EntryName).To(Equal("demo-data")) + Expect(attachments[0].ProcessType).To(Equal("web")) +} + +// TestCommandMountReplaceAllowsPathInAReplacedScope is the other side of it: +// the scope being replaced is dropped before the check, so re-declaring a path +// that scope already held is the ordinary idempotent case, not a conflict. +func TestCommandMountReplaceAllowsPathInAReplacedScope(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + NameOrPath: "demo-data", + ContainerDir: "/app/storage", + })).To(Succeed()) + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-cache:/app/storage"}, + })).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].EntryName).To(Equal("demo-cache")) +} + func TestCommandMountReplaceIsIdempotent(t *testing.T) { RegisterTestingT(t) setupTestApp(t, "demo") diff --git a/plugins/storage/report.go b/plugins/storage/report.go index 9e4dd3121..5455c497c 100644 --- a/plugins/storage/report.go +++ b/plugins/storage/report.go @@ -72,7 +72,9 @@ func reportRunMounts(appName string) string { // registerAttachmentFlags adds one report flag per Attachment field for the // given 1-based index. Host path is resolved from the referenced Entry // (falling back to entry.Name when the entry has no host path, matching -// ListAppMountEntries). +// ListAppMountEntries). Process type is reported as the scope the attachment +// actually deploys under, so a row written before the field existed reads as +// _default_ rather than blank. func registerAttachmentFlags(flags map[string]common.ReportFunc, index int, att *Attachment, entry *Entry) { prefix := fmt.Sprintf("--storage-attachment.%d.", index) captured := att @@ -84,7 +86,7 @@ func registerAttachmentFlags(flags map[string]common.ReportFunc, index int, att flags[prefix+"host-path"] = func(string) string { return host } flags[prefix+"container-path"] = func(string) string { return captured.ContainerPath } flags[prefix+"phases"] = func(string) string { return strings.Join(captured.Phases, ",") } - flags[prefix+"process-type"] = func(string) string { return captured.ProcessType } + flags[prefix+"process-type"] = func(string) string { return captured.EffectiveProcessType() } flags[prefix+"subpath"] = func(string) string { return captured.Subpath } flags[prefix+"readonly"] = func(string) string { return strconv.FormatBool(captured.Readonly) } flags[prefix+"volume-options"] = func(string) string { return captured.VolumeOptions } diff --git a/plugins/storage/src/triggers/triggers.go b/plugins/storage/src/triggers/triggers.go index 565891290..6d05a4e82 100644 --- a/plugins/storage/src/triggers/triggers.go +++ b/plugins/storage/src/triggers/triggers.go @@ -28,12 +28,22 @@ func main() { appName := flag.Arg(0) phase := flag.Arg(1) err = storage.TriggerStorageAppMounts(appName, phase) + case "docker-args-process-deploy": + appName := flag.Arg(0) + processType := flag.Arg(3) + err = storage.TriggerDockerArgs(appName, storage.PhaseDeploy, processType) case "docker-args-deploy": - appName := flag.Arg(0) - err = storage.TriggerDockerArgs(appName, storage.PhaseDeploy) + // No longer registered in the plugin Makefile; storage emits its + // deploy-phase flags through docker-args-process-deploy, which every + // caller of this trigger also invokes. The case remains so a symlink + // left behind by an older build resolves to a no-op rather than + // falling through to the error below and failing the deploy. + err = nil case "docker-args-run": + // A `dokku run` container belongs to no process type, so only + // default-scoped attachments apply. appName := flag.Arg(0) - err = storage.TriggerDockerArgs(appName, storage.PhaseRun) + err = storage.TriggerDockerArgs(appName, storage.PhaseRun, storage.DefaultProcessType) case "post-delete": appName := flag.Arg(0) err = storage.TriggerPostDelete(appName) diff --git a/plugins/storage/subcommands.go b/plugins/storage/subcommands.go index aedf163c8..490437e0c 100644 --- a/plugins/storage/subcommands.go +++ b/plugins/storage/subcommands.go @@ -320,17 +320,6 @@ func resolveMountSpecEntry(appName string, appScheduler string, spec string, par return entry, false, nil } -// attachmentProcessType returns the process type an attachment is scoped to, -// treating an unset value as the default scope so attachments written before -// the field existed still sort into a scope. -func attachmentProcessType(attachment *Attachment) string { - if attachment.ProcessType == "" { - return DefaultProcessType - } - - return attachment.ProcessType -} - // replaceMounts swaps one process type's entire mount set for the declared one // in a single write, so a caller converging an app onto a declared set no // longer issues one storage:mount or storage:unmount per difference and can no @@ -417,13 +406,24 @@ func replaceMounts(input CommandMountInput) error { return err } - attachments := []*Attachment{} + retained := []*Attachment{} for _, attachment := range existing { - if attachmentProcessType(attachment) != processType { - attachments = append(attachments, attachment) + if attachment.EffectiveProcessType() != processType { + retained = append(retained, attachment) } } - attachments = append(attachments, declared...) + + // The declared set is already unique on container path within its own + // scope; what is left to check is the scopes this call does not replace, + // so a declared path cannot collide with one held by another process type. + // Checking before the write keeps the all-or-nothing contract. + for _, attachment := range declared { + if err := ensureContainerPathFree(input.AppName, retained, attachment); err != nil { + return err + } + } + + attachments := append(retained, declared...) if err := SaveAttachments(input.AppName, attachments); err != nil { return err @@ -578,7 +578,7 @@ func unmountAll(input CommandUnmountInput) error { keep := []*Attachment{} for _, attachment := range attachments { - if attachmentProcessType(attachment) != input.ProcessType { + if attachment.EffectiveProcessType() != input.ProcessType { keep = append(keep, attachment) } } diff --git a/plugins/storage/triggers.go b/plugins/storage/triggers.go index 2fd01d44b..f92c76fee 100644 --- a/plugins/storage/triggers.go +++ b/plugins/storage/triggers.go @@ -236,31 +236,59 @@ func TriggerStorageAppMounts(appName string, phase string) error { return nil } -// TriggerDockerArgs emits `-v` flags for each docker-local attachment in -// the requested phase. Plugn concatenates this with docker-options' -// equivalent trigger output, so docker-local apps continue to receive -// their bind mounts through the standard pipeline. -func TriggerDockerArgs(appName string, phase string) error { - attachments, err := AttachmentsForPhase(appName, phase) +// TriggerDockerArgs emits `-v` flags for the docker-local attachments in the +// requested phase that apply to the given process type. Plugn concatenates +// this with docker-options' equivalent trigger output, so docker-local apps +// continue to receive their bind mounts through the standard pipeline. +// +// An empty processType names a container that belongs to no process - the +// app.json deploy-task path invokes the trigger without one - and so receives +// the default scope. Unlike docker-options, which emits its default scope from +// a separate docker-args-deploy handler, this is storage's only deploy-time +// emitter: returning early on an empty process type would drop every mount. +func TriggerDockerArgs(appName string, phase string, processType string) error { + flags, err := dockerVFlagsForProcess(appName, phase, processType) if err != nil { return err } + for _, flag := range flags { + fmt.Printf(" %s", flag) + } + return nil +} + +// dockerVFlagsForProcess returns the `-v` flags a single process type's +// container receives for the given phase. +// +// Every attachment in the phase is resolved and scheduler-checked, not just +// the ones in scope, so an entry created for the wrong scheduler still fails +// the deploy no matter which process type happens to be starting. +func dockerVFlagsForProcess(appName string, phase string, processType string) ([]string, error) { + attachments, err := AttachmentsForPhase(appName, phase) + if err != nil { + return nil, err + } + + flags := []string{} for _, attachment := range attachments { entry, err := LoadEntry(attachment.EntryName) if err != nil { - return fmt.Errorf("attachment on %q references missing entry %q: %w", appName, attachment.EntryName, err) + return nil, fmt.Errorf("attachment on %q references missing entry %q: %w", appName, attachment.EntryName, err) } if entry.Scheduler != SchedulerDockerLocal { - return fmt.Errorf("storage entry %q is scheduler=%s but is mounted on a docker-local app; recreate it with --scheduler docker-local", entry.Name, entry.Scheduler) + return nil, fmt.Errorf("storage entry %q is scheduler=%s but is mounted on a docker-local app; recreate it with --scheduler docker-local", entry.Name, entry.Scheduler) + } + if !attachment.AppliesToProcessType(processType) { + continue } flag := buildDockerVFlag(entry, attachment) if flag == "" { continue } - fmt.Printf(" %s", flag) + flags = append(flags, flag) } - return nil + return flags, nil } // buildDockerVFlag formats the Docker -v argument for a docker-local diff --git a/plugins/storage/triggers_test.go b/plugins/storage/triggers_test.go index b15d35655..41843bec9 100644 --- a/plugins/storage/triggers_test.go +++ b/plugins/storage/triggers_test.go @@ -78,7 +78,85 @@ func TestTriggerDockerArgsRejectsSchedulerMismatch(t *testing.T) { }, }) - err := TriggerDockerArgs("demo", PhaseDeploy) + err := TriggerDockerArgs("demo", PhaseDeploy, "web") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(Equal(`storage entry "demo-pvc" is scheduler=k3s but is mounted on a docker-local app; recreate it with --scheduler docker-local`)) } + +// TestTriggerDockerArgsRejectsSchedulerMismatchOutOfScope pins that the +// scheduler check runs over every attachment in the phase, not just the ones +// the process type being deployed would actually mount. An entry created for +// the wrong scheduler has to fail the deploy whichever process starts first. +func TestTriggerDockerArgsRejectsSchedulerMismatchOutOfScope(t *testing.T) { + RegisterTestingT(t) + root := withTempLibRoot(t) + + Expect(SaveEntry(&Entry{ + Name: "demo-pvc", + Scheduler: SchedulerK3s, + })).To(Succeed()) + + writeAttachmentsFile(t, root, "demo", []*Attachment{ + { + EntryName: "demo-pvc", + ContainerPath: "/data", + Phases: []string{PhaseDeploy}, + ProcessType: "worker", + }, + }) + + _, err := dockerVFlagsForProcess("demo", PhaseDeploy, "web") + Expect(err).To(HaveOccurred()) +} + +// TestDockerVFlagsForProcessScopes is the docker-local half of #9059. An +// attachment scoped to a named process type reaches that process only; the +// default scope reaches every process, including containers that belong to no +// process at all - the app.json deploy-task path passes an empty process type. +func TestDockerVFlagsForProcessScopes(t *testing.T) { + RegisterTestingT(t) + root := withTempLibRoot(t) + + for _, name := range []string{"shared", "web-only", "legacy"} { + Expect(SaveEntry(&Entry{ + Name: name, + Scheduler: SchedulerDockerLocal, + HostPath: "/host/" + name, + })).To(Succeed()) + } + + writeAttachmentsFile(t, root, "demo", []*Attachment{ + { + EntryName: "shared", + ContainerPath: "/shared", + Phases: []string{PhaseDeploy}, + ProcessType: DefaultProcessType, + }, + { + EntryName: "web-only", + ContainerPath: "/web", + Phases: []string{PhaseDeploy}, + ProcessType: "web", + }, + { + // Written before the process-type field existed. + EntryName: "legacy", + ContainerPath: "/legacy", + Phases: []string{PhaseDeploy}, + }, + }) + + for _, testCase := range []struct { + processType string + expected []string + }{ + {processType: "web", expected: []string{"-v /host/legacy:/legacy", "-v /host/shared:/shared", "-v /host/web-only:/web"}}, + {processType: "worker", expected: []string{"-v /host/legacy:/legacy", "-v /host/shared:/shared"}}, + {processType: "", expected: []string{"-v /host/legacy:/legacy", "-v /host/shared:/shared"}}, + {processType: DefaultProcessType, expected: []string{"-v /host/legacy:/legacy", "-v /host/shared:/shared"}}, + } { + flags, err := dockerVFlagsForProcess("demo", PhaseDeploy, testCase.processType) + Expect(err).NotTo(HaveOccurred()) + Expect(flags).To(Equal(testCase.expected), "process type %q", testCase.processType) + } +} diff --git a/tests/unit/storage-1.bats b/tests/unit/storage-1.bats index 1f702d95d..c1e04ac68 100644 --- a/tests/unit/storage-1.bats +++ b/tests/unit/storage-1.bats @@ -670,12 +670,14 @@ teardown() { assert_success assert_output "1" - # Property marker is set after a successful migration with mounts. + # Property marker is set after a successful migration with mounts. It + # carries the migration version rather than a bare "true" so an app + # migrated by an older release is rescanned once for process-scoped mounts. run /bin/bash -c "sudo test -f /var/lib/dokku/config/storage/$TEST_APP/legacy-mounts-migrated" assert_success run /bin/bash -c "sudo cat /var/lib/dokku/config/storage/$TEST_APP/legacy-mounts-migrated" assert_success - assert_output "true" + assert_output "2" # Cleanup: unmount + destroy the synthesized legacy entry. We need # the entry name from list-entries since legacy- is content-derived. @@ -727,11 +729,13 @@ teardown() { echo "status: $status" assert_success + # The flag file drains into the property, which the version gate then + # treats as stale and advances to the current migration version. run /bin/bash -c "sudo test -f /var/lib/dokku/config/storage/$TEST_APP/legacy-mounts-migrated" assert_success run /bin/bash -c "sudo cat /var/lib/dokku/config/storage/$TEST_APP/legacy-mounts-migrated" assert_success - assert_output "true" + assert_output "2" run /bin/bash -c "sudo test -e $flag_dir/$TEST_APP" assert_failure diff --git a/tests/unit/storage-4.bats b/tests/unit/storage-4.bats new file mode 100644 index 000000000..8bc37ea17 --- /dev/null +++ b/tests/unit/storage-4.bats @@ -0,0 +1,239 @@ +#!/usr/bin/env bats + +load test_helper + +setup() { + global_setup + create_app + rm -rf "$DOKKU_LIB_ROOT/data/storage/rdmtestapp*" +} + +teardown() { + destroy_app + global_teardown +} + +@test "(storage) [process-type] docker-args-process-deploy emits default and matching mounts" { + run /bin/bash -c "dokku storage:create rdmtest-shared" + echo "output: $output" + echo "status: $status" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-web" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-shared --container-dir /shared" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-web --container-dir /web --process-type web" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "echo '' | dokku plugin:trigger docker-args-process-deploy $TEST_APP herokuish latest web | grep -o -- '-v [^ ]*' | cut -d: -f2 | sort | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "/shared /web" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-shared --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-web --force" + assert_success +} + +@test "(storage) [process-type] docker-args-process-deploy omits mounts scoped to another process" { + run /bin/bash -c "dokku storage:create rdmtest-shared" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-web" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-shared --container-dir /shared" + assert_success + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-web --container-dir /web --process-type web" + assert_success + + run /bin/bash -c "echo '' | dokku plugin:trigger docker-args-process-deploy $TEST_APP herokuish latest worker | grep -o -- '-v [^ ]*' | cut -d: -f2 | sort | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "/shared" + + # An ephemeral container with no process type of its own - the app.json + # deploy-task path invokes the trigger this way - still gets the default + # scope, and only the default scope. + run /bin/bash -c "echo '' | dokku plugin:trigger docker-args-process-deploy $TEST_APP herokuish latest | grep -o -- '-v [^ ]*' | cut -d: -f2 | sort | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "/shared" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-shared --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-web --force" + assert_success +} + +@test "(storage) [process-type] docker-args-run emits only default-scoped mounts" { + run /bin/bash -c "dokku storage:create rdmtest-shared" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-web" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-shared --container-dir /shared" + assert_success + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-web --container-dir /web --process-type web" + assert_success + + run /bin/bash -c "echo '' | dokku plugin:trigger docker-args-run $TEST_APP latest | grep -o -- '-v [^ ]*' | cut -d: -f2 | sort | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "/shared" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-shared --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-web --force" + assert_success +} + +@test "(storage:mount) rejects a container path already mounted in an overlapping process type" { + run /bin/bash -c "dokku storage:create rdmtest-data" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-cache" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-data --container-dir /app/storage" + echo "output: $output" + echo "status: $status" + assert_success + + # The default scope also applies to web, so both mounts would target + # /app/storage on the same container. + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-cache --container-dir /app/storage --process-type web" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "is already mounted by storage entry rdmtest-data for process type _default_" + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '. | length'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "1" + + # Two named scopes never share a container, so they may each claim the path. + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-data --container-dir /scoped --process-type web" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-cache --container-dir /scoped --process-type worker" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-data --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-cache --force" + assert_success +} + +@test "(storage:migrate) drains process-scoped docker-options mounts" { + run /bin/bash -c "mkdir -p /tmp/rdmtest-spool" + assert_success + + run /bin/bash -c "dokku docker-options:add --process worker $TEST_APP deploy '-v /tmp/rdmtest-spool:/spool'" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:migrate $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.container-path\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "/spool" + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.process-type\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "worker" + + run /bin/bash -c "dokku docker-options:report $TEST_APP --format json | jq -r '.. | strings' | grep -c 'rdmtest-spool' || true" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "0" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "rm -rf /tmp/rdmtest-spool" + assert_success +} + +@test "(storage) [process-type] a process-scoped mount reaches only its own process" { + run /bin/bash -c "dokku storage:create rdmtest-shared" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-web" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-shared --container-dir /shared" + assert_success + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-web --container-dir /web --process-type web" + assert_success + + run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP web_worker_callback + echo "output: $output" + echo "status: $status" + assert_success + + # Only web is scaled up by default, so the worker needs a container of its + # own before there is anything to compare against. + run /bin/bash -c "dokku ps:scale $TEST_APP worker=1" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "docker inspect -f '{{range .HostConfig.Binds}}{{println .}}{{end}}' \$(< $DOKKU_ROOT/$TEST_APP/CONTAINER.web.1) | cut -d: -f2 | sort | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "/shared /web" + + run /bin/bash -c "docker inspect -f '{{range .HostConfig.Binds}}{{println .}}{{end}}' \$(< $DOKKU_ROOT/$TEST_APP/CONTAINER.worker.1) | cut -d: -f2 | sort | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "/shared" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-shared --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-web --force" + assert_success +} + +web_worker_callback() { + local APP="$1" + local APP_REPO_DIR="$2" + cat >"$APP_REPO_DIR/Procfile" <