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

move render events to after template command #1370

Merged
merged 2 commits into from
Apr 23, 2020

Conversation

eikenb
Copy link
Contributor

@eikenb eikenb commented Apr 21, 2020

When using CT as a library, the "standard" way to monitor and know when
to exit is to watch for messages on the TemplateRenderedCh(). The
problem is that the message is sent down that channel before the
commands are run, so the program could exit before the command has run.

The problem is more obvious when CT is run in a transient container,
where even if the commands are run the container might exit before they
finish and they get killed.

Moving the render events to after the command avoids these issues (the
container exit issue also requires a command_timeout > 0s).

Fixes #1369

When using CT as a library, the "standard" way to monitor and know when
to exit is to watch for messages on the TemplateRenderedCh(). The
problem is that the message is sent down that channel before the
commands are run, so the program could exit before the command has run.

The problem is more obvious when CT is run in a transient container,
where even if the commands are run the container might exit before they
finish and they get killed.

Moving the render events to after the command avoids these issues (the
container exit issue also requires a command_timeout > 0s).
@eikenb eikenb added bug vault Related to the Vault integration labels Apr 21, 2020
@eikenb eikenb added this to the 0.25.0 milestone Apr 21, 2020
@eikenb eikenb requested review from a team April 21, 2020 01:17
Copy link
Contributor

@findkim findkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the context on the use case this solves! The changes makes sense wrt CT running in a transient container.

@eikenb
Copy link
Contributor Author

eikenb commented Apr 21, 2020

I'm going to update the PR to tweak to documentation to mention the 0s/fire-n-forget case.

@eikenb
Copy link
Contributor Author

eikenb commented Apr 21, 2020

@findkim I updated the PR with the documentation change.

Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this functionally by adding varying sleeps as commands, all which ran and then Consul Template notified the render event. Thanks for this fix!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug vault Related to the Vault integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event vs Command race when used as library
3 participants