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

@@ -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>`, and the `storage` plugin implements it to emit the `-v` flags for mounts made with `storage:mount --process-type <PROC>`. `$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 <PROC>`, and the `storage` plugin implements it to emit the `-v` flags for mounts made with `storage:mount --process-type <PROC>`. `$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:

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 {
+	if err != nil {
-		words, err := shellquote.Split(results.StdoutContents())
+		return fmt.Errorf("Error getting deploy docker args for %s task: %w", phase, err)
-		if err != nil {
-			return err
-		}
-
-		dockerArgs = append(dockerArgs, words...)
 	}
+	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 {
+	if err != nil {
-		words, err := shellquote.Split(results.StdoutContents())
+		return fmt.Errorf("Error getting process deploy docker args for %s task: %w", phase, err)
-		if err != nil {
-			return err
-		}
-
-		dockerArgs = append(dockerArgs, words...)
 	}
+	words, err = shellquote.Split(results.StdoutContents())
+	if err != nil {
+		return err
+	}
+	dockerArgs = append(dockerArgs, words...)
 
 	filteredArgs := []string{
 		"--cpus",

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 {

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 {

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