-
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
Add optional shutdown delay to tasks #3043
Changes from 1 commit
beae45b
ecf0d20
32d782d
b4480cd
08d9c4b
fbe85b5
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 |
---|---|---|
|
@@ -1614,3 +1614,67 @@ func TestTaskRunner_Pre06ScriptCheck(t *testing.T) { | |
t.Run(run("0.5.6", "java", "tcp", false)) | ||
t.Run(run("0.5.6", "mock_driver", "tcp", false)) | ||
} | ||
|
||
func TestTaskRunner_ShutdownDelay(t *testing.T) { | ||
t.Parallel() | ||
|
||
alloc := mock.Alloc() | ||
task := alloc.Job.TaskGroups[0].Tasks[0] | ||
task.Driver = "mock_driver" | ||
task.Config = map[string]interface{}{ | ||
"run_for": "1000s", | ||
} | ||
|
||
// No shutdown escape hatch for this delay, so don't set it too high | ||
task.ShutdownDelay = 5 * time.Second | ||
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. Way too long :) Try to avoid sleeping for any longer than a second. 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. If we're unable to check the Consul ops before the delay is up we'll get a false failure. Mind if I make it |
||
|
||
ctx := testTaskRunnerFromAlloc(t, true, alloc) | ||
ctx.tr.MarkReceived() | ||
go ctx.tr.Run() | ||
defer ctx.Cleanup() | ||
|
||
// Wait for the task to start | ||
testWaitForTaskToStart(t, ctx) | ||
|
||
// Begin the tear down | ||
ctx.tr.Destroy(structs.NewTaskEvent(structs.TaskKilled)) | ||
destroyed := time.Now() | ||
|
||
// Service should get removed quickly; loop until RemoveTask is called | ||
found := false | ||
mockConsul := ctx.tr.consul.(*mockConsulServiceClient) | ||
deadline := destroyed.Add(task.ShutdownDelay) | ||
for time.Now().Before(deadline) { | ||
time.Sleep(5 * time.Millisecond) | ||
|
||
mockConsul.mu.Lock() | ||
n := len(mockConsul.ops) | ||
if n < 2 { | ||
mockConsul.mu.Unlock() | ||
continue | ||
} | ||
|
||
lastOp := mockConsul.ops[n-1].op | ||
mockConsul.mu.Unlock() | ||
|
||
if lastOp == "remove" { | ||
found = true | ||
break | ||
} | ||
} | ||
if !found { | ||
t.Errorf("task was not removed from Consul first") | ||
} | ||
|
||
// Wait for actual exit | ||
select { | ||
case <-ctx.tr.WaitCh(): | ||
case <-time.After(time.Duration(testutil.TestMultiplier()*15) * time.Second): | ||
t.Fatalf("timeout") | ||
} | ||
|
||
// It should be impossible to reach here in less time than the shutdown delay | ||
if time.Now().Before(destroyed.Add(task.ShutdownDelay)) { | ||
t.Fatalf("task exited before shutdown delay") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1922,8 +1922,8 @@ func TestTaskGroupDiff(t *testing.T) { | |
Driver: "docker", | ||
}, | ||
{ | ||
Name: "baz", | ||
Driver: "docker", | ||
Name: "baz", | ||
ShutdownDelay: 1 * time.Second, | ||
}, | ||
}, | ||
}, | ||
|
@@ -1934,8 +1934,8 @@ func TestTaskGroupDiff(t *testing.T) { | |
Driver: "docker", | ||
}, | ||
{ | ||
Name: "baz", | ||
Driver: "exec", | ||
Name: "baz", | ||
ShutdownDelay: 2 * time.Second, | ||
}, | ||
{ | ||
Name: "bam", | ||
|
@@ -1981,8 +1981,8 @@ func TestTaskGroupDiff(t *testing.T) { | |
{ | ||
Type: DiffTypeEdited, | ||
Name: "Driver", | ||
Old: "docker", | ||
New: "exec", | ||
Old: "1s", | ||
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. Not the intended change I don't think? |
||
New: "2s", | ||
}, | ||
}, | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,12 @@ job "docs" { | |
[Consul][] for service discovery. Nomad automatically registers when a task | ||
is started and de-registers it when the task dies. | ||
|
||
- `shutdown_delay` `(string: "0s")` - Specifies the duration to wait when | ||
killing a task between removing it from Consul and sending it a shutdown | ||
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. JSON job spec as well |
||
signal. Ideally services would fail healthchecks once they receive a shutdown | ||
signal. Alternatively `shutdown_delay` may be set to give in flight requests | ||
time to complete before shutting down. | ||
|
||
- `user` `(string: <varies>)` - Specifies the user that will run the task. | ||
Defaults to `nobody` for the [`exec`][exec] and [`java`][java] drivers. | ||
[Docker][] and [rkt][] images specify their own default users. This can only | ||
|
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.
Seems more of a debug?
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.
I just worry people are going to forget they set a shutdown delay and wonder why Nomad seems to hang since there's no indication outside of looking at the job file that this task is blocked on a shutdown delay...
I'll make it DEBUG though... users stopping/updating jobs should be aware of the job's settings and behavior.