diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index ff25a2c20ae9..bf0f6d380f24 100644 --- a/drivers/docker/driver.go +++ b/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 { diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go index 27c2e5c8e685..aa0774b092d5 100644 --- a/drivers/docker/driver_test.go +++ b/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") + } + } + }) + } } diff --git a/drivers/docker/handle.go b/drivers/docker/handle.go index 679a6402cfe7..7913796324a7 100644 --- a/drivers/docker/handle.go +++ b/drivers/docker/handle.go @@ -3,6 +3,7 @@ package docker import ( "fmt" "os" + "runtime" "strings" "sync" "syscall" @@ -10,6 +11,7 @@ import ( "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") diff --git a/drivers/docker/test-resources/docker/busybox_stopsignal.tar b/drivers/docker/test-resources/docker/busybox_stopsignal.tar new file mode 100644 index 000000000000..22b3bdf18f4d Binary files /dev/null and b/drivers/docker/test-resources/docker/busybox_stopsignal.tar differ diff --git a/drivers/docker/test-resources/docker/gen_stopsignal.sh b/drivers/docker/test-resources/docker/gen_stopsignal.sh new file mode 100755 index 000000000000..1fa430bcc965 --- /dev/null +++ b/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