scripts: improve panic helpers#3155
Conversation
An additonal argument can now be given to `_shutdown` to indicate immediate exit. The default has been changed to just wait for Supervisor to kill all processes (which fixes #3140). The panic handlers were adjusted accordingly and they now follow our naming convention.
They were replaced by the `_dms_panic__*` handlers, and where I deemed it safe, I added the `immediate` argument so we can shutdown fast.
georglauterbach
added
area/scripts
kind/improvement
|
Remember, when we once had the styling discussion? I wasn't a fan of putting everything in functions, even when they are only called once. This is the downside of it - the need for subshells to assign values 😉 But I have to admit, that I started liking it 😆 |
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
It might make sense to use a different approach and not think about the arg dance, or if blocking is important inverting the arg opt-in.
Feel free to merge if you disagree 👍
| if [[ ${2:-wait} == 'immediate' ]] | ||
| then | ||
| # In case the user requested an immediate exit, he ensure he is not in a subshell | ||
| # call and exiting the whole script is safe. This way, we make the shutdown quicker. | ||
| exit 1 | ||
| else | ||
| # We can simply wait until Supervisord has terminated all processes; this way, | ||
| # we do not return from a subshell call and continue as if nothing happened. | ||
| sleep 1000 | ||
| fi |
There was a problem hiding this comment.
Would it not be cleaner to approach with the first panic call (_shutdown() rather) creating some state like /tmp/panic, and just checking if this file exists to block with the sleep 1000?
No need for the immediate arg then, which avoids calls with empty arg before it for scope.
At least that seems simpler and pretty much always the behaviour desired? (exit 1 first call, any subsequent calls block)
In the cited failure case, the log should stop where it expected from the looks of it.
I understand the drawback would be scripts continue to run after the first exit 1? But I don't quite understand how that exit 1 interacts with the scope of the ssl.sh helper, these are all panic calls within the same method, so what happens if sleep 1000 was at the end of the panic call? It always hits the sleep 1000?
We are avoiding the immediate arg only for ssl.sh and dns.sh, if keeping the arg approach, it may be simpler to invert this arg to opt-in to blocking instead?
There was a problem hiding this comment.
I chose not to take the file approach because I really dislike scripts continuing after _shutdown was called (potentially causing behaviour we do not expect at all). I would definitely avoid going down UB-lane.
This is also the reason for the arg dance; using sleep is the conservative but also very safe method. Hence, it should be the default. I am not a big fan of '' args either, but it's working fine.
I'd keep it as is for now. If we ever feel the need to tackle this again, I will assist.
I provided feedback to your comment :) Hope that helps in explaining my choices.
I think this is fine for now :) |
3 participants
Description
_shutdownwil now wait for Supervisord by default. Added option to exit immedaitely though. Adjusted panic handlers; made them more coherent and added one more panic type.Fixes #3140
Type of change
Checklist:
docs/)