Group shutdown_delay

[drewbailey]

Adds a shutdown delay option to the group stanza to allow time between de-registering a consul service and a task group being shutting down.

[nickethier]

Great start!

[schmichael]

How does deregistering a service out from underneath Consul Connect effect Envoy? I'm concerned it could detect the deregistration and immediately stop proxying which would defeat the purpose of shutdown_delay for Connect tasks.

We'd also still need to open an issue to cover the case where shutdown_delay is used without a service.

Alternatively we could propagate the group shutdown_delay to each task and make a new delay prekill hook for tasks that runs after the service hook. I think that addresses all of the use cases in #6704 and allows users to set either a group delay that applies to all tasks or a per-task delay for fine grained control (eg kill the log shipper last).

It also avoids conflating shutdown delay hooks with services which is the source of the original issue: we coupled the logic instead of making them distinct hooks.

Don't forget docs for the new field.

[drewbailey]

The way that this is currently implemented will delay shutdown regardless if there are registered group services or not, so it should cover the case linked in #6704.

I'll look into how it affects envoy. It's implemented the same way that individual task shutdown_delays deregister from consul

[jippi]

@drewbailey @nickethier could we make group->shutdown_delay default to max(task->*->shutdown_delay) ? :) seem to be what you would want in 9.9/10 cases?

[djenriquez]

How does deregistering a service out from underneath Consul Connect effect Envoy? I'm concerned it could detect the deregistration and immediately stop proxying which would defeat the purpose of shutdown_delay for Connect tasks.

@schmichael I'm confused, I thought the purpose of the shutdown_delay was to stop all new requests from hitting the allocations represented by the consul nodes immediately, and allow in-flight requests to complete. Wouldn't you want it to deregister and stop proxying immediately even for connect tasks?

[schmichael]

The way that this is currently implemented will delay shutdown regardless if there are registered group services or not, so it should cover the case linked in #6704.

-- @drewbailey

Right, sorry I wasn't clear. I meant to say this implementation doesn't fix the bug where a task's shutdown_delay is ignored if the task lacks a service. We need to file a new issue/PR for that if this PR closes the original issue.

How does deregistering a service out from underneath Consul Connect effect Envoy? I'm concerned it could detect the deregistration and immediately stop proxying which would defeat the purpose of shutdown_delay for Connect tasks.

@schmichael I'm confused, I thought the purpose of the shutdown_delay was to stop all new requests from hitting the allocations represented by the consul nodes immediately, and allow in-flight requests to complete. Wouldn't you want it to deregister and stop proxying immediately even for connect tasks?

-- @djenriquez

Yes, but we need to ensure Envoy won't immediately stop proxying inflight tasks. I've never tested what the behavior of deregistering a service is on requests inflight in Envoy.

@drewbailey @nickethier could we make group->shutdown_delay default to max(task->*->shutdown_delay) ? :) seem to be what you would want in 9.9/10 cases?

-- @jippi

As this is currently implemented I think that would cause a double delay: first at the group level, then each individual task's level. I also don't think we propagate parameters "up" stanzas, so this seems like a potentially surprising behavior.

Which brings up a good point: as implemented the group and task shutdown delays are completely distinct. When an allocation is stopping for whatever reason, first the group delay applies, then the tasks delays, then tasks are killed.

That may be what users want, but we need to document it carefully as usually settings with the same name at different levels simply propagate down from job -> group -> task.

[djenriquez]

Any updates with this PR?

[schmichael]

Looks great! Just a couple comments but nothing I need to re-review. Remember the changelog entry (can be a followup PR).

[schmichael]

This doesn't need to be a pointer as there's no difference between nil and 0.

[drewbailey]

This needed to be a pointer so that a job diff knew if it was set or not, otherwise I was getting

Failed
=== RUN   TestJobDiff
--- FAIL: TestJobDiff (0.00s)
    diff_test.go:1196: case 19: got:
        Job "" (Edited):
        Group "bam" (Added):
        "Count" (Added): "" => "1"
        "ShutdownDelay" (Added): "" => "0"

[github-actions]

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.