From a1230801e768b96d608503b7b13db3cc98670f37 Mon Sep 17 00:00:00 2001
From: Seth Hoenig <shoenig@hashicorp.com>
Date: Wed, 30 Sep 2020 11:36:26 -0500
Subject: [PATCH] docs: document docker signal fix, add tests

This PR adds a version specific upgrade note about the docker stop
signal behavior. Also adds test for the signal logic in docker driver.

Closes #8932 which was fixed in #8933
---
 CHANGELOG.md                                  |  3 ++
 drivers/docker/driver.go                      | 28 +++++++++++------
 drivers/docker/driver_test.go                 | 30 +++++++++++++++++++
 .../pages/docs/upgrade/upgrade-specific.mdx   |  9 ++++++
 4 files changed, 61 insertions(+), 9 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2d7d826002af..8ff8205d090a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,9 @@ IMPROVEMENTS:
  * api: Added support for cancellation contexts to HTTP API. [[GH-8836](https://github.com/hashicorp/nomad/issues/8836)]
  * driver/docker: upgrade pause container and detect architecture [[GH-8957](https://github.com/hashicorp/nomad/pull/8957)]
 
+__BACKWARDS INCOMPATIBILITIES:__
+ * driver/docker: Tasks are now issued SIGTERM instead of SIGINT when stopping [[GH-8932](https://github.com/hashicorp/nomad/issues/8932)]
+
 BUG FIXES:
 
  * core: Fixed a bug where blocking queries would not include the query's maximum wait time when calculating whether it was safe to retry. [[GH-8921](https://github.com/hashicorp/nomad/issues/8921)]
diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go
index 1823e74caedb..d4fd72a2af5d 100644
--- a/drivers/docker/driver.go
+++ b/drivers/docker/driver.go
@@ -1292,12 +1292,13 @@ func (d *Driver) handleWait(ctx context.Context, ch chan *drivers.ExitResult, h
 	}
 }
 
-func (d *Driver) StopTask(taskID string, timeout time.Duration, signal string) error {
-	h, ok := d.tasks.Get(taskID)
-	if !ok {
-		return drivers.ErrTaskNotFound
-	}
-
+// 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"
 	}
@@ -1306,11 +1307,20 @@ func (d *Driver) StopTask(taskID string, timeout time.Duration, signal string) e
 	// 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 runtime.GOOS == "windows" && signal == "SIGINT" {
+	if os == "windows" && signal == "SIGINT" {
 		signal = "SIGTERM"
 	}
 
-	sig, err := signals.Parse(signal)
+	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)
 	}
@@ -1553,7 +1563,7 @@ func (d *Driver) dockerClients() (*docker.Client, *docker.Client, error) {
 
 	var err error
 
-	// Onlt initialize the client if it hasn't yet been done
+	// Only initialize the client if it hasn't yet been done
 	if client == nil {
 		client, err = d.newDockerClient(dockerTimeout)
 		if err != nil {
diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go
index b12519fe5e8c..40e178e2dfce 100644
--- a/drivers/docker/driver_test.go
+++ b/drivers/docker/driver_test.go
@@ -10,6 +10,7 @@ import (
 	"runtime"
 	"runtime/debug"
 	"strings"
+	"syscall"
 	"testing"
 	"time"
 
@@ -2662,3 +2663,32 @@ func TestDockerDriver_memoryLimits(t *testing.T) {
 		require.Equal(t, int64(256*1024*1024), memoryReservation)
 	})
 }
+
+func TestDockerDriver_parseSignal(t *testing.T) {
+	t.Parallel()
+
+	d := new(Driver)
+
+	t.Run("default", func(t *testing.T) {
+		s, err := d.parseSignal(runtime.GOOS, "")
+		require.NoError(t, err)
+		require.Equal(t, syscall.SIGTERM, 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)
+	})
+
+	t.Run("windows conversion", func(t *testing.T) {
+		s, err := d.parseSignal("windows", "SIGINT")
+		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)
+	})
+}
diff --git a/website/pages/docs/upgrade/upgrade-specific.mdx b/website/pages/docs/upgrade/upgrade-specific.mdx
index 7ab2c2cdf5ab..b18143a19168 100644
--- a/website/pages/docs/upgrade/upgrade-specific.mdx
+++ b/website/pages/docs/upgrade/upgrade-specific.mdx
@@ -15,6 +15,15 @@ details provided for their upgrades as a result of new features or changed
 behavior. This page is used to document those details separately from the
 standard upgrade flow.
 
+## Nomad 0.13.0
+
+### Signal used when stopping Docker tasks
+
+When stopping tasks running with the Docker task driver, Nomad documents that a
+`SIGTERM` will be issued (unless configured with `kill_signal`). However, recent
+versions of Nomad would issue `SIGINT` instead. Starting again with Nomad v0.13.0
+`SIGTERM` will be sent by default when stopping Docker tasks.
+
 ## Nomad 0.12.0
 
 ### `mbits` and Task Network Resource deprecation