From 17cf249f2e734730a0631348a2bae2f1f65e37c9 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 21 Sep 2026 23:35:28 -0400 Subject: [PATCH] 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