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

Allow controlling the stop signal for drivers #3591

Merged
merged 14 commits into from
Dec 7, 2017
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ IMPROVEMENTS:
* driver/docker: Adds support for `ulimit` and `sysctl` options [GH-3568]
* driver/docker: Adds support for StopTimeout (set to the same value as
kill_timeout [GH-3601]
* driver/exec: allow controlling the stop signal in exec/raw_exec [GH-1755]
* driver/rkt: Add support for passing through user [GH-3612]
* driver/qemu: Support graceful shutdowns on unix platforms [GH-3411]
* template: Updated to consul template 0.19.4 [GH-3543]
Expand Down
1 change: 1 addition & 0 deletions api/tasks.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ type Task struct {
DispatchPayload *DispatchPayloadConfig
Leader bool
ShutdownDelay time.Duration `mapstructure:"shutdown_delay"`
KillSignal string `mapstructure:"kill_signal"`
}

func (t *Task) Canonicalize(tg *TaskGroup, job *Job) {
Expand Down
7 changes: 6 additions & 1 deletion client/driver/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ type DockerVolumeDriverConfig struct {
Options []map[string]string `mapstructure:"options"`
}

// DockerDriverConfig defines the user specified config block in a jobspec
type DockerDriverConfig struct {
ImageName string `mapstructure:"image"` // Container's Image Name
LoadImage string `mapstructure:"load"` // LoadImage is a path to an image archive file
Expand Down Expand Up @@ -712,7 +713,6 @@ func (d *DockerDriver) Prestart(ctx *ExecContext, task *structs.Task) (*Prestart
}

func (d *DockerDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, error) {

pluginLogFile := filepath.Join(ctx.TaskDir.Dir, "executor.out")
executorConfig := &dstructs.ExecutorConfig{
LogFile: pluginLogFile,
Expand Down Expand Up @@ -1046,6 +1046,7 @@ func (d *DockerDriver) createContainerConfig(ctx *ExecContext, task *structs.Tas
return c, err
}

// create the config block that will later be consumed by go-dockerclient
config := &docker.Config{
Image: d.imageID,
Hostname: driverConfig.Hostname,
Expand All @@ -1055,6 +1056,10 @@ func (d *DockerDriver) createContainerConfig(ctx *ExecContext, task *structs.Tas
StopTimeout: int(task.KillTimeout.Seconds()),
}

if task.KillSignal != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this become:

config := &docker.Config{
  ...
  StopTimeout: int(task.KillTimeout.Seconds()),
  StopSignal: task.KillSignal,
}

config.StopSignal = task.KillSignal
}

if driverConfig.WorkDir != "" {
config.WorkingDir = driverConfig.WorkDir
}
Expand Down
47 changes: 47 additions & 0 deletions client/driver/docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2045,3 +2045,50 @@ func TestDockerDriver_Device_Success(t *testing.T) {
assert.Equal(t, expectedDevice, container.HostConfig.Devices[0], "Incorrect device ")

}

func TestDockerDriver_Kill(t *testing.T) {
assert := assert.New(t)
if !tu.IsTravis() {
t.Parallel()
}
if !testutil.DockerIsConnected(t) {
t.Skip("Docker not connected")
}

// Tasks started with a signal that is not supported should not error
task := &structs.Task{
Name: "nc-demo",
Driver: "docker",
KillSignal: "SIGKILL",
Config: map[string]interface{}{
"load": "busybox.tar",
"image": "busybox",
"command": "/bin/nc",
"args": []string{"-l", "127.0.0.1", "-p", "0"},
},
LogConfig: &structs.LogConfig{
MaxFiles: 10,
MaxFileSizeMB: 10,
},
Resources: basicResources,
}

ctx := testDockerDriverContexts(t, task)
defer ctx.AllocDir.Destroy()
d := NewDockerDriver(ctx.DriverCtx)
copyImage(t, ctx.ExecCtx.TaskDir, "busybox.tar")

_, err := d.Prestart(ctx.ExecCtx, task)
if err != nil {
t.Fatalf("error in prestart: %v", err)
}

resp, err := d.Start(ctx.ExecCtx, task)
assert.Nil(err)
assert.NotNil(resp.Handle)

handle := resp.Handle.(*DockerHandle)
waitForExist(t, client, handle)
err = handle.Kill()
assert.Nil(err)
}
6 changes: 6 additions & 0 deletions client/driver/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,15 @@ func (d *ExecDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse
return nil, fmt.Errorf("failed to set executor context: %v", err)
}

taskKillSignal, err := getTaskKillSignal(task.KillSignal)
if err != nil {
return nil, err
}

execCmd := &executor.ExecCommand{
Cmd: command,
Args: driverConfig.Args,
TaskKillSignal: taskKillSignal,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java driver too

Copy link
Contributor Author

@chelseakomlo chelseakomlo Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 27b666e

FSIsolation: true,
ResourceLimits: true,
User: getExecutorUser(task),
Expand Down
7 changes: 6 additions & 1 deletion client/driver/executor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ type ExecCommand struct {
// Args is the args of the command that the user wants to run.
Args []string

// TaskKillSignal is an optional field which signal to kill the process
TaskKillSignal os.Signal

// FSIsolation determines whether the command would be run in a chroot.
FSIsolation bool

Expand Down Expand Up @@ -496,9 +499,11 @@ func (e *UniversalExecutor) ShutDown() error {
}
return nil
}
if err = proc.Signal(os.Interrupt); err != nil && err.Error() != finishedErr {

if err = proc.Signal(e.command.TaskKillSignal); err != nil && err.Error() != finishedErr {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you will still need to default since it isn't being set on rkt

return fmt.Errorf("executor.shutdown error: %v", err)
}

return nil
}

Expand Down
12 changes: 9 additions & 3 deletions client/driver/raw_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,16 @@ func (d *RawExecDriver) Start(ctx *ExecContext, task *structs.Task) (*StartRespo
return nil, fmt.Errorf("failed to set executor context: %v", err)
}

taskKillSignal, err := getTaskKillSignal(task.KillSignal)
if err != nil {
return nil, err
}

execCmd := &executor.ExecCommand{
Cmd: command,
Args: driverConfig.Args,
User: task.User,
Cmd: command,
Args: driverConfig.Args,
User: task.User,
TaskKillSignal: taskKillSignal,
}
ps, err := exec.LaunchCmd(execCmd)
if err != nil {
Expand Down
16 changes: 16 additions & 0 deletions client/driver/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"strings"
"time"

"github.com/hashicorp/consul-template/signals"
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/go-plugin"
"github.com/hashicorp/nomad/client/allocdir"
Expand Down Expand Up @@ -204,3 +205,18 @@ func SetEnvvars(envBuilder *env.Builder, fsi cstructs.FSIsolation, taskDir *allo
envBuilder.SetHostEnvvars(filter)
}
}

// getTaskKillSignal looks up the signal specified for the task if it has been
// specified. If it is not supported on the platform, returns an error.
func getTaskKillSignal(signal string) (os.Signal, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this same validation be used for signals to docker/other containers?

if signal == "" {
return os.Interrupt, nil
}

taskKillSignal := signals.SignalLookup[signal]
if taskKillSignal == nil {
return nil, fmt.Errorf("Signal %s is not supported", signal)
}

return taskKillSignal, nil
}
35 changes: 35 additions & 0 deletions client/driver/utils_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
package driver

import (
"os"
"runtime"
"syscall"
"testing"
"time"

"github.com/stretchr/testify/assert"
)

func TestDriver_KillTimeout(t *testing.T) {
Expand All @@ -21,3 +26,33 @@ func TestDriver_KillTimeout(t *testing.T) {
t.Fatalf("KillTimeout() returned %v; want %v", actual, expected)
}
}

func TestDriver_getTaskKillSignal(t *testing.T) {
assert := assert.New(t)
t.Parallel()

if runtime.GOOS != "linux" {
t.Skip("Linux only test")
}

// Test that the default is SIGINT
{
sig, err := getTaskKillSignal("")
assert.Nil(err)
assert.Equal(sig, os.Interrupt)
}

// Test that unsupported signals return an error
{
_, err := getTaskKillSignal("ABCDEF")
assert.NotNil(err)
assert.Contains(err.Error(), "Signal ABCDEF is not supported")
}

// Test that supported signals return that signal
{
sig, err := getTaskKillSignal("SIGKILL")
assert.Nil(err)
assert.Equal(sig, syscall.SIGKILL)
}
}
3 changes: 3 additions & 0 deletions command/agent/job_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,8 @@ func ApiTgToStructsTG(taskGroup *api.TaskGroup, tg *structs.TaskGroup) {
}
}

// ApiTaskToStructsTask is a copy and type conversion between the API
// representation of a task from a struct representation of a task.
func ApiTaskToStructsTask(apiTask *api.Task, structsTask *structs.Task) {
structsTask.Name = apiTask.Name
structsTask.Driver = apiTask.Driver
Expand All @@ -676,6 +678,7 @@ func ApiTaskToStructsTask(apiTask *api.Task, structsTask *structs.Task) {
structsTask.Meta = apiTask.Meta
structsTask.KillTimeout = *apiTask.KillTimeout
structsTask.ShutdownDelay = apiTask.ShutdownDelay
structsTask.KillSignal = apiTask.KillSignal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to test


if l := len(apiTask.Constraints); l != 0 {
structsTask.Constraints = make([]*structs.Constraint, l)
Expand Down
2 changes: 2 additions & 0 deletions command/agent/job_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1260,6 +1260,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
"lol": "code",
},
KillTimeout: helper.TimeToPtr(10 * time.Second),
KillSignal: "SIGQUIT",
LogConfig: &api.LogConfig{
MaxFiles: helper.IntToPtr(10),
MaxFileSizeMB: helper.IntToPtr(100),
Expand Down Expand Up @@ -1455,6 +1456,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
"lol": "code",
},
KillTimeout: 10 * time.Second,
KillSignal: "SIGQUIT",
LogConfig: &structs.LogConfig{
MaxFiles: 10,
MaxFileSizeMB: 100,
Expand Down
2 changes: 2 additions & 0 deletions jobspec/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,7 @@ func parseTasks(jobName string, taskGroupName string, result *[]*api.Task, list
"template",
"user",
"vault",
"kill_signal",
}
if err := helper.CheckHCLKeys(listVal, valid); err != nil {
return multierror.Prefix(err, fmt.Sprintf("'%s' ->", n))
Expand Down Expand Up @@ -623,6 +624,7 @@ func parseTasks(jobName string, taskGroupName string, result *[]*api.Task, list
WeaklyTypedInput: true,
Result: &t,
})

if err != nil {
return err
}
Expand Down
26 changes: 25 additions & 1 deletion jobspec/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ func TestParse(t *testing.T) {
RightDelim: helper.StringToPtr("__"),
},
},
Leader: true,
Leader: true,
KillSignal: "",
},
{
Name: "storagelocker",
Expand Down Expand Up @@ -559,6 +560,29 @@ func TestParse(t *testing.T) {
},
false,
},
{
"job-with-kill-signal.hcl",
&api.Job{
ID: helper.StringToPtr("foo"),
Name: helper.StringToPtr("foo"),
TaskGroups: []*api.TaskGroup{
{
Name: helper.StringToPtr("bar"),
Tasks: []*api.Task{
{
Name: "bar",
Driver: "docker",
KillSignal: "SIGQUIT",
Config: map[string]interface{}{
"image": "hashicorp/image",
},
},
},
},
},
},
false,
},
}

for _, tc := range cases {
Expand Down
10 changes: 10 additions & 0 deletions jobspec/test-fixtures/job-with-kill-signal.hcl
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
job "foo" {
task "bar" {
driver = "docker"
kill_signal = "SIGQUIT"

config {
image = "hashicorp/image"
}
}
}
29 changes: 29 additions & 0 deletions nomad/job_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3748,6 +3748,35 @@ func TestJobEndpoint_ValidateJob_InvalidSignals(t *testing.T) {
}
}

func TestJobEndpoint_ValidateJob_KillSignal(t *testing.T) {
assert := assert.New(t)
t.Parallel()

// test validate fails if the driver does not support sending signals, but a
// stop_signal has been specified
{
job := mock.Job()
job.TaskGroups[0].Tasks[0].Driver = "qemu" // qemu does not support sending signals
job.TaskGroups[0].Tasks[0].KillSignal = "SIGINT"

err, warnings := validateJob(job)
assert.NotNil(err)
assert.True(strings.Contains(err.Error(), "support sending signals"))
assert.Nil(warnings)
}

// test validate succeeds if the driver does support sending signals, and
// a stop_signal has been specified
{
job := mock.Job()
job.TaskGroups[0].Tasks[0].KillSignal = "SIGINT"

err, warnings := validateJob(job)
assert.Nil(err)
assert.Nil(warnings)
}
}

func TestJobEndpoint_ValidateJobUpdate(t *testing.T) {
t.Parallel()
old := mock.Job()
Expand Down
9 changes: 9 additions & 0 deletions nomad/structs/structs.go
Original file line number Diff line number Diff line change
Expand Up @@ -1978,6 +1978,11 @@ func (j *Job) RequiredSignals() map[string]map[string][]string {
taskSignals[task.Vault.ChangeSignal] = struct{}{}
}

// If a user has specified a KillSignal, add it to required signals
if task.KillSignal != "" {
taskSignals[task.KillSignal] = struct{}{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

// Check if any template change mode uses signals
for _, t := range task.Templates {
if t.ChangeMode != TemplateChangeModeSignal {
Expand Down Expand Up @@ -3221,6 +3226,10 @@ type Task struct {
// ShutdownDelay is the duration of the delay between deregistering a
// task from Consul and sending it a signal to shutdown. See #2441
ShutdownDelay time.Duration

// The kill signal to use for the task. This is an optional specification,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KillSignal is the kill signal to use for the task. See: https://github.com/golang/go/wiki/Comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// and if not set, the driver will default to SIGINT
KillSignal string
}

func (t *Task) Copy() *Task {
Expand Down