Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: document docker signal fix, add tests #9009

Merged
merged 1 commit into from
Oct 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ IMPROVEMENTS:
* driver/docker: Upgrade pause container and detect architecture [[GH-8957](https://github.com/hashicorp/nomad/pull/8957)]
* jobspec: Lowered minimum CPU allowed from 10 to 1. [[GH-8996](https://github.com/hashicorp/nomad/issues/8996)]

__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)]
Expand Down
28 changes: 19 additions & 9 deletions drivers/docker/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down
30 changes: 30 additions & 0 deletions drivers/docker/driver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"runtime"
"runtime/debug"
"strings"
"syscall"
"testing"
"time"

Expand Down Expand Up @@ -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)
})
}
9 changes: 9 additions & 0 deletions website/pages/docs/upgrade/upgrade-specific.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down