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
}

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 @@ -201,6 +201,7 @@ type DockerDriverConfig struct {
MacAddress string `mapstructure:"mac_address"` // Pin mac address to container
SecurityOpt []string `mapstructure:"security_opt"` // Flags to pass directly to security-opt
Devices []DockerDevice `mapstructure:"devices"` // To allow mounting USB or other serial control devices
StopSignal string `mapstructure:"stop_signal"` // allow passing through a specific stop signal
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed since it is specified at the task level now

}

func sliceMergeUlimit(ulimitsRaw map[string]string) ([]docker.ULimit, error) {
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 @@ -1055,6 +1055,11 @@ func (d *DockerDriver) createContainerConfig(ctx *ExecContext, task *structs.Tas
StopTimeout: int(task.KillTimeout.Seconds()),
}

// Set the stop signal for the docker container.
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
82 changes: 82 additions & 0 deletions client/driver/docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2045,3 +2045,85 @@ 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 error
{
task := &structs.Task{
Name: "nc-demo",
Driver: "docker",
KillSignal: "ABCDEF",
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)
}

_, err = d.Start(ctx.ExecCtx, task)
assert.NotNil(err)
}

// Tasks started with a signal that is not supported should not error
{
task := &structs.Task{
Name: "nc-demo",
Driver: "docker",
KillSignal: "SIGQUIT",
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
14 changes: 13 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,18 @@ func (e *UniversalExecutor) ShutDown() error {
}
return nil
}
if err = proc.Signal(os.Interrupt); err != nil && err.Error() != finishedErr {

var osSignal os.Signal
if e.command.TaskKillSignal != nil {
osSignal = e.command.TaskKillSignal
} else {
osSignal = os.Interrupt
}

if err = proc.Signal(osSignal); err != nil && err.Error() != finishedErr {
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
15 changes: 15 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,17 @@ 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 nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have it default to SIGINT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was debating whether it should be here or in executor.go- having the default here would be good; changing.

}

taskKillSignal := signals.SignalLookup[signal]
if taskKillSignal == nil {
return nil, fmt.Errorf("Signal %s is not supported", signal)
}
return taskKillSignal, nil
}
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
8 changes: 8 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,13 @@ func parseTasks(jobName string, taskGroupName string, result *[]*api.Task, list
WeaklyTypedInput: true,
Result: &t,
})

// this needs to be manually assigned
killsig := m["kill_signal"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this the case? The mapstructure decode should get this?

if killsig != nil {
t.KillSignal = killsig.(string)
}

if err != nil {
return err
}
Expand Down
27 changes: 26 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,30 @@ func TestParse(t *testing.T) {
},
false,
},
{
"job-with-kill-signal.hcl",
&api.Job{
ID: helper.StringToPtr("example"),
Name: helper.StringToPtr("example"),

TaskGroups: []*api.TaskGroup{
{
Name: helper.StringToPtr("webservice"),
Tasks: []*api.Task{
{
Name: "webservice",
Driver: "docker",
KillSignal: "SIGINT",
Config: map[string]interface{}{
"image": "hashicorp/image",
},
},
},
},
},
},
false,
},
}

for _, tc := range cases {
Expand Down
12 changes: 12 additions & 0 deletions jobspec/test-fixtures/job-with-kill-signal.hcl
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
job "example" {
task "webservice" {
kill_signal = "SIGINT"
driver = "docker"
config
{
image = "hashicorp/image"
}
}

}

32 changes: 32 additions & 0 deletions nomad/job_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3748,6 +3748,38 @@ 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)
if err == nil || !strings.Contains(err.Error(), "support sending signals") {
t.Fatalf("Expected signal feasibility error; got %v", err)
}
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)
if err != nil {
t.Fatalf("Expected error to be nil; got %v", err.Error())
}
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