Skip to content
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

Don't wait for splay when stopping child runner #1141

Closed

Conversation

KJTsanaktsidis
Copy link
Contributor

When consul-template itself receives its kill signal, and tries to gracefully shutdown the child runner, it actually waits for splay to occur before shutting down the child. This is, in my opinion, undesireable behaviour because the splay is intended to protect against stampeding herds of simultaneous child starts, not stops.

My particular use case is that I run consul-template as a parent in a Kubernetes container; when doing a deployment, the old pods take a long time to shut down because of the splay timeout. However, there's no need to wait for the splay in this case, because Kubernetes itself is not restarting all instances of the container at the same time.

If the original behaviour is actually intended or desired, I'm happy to put this change behind a flag as well.

When consul-template itself receives its kill signal, and tries to
gracefully shutdown the child runner, it actually waits for splay to
occur before shutting down the child. This is undesireable behaviour
because the splay is intended to protect against stampeding herds of
simultaneous child _starts_, not _stops_.
KJTsanaktsidis added a commit to zendesk/envconsul that referenced this pull request Sep 21, 2018
KJTsanaktsidis added a commit to zendesk/envconsul that referenced this pull request Sep 21, 2018
@KJTsanaktsidis
Copy link
Contributor Author

Having a second look at this I think I need to rethink this - child.Stop() gets called in other cases like when the reload signal is received, where the splay is appropriate. I also need to think about what happens when a Vault token expires - ideally splay would be applied there as well.

We probably _do_ want to wait for the splay when the reload signal is
received, just not the kill signal.
@KJTsanaktsidis
Copy link
Contributor Author

So I pushed the choice as to whether to wait for splay or not all the way up to the CLI module, so that we can have different behaviour for the reload and kill signal. It will also make it easier to gate the behaviour on a flag if that's what we want to do.

@eikenb
Copy link
Contributor

eikenb commented Jun 19, 2019

Hey @KJTsanaktsidis, since you submitted your pull request we've implemented a policy of requiring a signed CLA before merging. Would you please consider signing the CLA? Thanks.

@plonergan
Copy link

plonergan commented Jun 25, 2019

@eikenb Is it possible for our organization to sign the CLA on behalf of the organization to cover this PR instead of having the individual developers sign the CLA?

@eikenb
Copy link
Contributor

eikenb commented Jun 25, 2019

Hey @plonergan. I asked and it seems that isn't possible. Githubs CLA tech is oriented strictly towards individual contributors, not organizations, and we geared are CLA toward that. Ie. it is not possible technically and the document reflects that.

Is the problem that @KJTsanaktsidis left your organization since submitting this?

@KJTsanaktsidis
Copy link
Contributor Author

KJTsanaktsidis commented Jun 30, 2019 via email

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 10, 2019

CLA assistant check
All committers have signed the CLA.

@KJTsanaktsidis
Copy link
Contributor Author

I resolved conflicts and signed the CLA - @plonergan said that we've agreed to a corporate CLA but I needed to click the buttons in Github to make all the machinery work, so I've gone ahead and done that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants