Improve process shutdown handling #462
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If killing just the forked PID is not working, try to kill the entire process group. This should help in scenarios where a wrapper script is used, which doesn't pass the signal to all of its children.
mpfz0r
force-pushed
the
fix/restart-loop
branch
from
db2b08e to
8d90c8b
Compare
This needed a bigger refactoring to deal with import cycles: - Move exec_helpers to daemon package - Extract some helper functions into new helper package
Some collectors might be started wit a wrapper script, which runs the collector with elevated privileges (e.g. using sudo). Such collectors might not be killed from the sidecar. The restart func had to timeout loop to ignore that, but the logic was wrong and it would loop indefininatly once it passed the 5 second mark. Furthermore, the existing goroutine that is calling Wait() on the process might mess up the running state of the restarted collector. Thus we provide a channel that will abort the goroutine. Also improve the status of the backands reporting to Graylog. The GetRotatedLog() writers were closed immideatly after creating them. This had no effect and can be removed.
|
Fixes #463 |
Timeout after 30 seconds
And fix early review comments from @thll
thll
requested changes
Jan 6, 2023
bernd
reviewed
Jan 9, 2023
- Use SIGTERM instead of SIGHUP - Immediatly signal the process group instead of the process Co-authored-by: Bernd Ahlers <[email protected]>
so we give the cmd.Wait() a chance to detect the finished process.
It's enough to have one wait loop in stop()
Otherwise we could only restart() hanging processes. stop() and start() would still fail.
`collector_shutdown_timeout: "10s"`
thll
approved these changes
Jan 9, 2023
We don't need a separate timeout here, because each runner is guaranteed to stop with their individual stop timeouts.
bernd
approved these changes
Jan 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are scenarios in which the sidecar handles stopping / restarting it's forked collectors poorly:
This PR tries to improve handling this as much as possible.
Instead of killing just the forked PID, it also resorts to killing the entire process group.
This should cover case
1For case
2the sidecar can only avoid waiting forever on a process it is not allowed to kill. (lack of permissions)This is fixed by adjusting the timeout loop and introducing a terminate channel to abort the goroutine that is endlessly wait(2)ing.
The timeout between sending a
SIGTERMandSIGKILLcan be adjusted with a new configration option:collector_shutdown_timeout: "10s"In addition:
This had no effect and can be removed.