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..b0b41a90a00f 100644 --- a/drivers/docker/driver_test.go +++ b/drivers/docker/driver_test.go @@ -11,7 +11,6 @@ import ( "runtime/debug" "sort" "strings" - "syscall" "testing" "time" @@ -32,6 +31,7 @@ import ( tu "github.com/hashicorp/nomad/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" ) var ( @@ -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: "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, unix.SignalNum(tc.expectedSignal), 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) { + 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)) + + 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 == "stop-demo" || task.Name == "stop-kill-demo" { + 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) - t.Run("windows conversion", func(t *testing.T) { - s, err := d.parseSignal("windows", "SIGINT") + 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(), 8*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, 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) + }) + } } 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