From 804f9fdb93f17e1eda20be1d122e4abd37acef0b Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Wed, 22 Feb 2023 10:22:48 -0600 Subject: [PATCH] services: ensure task group is set on service hook (#16240) This PR fixes a bug where the task group information was not being set on the serviceHook.AllocInfo struct, which is needed later on for calculating the CheckID of a nomad service check. The CheckID is calculated independently from multiple callsites, and the information being passed in must be consistent, including the group name. The workload.AllocInfo.Group was not set at this callsite, due to the bug fixed in this PR. https://github.com/hashicorp/nomad/blob/main/client/serviceregistration/nsd/nsd.go#L114 --- .changelog/16240.txt | 3 +++ .../taskrunner/script_check_hook_test.go | 16 ++++++++++------ client/allocrunner/taskrunner/service_hook.go | 1 + 3 files changed, 14 insertions(+), 6 deletions(-) create mode 100644 .changelog/16240.txt diff --git a/.changelog/16240.txt b/.changelog/16240.txt new file mode 100644 index 000000000..b0f63a3d5 --- /dev/null +++ b/.changelog/16240.txt @@ -0,0 +1,3 @@ +```release-note:bug +services: Fixed a bug where check_restart on nomad services on tasks failed with incorrect CheckIDs +``` diff --git a/client/allocrunner/taskrunner/script_check_hook_test.go b/client/allocrunner/taskrunner/script_check_hook_test.go index 60b557498..cfe127bb2 100644 --- a/client/allocrunner/taskrunner/script_check_hook_test.go +++ b/client/allocrunner/taskrunner/script_check_hook_test.go @@ -8,7 +8,7 @@ import ( "time" "github.com/hashicorp/consul/api" - hclog "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-hclog" "github.com/hashicorp/nomad/ci" "github.com/hashicorp/nomad/client/allocrunner/taskrunner/interfaces" "github.com/hashicorp/nomad/client/serviceregistration" @@ -19,6 +19,7 @@ import ( "github.com/hashicorp/nomad/helper/testlog" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" + "github.com/shoenig/test/must" "github.com/stretchr/testify/require" ) @@ -271,14 +272,17 @@ func TestScript_TaskEnvInterpolation(t *testing.T) { scHook.taskEnv = env scHook.driverExec = exec - expectedSvc := svcHook.getWorkloadServices().Services[0] + workload := svcHook.getWorkloadServices() + must.Eq(t, "web", workload.AllocInfo.Group) + + expectedSvc := workload.Services[0] expected := agentconsul.MakeCheckID(serviceregistration.MakeAllocServiceID( alloc.ID, task.Name, expectedSvc), expectedSvc.Checks[0]) actual := scHook.newScriptChecks() check, ok := actual[expected] - require.True(t, ok) - require.Equal(t, "my-job-frontend-check", check.check.Name) + must.True(t, ok) + must.Eq(t, "my-job-frontend-check", check.check.Name) // emulate an update env = taskenv.NewBuilder(mock.Node(), alloc, task, "global").SetHookEnv( @@ -293,8 +297,8 @@ func TestScript_TaskEnvInterpolation(t *testing.T) { actual = scHook.newScriptChecks() check, ok = actual[expected] - require.True(t, ok) - require.Equal(t, "my-job-backend-check", check.check.Name) + must.True(t, ok) + must.Eq(t, "my-job-backend-check", check.check.Name) } func TestScript_associated(t *testing.T) { diff --git a/client/allocrunner/taskrunner/service_hook.go b/client/allocrunner/taskrunner/service_hook.go index d9dfe59bc..2f506c702 100644 --- a/client/allocrunner/taskrunner/service_hook.go +++ b/client/allocrunner/taskrunner/service_hook.go @@ -224,6 +224,7 @@ func (h *serviceHook) getWorkloadServices() *serviceregistration.WorkloadService info := structs.AllocInfo{ AllocID: h.allocID, JobID: h.jobID, + Group: h.groupName, Task: h.taskName, Namespace: h.namespace, }