From e9a53ac82dcab8b0e1c3f4ab7dfb22307e18c889 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 30 Apr 2026 01:10:00 -0400 Subject: [PATCH] fix: propagate storage:exec exit codes and route legacy mounts to attachments CallExecCommand wraps non-zero exit codes as errors with the code populated on the response, so storage:exec was returning the wrapped error and being collapsed to exit 1 by LogFailWithError before the os.Exit branch ran. Both storage's CommandExec and scheduler-docker-local's TriggerSchedulerStorageExec now check ExitCode before err so the underlying tool's status flows through verbatim. Separately, the legacy host:container colon form of storage:mount used to write straight into docker-options, but storage:list now reads only attachments, so newly mounted legacy-form storage was invisible. CommandMount and CommandUnmount route the colon form through LegacyMountToEntry plus AddAttachment / RemoveAttachment, making storage:list show every mount regardless of form while preserving the existing "Mount path already exists." / "Mount path does not exist." error wording. --- .../scheduler-docker-local/storage_exec.go | 10 +- plugins/storage/commands_entries.go | 11 ++- plugins/storage/subcommands.go | 93 +++++++++++++++---- 3 files changed, 91 insertions(+), 23 deletions(-) diff --git a/plugins/scheduler-docker-local/storage_exec.go b/plugins/scheduler-docker-local/storage_exec.go index 15938ba02..4db397e29 100644 --- a/plugins/scheduler-docker-local/storage_exec.go +++ b/plugins/scheduler-docker-local/storage_exec.go @@ -64,12 +64,16 @@ func TriggerSchedulerStorageExec(scheduler string, input StorageExecInput) error Args: args, StreamStdio: true, }) - if err != nil { - return err - } + // CallExecCommand wraps non-zero exit as an error; for storage:exec + // the docker-run exit code is the signal we want to forward to the + // caller. Propagate it before falling through to the err return, + // which would otherwise be collapsed to exit 1 by the dispatcher. if result.ExitCode != 0 { os.Exit(result.ExitCode) } + if err != nil { + return err + } return nil } diff --git a/plugins/storage/commands_entries.go b/plugins/storage/commands_entries.go index 71b5bb3e3..ada6f68e3 100644 --- a/plugins/storage/commands_entries.go +++ b/plugins/storage/commands_entries.go @@ -295,12 +295,17 @@ func CommandExec(input CommandExecInput) error { Args: triggerArgs, StreamStdio: true, }) - if err != nil { - return err - } + // common.CallExecCommand wraps a non-zero exit code as an error, but + // for storage:exec the underlying tool's exit code IS the signal we + // want to surface - it's how `dokku storage:exec demo -- exit 42` + // reaches the user as exit 42. Check ExitCode first so that branch + // runs before err collapses to LogFailWithError → exit 1. if results.ExitCode != 0 { os.Exit(results.ExitCode) } + if err != nil { + return err + } return nil } diff --git a/plugins/storage/subcommands.go b/plugins/storage/subcommands.go index e3d489f1f..f8b857bf7 100644 --- a/plugins/storage/subcommands.go +++ b/plugins/storage/subcommands.go @@ -9,7 +9,6 @@ import ( "strings" "github.com/dokku/dokku/plugins/common" - dockeroptions "github.com/dokku/dokku/plugins/docker-options" ) const ( @@ -151,16 +150,12 @@ func CommandMount(input CommandMountInput) error { return err } - // Legacy colon form: keep writing into docker-options so existing - // users see no behavior change. + // Legacy colon form: synthesize a legacy- entry plus an + // attachment so storage:list (now attachment-only) sees the mount. + // The storage docker-args trigger emits the corresponding -v flag at + // deploy time, so behavior at the docker-run boundary is unchanged. if strings.Contains(input.NameOrPath, ":") { - if err := VerifyPaths(input.NameOrPath); err != nil { - return err - } - if CheckIfPathExists(input.AppName, input.NameOrPath, MountPhases) { - return errors.New("Mount path already exists.") - } - return dockeroptions.AddDockerOptionToPhases(input.AppName, MountPhases, fmt.Sprintf("-v %s", input.NameOrPath)) + return mountLegacyColon(input.AppName, input.NameOrPath) } // Named-entry form: persist as an attachment. @@ -215,18 +210,82 @@ func CommandUnmount(input CommandUnmountInput) error { } if strings.Contains(input.NameOrPath, ":") { - if err := VerifyPaths(input.NameOrPath); err != nil { - return err - } - if !CheckIfPathExists(input.AppName, input.NameOrPath, MountPhases) { - return errors.New("Mount path does not exist.") - } - return dockeroptions.RemoveDockerOptionFromPhases(input.AppName, MountPhases, fmt.Sprintf("-v %s", input.NameOrPath)) + return unmountLegacyColon(input.AppName, input.NameOrPath) } return RemoveAttachment(input.AppName, input.NameOrPath, input.ContainerDir) } +// mountLegacyColon translates a `:[:options]` mount +// string into a synthesized legacy- entry plus an attachment. +// Idempotent: re-running the same mount errors with the existing +// "already mounted" message via AddAttachment's duplicate check. +func mountLegacyColon(appName string, mountPath string) error { + if err := VerifyPaths(mountPath); err != nil { + return err + } + parsed := ParseMountPath(mountPath) + if parsed.ContainerPath == "" { + return errors.New("Storage path must be two valid paths divided by colon.") + } + + entry := LegacyMountToEntry(mountPath) + if !EntryExists(entry.Name) { + if err := SaveEntry(entry); err != nil { + return err + } + } + + attachment := &Attachment{ + EntryName: entry.Name, + ContainerPath: parsed.ContainerPath, + Phases: []string{PhaseDeploy, PhaseRun}, + ProcessType: DefaultProcessType, + } + switch parsed.VolumeOptions { + case "": + case "ro": + attachment.Readonly = true + default: + attachment.VolumeOptions = parsed.VolumeOptions + } + + if err := AddAttachment(appName, attachment); err != nil { + // AddAttachment's duplicate error mentions the entry name, but + // the legacy form historically said "Mount path already + // exists." - preserve that exact wording so existing automation + // and the bats suite keep matching. + if strings.Contains(err.Error(), "is already mounted at") { + return errors.New("Mount path already exists.") + } + return err + } + return nil +} + +// unmountLegacyColon is the inverse of mountLegacyColon. The legacy +// mount string identifies an entry+container-path tuple deterministically +// via LegacyMountToEntry, so we can route to RemoveAttachment. +func unmountLegacyColon(appName string, mountPath string) error { + if err := VerifyPaths(mountPath); err != nil { + return err + } + parsed := ParseMountPath(mountPath) + if parsed.ContainerPath == "" { + return errors.New("Storage path must be two valid paths divided by colon.") + } + + entry := LegacyMountToEntry(mountPath) + if err := RemoveAttachment(appName, entry.Name, parsed.ContainerPath); err != nil { + // Match the legacy wording for "not currently mounted". + if strings.Contains(err.Error(), "is not mounted") { + return errors.New("Mount path does not exist.") + } + return err + } + return nil +} + // CommandList lists all bind mounts for an app. Reads attachments // directly from the storage plugin's own state rather than going through // the deprecated `storage-list` plugn trigger.