client: improve group service stanza interpolation and check_restart support #6586

nickethier · 2019-10-30T03:30:03Z

Interpolation can now be done on group service stanzas. Note that some task runtime specific information
that was previously available when the service was registered poststart of a task is no longer available.

The check_restart stanza for checks defined on group services will now properly restart the allocation upon
check failures if configured.

fixes #6272

nickethier · 2019-10-30T03:37:22Z

client/allocrunner/alloc_runner.go

+		defer close(waitCh)
+		for tn, tr := range ar.tasks {
+			wg.Add(1)
+			go func(taskName string, r agentconsul.WorkloadRestarter) {


Note to reviews: This might be overkill but I wanted restarts to be as quick as possible especially when there are numerous tasks in the group. I'd like to hear your thoughts.

Should this replace the RestartAll method below?

Also, what happens when we're in the middle of this Restart but we get a call to update or kill? The alloc runner hooks get their actions serialized by a lock but allocRunner has a lot of "make sure you call this after/before this other method". (Although this is probably out of scope for this PR because that's the existing condition of things here.)

client/allocrunner/groupservice_hook_test.go

tgross

@nickethier I've left a few comments/questions but overall this looks like a big improvement. 👍

client/taskenv/env_test.go

tgross · 2019-11-08T16:28:04Z

client/consul/consul.go

-	UpdateTask(old, newTask *consul.TaskServices) error
+	RegisterWorkload(*consul.WorkloadServices) error
+	RemoveWorkload(*consul.WorkloadServices)
+	UpdateWorkload(old, newTask *consul.WorkloadServices) error


I really like the reduction in information sharing we're doing here by passing in this consul.WorkloadServices rather than the whole structs.Allocation. 👍

command/agent/consul/client.go

tgross · 2019-11-08T16:56:47Z

client/allocrunner/alloc_runner.go

+		defer close(waitCh)
+		for tn, tr := range ar.tasks {
+			wg.Add(1)
+			go func(taskName string, r agentconsul.WorkloadRestarter) {


Should this replace the RestartAll method below?

Also, what happens when we're in the middle of this Restart but we get a call to update or kill? The alloc runner hooks get their actions serialized by a lock but allocRunner has a lot of "make sure you call this after/before this other method". (Although this is probably out of scope for this PR because that's the existing condition of things here.)

…support Interpolation can now be done on group service stanzas. Note that some task runtime specific information that was previously available when the service was registered poststart of a task is no longer available. The check_restart stanza for checks defined on group services will now properly restart the allocation upon check failures if configured.

schmichael

bindata_assetfs got removed.

This looks really great! I love the renaming/refactoring. I don't think anything I suggested would take a re-review as it's all trivial or a removal, so I'll mark as Approved.

schmichael · 2019-11-08T16:59:59Z

client/allocrunner/alloc_runner.go

+				if e != nil {
+					errMutex.Lock()
+					defer errMutex.Unlock()
+					err = multierror.Append(err, fmt.Errorf("failed to restart task %s: %v", taskName, e))


We do this pattern elsewhere. Might be nice to create an errgroup like package on top of multierror that runs all goroutines and returns all errors, unlike errgroup which only returns the first error and cancels the other goroutines.

Not a blocker and would make a nice followup PR if you want to chase it.

I made a PR to multierror for this pattern. Will open up a followup Nomad PR to make use of it once I get it merged. hashicorp/go-multierror#28

client/allocrunner/alloc_runner.go

client/allocrunner/alloc_runner_hooks.go

client/allocrunner/groupservice_hook.go

schmichael · 2019-11-08T23:32:27Z

client/allocrunner/groupservice_hook.go

+
+	var networks structs.Networks
+	if cfg.alloc.AllocatedResources != nil {
+		networks = cfg.alloc.AllocatedResources.Shared.Networks


Suggested change

networks = cfg.alloc.AllocatedResources.Shared.Networks

h.networks = cfg.alloc.AllocatedResources.Shared.Networks

I don't think you need the local networks var since its zero value is what h.networks is initialized to anyway.

client/allocrunner/groupservice_hook.go

schmichael · 2019-11-08T23:37:19Z

client/allocrunner/interfaces/runner_lifecycle.go

@@ -44,6 +44,7 @@ type RunnerUpdateHook interface {

 type RunnerUpdateRequest struct {
 	Alloc *structs.Allocation
+	Node  *structs.Node


As above: I think this is static and therefore doesn't need to be included in Update calls.

schmichael · 2019-11-08T23:40:22Z

client/taskenv/env.go

@@ -575,6 +578,7 @@ func (b *Builder) setAlloc(alloc *structs.Allocation) *Builder {
 	b.namespace = alloc.Namespace

 	// Set meta
+	// TODO nickethier make sure this works


😂 I eagerly await this comment being red too.

schmichael · 2019-11-08T23:41:01Z

client/taskenv/env_test.go

+	require.NotPanics(func() {
+		taskEnv = NewBuilder(node, alloc, nil, "global").SetAllocDir("/tmp/alloc").Build()
+	})
+


Did you mean to add some assertions here? NotPanics is a pretty low bar.

schmichael · 2019-11-08T23:42:27Z

command/agent/consul/check_watcher.go

 	Restart(ctx context.Context, event *structs.TaskEvent, failure bool) error
 }

+func NoopRestarter() WorkloadRestarter {


Is this necessary anymore outside of tests? Maybe move it to a testing.go file if it's needed for multiple package's tests.

github-actions · 2023-01-26T02:16:35Z

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.

nickethier commented Oct 30, 2019

View reviewed changes

client/allocrunner/groupservice_hook_test.go Outdated Show resolved Hide resolved

nickethier requested review from schmichael, notnoop and tgross November 8, 2019 15:31

nickethier marked this pull request as ready for review November 8, 2019 15:33

tgross reviewed Nov 8, 2019

View reviewed changes

nickethier added 4 commits November 8, 2019 14:46

ar: only attempt to register/deregister group services if present

7569bd2

ar: groupServicesHook nil check in constructor

022a016

ar: fix group services hook tests

02a9b54

nickethier force-pushed the b-group-service-interpolation branch from 13d0704 to 02a9b54 Compare November 8, 2019 19:46

schmichael approved these changes Nov 8, 2019

View reviewed changes

nickethier added 2 commits November 12, 2019 14:54

small fixes to group service hook from PR comments

d408876

fix lint errors

687aacf

nickethier merged commit 387b016 into master Nov 18, 2019

nickethier deleted the b-group-service-interpolation branch November 18, 2019 18:04

tgross mentioned this pull request Nov 22, 2019

Variable Interpolation not working in group level service #6756

Closed

tgross mentioned this pull request Jan 8, 2020

interpolate environment for services in script checks #6916

Merged

tgross mentioned this pull request Mar 26, 2020

Old script health checks come back when upgrading a cluster #6815

Open

github-actions bot locked as resolved and limited conversation to collaborators Jan 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: improve group service stanza interpolation and check_restart support #6586

client: improve group service stanza interpolation and check_restart support #6586

nickethier commented Oct 30, 2019

nickethier Oct 30, 2019

tgross Nov 8, 2019

tgross left a comment

tgross Nov 8, 2019

tgross Nov 8, 2019

schmichael left a comment

schmichael Nov 8, 2019

nickethier Nov 11, 2019

schmichael Nov 8, 2019

schmichael Nov 8, 2019

schmichael Nov 8, 2019

schmichael Nov 8, 2019

schmichael Nov 8, 2019

github-actions bot commented Jan 26, 2023

	networks = cfg.alloc.AllocatedResources.Shared.Networks
	h.networks = cfg.alloc.AllocatedResources.Shared.Networks

client: improve group service stanza interpolation and check_restart support #6586

client: improve group service stanza interpolation and check_restart support #6586

Conversation

nickethier commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael left a 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

github-actions bot commented Jan 26, 2023