Re: [PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[Lukas Tribus]

Hello,


2017-11-21 11:18 GMT+01:00 Willy Tarreau :
>> That's not it, the hold-off timer is only a consequence of this
>> problem.
>
> OK but if it's really 100ms, it can be a problem for people loading GeoIP
> maps of millions of entries, or large configs (the largest I saw had 30
> backends and 60 servers and took several seconds to process).

The default timeout to start a daemon is 90 seconds if I understand correctly:
https://www.freedesktop.org/software/systemd/man/systemd-system.conf.html#DefaultTimeoutStartSec=

The 100ms "timeout" limits the restart interval itself, so those
defaults should be sane.



Regards,
Lukas

Re: [PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[William Lallemand]

On Tue, Nov 21, 2017 at 11:28:55AM +0100, Willy Tarreau wrote:
> On Tue, Nov 21, 2017 at 11:19:41AM +0100, William Lallemand wrote:
> > I don't like the idea of the "daemon" keyword being ignored,
> 
> We already do that with -D which ignores "debug" for example. The command
> line purposely has precedence over the config.
> 
> > however, we could
> > exit with an error when we try to start with -Ws + -D or daemon.
> 
> Errors only cause gray hair to users, especially when they concern stuff
> that can be automatically recovered from. We could definitely consider
> that -Ws automatically implies MODE_FOREGROUND which will do exactly
> what -db does, but without having to think about it. After all, "-Ws"
> is "master-worker mode with systemd awareness" so it makes sense not
> to background in this case.
> 

That's right, I agree.

-- 
William Lallemand

Re: [PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[William Lallemand]

On Tue, Nov 21, 2017 at 11:12:45AM +0100, Lukas Tribus wrote:
> Hello,
> 
> 2017-11-21 8:39 GMT+01:00 William Lallemand :
> 
> That's not it, the hold-off timer is only a consequence of this
> problem. I believe the notification does not work in my case, which is
> why for systemd haproxy did not start correctly which is why systemd
> continues to restart haproxy.
> I found the root of the issue: the "daemon" keyword in the global
> configuration section. We need to remove "daemon" from the
> configuration for systemd to mode work correctly in notify mode.
> 
> We can override the "daemon" keyword in the configuration by passing
> -db to haproxy. Adding it to the unit file fixes the issue even if
> they keyword is in the configuration (so we pass "-Ws -db" ).
> 
> "-Ws -db" along with a note in the documentation that "daemon" may be
> ignored in systemd mode seems like a good compromise here? I will send
> an RFC patch shortly.
> 

You should never use systemd with the daemon mode, either in notify or default
mode, it's not the recommended setup.

I don't like the idea of the "daemon" keyword being ignored, however, we could
exit with an error when we try to start with -Ws + -D or daemon.

-- 
William Lallemand

Re: [PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[Willy Tarreau]

Hi Lukas,

On Tue, Nov 21, 2017 at 11:12:45AM +0100, Lukas Tribus wrote:
> > I suggest we configure it to a greater value per default, it can be set to
> > infinity too, but I don't like the idea.
> 
> That's not it, the hold-off timer is only a consequence of this
> problem.

OK but if it's really 100ms, it can be a problem for people loading GeoIP
maps of millions of entries, or large configs (the largest I saw had 30
backends and 60 servers and took several seconds to process).

> I believe the notification does not work in my case, which is
> why for systemd haproxy did not start correctly which is why systemd
> continues to restart haproxy.

I'm feeling less and less confident in using this mode by default :-/

> I found the root of the issue: the "daemon" keyword in the global
> configuration section. We need to remove "daemon" from the
> configuration for systemd to mode work correctly in notify mode.
> 
> We can override the "daemon" keyword in the configuration by passing
> -db to haproxy. Adding it to the unit file fixes the issue even if
> they keyword is in the configuration (so we pass "-Ws -db" ).
> 
> "-Ws -db" along with a note in the documentation that "daemon" may be
> ignored in systemd mode seems like a good compromise here?

Yes and it definitely is needed, just like we used to suggest users to
always have -D in init scripts to force backgrounding of the process
when there was no "deamon" statement in the conf.

> I will send an RFC patch shortly.

Thank you!
Willy

Re: [PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[Lukas Tribus]

Hello,



2017-11-21 8:39 GMT+01:00 William Lallemand :
> On Tue, Nov 21, 2017 at 07:16:19AM +0100, Willy Tarreau wrote:
>>
>> I really don't like this. My fears with becoming more systemd-friendly
>> was that we'd make users helpless when something decides not to work
>> just to annoy them, and this reports seems to confirm this feeling :-/
>>
>> Tim, have you seen this message about a "hold-off timer over" being
>> displayed at the same second as the startup message ? Isn't there a
>> timeout setting or something like this to place in the config file ?
>> And is there a way to disable it so that people with huge configs
>> are still allowed to load them ?
>>
>
> There is indeed a timeout value, which is configurable in the unit file, the
> default value is 100ms.
>
> https://www.freedesktop.org/software/systemd/man/systemd.service.html#RestartSec=
>
>
> There is also the start one there:
>
> https://www.freedesktop.org/software/systemd/man/systemd.service.html#TimeoutStartSec=
>
> I suggest we configure it to a greater value per default, it can be set to 
> infinity too, but I don't like the idea.

That's not it, the hold-off timer is only a consequence of this
problem. I believe the notification does not work in my case, which is
why for systemd haproxy did not start correctly which is why systemd
continues to restart haproxy.
I found the root of the issue: the "daemon" keyword in the global
configuration section. We need to remove "daemon" from the
configuration for systemd to mode work correctly in notify mode.

We can override the "daemon" keyword in the configuration by passing
-db to haproxy. Adding it to the unit file fixes the issue even if
they keyword is in the configuration (so we pass "-Ws -db" ).

"-Ws -db" along with a note in the documentation that "daemon" may be
ignored in systemd mode seems like a good compromise here? I will send
an RFC patch shortly.



Regards,
Lukas

Re: [PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[William Lallemand]

On Tue, Nov 21, 2017 at 07:16:19AM +0100, Willy Tarreau wrote:
> 
> I really don't like this. My fears with becoming more systemd-friendly
> was that we'd make users helpless when something decides not to work
> just to annoy them, and this reports seems to confirm this feeling :-/
> 
> Tim, have you seen this message about a "hold-off timer over" being
> displayed at the same second as the startup message ? Isn't there a
> timeout setting or something like this to place in the config file ?
> And is there a way to disable it so that people with huge configs
> are still allowed to load them ?
> 

There is indeed a timeout value, which is configurable in the unit file, the
default value is 100ms.

https://www.freedesktop.org/software/systemd/man/systemd.service.html#RestartSec=
 

There is also the start one there:

https://www.freedesktop.org/software/systemd/man/systemd.service.html#TimeoutStartSec=

I suggest we configure it to a greater value per default, it can be set to 
infinity too, but I don't like the idea.

-- 
William Lallemand

Re: [PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[Willy Tarreau]

Hi Lukas,

On Tue, Nov 21, 2017 at 01:12:10AM +0100, Lukas Tribus wrote:
> 2017-11-20 15:58 GMT+01:00 Tim Düsterhus :
> > From: Tim Duesterhus 
> >
> > This patch adds support for `Type=notify` to the systemd unit.
> >
> > Supporting `Type=notify` improves both starting as well as reloading
> > of the unit, because systemd will be let known when the action completed.
> 
> I can't start haproxy anymore this way.
> 
> I compiled with USE_SYSTEMD, updated the unit file with both changes,
> and reloaded systemd (systemctl daemon-reload), but systemd aborts the
> start for some reason:
> 
> 
> Nov 21 00:46:20 www systemd[1]: Starting HAProxy Load Balancer...
> Nov 21 00:46:20 www haproxy[814]: [WARNING] 324/004620 (814) : parsing
> [/etc/haproxy/haproxy.cfg:118] : a 'http-request' rule placed after a
> 'reqxxx' rule will still be processed before.
> [...]
> Nov 21 00:46:20 www haproxy[814]: Proxy port443 started.
> Nov 21 00:46:20 www haproxy[814]: Proxy port443 started.
> [...]
> Nov 21 00:46:20 www systemd[1]: Started HAProxy Load Balancer.
> Nov 21 00:46:20 www systemd[1]: haproxy.service: Service hold-off time
> over, scheduling restart.
> Nov 21 00:46:20 www systemd[1]: Stopped HAProxy Load Balancer.
> Nov 21 00:46:20 www systemd[1]: haproxy.service: Start request
> repeated too quickly.
> Nov 21 00:46:20 www systemd[1]: Failed to start HAProxy Load Balancer.
> 
> 
> Reverting back to "Type=forking" (while still using -Ws and
> USE_SYSTEMD=1) works for me. This is ubuntu xenial (16.04) with
> systemd 229. No multiprocess mode, no threading mode used. My
> configuration does produce its fair share of config warnings though,
> but that should not matter. Any troubleshooting advice for me here?

I really don't like this. My fears with becoming more systemd-friendly
was that we'd make users helpless when something decides not to work
just to annoy them, and this reports seems to confirm this feeling :-/

Tim, have you seen this message about a "hold-off timer over" being
displayed at the same second as the startup message ? Isn't there a
timeout setting or something like this to place in the config file ?
And is there a way to disable it so that people with huge configs
are still allowed to load them ?

Regards,
willy

Re: [PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[Lukas Tribus]

Hello Tim,



2017-11-20 15:58 GMT+01:00 Tim Düsterhus :
> From: Tim Duesterhus 
>
> This patch adds support for `Type=notify` to the systemd unit.
>
> Supporting `Type=notify` improves both starting as well as reloading
> of the unit, because systemd will be let known when the action completed.

I can't start haproxy anymore this way.

I compiled with USE_SYSTEMD, updated the unit file with both changes,
and reloaded systemd (systemctl daemon-reload), but systemd aborts the
start for some reason:


Nov 21 00:46:20 www systemd[1]: Starting HAProxy Load Balancer...
Nov 21 00:46:20 www haproxy[814]: [WARNING] 324/004620 (814) : parsing
[/etc/haproxy/haproxy.cfg:118] : a 'http-request' rule placed after a
'reqxxx' rule will still be processed before.
[...]
Nov 21 00:46:20 www haproxy[814]: Proxy port443 started.
Nov 21 00:46:20 www haproxy[814]: Proxy port443 started.
[...]
Nov 21 00:46:20 www systemd[1]: Started HAProxy Load Balancer.
Nov 21 00:46:20 www systemd[1]: haproxy.service: Service hold-off time
over, scheduling restart.
Nov 21 00:46:20 www systemd[1]: Stopped HAProxy Load Balancer.
Nov 21 00:46:20 www systemd[1]: haproxy.service: Start request
repeated too quickly.
Nov 21 00:46:20 www systemd[1]: Failed to start HAProxy Load Balancer.


Reverting back to "Type=forking" (while still using -Ws and
USE_SYSTEMD=1) works for me. This is ubuntu xenial (16.04) with
systemd 229. No multiprocess mode, no threading mode used. My
configuration does produce its fair share of config warnings though,
but that should not matter. Any troubleshooting advice for me here?



Thanks,
Lukas

Re: [PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[William Lallemand]

On Mon, Nov 20, 2017 at 03:58:35PM +0100, Tim Düsterhus wrote:
> From: Tim Duesterhus 
> 
> This patch adds support for `Type=notify` to the systemd unit.
 
Looks good to me, thanks!

Willy can you merge it?

-- 
William Lallemand

[PATCH v3 1/1] MEDIUM: mworker: Add systemd `Type=notify` support

[Tim Düsterhus]

From: Tim Duesterhus 

This patch adds support for `Type=notify` to the systemd unit.

Supporting `Type=notify` improves both starting as well as reloading
of the unit, because systemd will be let known when the action completed.

See this quote from `systemd.service(5)`:
> Note however that reloading a daemon by sending a signal (as with the
> example line above) is usually not a good choice, because this is an
> asynchronous operation and hence not suitable to order reloads of
> multiple services against each other. It is strongly recommended to
> set ExecReload= to a command that not only triggers a configuration
> reload of the daemon, but also synchronously waits for it to complete.

By making systemd aware of a reload in progress it is able to wait until
the reload actually succeeded.

This patch introduces both a new `USE_SYSTEMD` build option which controls
including the sd-daemon library as well as a `-Ws` runtime option which
runs haproxy in master-worker mode with systemd support.

When haproxy is running in master-worker mode with systemd support it will
send status messages to systemd using `sd_notify(3)` in the following cases:

- The master process forked off the worker processes (READY=1)
- The master process entered the `mworker_reload()` function (RELOADING=1)
- The master process received the SIGUSR1 or SIGTERM signal (STOPPING=1)

Change the unit file to specify `Type=notify` and replace master-worker
mode (`-W`) with master-worker mode with systemd support (`-Ws`).

Future evolutions of this feature could include making use of the `STATUS`
feature of `sd_notify()` to send information about the number of active
connections to systemd. This would require bidirectional communication
between the master and the workers and thus is left for future work.
---
 Makefile   |  7 +++
 contrib/systemd/haproxy.service.in |  4 ++--
 include/types/global.h |  1 +
 src/haproxy.c  | 29 +
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 200a5f60d..2a2a21ff9 100644
--- a/Makefile
+++ b/Makefile
@@ -43,6 +43,7 @@
 #   USE_DEVICEATLAS  : enable DeviceAtlas api.
 #   USE_51DEGREES: enable third party device detection library from 
51Degrees
 #   USE_WURFL: enable WURFL detection library from Scientiamobile
+#   USE_SYSTEMD  : enable sd_notify() support.
 #
 # Options can be forced by specifying "USE_xxx=1" or can be disabled by using
 # "USE_xxx=" (empty string).
@@ -700,6 +701,12 @@ BUILD_OPTIONS   += $(call ignore_implicit,USE_WURFL)
 OPTIONS_LDFLAGS += $(if $(WURFL_LIB),-L$(WURFL_LIB)) -lwurfl
 endif
 
+ifneq ($(USE_SYSTEMD),)
+BUILD_OPTIONS   += $(call ignore_implicit,USE_SYSTEMD)
+OPTIONS_CFLAGS  += -DUSE_SYSTEMD
+OPTIONS_LDFLAGS += -lsystemd
+endif
+
 ifneq ($(USE_PCRE)$(USE_STATIC_PCRE)$(USE_PCRE_JIT),)
 ifneq ($(USE_PCRE2)$(USE_STATIC_PCRE2)$(USE_PCRE2_JIT),)
 $(error cannot compile both PCRE and PCRE2 support)
diff --git a/contrib/systemd/haproxy.service.in 
b/contrib/systemd/haproxy.service.in
index 81b4951df..edbd4c292 100644
--- a/contrib/systemd/haproxy.service.in
+++ b/contrib/systemd/haproxy.service.in
@@ -7,12 +7,12 @@ After=network.target
 # socket if you want seamless reloads.
 Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid"
 ExecStartPre=@SBINDIR@/haproxy -f $CONFIG -c -q
-ExecStart=@SBINDIR@/haproxy -W -f $CONFIG -p $PIDFILE
+ExecStart=@SBINDIR@/haproxy -Ws -f $CONFIG -p $PIDFILE
 ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q
 ExecReload=/bin/kill -USR2 $MAINPID
 KillMode=mixed
 Restart=always
-Type=forking
+Type=notify
 
 [Install]
 WantedBy=multi-user.target
diff --git a/include/types/global.h b/include/types/global.h
index 6793c83cd..4440179fc 100644
--- a/include/types/global.h
+++ b/include/types/global.h
@@ -63,6 +63,7 @@
 #define GTUNE_USE_GAI(1<<5)
 #define GTUNE_USE_REUSEPORT  (1<<6)
 #define GTUNE_RESOLVE_DONTFAIL   (1<<7)
+#define GTUNE_USE_SYSTEMD(1<<10)
 
 #define GTUNE_SOCKET_TRANSFER   (1<<8)
 #define GTUNE_EXIT_ONFAILURE (1<<9)
diff --git a/src/haproxy.c b/src/haproxy.c
index ba5a4b208..b39a95fe3 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -61,6 +61,9 @@
 #ifdef DEBUG_FULL
 #include 
 #endif
+#if defined(USE_SYSTEMD)
+#include 
+#endif
 
 #include 
 #include 
@@ -404,6 +407,9 @@ static void usage(char *name)
"-V enters verbose mode (disables quiet mode)\n"
"-D goes daemon ; -C changes to  before loading 
files.\n"
"-W master-worker mode.\n"
+#if defined(USE_SYSTEMD)
+   "-Ws master-worker mode with systemd notify support.\n"
+#endif
"-q quiet mode : don't display messages\n"
"-c check mode : only check config files and exit\n"
"-n sets the maximum total # of connections