Re: [PATCH] Also accept SIGHUP/SIGTERM in systemd-wrapper
On 11 September 2014 07:44, Willy Tarreau w...@1wt.eu wrote: On Wed, Sep 10, 2014 at 10:38:55PM -0700, Matt Robenolt wrote: Awesome, thanks. :) Is it possible to also get this applied into the 1.5 branch since this is low risk and doesn???t break any backwards compatibility and whatnot? I've just backported it as well. 1.5 was still missing Conrad Hoffman's improved signal handling, but now both patches have been merged. Willy Iirc, the reason why I did not use SIGHUP for the reload (which I'd have preferred too) is that haproxy itself uses SIGHUP, and if I used it in the wrapper, it became a noop for haproxy. Maybe I did something wrong and with the actual state of it it works fine now, but did you check that haproxy still handles it properly?
Re: [PATCH] Also accept SIGHUP/SIGTERM in systemd-wrapper
Hi Marc-Antoine, On Thu, Sep 11, 2014 at 11:10:10AM +0200, Marc-Antoine Perennou wrote: On 11 September 2014 07:44, Willy Tarreau w...@1wt.eu wrote: On Wed, Sep 10, 2014 at 10:38:55PM -0700, Matt Robenolt wrote: Awesome, thanks. :) Is it possible to also get this applied into the 1.5 branch since this is low risk and doesn???t break any backwards compatibility and whatnot? I've just backported it as well. 1.5 was still missing Conrad Hoffman's improved signal handling, but now both patches have been merged. Willy Iirc, the reason why I did not use SIGHUP for the reload (which I'd have preferred too) is that haproxy itself uses SIGHUP, and if I used it in the wrapper, it became a noop for haproxy. Maybe I did something wrong and with the actual state of it it works fine now, but did you check that haproxy still handles it properly? Argh. Indeed that's a good reason. I must confess I never use it in haproxy so I hadn't thought about it. The SIGHUP is used to flush the memory pools. And I'm sure it will not do that anymore, instead it will reload haproxy. I don't know if that's a problem but I don't like much changing this behaviour in the stable branch. Matt, do you really need to use SIGHUP here ? For SIGTERM I understand, but SIGHUP is a bit different and less universal for that usage. Note, I still haven't pushed the patches to the public trees, so it's still time to revert/update them. Willy
Re: [PATCH] Also accept SIGHUP/SIGTERM in systemd-wrapper
Hmm, so right now this is a bit confusing. The wrapper doesn't pass along signals to the the actual haproxy process afaict, so I'm not sure that'd be an issue. If you needed to SIGHUP haproxy itself, you'd read the pid and whatnot and handle that. I look at this behavior as exactly what the init.d script is currently doing with `/etc/init.d/haproxy reload`. Using runit, there is a very specific semantic around what reload means, and that's by sending a SIGHUP. See: http://smarden.org/runit/sv.8.html There's no way to tell it to behave otherwise. Now, this isn't unsolvable because I *can* send the SIGUSR2 explicitly, but that just means that there's an oddball in a world of uniformity. Everything else we use works with a normal `sv reload ...` and haproxy handles `sv 2 ` instead. So yes, a SIGHUP is preferred. And to answer your original question, sending a SIGHUP to the wrapper today still doesn't flush haproxy's memory pools, does it? Unless I'm mistaken and the uncaught signal is somehow getting passed through, but I can't see how that's happening. So from my perspective, nothing is breaking or changing. On Thu, Sep 11, 2014 at 9:23 AM, Willy Tarreau w...@1wt.eu wrote: Hi Marc-Antoine, On Thu, Sep 11, 2014 at 11:10:10AM +0200, Marc-Antoine Perennou wrote: On 11 September 2014 07:44, Willy Tarreau w...@1wt.eu wrote: On Wed, Sep 10, 2014 at 10:38:55PM -0700, Matt Robenolt wrote: Awesome, thanks. :) Is it possible to also get this applied into the 1.5 branch since this is low risk and doesn???t break any backwards compatibility and whatnot? I've just backported it as well. 1.5 was still missing Conrad Hoffman's improved signal handling, but now both patches have been merged. Willy Iirc, the reason why I did not use SIGHUP for the reload (which I'd have preferred too) is that haproxy itself uses SIGHUP, and if I used it in the wrapper, it became a noop for haproxy. Maybe I did something wrong and with the actual state of it it works fine now, but did you check that haproxy still handles it properly? Argh. Indeed that's a good reason. I must confess I never use it in haproxy so I hadn't thought about it. The SIGHUP is used to flush the memory pools. And I'm sure it will not do that anymore, instead it will reload haproxy. I don't know if that's a problem but I don't like much changing this behaviour in the stable branch. Matt, do you really need to use SIGHUP here ? For SIGTERM I understand, but SIGHUP is a bit different and less universal for that usage. Note, I still haven't pushed the patches to the public trees, so it's still time to revert/update them. Willy
Re: [PATCH] Also accept SIGHUP/SIGTERM in systemd-wrapper
On Thu, Sep 11, 2014 at 04:35:40PM +, Matt Robenolt wrote: Hmm, so right now this is a bit confusing. The wrapper doesn't pass along signals to the the actual haproxy process afaict, so I'm not sure that'd be an issue. If you needed to SIGHUP haproxy itself, you'd read the pid and whatnot and handle that. Just looked at it, and you're right indeed. So Marc-Antoine, Matt's update doesn't make the existing behaviour worse considering that right now SIGHUP is not caught and causes the wrapper to die when it receives it (without killing haproxy). Thus I conclude that nobody currently uses SIGHUP on the wrapper, and thus that it is safe to catch it (and even safer than the current situation). Are you OK with this ? Thanks, Willy
[PATCH] Also accept SIGHUP/SIGTERM in systemd-wrapper
My proposal is to let haproxy-systemd-wrapper also accept normal
SIGHUP/SIGTERM signals to play nicely with other process managers
besides just systemd. In my use case, this will be for using with
runit which has to ability to change the signal used for a
reload or stop command. It also might be worth renaming this
bin to just haproxy-wrapper or something of that sort to separate
itself away from systemd. But that's a different discussion. :)
Thanks.
---
src/haproxy-systemd-wrapper.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/haproxy-systemd-wrapper.c b/src/haproxy-systemd-wrapper.c
index 90a94ce..cc8baa8 100644
--- a/src/haproxy-systemd-wrapper.c
+++ b/src/haproxy-systemd-wrapper.c
@@ -158,7 +158,9 @@ int main(int argc, char **argv)
memset(sa, 0, sizeof(struct sigaction));
sa.sa_handler = signal_handler;
sigaction(SIGUSR2, sa, NULL);
+ sigaction(SIGHUP, sa, NULL);
sigaction(SIGINT, sa, NULL);
+ sigaction(SIGTERM, sa, NULL);
if (getenv(REEXEC_FLAG) != NULL) {
/* We are being re-executed: restart HAProxy gracefully */
@@ -180,11 +182,11 @@ int main(int argc, char **argv)
status = -1;
while (-1 != wait(status) || errno == EINTR) {
- if (caught_signal == SIGUSR2) {
+ if (caught_signal == SIGUSR2 || caught_signal == SIGHUP) {
caught_signal = 0;
do_restart();
}
- else if (caught_signal == SIGINT) {
+ else if (caught_signal == SIGINT || caught_signal == SIGTERM) {
caught_signal = 0;
do_shutdown();
}
--
2.0.4
Re: [PATCH] Also accept SIGHUP/SIGTERM in systemd-wrapper
Hi Matt, On Thu, Sep 11, 2014 at 05:19:30AM +, Matt Robenolt wrote: My proposal is to let haproxy-systemd-wrapper also accept normal SIGHUP/SIGTERM signals to play nicely with other process managers besides just systemd. In my use case, this will be for using with runit which has to ability to change the signal used for a reload or stop command. It also might be worth renaming this bin to just haproxy-wrapper or something of that sort to separate itself away from systemd. But that's a different discussion. :) Thank you for this. I've got a recent report from someone who had to configure supervisord to use SIGINT instead of SIGTERM because of this. I agree that we should probably rename this wrapper. Another improvement would be to make it capable of only stripping -wrapper from its name to know what binary to call instead of searching the hardcoded haproxy. This is handy for people running multiple versions on the same system. I've applied your patch to 1.6. Thanks, Willy
Re: [PATCH] Also accept SIGHUP/SIGTERM in systemd-wrapper
Awesome, thanks. :) Is it possible to also get this applied into the 1.5 branch since this is low risk and doesn’t break any backwards compatibility and whatnot? -- Matt Robenolt @mattrobenolt On Thu, Sep 11, 2014 at 5:33 AM, Willy Tarreau w...@1wt.eu wrote: Hi Matt, On Thu, Sep 11, 2014 at 05:19:30AM +, Matt Robenolt wrote: My proposal is to let haproxy-systemd-wrapper also accept normal SIGHUP/SIGTERM signals to play nicely with other process managers besides just systemd. In my use case, this will be for using with runit which has to ability to change the signal used for a reload or stop command. It also might be worth renaming this bin to just haproxy-wrapper or something of that sort to separate itself away from systemd. But that's a different discussion. :) Thank you for this. I've got a recent report from someone who had to configure supervisord to use SIGINT instead of SIGTERM because of this. I agree that we should probably rename this wrapper. Another improvement would be to make it capable of only stripping -wrapper from its name to know what binary to call instead of searching the hardcoded haproxy. This is handy for people running multiple versions on the same system. I've applied your patch to 1.6. Thanks, Willy
Re: [PATCH] Also accept SIGHUP/SIGTERM in systemd-wrapper
On Wed, Sep 10, 2014 at 10:38:55PM -0700, Matt Robenolt wrote: Awesome, thanks. :) Is it possible to also get this applied into the 1.5 branch since this is low risk and doesn???t break any backwards compatibility and whatnot? I've just backported it as well. 1.5 was still missing Conrad Hoffman's improved signal handling, but now both patches have been merged. Willy
