drivers/docker: add support for STOPSIGNAL

This fixes a bug where Nomad overrides a Dockerfile's STOPSIGNAL with
the default kill_signal (SIGTERM).

This adds a check for kill_signal. If it's not set, it calls
StopContainer instead of Signal, which uses STOPSIGNAL if it's
specified. If both kill_signal and STOPSIGNAL are set, Nomad tries to
stop the container with kill_signal first, before then calling
StopContainer.

Fixes #9989

drivers/docker/driver.go

@@ -1375,40 +1375,13 @@ func (d *Driver) handleWait(ctx context.Context, ch chan *drivers.ExitResult, h
 	}
 }
 
-// parseSignal interprets the signal name into an os.Signal. If no name is
-// provided, the docker driver defaults to SIGTERM. If the OS is Windows and
-// SIGINT is provided, the signal is converted to SIGTERM.
-func (d *Driver) parseSignal(os, signal string) (os.Signal, error) {
-	// Unlike other drivers, docker defaults to SIGTERM, aiming for consistency
-	// with the 'docker stop' command.
-	// https://docs.docker.com/engine/reference/commandline/stop/#extended-description
-	if signal == "" {
-		signal = "SIGTERM"
-	}
-
-	// Windows Docker daemon does not support SIGINT, SIGTERM is the semantic equivalent that
-	// allows for graceful shutdown before being followed up by a SIGKILL.
-	// Supported signals:
-	//   https://github.com/moby/moby/blob/0111ee70874a4947d93f64b672f66a2a35071ee2/pkg/signal/signal_windows.go#L17-L26
-	if os == "windows" && signal == "SIGINT" {
-		signal = "SIGTERM"
-	}
-
-	return signals.Parse(signal)
-}
-
 func (d *Driver) StopTask(taskID string, timeout time.Duration, signal string) error {
 	h, ok := d.tasks.Get(taskID)
 	if !ok {
 		return drivers.ErrTaskNotFound
 	}
 
-	sig, err := d.parseSignal(runtime.GOOS, signal)
-	if err != nil {
-		return fmt.Errorf("failed to parse signal: %v", err)
-	}
-
-	return h.Kill(timeout, sig)
+	return h.Kill(timeout, signal)
 }
 
 func (d *Driver) DestroyTask(taskID string, force bool) error {

drivers/docker/driver_test.go

@@ -2911,28 +2911,154 @@ func TestDockerDriver_memoryLimits(t *testing.T) {
 func TestDockerDriver_parseSignal(t *testing.T) {
 	t.Parallel()
 
-	d := new(Driver)
+	tests := []struct {
+		name            string
+		runtime         string
+		specifiedSignal string
+		expectedSignal  string
+	}{
+		{
+			name:            "default",
+			runtime:         runtime.GOOS,
+			specifiedSignal: "",
+			expectedSignal:  "SIGTERM",
+		},
+		{
+			name:            "set",
+			runtime:         runtime.GOOS,
+			specifiedSignal: "SIGHUP",
+			expectedSignal:  "SIGHUP",
+		},
+		{
+			name:            "windows conversion",
+			runtime:         "windows",
+			specifiedSignal: "SIGINT",
+			expectedSignal:  "SIGTERM",
+		},
+		{
+			name:            "not signal",
+			runtime:         runtime.GOOS,
+			specifiedSignal: "SIGDOESNOTEXIST",
+			expectedSignal:  "", // throws error
+		},
+	}
 
-	t.Run("default", func(t *testing.T) {
-		s, err := d.parseSignal(runtime.GOOS, "")
-		require.NoError(t, err)
-		require.Equal(t, syscall.SIGTERM, s)
-	})
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			s, err := parseSignal(tc.runtime, tc.specifiedSignal)
+			if tc.expectedSignal == "" {
+				require.Error(t, err, "invalid signal")
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, s.(syscall.Signal), s)
+			}
+		})
+	}
+}
 
-	t.Run("set", func(t *testing.T) {
-		s, err := d.parseSignal(runtime.GOOS, "SIGHUP")
-		require.NoError(t, err)
-		require.Equal(t, syscall.SIGHUP, s)
-	})
+// This test asserts that Nomad isn't overriding the STOPSIGNAL in a Dockerfile
+func TestDockerDriver_StopSignal(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Skipped on windows, we don't have image variants available")
+	}
 
-	t.Run("windows conversion", func(t *testing.T) {
-		s, err := d.parseSignal("windows", "SIGINT")
-		require.NoError(t, err)
-		require.Equal(t, syscall.SIGTERM, s)
-	})
+	testutil.DockerCompatible(t)
+	cases := []struct {
+		name            string
+		variant         string
+		jobKillSignal   string
+		expectedSignals []string
+	}{
+		{
+			name:            "stopsignal-only",
+			variant:         "stopsignal",
+			jobKillSignal:   "",
+			expectedSignals: []string{"19", "9"},
+		},
+		{
+			name:            "stopsignal-killsignal",
+			variant:         "stopsignal",
+			jobKillSignal:   "SIGTERM",
+			expectedSignals: []string{"15", "19", "9"},
+		},
+		{
+			name:            "killsignal-only",
+			variant:         "",
+			jobKillSignal:   "SIGTERM",
+			expectedSignals: []string{"15", "15", "9"},
+		},
+		{
+			name:            "nosignals-default",
+			variant:         "",
+			jobKillSignal:   "",
+			expectedSignals: []string{"15", "9"},
+		},
+	}
 
-	t.Run("not a signal", func(t *testing.T) {
-		_, err := d.parseSignal(runtime.GOOS, "SIGDOESNOTEXIST")
-		require.Error(t, err)
-	})
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			taskCfg := newTaskConfig(c.variant, []string{"sleep", "9901"})
+
+			task := &drivers.TaskConfig{
+				ID:        uuid.Generate(),
+				Name:      c.name,
+				AllocID:   uuid.Generate(),
+				Resources: basicResources,
+			}
+			require.NoError(t, task.EncodeConcreteDriverConfig(&taskCfg))
+
+			d := dockerDriverHarness(t, nil)
+			cleanup := d.MkAllocDir(task, true)
+			defer cleanup()
+
+			if c.variant == "stopsignal" {
+				copyImage(t, task.TaskDir(), "busybox_stopsignal.tar") // Default busybox image with STOPSIGNAL 19 added
+			} else {
+				copyImage(t, task.TaskDir(), "busybox.tar")
+			}
+
+			client := newTestDockerClient(t)
+
+			listener := make(chan *docker.APIEvents)
+			err := client.AddEventListener(listener)
+			require.NoError(t, err)
+
+			defer func() {
+				err := client.RemoveEventListener(listener)
+				require.NoError(t, err)
+			}()
+
+			_, _, err = d.StartTask(task)
+			require.NoError(t, err)
+			require.NoError(t, d.WaitUntilStarted(task.ID, 5*time.Second))
+
+			go func() {
+				err := d.StopTask(task.ID, 1*time.Second, c.jobKillSignal)
+				if err != nil {
+					t.Errorf("stop task failed: %v", err)
+				}
+			}()
+
+			timeout := time.After(10 * time.Second)
+			var receivedSignals []string
+		WAIT:
+			for {
+				select {
+				case msg := <-listener:
+					// Only add kill signals
+					if msg.Action == "kill" {
+						sig := msg.Actor.Attributes["signal"]
+						receivedSignals = append(receivedSignals, sig)
+
+						if reflect.DeepEqual(receivedSignals, c.expectedSignals) {
+							break WAIT
+						}
+					}
+				case <-timeout:
+					// timeout waiting for signals
+					require.Equal(t, c.expectedSignals, receivedSignals, "timed out waiting for expected signals")
+				}
+			}
+		})
+	}
 }

drivers/docker/handle.go

@@ -3,13 +3,15 @@ package docker
 import (
 	"fmt"
 	"os"
+	"runtime"
 	"strings"
 	"sync"
 	"syscall"
 	"time"
 
 	"github.com/armon/circbuf"
 	docker "github.com/fsouza/go-dockerclient"
+	"github.com/hashicorp/consul-template/signals"
 	hclog "github.com/hashicorp/go-hclog"
 	plugin "github.com/hashicorp/go-plugin"
 	"github.com/hashicorp/nomad/drivers/docker/docklog"
@@ -121,17 +123,49 @@ func (h *taskHandle) Signal(ctx context.Context, s os.Signal) error {
 		Context: ctx,
 	}
 	return h.client.KillContainer(opts)
+}
+
+// parseSignal interprets the signal name into an os.Signal. If no name is
+// provided, the docker driver defaults to SIGTERM. If the OS is Windows and
+// SIGINT is provided, the signal is converted to SIGTERM.
+func parseSignal(os, signal string) (os.Signal, error) {
+	// Unlike other drivers, docker defaults to SIGTERM, aiming for consistency
+	// with the 'docker stop' command.
+	// https://docs.docker.com/engine/reference/commandline/stop/#extended-description
+	if signal == "" {
+		signal = "SIGTERM"
+	}
+
+	// Windows Docker daemon does not support SIGINT, SIGTERM is the semantic equivalent that
+	// allows for graceful shutdown before being followed up by a SIGKILL.
+	// Supported signals:
+	//   https://github.com/moby/moby/blob/0111ee70874a4947d93f64b672f66a2a35071ee2/pkg/signal/signal_windows.go#L17-L26
+	if os == "windows" && signal == "SIGINT" {
+		signal = "SIGTERM"
+	}
 
+	return signals.Parse(signal)
 }
 
 // Kill is used to terminate the task.
-func (h *taskHandle) Kill(killTimeout time.Duration, signal os.Signal) error {
-	// Only send signal if killTimeout is set, otherwise stop container
-	if killTimeout > 0 {
+func (h *taskHandle) Kill(killTimeout time.Duration, signal string) error {
+	var err error
+	// Calling StopContainer lets docker handle the stop signal (specified
+	// in the Dockerfile or defaulting to SIGTERM). If kill_signal is specified,
+	// Signal is used to kill the container with the desired signal before
+	// calling StopContainer
+	if signal == "" {
+		err = h.client.StopContainer(h.containerID, uint(killTimeout.Seconds()))
+	} else {
 		ctx, cancel := context.WithTimeout(context.Background(), killTimeout)
 		defer cancel()
 
-		if err := h.Signal(ctx, signal); err != nil {
+		sig, parseErr := parseSignal(runtime.GOOS, signal)
+		if parseErr != nil {
+			return fmt.Errorf("failed to parse signal: %v", parseErr)
+		}
+
+		if err := h.Signal(ctx, sig); err != nil {
 			// Container has already been removed.
 			if strings.Contains(err.Error(), NoSuchContainerError) {
 				h.logger.Debug("attempted to signal nonexistent container")
@@ -152,12 +186,12 @@ func (h *taskHandle) Kill(killTimeout time.Duration, signal os.Signal) error {
 			return nil
 		case <-ctx.Done():
 		}
+
+		// Stop the container
+		err = h.client.StopContainer(h.containerID, 0)
 	}
 
-	// Stop the container
-	err := h.client.StopContainer(h.containerID, 0)
 	if err != nil {
-
 		// Container has already been removed.
 		if strings.Contains(err.Error(), NoSuchContainerError) {
 			h.logger.Debug("attempted to stop nonexistent container")

drivers/docker/test-resources/docker/gen_stopsignal.sh

@@ -0,0 +1,9 @@
+#!/bin/sh
+
+# Create the tarball used in TestDockerDriver_StopSignal
+cat <<'EOF' | docker build -t busybox:1.29.3-stopsignal -
+FROM busybox:1.29.3
+STOPSIGNAL 19
+EOF
+
+docker save busybox:1.29.3-stopsignal > busybox_stopsignal.tar