From b81a0018b4c52cc4e04a92f98eed71170288a2ba Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Sun, 5 Apr 2020 17:30:25 -0400 Subject: [PATCH 1/5] Group shutdown delay fixes Group shutdown delay updates were not properly handled in Update hook. This commit also ensures that plan output is displayed. --- client/allocrunner/groupservice_hook.go | 10 ++++++- client/allocrunner/groupservice_hook_test.go | 29 ++++++++++++++++++++ nomad/structs/diff.go | 12 ++++++++ nomad/structs/diff_test.go | 21 ++++++++++++++ 4 files changed, 71 insertions(+), 1 deletion(-) diff --git a/client/allocrunner/groupservice_hook.go b/client/allocrunner/groupservice_hook.go index 0325d1bae21..3c915e90dcb 100644 --- a/client/allocrunner/groupservice_hook.go +++ b/client/allocrunner/groupservice_hook.go @@ -112,11 +112,19 @@ func (h *groupServiceHook) Update(req *interfaces.RunnerUpdateRequest) error { } // Update group service hook fields + tg := req.Alloc.Job.LookupTaskGroup(h.group) h.networks = networks - h.services = req.Alloc.Job.LookupTaskGroup(h.group).Services + h.services = tg.Services h.canary = canary h.taskEnvBuilder.UpdateTask(req.Alloc, nil) + // Update shutdown delay + var shutdown time.Duration + if tg.ShutdownDelay != nil { + shutdown = *tg.ShutdownDelay + } + h.delay = shutdown + // Create new task services struct with those new values newWorkloadServices := h.getWorkloadServices() diff --git a/client/allocrunner/groupservice_hook_test.go b/client/allocrunner/groupservice_hook_test.go index 82ca09d7b68..8920c5284a8 100644 --- a/client/allocrunner/groupservice_hook_test.go +++ b/client/allocrunner/groupservice_hook_test.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/nomad/client/consul" "github.com/hashicorp/nomad/client/taskenv" agentconsul "github.com/hashicorp/nomad/command/agent/consul" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/testlog" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" @@ -56,6 +57,34 @@ func TestGroupServiceHook_NoGroupServices(t *testing.T) { require.Equal(t, "remove", ops[2].Op) } +// TestGroupServiceHook_ShutdownDelayUpdate asserts calling group service hooks +// update updates the hooks delay value. +func TestGroupServiceHook_ShutdownDelayUpdate(t *testing.T) { + t.Parallel() + + alloc := mock.Alloc() + alloc.Job.TaskGroups[0].ShutdownDelay = helper.TimeToPtr(10 * time.Second) + + logger := testlog.HCLogger(t) + consulClient := consul.NewMockConsulServiceClient(t, logger) + + h := newGroupServiceHook(groupServiceHookConfig{ + alloc: alloc, + consul: consulClient, + restarter: agentconsul.NoopRestarter(), + taskEnvBuilder: taskenv.NewBuilder(mock.Node(), alloc, nil, alloc.Job.Region), + logger: logger, + }) + require.NoError(t, h.Prerun()) + + alloc.Job.TaskGroups[0].ShutdownDelay = helper.TimeToPtr(15 * time.Second) + req := &interfaces.RunnerUpdateRequest{Alloc: alloc} + require.NoError(t, h.Update(req)) + + // Assert that update updated the delay value + require.Equal(t, h.delay, 15*time.Second) +} + // TestGroupServiceHook_GroupServices asserts group service hooks with group // services does not error. func TestGroupServiceHook_GroupServices(t *testing.T) { diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 5e6eaf30654..251ceee5d3b 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -224,6 +224,18 @@ func (tg *TaskGroup) Diff(other *TaskGroup, contextual bool) (*TaskGroupDiff, er newPrimitiveFlat = flatmap.Flatten(other, filter, true) } + // ShutdownDelay diff + if tg.ShutdownDelay == nil { + oldPrimitiveFlat["ShutdownDelay"] = "nil" + } else { + oldPrimitiveFlat["ShutdownDelay"] = fmt.Sprintf("%d", *tg.ShutdownDelay) + } + if other.ShutdownDelay == nil { + newPrimitiveFlat["ShutdownDelay"] = "nil" + } else { + newPrimitiveFlat["ShutdownDelay"] = fmt.Sprintf("%d", *other.ShutdownDelay) + } + // Diff the primitive fields. diff.Fields = fieldDiffs(oldPrimitiveFlat, newPrimitiveFlat, false) diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index c51f38d0f80..06ac79ad1e8 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/hashicorp/nomad/helper" "github.com/stretchr/testify/require" ) @@ -3029,6 +3030,26 @@ func TestTaskGroupDiff(t *testing.T) { }, }, }, + { + TestCase: "TaskGroup shutdown_delay edited", + Old: &TaskGroup{ + ShutdownDelay: helper.TimeToPtr(30 * time.Second), + }, + New: &TaskGroup{ + ShutdownDelay: helper.TimeToPtr(5 * time.Second), + }, + Expected: &TaskGroupDiff{ + Type: DiffTypeEdited, + Fields: []*FieldDiff{ + { + Type: DiffTypeEdited, + Name: "ShutdownDelay", + Old: "30000000000", + New: "5000000000", + }, + }, + }, + }, } for i, c := range cases { From d45fc506e5bea9162760d11266468cbdc0790338 Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Mon, 6 Apr 2020 11:33:04 -0400 Subject: [PATCH 2/5] ensure shutdown delay can be removed --- client/allocrunner/groupservice_hook.go | 13 ++++++------- client/allocrunner/groupservice_hook_test.go | 9 +++++++++ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/client/allocrunner/groupservice_hook.go b/client/allocrunner/groupservice_hook.go index 3c915e90dcb..fb37fcca665 100644 --- a/client/allocrunner/groupservice_hook.go +++ b/client/allocrunner/groupservice_hook.go @@ -111,19 +111,18 @@ func (h *groupServiceHook) Update(req *interfaces.RunnerUpdateRequest) error { networks = req.Alloc.AllocatedResources.Shared.Networks } - // Update group service hook fields tg := req.Alloc.Job.LookupTaskGroup(h.group) - h.networks = networks - h.services = tg.Services - h.canary = canary - h.taskEnvBuilder.UpdateTask(req.Alloc, nil) - - // Update shutdown delay var shutdown time.Duration if tg.ShutdownDelay != nil { shutdown = *tg.ShutdownDelay } + + // Update group service hook fields + h.networks = networks + h.services = tg.Services + h.canary = canary h.delay = shutdown + h.taskEnvBuilder.UpdateTask(req.Alloc, nil) // Create new task services struct with those new values newWorkloadServices := h.getWorkloadServices() diff --git a/client/allocrunner/groupservice_hook_test.go b/client/allocrunner/groupservice_hook_test.go index 8920c5284a8..812d0a42f8b 100644 --- a/client/allocrunner/groupservice_hook_test.go +++ b/client/allocrunner/groupservice_hook_test.go @@ -77,12 +77,21 @@ func TestGroupServiceHook_ShutdownDelayUpdate(t *testing.T) { }) require.NoError(t, h.Prerun()) + // Incease shutdown Delay alloc.Job.TaskGroups[0].ShutdownDelay = helper.TimeToPtr(15 * time.Second) req := &interfaces.RunnerUpdateRequest{Alloc: alloc} require.NoError(t, h.Update(req)) // Assert that update updated the delay value require.Equal(t, h.delay, 15*time.Second) + + // Remove shutdown delay + alloc.Job.TaskGroups[0].ShutdownDelay = nil + req = &interfaces.RunnerUpdateRequest{Alloc: alloc} + require.NoError(t, h.Update(req)) + + // Assert that update updated the delay value + require.Equal(t, h.delay, 0*time.Second) } // TestGroupServiceHook_GroupServices asserts group service hooks with group From 0b9827dfb7547e77c3980f676d2fd71c2c1e3e77 Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Mon, 6 Apr 2020 11:53:46 -0400 Subject: [PATCH 3/5] test added and removed --- nomad/structs/diff.go | 4 ++-- nomad/structs/diff_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 251ceee5d3b..812a28e17ad 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -226,12 +226,12 @@ func (tg *TaskGroup) Diff(other *TaskGroup, contextual bool) (*TaskGroupDiff, er // ShutdownDelay diff if tg.ShutdownDelay == nil { - oldPrimitiveFlat["ShutdownDelay"] = "nil" + oldPrimitiveFlat["ShutdownDelay"] = "" } else { oldPrimitiveFlat["ShutdownDelay"] = fmt.Sprintf("%d", *tg.ShutdownDelay) } if other.ShutdownDelay == nil { - newPrimitiveFlat["ShutdownDelay"] = "nil" + newPrimitiveFlat["ShutdownDelay"] = "" } else { newPrimitiveFlat["ShutdownDelay"] = fmt.Sprintf("%d", *other.ShutdownDelay) } diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 06ac79ad1e8..ed6ea07970c 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -3050,6 +3050,42 @@ func TestTaskGroupDiff(t *testing.T) { }, }, }, + { + TestCase: "TaskGroup shutdown_delay removed", + Old: &TaskGroup{ + ShutdownDelay: helper.TimeToPtr(30 * time.Second), + }, + New: &TaskGroup{}, + Expected: &TaskGroupDiff{ + Type: DiffTypeEdited, + Fields: []*FieldDiff{ + { + Type: DiffTypeDeleted, + Name: "ShutdownDelay", + Old: "30000000000", + New: "", + }, + }, + }, + }, + { + TestCase: "TaskGroup shutdown_delay added", + Old: &TaskGroup{}, + New: &TaskGroup{ + ShutdownDelay: helper.TimeToPtr(30 * time.Second), + }, + Expected: &TaskGroupDiff{ + Type: DiffTypeEdited, + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "ShutdownDelay", + Old: "", + New: "30000000000", + }, + }, + }, + }, } for i, c := range cases { From a14230277c3068aa8e1ae3fa830248a903c2b01a Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Mon, 6 Apr 2020 11:57:28 -0400 Subject: [PATCH 4/5] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59014f5b3ba..fbbe636d4b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ BUG FIXES: * connect: Fixed a bug where Connect enabled allocation would not stop after promotion [[GH-7540](https://github.com/hashicorp/nomad/issues/7540)] * driver/docker: Fixed handling of seccomp `security_opts` option [[GH-7554](https://github.com/hashicorp/nomad/issues/7554)] * driver/docker: Fixed a bug causing docker containers to use swap memory unexpectedly [[GH-7550](https://github.com/hashicorp/nomad/issues/7550)] + * scheduler: Fixed a bug where changes to task group `shutdown_delay` were not persisted or displayed in plan output [[GH-7618](https://github.com/hashicorp/nomad/issues/7618)] * ui: Fixed handling of multi-byte unicode characters in allocation log view [[GH-7470](https://github.com/hashicorp/nomad/issues/7470)] [[GH-7551](https://github.com/hashicorp/nomad/pull/7551)] ## 0.10.5 (March 24, 2020) From da93e236a93f5a6d9365986175acfc6d04823785 Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Mon, 6 Apr 2020 12:25:50 -0400 Subject: [PATCH 5/5] guard against nil maps --- nomad/structs/diff.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 812a28e17ad..34533c0584c 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -225,15 +225,17 @@ func (tg *TaskGroup) Diff(other *TaskGroup, contextual bool) (*TaskGroupDiff, er } // ShutdownDelay diff - if tg.ShutdownDelay == nil { - oldPrimitiveFlat["ShutdownDelay"] = "" - } else { - oldPrimitiveFlat["ShutdownDelay"] = fmt.Sprintf("%d", *tg.ShutdownDelay) - } - if other.ShutdownDelay == nil { - newPrimitiveFlat["ShutdownDelay"] = "" - } else { - newPrimitiveFlat["ShutdownDelay"] = fmt.Sprintf("%d", *other.ShutdownDelay) + if oldPrimitiveFlat != nil && newPrimitiveFlat != nil { + if tg.ShutdownDelay == nil { + oldPrimitiveFlat["ShutdownDelay"] = "" + } else { + oldPrimitiveFlat["ShutdownDelay"] = fmt.Sprintf("%d", *tg.ShutdownDelay) + } + if other.ShutdownDelay == nil { + newPrimitiveFlat["ShutdownDelay"] = "" + } else { + newPrimitiveFlat["ShutdownDelay"] = fmt.Sprintf("%d", *other.ShutdownDelay) + } } // Diff the primitive fields.