From 3bb21b6987318c9fb88b3fb351827bb89e24dd3d Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Sat, 30 May 2026 03:23:32 -0400 Subject: [PATCH] feat: make storage:mount upsert the existing attachment Re-running `storage:mount --container-dir ` against an existing `(entry, container_dir, process_type)` tuple now updates the attachment's mount-time fields in place instead of erroring with `already mounted`. Declarative tooling that wants to change `--volume-options`, `--volume-chown`, `--phase`, `--volume-subpath`, or `--volume-readonly` on an existing mount no longer has to unmount-then-remount, which briefly dropped the volume from `storage:report` and raced against any deploy that fired in the window. The legacy `host:container[:opts]` form keeps its strict `Mount path already exists.` failure. --- docs/advanced-usage/persistent-storage.md | 2 + plugins/storage/attachment.go | 37 +++++++ plugins/storage/attachment_test.go | 126 ++++++++++++++++++++++ plugins/storage/subcommands.go | 16 ++- tests/unit/storage.bats | 77 +++++++++++++ 5 files changed, 257 insertions(+), 1 deletion(-) diff --git a/docs/advanced-usage/persistent-storage.md b/docs/advanced-usage/persistent-storage.md index f7eecb808..742872ecc 100644 --- a/docs/advanced-usage/persistent-storage.md +++ b/docs/advanced-usage/persistent-storage.md @@ -141,6 +141,8 @@ dokku storage:mount node-js-app node-js-data --container-dir /app/storage --volu When combined with `--volume-readonly`, the rendered options become `ro,` - for example, `--volume-options noexec,nosuid --volume-readonly` renders as `:ro,noexec,nosuid`. +Re-running `storage:mount` against a named entry with the same `--container-dir` and `--process-type` updates the existing attachment's mount-time attributes (`--phase`, `--volume-subpath`, `--volume-readonly`, `--volume-chown`, `--volume-options`) in place rather than appending a duplicate. This is the idempotent equivalent of `storage:set` for entries, and lets declarative tooling change a mount-time attribute without an unmount-then-remount dance that would briefly drop the volume from `storage:report`. Mount-time fields are rewritten wholesale, not merged - omitting a flag on a re-mount clears any previously-set value. The legacy `host:container[:opts]` form still rejects duplicates with `Mount path already exists.`. + Once persistent storage is mounted, the app requires a restart. See the [process scaling documentation](/docs/processes/process-management.md) for more information. ```shell diff --git a/plugins/storage/attachment.go b/plugins/storage/attachment.go index f43070fab..8b02fcec4 100644 --- a/plugins/storage/attachment.go +++ b/plugins/storage/attachment.go @@ -126,6 +126,43 @@ func AddAttachment(appName string, attachment *Attachment) error { return SaveAttachments(appName, attachments) } +// UpsertAttachment inserts an attachment, or updates the mount-time fields +// on an existing one matching the same (entry_name, container_path, +// process_type) tuple. Returns created=true when a new attachment was +// appended, created=false when an existing one was rewritten. +// +// Mount-time fields (Phases, Subpath, Readonly, VolumeOptions, VolumeChown) +// are overwritten wholesale, not merged: passing an empty Subpath or a +// 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. +func UpsertAttachment(appName string, attachment *Attachment) (bool, error) { + if err := attachment.Validate(); err != nil { + return false, err + } + + attachments, err := LoadAttachments(appName) + if err != nil { + return false, err + } + + for _, existing := range attachments { + if existing.EntryName == attachment.EntryName && + existing.ContainerPath == attachment.ContainerPath && + existing.ProcessType == attachment.ProcessType { + existing.Phases = attachment.Phases + existing.Subpath = attachment.Subpath + existing.Readonly = attachment.Readonly + existing.VolumeOptions = attachment.VolumeOptions + existing.VolumeChown = attachment.VolumeChown + return false, SaveAttachments(appName, attachments) + } + } + + attachments = append(attachments, attachment) + return true, SaveAttachments(appName, attachments) +} + // RemoveAttachment removes the attachment matching the given entry and // optional container path. If containerPath is empty and there is more // than one match, returns an error. diff --git a/plugins/storage/attachment_test.go b/plugins/storage/attachment_test.go index 63cd9889d..d43f7ec53 100644 --- a/plugins/storage/attachment_test.go +++ b/plugins/storage/attachment_test.go @@ -217,3 +217,129 @@ func TestListAppMountEntriesPhaseFilter(t *testing.T) { Expect(runRows).To(HaveLen(1)) Expect(runRows[0].EntryName).To(Equal("demo-run-only")) } + +func TestUpsertAttachmentInsert(t *testing.T) { + RegisterTestingT(t) + withTempLibRoot(t) + + created, err := UpsertAttachment("demo", &Attachment{ + EntryName: "demo-data", + ContainerPath: "/data", + Phases: []string{PhaseDeploy, PhaseRun}, + ProcessType: DefaultProcessType, + VolumeOptions: "Z", + }) + Expect(err).NotTo(HaveOccurred()) + Expect(created).To(BeTrue()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].VolumeOptions).To(Equal("Z")) +} + +func TestUpsertAttachmentUpdatesInPlace(t *testing.T) { + RegisterTestingT(t) + withTempLibRoot(t) + + base := &Attachment{ + EntryName: "demo-data", + ContainerPath: "/data", + Phases: []string{PhaseDeploy, PhaseRun}, + ProcessType: DefaultProcessType, + VolumeOptions: "Z", + Subpath: "uploads", + Readonly: false, + VolumeChown: "herokuish", + } + created, err := UpsertAttachment("demo", base) + Expect(err).NotTo(HaveOccurred()) + Expect(created).To(BeTrue()) + + // Same identity, new mount-time fields - update in place. + created, err = UpsertAttachment("demo", &Attachment{ + EntryName: "demo-data", + ContainerPath: "/data", + Phases: []string{PhaseDeploy, PhaseRun}, + ProcessType: DefaultProcessType, + VolumeOptions: "noexec,nosuid", + Readonly: true, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(created).To(BeFalse()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].VolumeOptions).To(Equal("noexec,nosuid")) + Expect(attachments[0].Readonly).To(BeTrue()) + // Wholesale rewrite: Subpath and VolumeChown cleared since the second + // call didn't supply them. + Expect(attachments[0].Subpath).To(BeEmpty()) + Expect(attachments[0].VolumeChown).To(BeEmpty()) +} + +func TestUpsertAttachmentDistinctIdentity(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()) + + // Different container path on the same entry is a distinct attachment. + created, err = UpsertAttachment("demo", &Attachment{ + EntryName: "demo-data", + ContainerPath: "/other", + Phases: []string{PhaseDeploy}, + ProcessType: DefaultProcessType, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(created).To(BeTrue()) + + // Different process type on the same (entry, container_path) is also distinct. + created, err = UpsertAttachment("demo", &Attachment{ + EntryName: "demo-data", + ContainerPath: "/data", + Phases: []string{PhaseDeploy}, + ProcessType: "web", + }) + Expect(err).NotTo(HaveOccurred()) + Expect(created).To(BeTrue()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(3)) +} + +func TestUpsertAttachmentValidationFailureLeavesStoreUntouched(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()) + + // Invalid attachment (empty ContainerPath) fails validation and the + // existing list stays exactly as it was. + _, err = UpsertAttachment("demo", &Attachment{ + EntryName: "demo-data", + Phases: []string{PhaseDeploy}, + }) + Expect(err).To(HaveOccurred()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].ContainerPath).To(Equal("/data")) +} diff --git a/plugins/storage/subcommands.go b/plugins/storage/subcommands.go index 9001cd1fc..3b264834c 100644 --- a/plugins/storage/subcommands.go +++ b/plugins/storage/subcommands.go @@ -193,7 +193,21 @@ func CommandMount(input CommandMountInput) error { VolumeChown: input.VolumeChown, } - return AddAttachment(input.AppName, attachment) + // Idempotent contract: same (entry, container_dir, process_type) + // tuple updates the mount-time fields in place rather than appending + // a duplicate attachment. Lets declarative tooling change + // --volume-options / --volume-chown / --phase / etc. without an + // unmount-then-remount dance that briefly drops the volume. + created, err := UpsertAttachment(input.AppName, attachment) + if err != nil { + return err + } + if created { + common.LogInfo1(fmt.Sprintf("Storage entry %s mounted at %s on %s", entry.Name, input.ContainerDir, input.AppName)) + } else { + common.LogInfo1(fmt.Sprintf("Storage entry %s mount at %s on %s updated", entry.Name, input.ContainerDir, input.AppName)) + } + return nil } // CommandUnmountInput captures the flags accepted by storage:unmount when diff --git a/tests/unit/storage.bats b/tests/unit/storage.bats index 7e78f5848..a8bbc02be 100644 --- a/tests/unit/storage.bats +++ b/tests/unit/storage.bats @@ -417,6 +417,83 @@ teardown() { assert_success } +@test "(storage:mount) re-running against an existing (entry, container-dir) updates in place" { + run /bin/bash -c "dokku storage:create rdmtest-upsert" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-upsert --container-dir /data --volume-options Z" + echo "output: $output" + echo "status: $status" + assert_success + + # Re-mount the same (entry, container-dir) with a different --volume-options. + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-upsert --container-dir /data --volume-options noexec,nosuid" + echo "output: $output" + echo "status: $status" + assert_success + + # Single attachment, not two. + 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" + + # Latest --volume-options wins. + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].volume_options'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "noexec,nosuid" + + # Re-mount with --volume-readonly added; readonly toggles on. + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-upsert --container-dir /data --volume-options noexec,nosuid --volume-readonly" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].readonly'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "true" + + # Re-mount without --volume-readonly: mount-time fields are rewritten, + # not merged, so the readonly key disappears from the JSON output. + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-upsert --container-dir /data --volume-options noexec,nosuid" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[] | has(\"readonly\")'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "false" + + # A different --container-dir on the same entry is still a distinct attachment. + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-upsert --container-dir /other" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '. | length'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "2" + + # cleanup + run /bin/bash -c "dokku storage:unmount $TEST_APP rdmtest-upsert --container-dir /data" + assert_success + run /bin/bash -c "dokku storage:unmount $TEST_APP rdmtest-upsert --container-dir /other" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-upsert --force" + assert_success +} + @test "(storage) storage:destroy refuses to remove a still-mounted entry" { run /bin/bash -c "dokku storage:create rdmtest-busy" assert_success