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,159 @@ 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:  "NA", // 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 == "NA" {
+				require.Error(t, err)
+			} 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")
+	}
+
+	testutil.DockerCompatible(t)
+	stopSigTaskCfg := newTaskConfig("stopsignal", []string{"sleep", "9001"})
+	defaultTaskCfg := newTaskConfig("", []string{"sleep", "9001"})
+	stopTask := &drivers.TaskConfig{
+		ID:        uuid.Generate(),
+		Name:      "stopsig-only",
+		AllocID:   uuid.Generate(),
+		Resources: basicResources,
+	}
+	require.NoError(t, stopTask.EncodeConcreteDriverConfig(&stopSigTaskCfg))
+
+	stopKillTask := &drivers.TaskConfig{
+		ID:        uuid.Generate(),
+		Name:      "stopsig-killsig",
+		AllocID:   uuid.Generate(),
+		Resources: basicResources,
+	}
+	require.NoError(t, stopKillTask.EncodeConcreteDriverConfig(&stopSigTaskCfg))
+
+	killTask := &drivers.TaskConfig{
+		ID:        uuid.Generate(),
+		Name:      "killsig-only",
+		AllocID:   uuid.Generate(),
+		Resources: basicResources,
+	}
+	require.NoError(t, killTask.EncodeConcreteDriverConfig(&defaultTaskCfg))
 
-	t.Run("windows conversion", func(t *testing.T) {
-		s, err := d.parseSignal("windows", "SIGINT")
+	defaultTask := &drivers.TaskConfig{
+		ID:        uuid.Generate(),
+		Name:      "nosigs-default",
+		AllocID:   uuid.Generate(),
+		Resources: basicResources,
+	}
+	require.NoError(t, defaultTask.EncodeConcreteDriverConfig(&defaultTaskCfg))
+
+	type taskInfo struct {
+		killSig      string
+		expectedSigs []string
+	}
+
+	taskList := map[*drivers.TaskConfig]taskInfo{
+		stopTask:     {killSig: "", expectedSigs: []string{"19", "9"}},
+		defaultTask:  {killSig: "", expectedSigs: []string{"15", "9"}},
+		stopKillTask: {killSig: "SIGTERM", expectedSigs: []string{"15", "19", "9"}},
+		killTask:     {killSig: "SIGTERM", expectedSigs: []string{"15", "15", "9"}},
+	}
+
+	d := dockerDriverHarness(t, nil)
+
+	for task := range taskList {
+		cleanup := d.MkAllocDir(task, true)
+		defer cleanup()
+
+		if task.Name == "stopsig-only" || task.Name == "stopsig-killsig" {
+			copyImage(t, task.TaskDir(), "busybox_stopsignal.tar") // Default busybox image with STOPSIGNAL 19 added
+		} else {
+			copyImage(t, task.TaskDir(), "busybox.tar")
+		}
+	}
+
+	sigList := map[string][]string{} // maps Name-AllocID to signal slices
+
+	client := newTestDockerClient(t)
+	var err error
+
+	listener := make(chan *docker.APIEvents)
+	err = client.AddEventListener(listener)
+	require.NoError(t, err)
+
+	defer func() {
+		err = client.RemoveEventListener(listener)
 		require.NoError(t, err)
-		require.Equal(t, syscall.SIGTERM, s)
-	})
+	}()
 
-	t.Run("not a signal", func(t *testing.T) {
-		_, err := d.parseSignal(runtime.GOOS, "SIGDOESNOTEXIST")
-		require.Error(t, err)
-	})
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	go func() {
+		for {
+			select {
+			case msg := <-listener:
+				// Only add kill signals
+				if msg.Action == "kill" {
+					name := msg.Actor.Attributes["name"]
+					sig := msg.Actor.Attributes["signal"]
+					sigList[name] = append(sigList[name], sig)
+				}
+			case <-ctx.Done():
+				return
+			}
+		}
+	}()
+
+	for task, info := range taskList {
+		t.Run(task.Name, func(*testing.T) {
+			_, _, err = d.StartTask(task)
+			require.NoError(t, err)
+			require.NoError(t, d.WaitUntilStarted(task.ID, 5*time.Second))
+			err = d.StopTask(task.ID, 1*time.Second, info.killSig)
+
+			actualSigs := sigList[fmt.Sprintf("%s-%s", task.Name, task.AllocID)]
+			require.Equal(t, info.expectedSigs, actualSigs)
+		})
+	}
 }

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