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

Optionally send SIGTERM after reload. #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,8 @@ are [documented here](Longhelp.md#templates).
>
< HTTP/1.1 200 OK
```
* Some of the features of marathon-lb assume that it is the only instance of itself running in a PID namespace. i.e. marathon-lb assumes that it is running in a container. Certain features like the `/_mlb_signal` endpoints and the `/_haproxy_getpids` endpoint (and by extension, zero-downtime deployments) may behave unexpectedly if more than one instance of marathon-lb is running in the same PID namespace or if there are other HAProxy processes in the same PID namespace.
* Some of the features of marathon-lb assume that it is the only instance of itself running in a PID namespace. i.e. marathon-lb assumes that it is running in a container. Certain features like the `/_mlb_signal` endpoints and the `/_haproxy_getpids` endpoint (and by extension, zero-downtime deployments) may behave unexpectedly if more than one instance of marathon-lb is running in the same PID namespace or if there are other HAProxy processes in the same PID namespace.
* You may want to set the `HAPROXY_RELOAD_SIGTERM_DELAY` environment variable to a value such as `5m`. This value is passed directly to the `sleep` command, which is executed after every HAProxy reload before sending a SIGTERM to the old HAProxy PIDs (see [service/haproxy/run](service/haproxy/run)). For cases where you expect long-lived TCP connections, you may _not_ want to terminate HAProxy before all connections finish. See [this discussion](http://www.serverphorums.com/read.php?10,862139) for more on HAProxy reloads, and issues [#5](https://github.com/mesosphere/marathon-lb/issues/5), [#71](https://github.com/mesosphere/marathon-lb/issues/71), [#267](https://github.com/mesosphere/marathon-lb/issues/267), [#276](https://github.com/mesosphere/marathon-lb/issues/276), and [#318](https://github.com/mesosphere/marathon-lb/issues/318) for more. If you are reloading so frequently that PIDs are being reused within the delay you specify, this may result in SIGTERMs being sent to the wrong PIDs.

## Zero-downtime Deployments

Expand Down
5 changes: 5 additions & 0 deletions run
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ else
exit 1
fi

if [ -n "${HAPROXY_RELOAD_SIGTERM_DELAY-}" ]; then
echo $HAPROXY_RELOAD_SIGTERM_DELAY > $HAPROXY_SERVICE/env/HAPROXY_RELOAD_SIGTERM_DELAY
fi


# Find the --ssl-certs arg if one was provided,
# get the certs and remove them and the arg from the list
# of positional parameters so we don't duplicate them
Expand Down
6 changes: 5 additions & 1 deletion service/haproxy/run
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ reload() {
socat /var/run/haproxy/socket - <<< "show servers state" > /var/state/haproxy/global

# Trigger reload
haproxy -p $PIDFILE -f /marathon-lb/haproxy.cfg -D -sf $(pidof haproxy)
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 •
edited
Loading

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 •
edited
Loading

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.

fi

# Remove the firewall rules
removeFirewallRules
Expand Down