From b612285392508918bf99e9e159ff1f56813a3288 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 21 Sep 2026 18:08:42 -0400 Subject: [PATCH 1/3] 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" < Date: Mon, 21 Sep 2026 23:35:25 -0400 Subject: [PATCH 2/3] fix: declare one k3s pod volume per storage entry Mounting one storage entry at two container paths named the same pod volume twice, which Kubernetes rejects as a duplicate, so the chart never applied even though the k3s documentation offers mounting a claim "into the same app at multiple subpaths" as a supported shape. A pod now declares each entry once and binds it at each path it was mounted at. Rendering the chart in a test also surfaced that a cron schedule was emitted unquoted, so any schedule beginning with `*` produced a manifest that could not be parsed at all. --- plugins/scheduler-k3s/chart.go | 22 +- plugins/scheduler-k3s/chart_render_test.go | 240 ++++++++++++++++++ plugins/scheduler-k3s/storage.go | 55 +++- plugins/scheduler-k3s/storage_test.go | 72 +++++- plugins/scheduler-k3s/template.go | 11 +- .../templates/chart/cron-job.yaml | 6 +- .../templates/chart/deployment.yaml | 4 + 7 files changed, 387 insertions(+), 23 deletions(-) create mode 100644 plugins/scheduler-k3s/chart_render_test.go diff --git a/plugins/scheduler-k3s/chart.go b/plugins/scheduler-k3s/chart.go index b254b57d3..b019cf0aa 100644 --- a/plugins/scheduler-k3s/chart.go +++ b/plugins/scheduler-k3s/chart.go @@ -359,6 +359,15 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti return cleanup(err) } + // Every cron job gets the same volumes: 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 scope only. + cronVolumes, err := processVolumesFor(baseVolumes, deployMountPairs, storage.DefaultProcessType) + if err != nil { + return cleanup(fmt.Errorf("Error building volumes for cron tasks: %w", err)) + } + for processType, processCount := range processes { healthchecks, ok := appJSON.Healthchecks[processType] if !ok { @@ -403,6 +412,11 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti return cleanup(fmt.Errorf("Error getting autoscaling: %w", err)) } + processVolumes, err := processVolumesFor(baseVolumes, deployMountPairs, processType) + if err != nil { + return cleanup(fmt.Errorf("Error building volumes for process type %s: %w", processType, err)) + } + processValues := ProcessValues{ Annotations: annotations, Autoscaling: autoscaling, @@ -413,7 +427,7 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti ProcessType: ProcessType_Worker, Replicas: int32(processCount), Resources: processResources, - Volumes: processVolumesFor(baseVolumes, deployMountPairs, processType), + Volumes: processVolumes, } if processType == "web" { @@ -591,11 +605,7 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti ProcessType: ProcessType_Cron, Replicas: 1, Resources: processResources, - // 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), + Volumes: cronVolumes, } values.Processes[cronTask.ID] = processValues } diff --git a/plugins/scheduler-k3s/chart_render_test.go b/plugins/scheduler-k3s/chart_render_test.go new file mode 100644 index 000000000..b280bfb69 --- /dev/null +++ b/plugins/scheduler-k3s/chart_render_test.go @@ -0,0 +1,240 @@ +package scheduler_k3s + +import ( + "strings" + "testing" + + "github.com/dokku/dokku/plugins/storage" + "gopkg.in/yaml.v3" + "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chartutil" + "helm.sh/helm/v3/pkg/engine" +) + +func storageEntryK3s(name string) *storage.Entry { + return &storage.Entry{Name: name, Scheduler: storage.SchedulerK3s} +} + +func storageAttachment(containerPath string, subpath string) *storage.Attachment { + return &storage.Attachment{ContainerPath: containerPath, Subpath: subpath} +} + +// renderProcessChart renders one of the chart templates against the given +// process values, going through YAML the same way a real deploy does. That +// round trip is the point: it exercises the struct's yaml tags against the +// keys the template actually reads, so a rename on one side without the other +// fails here instead of at helm upgrade time. +func renderProcessChart(t *testing.T, templateName string, processName string, values ProcessValues) string { + t.Helper() + + appValues := AppValues{ + Global: GlobalValues{ + AppName: "demo", + DeploymentID: "1", + Namespace: "default", + Image: GlobalImage{ + Name: "dokku/demo:latest", + Type: "herokuish", + }, + }, + Processes: map[string]ProcessValues{processName: values}, + } + + encoded, err := yaml.Marshal(appValues) + if err != nil { + t.Fatalf("marshal values: %v", err) + } + rendered := map[string]interface{}{} + if err := yaml.Unmarshal(encoded, &rendered); err != nil { + t.Fatalf("unmarshal values: %v", err) + } + + templateBody, err := templates.ReadFile("templates/chart/" + templateName) + if err != nil { + t.Fatalf("read template: %v", err) + } + helpers, err := templates.ReadFile("templates/chart/_helpers.tpl") + if err != nil { + t.Fatalf("read helpers: %v", err) + } + + c := &chart.Chart{ + Metadata: &chart.Metadata{APIVersion: chart.APIVersionV2, Name: "demo", Version: "0.0.1"}, + Templates: []*chart.File{ + {Name: "templates/" + templateName, Data: templateBody}, + {Name: "templates/_helpers.tpl", Data: helpers}, + }, + } + + top, err := chartutil.ToRenderValues(c, rendered, chartutil.ReleaseOptions{Name: "demo", Namespace: "default"}, nil) + if err != nil { + t.Fatalf("render values: %v", err) + } + out, err := engine.Render(c, top) + if err != nil { + t.Fatalf("render chart: %v", err) + } + + return out["demo/templates/"+templateName] +} + +// podSpecFrom decodes the rendered manifest and returns the pod spec at the +// given path, so the assertions below read the same structure the API server +// would rather than counting indentation. +func podSpecFrom(t *testing.T, manifest string, path ...string) map[string]interface{} { + t.Helper() + + decoder := yaml.NewDecoder(strings.NewReader(manifest)) + doc := map[string]interface{}{} + for { + candidate := map[string]interface{}{} + if err := decoder.Decode(&candidate); err != nil { + break + } + if len(candidate) > 0 { + doc = candidate + break + } + } + if len(doc) == 0 { + t.Fatalf("no document decoded from manifest:\n%s", manifest) + } + + current := doc + for _, key := range path { + next, ok := current[key].(map[string]interface{}) + if !ok { + t.Fatalf("key %q missing while walking %v in manifest:\n%s", key, path, manifest) + } + current = next + } + return current +} + +// volumeNamesFrom returns the pod-level volume names, and mountsFrom the +// container-level mounts, of a decoded pod spec. +func volumeNamesFrom(t *testing.T, podSpec map[string]interface{}) []string { + t.Helper() + + names := []string{} + volumes, _ := podSpec["volumes"].([]interface{}) + for _, entry := range volumes { + volume, ok := entry.(map[string]interface{}) + if !ok { + t.Fatalf("unexpected volume entry %#v", entry) + } + names = append(names, volume["name"].(string)) + } + return names +} + +func mountsFrom(t *testing.T, podSpec map[string]interface{}) []map[string]interface{} { + t.Helper() + + containers, _ := podSpec["containers"].([]interface{}) + if len(containers) == 0 { + t.Fatalf("no containers in pod spec %#v", podSpec) + } + container, ok := containers[0].(map[string]interface{}) + if !ok { + t.Fatalf("unexpected container entry %#v", containers[0]) + } + + mounts := []map[string]interface{}{} + raw, _ := container["volumeMounts"].([]interface{}) + for _, entry := range raw { + mount, ok := entry.(map[string]interface{}) + if !ok { + t.Fatalf("unexpected volumeMount entry %#v", entry) + } + mounts = append(mounts, mount) + } + return mounts +} + +// TestDeploymentRendersOneVolumePerNameAndOneMountPerPath is the regression +// for a storage entry mounted at two container paths. Kubernetes requires pod +// volume names to be unique, so the pod must declare the PVC once and bind it +// twice - emitting it twice is rejected with "Duplicate value". +func TestDeploymentRendersOneVolumePerNameAndOneMountPerPath(t *testing.T) { + pairs := []AppMountPair{ + { + Entry: storageEntryK3s("demo-pvc"), + Attachment: storageAttachment("/one", ""), + }, + { + Entry: storageEntryK3s("demo-pvc"), + Attachment: storageAttachment("/two", "sub"), + }, + } + + manifest := renderProcessChart(t, "deployment.yaml", "web", ProcessValues{ + ProcessType: ProcessType_Worker, + Replicas: 1, + Volumes: ToProcessVolumes(pairs, "web"), + }) + + podSpec := podSpecFrom(t, manifest, "spec", "template", "spec") + + names := volumeNamesFrom(t, podSpec) + if len(names) != 1 || names[0] != "demo-pvc" { + t.Fatalf("expected a single pod volume named demo-pvc, got %v\n%s", names, manifest) + } + + mounts := mountsFrom(t, podSpec) + if len(mounts) != 2 { + t.Fatalf("expected 2 volume mounts, got %d\n%s", len(mounts), manifest) + } + if mounts[0]["mountPath"] != "/one" || mounts[1]["mountPath"] != "/two" { + t.Errorf("expected mounts at /one and /two, got %v and %v", mounts[0]["mountPath"], mounts[1]["mountPath"]) + } + if mounts[1]["subPath"] != "sub" { + t.Errorf("expected subPath sub on the second mount, got %v", mounts[1]["subPath"]) + } + if !strings.Contains(manifest, "claimName: demo-pvc") { + t.Errorf("expected the PVC to be referenced, got:\n%s", manifest) + } +} + +// TestCronJobRendersOneVolumePerName covers the same rule in the cron-job +// template, which renders its volumes from the same two lists. +func TestCronJobRendersOneVolumePerName(t *testing.T) { + pairs := []AppMountPair{ + { + Entry: storageEntryK3s("demo-pvc"), + Attachment: storageAttachment("/one", ""), + }, + { + Entry: storageEntryK3s("demo-pvc"), + Attachment: storageAttachment("/two", ""), + }, + } + + manifest := renderProcessChart(t, "cron-job.yaml", "cron-1", ProcessValues{ + ProcessType: ProcessType_Cron, + Replicas: 1, + Cron: ProcessCron{ + ID: "abc123", + Hash: "abc123", + Schedule: "*/5 * * * *", + Suffix: "aaaaa", + }, + Volumes: ToProcessVolumes(pairs, ""), + }) + + podSpec := podSpecFrom(t, manifest, "spec", "jobTemplate", "spec", "template", "spec") + + if names := volumeNamesFrom(t, podSpec); len(names) != 1 || names[0] != "demo-pvc" { + t.Fatalf("expected a single pod volume named demo-pvc, got %v\n%s", names, manifest) + } + if mounts := mountsFrom(t, podSpec); len(mounts) != 2 { + t.Fatalf("expected 2 volume mounts, got %d\n%s", len(mounts), manifest) + } + + // A cron expression starting with `*` is not a valid plain YAML scalar, so + // the schedule has to render quoted or the whole manifest fails to parse. + spec := podSpecFrom(t, manifest, "spec") + if spec["schedule"] != "*/5 * * * *" { + t.Errorf("expected the schedule to survive rendering, got %v", spec["schedule"]) + } +} diff --git a/plugins/scheduler-k3s/storage.go b/plugins/scheduler-k3s/storage.go index 61f08f447..72d150d4c 100644 --- a/plugins/scheduler-k3s/storage.go +++ b/plugins/scheduler-k3s/storage.go @@ -293,8 +293,9 @@ func ValidateMountPairs(pairs []AppMountPair) error { } // 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. +// ProcessVolumes, one per container mount. 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. @@ -320,20 +321,54 @@ func ToProcessVolumes(pairs []AppMountPair, processType string) []ProcessVolume 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 { +// processVolumesFor returns the volumes one process type's pods receive: what +// 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. +// +// Entries repeat a Name once per container path, which is what the chart wants: +// it renders a volumeMounts entry per element and a pod volume per distinct +// Name. Two different sources claiming one Name cannot be rendered that way, so +// they are rejected rather than silently collapsed to whichever came first. +func processVolumesFor(base []ProcessVolume, pairs []AppMountPair, processType string) ([]ProcessVolume, error) { scoped := ToProcessVolumes(pairs, processType) + volumes := make([]ProcessVolume, 0, len(base)+len(scoped)) volumes = append(volumes, base...) volumes = append(volumes, scoped...) - return volumes + + sources := map[string]ProcessVolume{} + for _, volume := range volumes { + existing, seen := sources[volume.Name] + if seen && !sameVolumeSource(existing, volume) { + return nil, fmt.Errorf("volume name %q is claimed by two different sources in one pod", volume.Name) + } + sources[volume.Name] = volume + } + + return volumes, nil } -// asK8sVolumeMount is a small helper used by tests / other callers that -// want a corev1.VolumeMount from a ProcessVolume. +// sameVolumeSource reports whether two same-named volumes resolve to the same +// backing source and can therefore share one pod volume. +func sameVolumeSource(first ProcessVolume, second ProcessVolume) bool { + if (first.PersistentClaim == nil) != (second.PersistentClaim == nil) { + return false + } + if first.PersistentClaim != nil && *first.PersistentClaim != *second.PersistentClaim { + return false + } + if (first.EmptyDir == nil) != (second.EmptyDir == nil) { + return false + } + if first.EmptyDir != nil && *first.EmptyDir != *second.EmptyDir { + return false + } + return true +} + +// asK8sVolumeMount converts a ProcessVolume into the corev1 mount shape for +// callers that work against the Kubernetes API types directly. func asK8sVolumeMount(v ProcessVolume) corev1.VolumeMount { return corev1.VolumeMount{ Name: v.Name, diff --git a/plugins/scheduler-k3s/storage_test.go b/plugins/scheduler-k3s/storage_test.go index a43d607eb..566163754 100644 --- a/plugins/scheduler-k3s/storage_test.go +++ b/plugins/scheduler-k3s/storage_test.go @@ -111,6 +111,63 @@ func TestToProcessVolumesAcceptsK3sEntry(t *testing.T) { } } +// TestToProcessVolumesCollapsesOneEntryMountedTwice covers the pod-volume +// uniqueness rule: mounting one storage entry at two container paths has to +// produce two volumeMounts and a single volume, or the API server rejects the +// chart with a duplicate volume name. +func TestToProcessVolumesCollapsesOneEntryMountedTwice(t *testing.T) { + pairs := []AppMountPair{ + { + Entry: &storage.Entry{Name: "demo-pvc", Scheduler: storage.SchedulerK3s}, + Attachment: &storage.Attachment{ContainerPath: "/one", Subpath: "one"}, + }, + { + Entry: &storage.Entry{Name: "demo-pvc", Scheduler: storage.SchedulerK3s}, + Attachment: &storage.Attachment{ContainerPath: "/two", Subpath: "two", Readonly: true}, + }, + } + + volumes := ToProcessVolumes(pairs, "web") + if len(volumes) != 2 { + t.Fatalf("expected 2 entries, got %d", len(volumes)) + } + if volumes[0].Name != volumes[1].Name { + t.Fatalf("expected both entries to name one volume, got %q and %q", volumes[0].Name, volumes[1].Name) + } + if volumes[0].MountPath != "/one" || volumes[1].MountPath != "/two" { + t.Fatalf("expected mounts at /one and /two, got %q and %q", volumes[0].MountPath, volumes[1].MountPath) + } + if !volumes[1].ReadOnly || volumes[0].ReadOnly { + t.Errorf("expected only the second mount to be read only") + } +} + +// TestProcessVolumesForRejectsSameNameDifferentSource guards the name collapse +// the chart performs: the pod volume is emitted from the first entry with a +// given name, so two different sources sharing one name would silently mount +// whichever came first. An app with a storage entry called "shmem" alongside a +// configured shm size is the way to reach it. +func TestProcessVolumesForRejectsSameNameDifferentSource(t *testing.T) { + base := []ProcessVolume{ + { + Name: "shmem", + MountPath: "/dev/shm", + EmptyDir: &ProcessVolumeEmptyDir{Medium: "Memory", SizeLimit: "64Mi"}, + }, + } + + pairs := []AppMountPair{ + { + Entry: &storage.Entry{Name: "shmem", Scheduler: storage.SchedulerK3s}, + Attachment: &storage.Attachment{ContainerPath: "/data"}, + }, + } + + if _, err := processVolumesFor(base, pairs, "web"); err == nil { + t.Fatal("expected an error for two sources claiming one volume name, got nil") + } +} + func TestToProcessVolumesSkipsNilPairs(t *testing.T) { pairs := []AppMountPair{ {Entry: nil, Attachment: nil}, @@ -150,9 +207,8 @@ func TestToProcessVolumesScopesToProcessType(t *testing.T) { {processType: "worker", expected: []string{"shared", "legacy"}}, {processType: "", expected: []string{"shared", "legacy"}}, } { - volumes := ToProcessVolumes(pairs, tc.processType) names := []string{} - for _, volume := range volumes { + for _, volume := range ToProcessVolumes(pairs, tc.processType) { names = append(names, volume.Name) } if len(names) != len(tc.expected) { @@ -171,7 +227,7 @@ func TestToProcessVolumesScopesToProcessType(t *testing.T) { // 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"}) + base = append(base, ProcessVolume{Name: "shmem", MountPath: "/dev/shm", EmptyDir: &ProcessVolumeEmptyDir{Medium: "Memory"}}) pairs := []AppMountPair{ { @@ -180,8 +236,14 @@ func TestProcessVolumesForDoesNotAliasBase(t *testing.T) { }, } - web := processVolumesFor(base, pairs, "web") - worker := processVolumesFor(base, pairs, "worker") + web, err := processVolumesFor(base, pairs, "web") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + worker, err := processVolumesFor(base, pairs, "worker") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if len(web) != 2 { t.Fatalf("expected web to get 2 volumes, got %d", len(web)) diff --git a/plugins/scheduler-k3s/template.go b/plugins/scheduler-k3s/template.go index c506bfb40..c98045fc6 100644 --- a/plugins/scheduler-k3s/template.go +++ b/plugins/scheduler-k3s/template.go @@ -115,9 +115,18 @@ type ProcessValues struct { Replicas int32 `yaml:"replicas"` Resources ProcessResourcesMap `yaml:"resources,omitempty"` Web ProcessWeb `yaml:"web,omitempty"` - Volumes []ProcessVolume `yaml:"volumes,omitempty"` + + // Volumes holds one entry per container mount. The chart renders a + // volumeMounts entry for each of them and a pod volume for each distinct + // Name, so mounting one storage entry at two container paths belongs here + // twice: Kubernetes allows repeated volumeMounts names but requires pod + // volume names to be unique. + Volumes []ProcessVolume `yaml:"volumes,omitempty"` } +// ProcessVolume is one container mount together with the source backing it. +// Entries sharing a Name must agree on their source - they describe one pod +// volume bound at several paths. type ProcessVolume struct { Name string `yaml:"name"` MountPath string `yaml:"mount_path"` diff --git a/plugins/scheduler-k3s/templates/chart/cron-job.yaml b/plugins/scheduler-k3s/templates/chart/cron-job.yaml index 2076ca590..765a020f4 100644 --- a/plugins/scheduler-k3s/templates/chart/cron-job.yaml +++ b/plugins/scheduler-k3s/templates/chart/cron-job.yaml @@ -163,7 +163,10 @@ spec: serviceAccountName: {{ $.Values.global.app_name }} {{- if $config.volumes }} volumes: + {{- $seen := dict }} {{- range $volume := $config.volumes }} + {{- if not (hasKey $seen $volume.name) }} + {{- $_ := set $seen $volume.name true }} - name: {{ $volume.name }} {{- if $volume.empty_dir }} emptyDir: @@ -179,8 +182,9 @@ spec: claimName: {{ $volume.persistent_claim.claim_name }} {{- end }} {{- end }} + {{- end }} {{- end }} - schedule: {{ $config.cron.schedule }} + schedule: {{ $config.cron.schedule | quote }} startingDeadlineSeconds: 60 successfulJobsHistoryLimit: 10 suspend: {{ $config.cron.suspend }} diff --git a/plugins/scheduler-k3s/templates/chart/deployment.yaml b/plugins/scheduler-k3s/templates/chart/deployment.yaml index b8a25c3dc..45151e564 100644 --- a/plugins/scheduler-k3s/templates/chart/deployment.yaml +++ b/plugins/scheduler-k3s/templates/chart/deployment.yaml @@ -179,7 +179,10 @@ spec: serviceAccountName: {{ $.Values.global.app_name }} {{- if $config.volumes }} volumes: + {{- $seen := dict }} {{- range $volume := $config.volumes }} + {{- if not (hasKey $seen $volume.name) }} + {{- $_ := set $seen $volume.name true }} - name: {{ $volume.name }} {{- if $volume.empty_dir }} emptyDir: @@ -195,5 +198,6 @@ spec: claimName: {{ $volume.persistent_claim.claim_name }} {{- end }} {{- end }} + {{- end }} {{- end }} {{- end }} From 17cf249f2e734730a0631348a2bae2f1f65e37c9 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 21 Sep 2026 23:35:28 -0400 Subject: [PATCH 3/3] fix: fail deploy tasks when a docker-args trigger fails The ephemeral containers that run `app.json` deploy tasks ignored a failure from the `docker-args-deploy` and `docker-args-process-deploy` triggers, so a plugin refusing to describe the container produced a task that ran without the arguments it had asked for, while the very same failure aborted the deploy moments later on the scheduler's own path. Both are now fatal, which the trigger documentation states. Storage no longer reports a scheduler mismatch for an app deploying with another scheduler, since those triggers are fired for every app whatever its scheduler and a k3s app's volumes are mounted by k3s rather than bound into a container here. --- docs/development/plugin-triggers.md | 2 +- plugins/app-json/functions.go | 33 +++++++++++++++++------------ plugins/storage/entry_test.go | 4 ++++ plugins/storage/triggers.go | 14 ++++++++++++ plugins/storage/triggers_test.go | 32 ++++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 15 deletions(-) diff --git a/docs/development/plugin-triggers.md b/docs/development/plugin-triggers.md index 76e2f1543..b5cc0445f 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 `, 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. +- 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. A handler that exits non-zero aborts the deploy, including the ephemeral containers that run `app.json` deploy tasks, so a handler that has nothing to contribute for an app should emit nothing and exit zero rather than fail. - Invoked by: `dokku deploy` - Arguments: `$APP $IMAGE_SOURCE_TYPE $IMAGE_TAG [$PROC_TYPE $CONTAINER_INDEX]` - Example: diff --git a/plugins/app-json/functions.go b/plugins/app-json/functions.go index 1422c37f5..8fa148b51 100644 --- a/plugins/app-json/functions.go +++ b/plugins/app-json/functions.go @@ -182,34 +182,39 @@ func executeScript(appName string, image string, imageTag string, phase string) imageSourceType = "pack" } + // Both triggers are fatal here, the same way they are on the scheduler's + // own deploy path. Swallowing the error meant a plugin refusing to describe + // the container - a storage entry belonging to another scheduler, say - + // silently produced a deploy task running without the arguments it asked + // for, while the identical failure aborted the deploy a moment later. var dockerArgs []string results, err := common.CallPlugnTrigger(common.PlugnTriggerInput{ Trigger: "docker-args-deploy", Args: []string{appName, imageTag}, Stdin: strings.NewReader(""), }) - if err == nil { - words, err := shellquote.Split(results.StdoutContents()) - if err != nil { - return err - } - - dockerArgs = append(dockerArgs, words...) + if err != nil { + return fmt.Errorf("Error getting deploy docker args for %s task: %w", phase, err) } + words, err := shellquote.Split(results.StdoutContents()) + if err != nil { + return err + } + dockerArgs = append(dockerArgs, words...) results, err = common.CallPlugnTrigger(common.PlugnTriggerInput{ Trigger: "docker-args-process-deploy", Args: []string{appName, imageSourceType, imageTag}, Stdin: strings.NewReader(""), }) - if err == nil { - words, err := shellquote.Split(results.StdoutContents()) - if err != nil { - return err - } - - dockerArgs = append(dockerArgs, words...) + if err != nil { + return fmt.Errorf("Error getting process deploy docker args for %s task: %w", phase, err) } + words, err = shellquote.Split(results.StdoutContents()) + if err != nil { + return err + } + dockerArgs = append(dockerArgs, words...) filteredArgs := []string{ "--cpus", diff --git a/plugins/storage/entry_test.go b/plugins/storage/entry_test.go index edbb4caa5..f53806efc 100644 --- a/plugins/storage/entry_test.go +++ b/plugins/storage/entry_test.go @@ -20,6 +20,10 @@ func withTempLibRoot(t *testing.T) string { t.Helper() dir := t.TempDir() t.Setenv("DOKKU_LIB_ROOT", dir) + // Point plugn at an empty tree so anything resolving a plugin trigger in + // these tests falls back to its default instead of reading the host's + // real dokku install. + t.Setenv("PLUGIN_PATH", filepath.Join(dir, "plugins")) current, err := user.Current() if err != nil { diff --git a/plugins/storage/triggers.go b/plugins/storage/triggers.go index f92c76fee..144986331 100644 --- a/plugins/storage/triggers.go +++ b/plugins/storage/triggers.go @@ -264,7 +264,17 @@ func TriggerDockerArgs(appName string, phase string, processType string) error { // 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. +// +// Apps on another scheduler emit nothing rather than failing: the docker-args +// triggers are invoked for every app, whatever its scheduler, because the +// app.json deploy-task path builds its ephemeral container with docker +// directly. A k3s app's volumes are PersistentVolumeClaims mounted by its own +// scheduler and there is nothing to bind-mount here. func dockerVFlagsForProcess(appName string, phase string, processType string) ([]string, error) { + if appSchedulerFor(appName) != SchedulerDockerLocal { + return nil, nil + } + attachments, err := AttachmentsForPhase(appName, phase) if err != nil { return nil, err @@ -291,6 +301,10 @@ func dockerVFlagsForProcess(appName string, phase string, processType string) ([ return flags, nil } +// appSchedulerFor resolves the scheduler an app deploys with. It is a variable +// so tests can exercise the non-docker-local path without a plugn install. +var appSchedulerFor = common.GetAppScheduler + // buildDockerVFlag formats the Docker -v argument for a docker-local // attachment. func buildDockerVFlag(entry *Entry, attachment *Attachment) string { diff --git a/plugins/storage/triggers_test.go b/plugins/storage/triggers_test.go index 41843bec9..29b4b311e 100644 --- a/plugins/storage/triggers_test.go +++ b/plugins/storage/triggers_test.go @@ -109,6 +109,38 @@ func TestTriggerDockerArgsRejectsSchedulerMismatchOutOfScope(t *testing.T) { Expect(err).To(HaveOccurred()) } +// TestDockerVFlagsForProcessSkipsOtherSchedulers covers apps that do not +// deploy with docker-local. The docker-args triggers are fired for every app - +// the app.json deploy-task path builds its ephemeral container with docker +// whatever the app's scheduler is - so a k3s app reaches this code with k3s +// entries attached. Those are PersistentVolumeClaims its own scheduler mounts, +// so the right answer is no flags, not an error. +func TestDockerVFlagsForProcessSkipsOtherSchedulers(t *testing.T) { + RegisterTestingT(t) + root := withTempLibRoot(t) + + previous := appSchedulerFor + appSchedulerFor = func(string) string { return SchedulerK3s } + t.Cleanup(func() { appSchedulerFor = previous }) + + Expect(SaveEntry(&Entry{ + Name: "demo-pvc", + Scheduler: SchedulerK3s, + })).To(Succeed()) + + writeAttachmentsFile(t, root, "demo", []*Attachment{ + { + EntryName: "demo-pvc", + ContainerPath: "/data", + Phases: []string{PhaseDeploy}, + }, + }) + + flags, err := dockerVFlagsForProcess("demo", PhaseDeploy, "web") + Expect(err).NotTo(HaveOccurred()) + Expect(flags).To(BeEmpty()) +} + // 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