Allow controlling the stop signal for drivers #3591

chelseakomlo · 2017-11-27T23:00:42Z

See initial issue: #1755

This feature is only supported by drivers which accept signals.

This excludes rkt- we first need this PR to merge before extending support: rkt/rkt#3732

nugend · 2017-11-29T01:02:42Z

Barring said idiomatic translation could you please expand the subset to include HUP, QUIT, and TERM

That should cover all the sane process termination signals.

edit: Sorry, I see now that QUIT is being used as the default. Does that mean that if someone accidentally typed "SIGTRM" as the value to use it would send SIGQUIT?

schmichael

A few things to get this one merged:

Broke the Windows build. (see code comment)
Missing raw_exec support.
Need to update driver's Validate method to make sure a valid signal is specified. (thanks @nugend!)

schmichael · 2017-11-29T23:58:09Z

client/driver/executor/executor.go

+	case "SIGUSR2":
+		return syscall.SIGUSR2
+	default:
+		return syscall.SIGQUIT


Not all of these signals are available on every platform: https://ci.appveyor.com/project/hashicorp/nomad/build/build-b-1755-stop-1969/job/tt7wpyqorw9lvf7e

Oddly both executor_unix.go and executor_basic.go include windows in their build tags... It might be best to see if you can make executor_unix.go actually Unix-only and add a executor_windows.go for the Windows implementation of this function.

Good point. What do you think about validating on a per-platform basis then, against the subset of signals that are supported for that platform?

Yeah! We already fingerprint signals. Looks like consul-template already has the list we need: https://github.com/hashicorp/nomad/blob/master/client/fingerprint/signal.go

add conversion for KillSignal for api/struct representation of task

dadgar · 2017-12-06T20:01:50Z

client/driver/utils.go

+// specified. If it is not supported on the platform, returns an error.
+func getTaskKillSignal(signal string) (os.Signal, error) {
+	if signal == "" {
+		return nil, nil


Why not have it default to SIGINT

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

dadgar · 2017-12-06T20:02:05Z

client/driver/docker.go

@@ -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


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

dadgar · 2017-12-06T20:02:55Z

command/agent/job_endpoint.go

@@ -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


Add to test

dadgar · 2017-12-06T20:03:53Z

jobspec/parse.go

@@ -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"]


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

chelseakomlo · 2017-12-06T22:52:46Z

client/driver/utils.go

+
+// 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) {


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

dadgar · 2017-12-06T23:16:14Z

nomad/structs/structs.go

@@ -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,


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

Thanks, see 7d8dda8#diff-396d9428285e4245041a3d0b15546d5dR3232

dadgar · 2017-12-06T23:18:16Z

nomad/structs/structs.go

@@ -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{}{}


Add to test

See 21e10d1#diff-d8207d79ddf876021a4e0225297fa6b9R809

dadgar · 2017-12-07T00:05:49Z

client/driver/exec.go

 	execCmd := &executor.ExecCommand{
 		Cmd:            command,
 		Args:           driverConfig.Args,
+		TaskKillSignal: taskKillSignal,


Java driver too

See 27b666e

dadgar · 2017-12-07T00:05:57Z

client/driver/executor/executor.go

@@ -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 {


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

update changelog

dadgar · 2017-12-07T18:07:07Z

website/source/docs/job-specification/task.html.md

@@ -54,6 +54,11 @@ job "docs" {
  [`max_kill_timeout`][max_kill] on the agent running the task, which has a
  default value of 30 seconds.

+- `kill_signal` `(string)` - Specifies a configurable kill signal for a task,
+  where the default is SIGINT. Note tha this is only supported for drivers
+  which accept sending signals (currently docker, exec, raw_exec, and java


Capitalize Docker and Java.

dadgar · 2017-12-07T18:07:45Z

CHANGELOG.md

@@ -11,6 +11,8 @@ __BACKWARDS INCOMPATIBILITIES:__
 IMPROVEMENTS:
 * core: Allow operators to reload TLS certificate and key files via SIGHUP
   [GH-3479]
+ * core:  allow configurable stop signals for a task, when drivers support


Capitalize allow. Remove period before [GH-

dadgar · 2017-12-07T18:08:39Z

client/driver/docker.go

@@ -1055,6 +1056,10 @@ func (d *DockerDriver) createContainerConfig(ctx *ExecContext, task *structs.Tas
 		StopTimeout: int(task.KillTimeout.Seconds()),
 	}

+	if task.KillSignal != "" {


Can this become:

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

nugend · 2017-12-07T19:59:39Z

@chelseakomlo Thank you so much for working on this!

github-actions · 2023-03-16T02:15:39Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

chelseakomlo force-pushed the b-1755-stop branch from 845904a to 70324a1 Compare November 28, 2017 21:42

schmichael requested changes Nov 30, 2017

View reviewed changes

chelseakomlo force-pushed the b-1755-stop branch 5 times, most recently from 2585f3e to d2e5ca7 Compare December 6, 2017 16:25

chelseakomlo changed the title ~~Allow controlling the stop signal in exec/raw_exec~~ Allow controlling the stop signal for drivers Dec 6, 2017

chelseakomlo force-pushed the b-1755-stop branch from d2e5ca7 to a9ad4f8 Compare December 6, 2017 16:27

allow controlling the stop signal in exec/raw_exec

079e6b5

chelseakomlo force-pushed the b-1755-stop branch 5 times, most recently from 7f1d929 to 4adf93a Compare December 6, 2017 19:26

chelseakomlo added 4 commits December 6, 2017 14:36

move kill_signal to task level, extend to docker

9e49836

extract signal helper into utils

8356c61

add missing new file

a693666

fix up basic test

359613e

add conversion for KillSignal for api/struct representation of task

chelseakomlo force-pushed the b-1755-stop branch from 4adf93a to 359613e Compare December 6, 2017 19:37

use assert library

0c17fd5

dadgar requested changes Dec 6, 2017

View reviewed changes

chelseakomlo added 3 commits December 6, 2017 16:23

fix up test fixture to properly parse

20d1a3b

code review fixups

b1eda32

change location of default kill signal

927c0a4

chelseakomlo commented Dec 6, 2017

View reviewed changes

dadgar requested changes Dec 7, 2017

View reviewed changes

chelseakomlo force-pushed the b-1755-stop branch from 21e10d1 to 1b65853 Compare December 7, 2017 16:03

extend configurable kill signal to java driver

27b666e

chelseakomlo added 2 commits December 7, 2017 11:40

set default kill signal on executor shutdown

f09e3ec

add test for kill signal in required signals

7d8dda8

update changelog

chelseakomlo force-pushed the b-1755-stop branch from 1b65853 to 7d8dda8 Compare December 7, 2017 16:40

add documention

73a4a46

dadgar approved these changes Dec 7, 2017

View reviewed changes

code review fixes

ae10b63

chelseakomlo merged commit dfb95fd into master Dec 7, 2017

chelseakomlo deleted the b-1755-stop branch December 7, 2017 22:06

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow controlling the stop signal for drivers #3591

Allow controlling the stop signal for drivers #3591

chelseakomlo commented Nov 27, 2017 •

edited

Loading

nugend commented Nov 29, 2017 •

edited

Loading

schmichael left a comment •

edited

Loading

schmichael Nov 29, 2017

chelseakomlo Nov 30, 2017

schmichael Nov 30, 2017

dadgar Dec 6, 2017

chelseakomlo Dec 6, 2017

dadgar Dec 6, 2017

dadgar Dec 6, 2017

dadgar Dec 6, 2017

chelseakomlo Dec 6, 2017

dadgar Dec 6, 2017

chelseakomlo Dec 7, 2017

dadgar Dec 6, 2017

chelseakomlo Dec 7, 2017

dadgar Dec 7, 2017

chelseakomlo Dec 7, 2017 •

edited

Loading

dadgar Dec 7, 2017

dadgar Dec 7, 2017

dadgar Dec 7, 2017

dadgar Dec 7, 2017

nugend commented Dec 7, 2017

github-actions bot commented Mar 16, 2023

Allow controlling the stop signal for drivers #3591

Allow controlling the stop signal for drivers #3591

Conversation

chelseakomlo commented Nov 27, 2017 • edited Loading

nugend commented Nov 29, 2017 • edited Loading

schmichael left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chelseakomlo Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nugend commented Dec 7, 2017

github-actions bot commented Mar 16, 2023

chelseakomlo commented Nov 27, 2017 •

edited

Loading

nugend commented Nov 29, 2017 •

edited

Loading

schmichael left a comment •

edited

Loading

chelseakomlo Dec 7, 2017 •

edited

Loading