-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
drivers/docker: add support for STOPSIGNAL #10441
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great test. Captures all conditions :). |
||
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") | ||
} | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Good explanatory comment for summarizing the domain-specific knowledge you'd need to understand the code here. |
||
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") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that Golang has a convention about test files: If the tested function is in
handle.go
, the tests belong inhandle_test.go
. It's odd thathandle_test.go
. Seeing how the two files are intertwined, it's fine to leave the test here.