Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

Optionally send SIGTERM after reload. #321

Merged
merged 1 commit into from
Sep 29, 2016
Merged

Conversation

brndnmtthws
Copy link
Contributor

This is to address issues #5, #71, #267, #276, and #318.

HAPROXY_PIDS=$(pidof haproxy)
haproxy -p $PIDFILE -f /marathon-lb/haproxy.cfg -D -sf $HAPROXY_PIDS
if [ -n "${HAPROXY_RELOAD_SIGTERM_DELAY-}" ]; then
sleep $HAPROXY_RELOAD_SIGTERM_DELAY && kill $HAPROXY_PIDS 2> /dev/null &
Copy link

Choose a reason for hiding this comment

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

What is the purpose of & at the end of this line? The sleep runs in foreground and the kill runs in background, is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe that's the case. Test it yourself:

$ sleep 5s && echo finished &

Copy link
Contributor

@lloesche lloesche Sep 27, 2016

Choose a reason for hiding this comment

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

& and && are both posix shell pipeline separators. && executes sequentially until a nonzero status. & executes in parallel. If the purpose of the code is to wait $HAPROXY_RELOAD_SIGTERM_DELAY seconds before killing $HAPROXY_PIDS that's what it'll do, if it's parent run is still executing after the delay. If run was for whatever reason killed/restarted in the meantime the kill won't be executed.

Edit: Actually the kill might get executed depending on how the parent reaps it's children. I.e. in case of this bash script whether shopt huponexit is on or off. Likely also dependent on how runsv behaves.

Copy link

Choose a reason for hiding this comment

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

So sleep and kill are sequentially executed but kill runs in parallel with the parent. I guess my real question is: How will this affect the overall time it takes to perform a zdd deploy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. If you set the delay to 5m, then the old HAProxy processes will get reaped after 5 minutes, no matter what (and the deploy should proceed).

Copy link

@h0tbird h0tbird Sep 27, 2016

Choose a reason for hiding this comment

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

Ok, I was hoping for zdd to handle the killing of old HAProxy asynchronously but I don't know if this is doable because it might lead to a corrupted list of listeners.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Is this PR applicable only when we explicitly specify the reload command? Because the reload function here still uses default commands to reload if we don't specify anything explicitly.
  • Also we need to explicitely specify the haproxy config file path as /marathon-lb/haproxy.cfg as per this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this PR will only affect people who run MLB using the Docker image, or the built-in run script. I'll fix the haproxy.cfg thing in another PR.

@brndnmtthws brndnmtthws force-pushed the send-sigterm-after-reload branch from 806d93a to f1df667 Compare September 29, 2016 10:56
This is to address issues #5, #71, #267, #276, and #318.
@brndnmtthws brndnmtthws force-pushed the send-sigterm-after-reload branch from f1df667 to 568ec34 Compare September 29, 2016 11:14
@brndnmtthws brndnmtthws merged commit 1b82aa0 into master Sep 29, 2016
@brndnmtthws brndnmtthws deleted the send-sigterm-after-reload branch September 29, 2016 11:16
@vixns vixns mentioned this pull request Oct 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants