Merge pull request #8713 from dokku/8709-storage-mount-update-existing-attachment-in-place-instead-of-duplicating

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,<volume-options>` - 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

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.

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"))
+}

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

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