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

[Tim Düsterhus]

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.

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 a new `USE_SYSTEMD` option which controls including
the sd-daemon library.

When `USE_SYSTEMD` is enabled and haproxy is running in mworker mode it will
send status messages to systemd using `sd_notify()` 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)

Status messages are not sent when not using the mworker mode, because using
mworker is recommended when using systemd.

Change the unit file to specify `Type=notify` as well as `KillSignal=USR1` to
support cleaner shutdowns. Systemd will kill haproxy using SIGKILL if
connections remain after 90 seconds (by default). Remove KillSignal=USR1 if
the SIGKILL is undesired.

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 | 6 ++++++
contrib/systemd/haproxy.service.in | 3 ++-
src/haproxy.c | 16 ++++++++++++++++
3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 200a5f60d..e1eab0c2e 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).
@@ -608,6 +609,11 @@ endif
OPTIONS_OBJS += src/ssl_sock.o
endif

+ifneq ($(USE_SYSTEMD),)
+OPTIONS_CFLAGS += -DUSE_SYSTEMD
+OPTIONS_LDFLAGS += -lsystemd
+endif
+
# The private cache option affect the way the shctx is built
ifneq ($(USE_PRIVATE_CACHE),)
OPTIONS_CFLAGS += -DUSE_PRIVATE_CACHE
diff --git a/contrib/systemd/haproxy.service.in b/contrib/systemd/haproxy.service.in
index 81b4951df..895e3b036 100644
--- a/contrib/systemd/haproxy.service.in
+++ b/contrib/systemd/haproxy.service.in
@@ -11,8 +11,9 @@ ExecStart=@SBINDIR@/haproxy -W -f $CONFIG -p $PIDFILE
ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q
ExecReload=/bin/kill -USR2 $MAINPID
KillMode=mixed
+KillSignal=USR1
Restart=always
-Type=forking
+Type=notify

[Install]
WantedBy=multi-user.target
diff --git a/src/haproxy.c b/src/haproxy.c
index ba5a4b208..63c29023d 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -61,6 +61,9 @@
#ifdef DEBUG_FULL
#include <assert.h>
#endif
+#ifdef USE_SYSTEMD
+#include <systemd/sd-daemon.h>
+#endif

#include <common/base64.h>
#include <common/cfgparse.h>
@@ -635,6 +638,9 @@ static void mworker_reload()

mworker_block_signals();
mworker_unregister_signals();
+#if USE_SYSTEMD
+ sd_notify(0, "RELOADING=1");
+#endif
setenv("HAPROXY_MWORKER_REEXEC", "1", 1);

/* compute length */
@@ -682,6 +688,7 @@ static void mworker_reload()
return;

alloc_error:
+
Warning("Failed to reexecute the master processs [%d]: Cannot allocate memory\n", pid);
return;
}
@@ -698,6 +705,10 @@ static void mworker_wait()

restart_wait:

+#if USE_SYSTEMD
+ sd_notifyf(0, "READY=1\nMAINPID=%lu", (unsigned long)getpid());
+#endif
+
mworker_register_signals();
mworker_unblock_signals();

@@ -710,6 +721,11 @@ restart_wait:
/* should reach there only if it fail */
goto restart_wait;
} else {
+#if USE_SYSTEMD
+ if (sig == SIGUSR1 || sig == SIGTERM) {
+ sd_notify(0, "STOPPING=1");
+ }
+#endif
Warning("Exiting Master process...\n");
mworker_kill(sig);
mworker_unregister_signals();

[Aleksandar Lazic]

Hi Tim.

------ Originalnachricht ------
Von: "Tim Düsterhus" <***@bastelstu.be>
An: ***@formilux.org
Cc: ***@haproxy.com; "Tim Düsterhus" <***@bastelstu.be>
Gesendet: 18.11.2017 20:02:23
Betreff: [PATCH] MEDIUM: mworker: Add systemd `Type=notify` support

Post by Tim Düsterhus
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.

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 a new `USE_SYSTEMD` option which controls
including
the sd-daemon library.
When `USE_SYSTEMD` is enabled and haproxy is running in mworker mode it will
- 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)
Status messages are not sent when not using the mworker mode, because using
mworker is recommended when using systemd.
Change the unit file to specify `Type=notify` as well as
`KillSignal=USR1` to
support cleaner shutdowns. Systemd will kill haproxy using SIGKILL if
connections remain after 90 seconds (by default). Remove
KillSignal=USR1 if
the SIGKILL is undesired.
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 | 6 ++++++
contrib/systemd/haproxy.service.in | 3 ++-
src/haproxy.c | 16 ++++++++++++++++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 200a5f60d..e1eab0c2e 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).
@@ -608,6 +609,11 @@ endif
OPTIONS_OBJS += src/ssl_sock.o
endif
+ifneq ($(USE_SYSTEMD),)
+OPTIONS_CFLAGS += -DUSE_SYSTEMD
+OPTIONS_LDFLAGS += -lsystemd
+endif
+
# The private cache option affect the way the shctx is built
ifneq ($(USE_PRIVATE_CACHE),)
OPTIONS_CFLAGS += -DUSE_PRIVATE_CACHE

Please can you add this flag also to BUILD_OPTIONS so that we can see
at haproxy -vv that it was build with USE_SYSTEMD

Thanks

Post by Tim Düsterhus
diff --git a/contrib/systemd/haproxy.service.in
b/contrib/systemd/haproxy.service.in
index 81b4951df..895e3b036 100644
--- a/contrib/systemd/haproxy.service.in
+++ b/contrib/systemd/haproxy.service.in
@@ -11,8 +11,9 @@ ExecStart=@SBINDIR@/haproxy -W -f $CONFIG -p $PIDFILE
ExecReload=/bin/kill -USR2 $MAINPID
KillMode=mixed
+KillSignal=USR1
Restart=always
-Type=forking
+Type=notify
[Install]
WantedBy=multi-user.target
diff --git a/src/haproxy.c b/src/haproxy.c
index ba5a4b208..63c29023d 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -61,6 +61,9 @@
#ifdef DEBUG_FULL
#include <assert.h>
#endif
+#ifdef USE_SYSTEMD
+#include <systemd/sd-daemon.h>
+#endif
#include <common/base64.h>
#include <common/cfgparse.h>
@@ -635,6 +638,9 @@ static void mworker_reload()
mworker_block_signals();
mworker_unregister_signals();
+#if USE_SYSTEMD
+ sd_notify(0, "RELOADING=1");
+#endif
setenv("HAPROXY_MWORKER_REEXEC", "1", 1);
/* compute length */
@@ -682,6 +688,7 @@ static void mworker_reload()
return;
+
Warning("Failed to reexecute the master processs [%d]: Cannot allocate memory\n", pid);
return;
}
@@ -698,6 +705,10 @@ static void mworker_wait()
+#if USE_SYSTEMD
+ sd_notifyf(0, "READY=1\nMAINPID=%lu", (unsigned long)getpid());
+#endif
+
mworker_register_signals();
mworker_unblock_signals();
/* should reach there only if it fail */
goto restart_wait;
} else {
+#if USE_SYSTEMD
+ if (sig == SIGUSR1 || sig == SIGTERM) {
+ sd_notify(0, "STOPPING=1");
+ }
+#endif
Warning("Exiting Master process...\n");
mworker_kill(sig);
mworker_unregister_signals();
--
2.15.0

[Tim Düsterhus]

From: Tim Duesterhus <***@bastelstu.be>

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.

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 a new `USE_SYSTEMD` option which controls including
the sd-daemon library.

When `USE_SYSTEMD` is enabled and haproxy is running in mworker mode it will
send status messages to systemd using `sd_notify()` 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)

Status messages are not sent when not using the mworker mode, because using
mworker is recommended when using systemd.

Change the unit file to specify `Type=notify` as well as `KillSignal=USR1` to
support cleaner shutdowns. Systemd will kill haproxy using SIGKILL if
connections remain after 90 seconds (by default). Remove KillSignal=USR1 if
the SIGKILL is undesired.

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 | 3 ++-
src/haproxy.c | 15 +++++++++++++++
3 files changed, 24 insertions(+), 1 deletion(-)

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..895e3b036 100644
--- a/contrib/systemd/haproxy.service.in
+++ b/contrib/systemd/haproxy.service.in
@@ -11,8 +11,9 @@ ExecStart=@SBINDIR@/haproxy -W -f $CONFIG -p $PIDFILE
ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q
ExecReload=/bin/kill -USR2 $MAINPID
KillMode=mixed
+KillSignal=USR1
Restart=always
-Type=forking
+Type=notify

[Install]
WantedBy=multi-user.target
diff --git a/src/haproxy.c b/src/haproxy.c
index ba5a4b208..3bbd6f2b7 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -61,6 +61,9 @@
#ifdef DEBUG_FULL
#include <assert.h>
#endif
+#ifdef USE_SYSTEMD
+#include <systemd/sd-daemon.h>
+#endif

#include <common/base64.h>
#include <common/cfgparse.h>
@@ -635,6 +638,9 @@ static void mworker_reload()

mworker_block_signals();
mworker_unregister_signals();
+#if USE_SYSTEMD
+ sd_notify(0, "RELOADING=1");
+#endif
setenv("HAPROXY_MWORKER_REEXEC", "1", 1);

/* compute length */
@@ -698,6 +704,10 @@ static void mworker_wait()

restart_wait:

+#if USE_SYSTEMD
+ sd_notifyf(0, "READY=1\nMAINPID=%lu", (unsigned long)getpid());
+#endif
+
mworker_register_signals();
mworker_unblock_signals();

@@ -710,6 +720,11 @@ restart_wait:
/* should reach there only if it fail */
goto restart_wait;
} else {
+#if USE_SYSTEMD
+ if (sig == SIGUSR1 || sig == SIGTERM) {
+ sd_notify(0, "STOPPING=1");
+ }
+#endif
Warning("Exiting Master process...\n");
mworker_kill(sig);
mworker_unregister_signals();

[Willy Tarreau]

Hi Aleks,

Maybe my client is crap but I see the patches inline.
Please can you be so kind and create a patch file which is attached to the
mail.

Well, inline patches are not a problem, they apply well (it's even the
original git way of doing it).

Tim, I'll discuss this with William. I'm a bit embarrassed with taking
this change right now while everyone is still working on fixing existing
issues. We may possibly end up postponing it for 1.9 and then backporting
it to existing versions once properly validated, or merging it anyway but
reverting it at the first problem report.

In the mean time, since you mentionned that building without USE_SYSTEMD
could cause problems, I think the unit file needs to at least get some
comments to indicate the values to use depending on the build options.
Also I don't know what systemd versions are found in field, but to be
honnest I don't feel much comfortable with the possibility that users
easily get the wrong unit file for their build options or the wrong
build options for their system, I already predict some reports here.
Maybe two distinct unit files should be provided, and instead of naming
the option "USE_SYSTEMD", it should be named "USE_SD_NOTIFY" to make it
clear what it implies ?

Thanks!
Willy

[Tim Düsterhus]

Willy,

Post by Willy Tarreau
Tim, I'll discuss this with William. I'm a bit embarrassed with taking
this change right now while everyone is still working on fixing existing
issues.

Understood. I see that my timing is unfortunate, but I just got my feet
wet with the `execvp` bugfix and wanted to try a bit more. :-)

Post by Willy Tarreau
We may possibly end up postponing it for 1.9 and then backporting
it to existing versions once properly validated, or merging it anyway but
reverting it at the first problem report.

I consider the patch to the C source code to be safe (as outlined
below). The patch to the unit file is up for discussion, because there
is no single good value (outlined below as well).

Post by Willy Tarreau
In the mean time, since you mentionned that building without USE_SYSTEMD
could cause problems, I think the unit file needs to at least get some
comments to indicate the values to use depending on the build options.

It really is an unfortunate situation, because the correct value depends
on the haproxy.cfg:

Type=simple (the default and what's provided for haproxy < 1.8):
This is safe iff `daemon` is *not* set.

Type=forking (what's provided for haproxy 1.8+):
This is safe iff `daemon` is set. This is a possible breakage with
haproxy 1.8+: See my initial email (Add systemd `Type=notify` support)
in this thread.

Type=notify (what my patch proposes):
This is safe iff my patch is compiled in. If `daemon` is set then
`NotifyAccess=all` will need to be configured in the unit file as well.
We can put `NotifyAccess=all` by default, but that has some security
implications as any process in haproxy's control group could send
messages to systemd.

My patch will not cause issues if `Type` is not equal to `notify`. Old
unit files will continue to work, even if it is compiled in. Quoting

Post by Willy Tarreau
RETURN VALUE
On failure, these calls return a negative errno-style error code. If
$NOTIFY_SOCKET was not set and hence no status data could be sent, 0 is
returned. If the status was sent, these functions return with a
positive return value. In order to support both, init systems that
implement this scheme and those which do not, it is generally
recommended to ignore the return value of this call.

So there is no value that is universally good. In my opinion the best
would be Type=notify, because it is not dependent on haproxy.cfg.

I don't know how large the percentage of users is that don't use their
distro provided haproxy. Personally I trust the distributions to add the
`USE_SYSTEMD` build option if they support systemd and then the "end
user" would not be able to break startup using haproxy.cfg alone.

Post by Willy Tarreau
Also I don't know what systemd versions are found in field, but to be
honnest I don't feel much comfortable with the possibility that users
easily get the wrong unit file for their build options or the wrong
build options for their system, I already predict some reports here.

See above on why there is no single good value. The `sd_notify()` API is
supported as of systemd v1, though. The commit documenting it is from 2010:

https://github.com/systemd/systemd/commit/f9378423b9758861850748aeb49ae0d3300e56e6

On Ubuntu I only had to install the `libsystemd-dev` package to get the
necessary headers.

Post by Willy Tarreau
Maybe two distinct unit files should be provided, and instead of naming
the option "USE_SYSTEMD", it should be named "USE_SD_NOTIFY" to make it
clear what it implies ?

I opted for USE_SYSTEMD, because the functionality is provided by
libsystemd. It also provides the `sd_listen_fds()` function if one wants
to build support for systemd's socket passing in the future. So
USE_SD_NOTIFY would be a bit of a misnomer. The build flag is only
required, because of the non-Linux or non-systemd systems. The function
calls themselves won't cause issues as outlined above.

I hope I was able to give a good overview so that you can decide on the
proper course going forward.

Thanks!
Tim Düsterhus

[Tim Düsterhus]

William,

I think a good way to activate this feature will be to use it with a -Ws command
line option instead of changing the behavior of the -W one.
This way we can integrate the -Ws command in the unit file without changing the
behavior of the normal option.

Just to make sure you understood correctly: The patch does not really
modify the behaviour of the normal option. `sd_notify(3)` is documented

RETURN VALUE
On failure, these calls return a negative errno-style error code. If
$NOTIFY_SOCKET was not set and hence no status data could be sent, 0 is
returned. *snip*

The only difference I can see is that haproxy will fail to start for a
different reason (use of undefined option, instead of not sending the
READY=1 notification) if one uses the provided unit file *without*
compiling with USE_SYSTEMD. Thus in my opinion a separate -Ws option
will increase cognitive load (which option should I use?) for next to no
benefit.

Best regards
Tim Düsterhus

[Willy Tarreau]

Hi Tim,

Post by Tim Düsterhus
William,

I think a good way to activate this feature will be to use it with a -Ws command
line option instead of changing the behavior of the -W one.
This way we can integrate the -Ws command in the unit file without changing the
behavior of the normal option.

Just to make sure you understood correctly: The patch does not really
modify the behaviour of the normal option. `sd_notify(3)` is documented

RETURN VALUE
On failure, these calls return a negative errno-style error code. If
$NOTIFY_SOCKET was not set and hence no status data could be sent, 0 is
returned. *snip*

The only difference I can see is that haproxy will fail to start for a
different reason (use of undefined option, instead of not sending the
READY=1 notification) if one uses the provided unit file *without*
compiling with USE_SYSTEMD. Thus in my opinion a separate -Ws option
will increase cognitive load (which option should I use?) for next to no
benefit.

But from what you explained (or at least what I understood), the mode
specified in the unit file needs to match the build option. This is
not really cool, especially for users who want to replace their distro's
binary without having to modify the settings. Like William, I tend to
think that it's best if the behaviour can be changed at runtime with
an option. In my opinion it's less cognitive load to know that when you
build, the existing unit file will automatically pass the appropriate
option for its own settings (ie -Ws when using type=notify or -W for
type=forking for example).

Willy

[Tim Düsterhus]

Willy,

Post by Willy Tarreau
But from what you explained (or at least what I understood), the mode
specified in the unit file needs to match the build option. This is

If `Type=notify` is set then `USE_SYSTEMD` must be provided at build
time, yes. But as I outlined before: The Type must *also* match the
configuration (because whether the alternatives work correctly depends
on whether `daemon` is set or not).

Post by Willy Tarreau
not really cool, especially for users who want to replace their distro's
binary without having to modify the settings. Like William, I tend to
think that it's best if the behaviour can be changed at runtime with
an option. In my opinion it's less cognitive load to know that when you
build, the existing unit file will automatically pass the appropriate
option for its own settings (ie -Ws when using type=notify or -W for
type=forking for example).

Is it possible to modify the unit file depending on the build flags?
Then I absolutely agree that `Type=notify` should be taken out if
`USE_SYSTEMD` is not set.

See my sibling mail for another suggestion to avoid the "silent failure"
when `USE_SYSTEMD` is not provided.

Best regards
Tim Düsterhus

[Tim Düsterhus]

William,

Post by Tim Düsterhus
The only difference I can see is that haproxy will fail to start for a
different reason (use of undefined option, instead of not sending the
READY=1 notification) if one uses the provided unit file *without*
compiling with USE_SYSTEMD.
Thus in my opinion a separate -Ws option will increase cognitive load (which
option should I use?) for next to no benefit.

Well, it's a matter of documentation, and a line to add in the usage() function
IMO.
This option ensures one thing, if the -Ws option is used in the unit file by
default, and if by any chance a binary was not built with the right
USE_SYSTEMD=1 option, it won't work at all preventing useless bug reports. We
can even put a Warning when trying to use this option when it's not built.

May I suggest the following: If haproxy is *not* compiled with the
`USE_SYSTEMD` option it checks for the existence of the `NOTIFY_SOCKET`
environment variable and refuses start up, if it is defined.

Then `Type=notify` will "just work" if haproxy is compiled with the
option and will emit a proper error message if it is not.

Best regards
Tim Düsterhus

[Tim Düsterhus]

William,

Post by Tim Düsterhus
May I suggest the following: If haproxy is *not* compiled with the
`USE_SYSTEMD` option it checks for the existence of the `NOTIFY_SOCKET`
environment variable and refuses start up, if it is defined.
Then `Type=notify` will "just work" if haproxy is compiled with the
option and will emit a proper error message if it is not.

If you're suggesting of doing this with -W, it's not a good idea, sometimes you
just want to start HAProxy for tests or development independently of any init
system.

but upon a manual start `NOTIFY_SOCKET` would not be set, no? I made the

$ sudo env NOTIFY_SOCKET=foo ./haproxy -W -f ./haproxy.cfg
[ALERT] 000/010000 (22263) : NOTIFY_SOCKET is defined, but haproxy is not compiled with USE_SYSTEMD=1. Change Type=notify in your unit file.
$ sudo env ./haproxy -W -f ./haproxy.cfg
^C[WARNING] 323/140435 (22278) : Exiting Master process...
[ALERT] 323/140435 (22278) : Current worker 22279 left with exit code 130
[WARNING] 323/140435 (22278) : All workers are left. Leaving... (130)

Rebuilding with USE_SYSTEMD and starting with the obviously incorrect

$ sudo env NOTIFY_SOCKET=foo ./haproxy -W -f ./haproxy.cfg
^C[WARNING] 323/140545 (22703) : Exiting Master process...
[ALERT] 323/140545 (22703) : Current worker 22704 left with exit code 130
[WARNING] 323/140545 (22703) : All workers are left. Leaving... (130)
$ sudo env ./haproxy -W -f ./haproxy.cfg
^C[WARNING] 323/140556 (22722) : Exiting Master process...
[ALERT] 323/140556 (22722) : Current worker 22723 left with exit code 130
[WARNING] 323/140556 (22722) : All workers are left. Leaving... (130)

Best regards
Tim Düsterhus

[Willy Tarreau]

Post by Tim Düsterhus

$ sudo env NOTIFY_SOCKET=foo ./haproxy -W -f ./haproxy.cfg
[ALERT] 000/010000 (22263) : NOTIFY_SOCKET is defined, but haproxy is not compiled with USE_SYSTEMD=1. Change Type=notify in your unit file.
$ sudo env ./haproxy -W -f ./haproxy.cfg
^C[WARNING] 323/140435 (22278) : Exiting Master process...
[ALERT] 323/140435 (22278) : Current worker 22279 left with exit code 130
[WARNING] 323/140435 (22278) : All workers are left. Leaving... (130)

Rebuilding with USE_SYSTEMD and starting with the obviously incorrect

I really don't like this at all, because it means that we'll annoy all
the happy people who are not forced to suffer from systemd, ie basically
all those not running on a recent linux distro, simply because we're starting
to declare that certain environment variables only belong to other operating
systems (which is even worse when such variables are detected when the build
option is *not* set). Also the fact that such variables could be inherited
across multiple layers of processes is even more disgusting.

My suggestion instead would be :

- USE_SYSTEMD=1 enables *build-time* support for lib-sdnotify, as you did.

- -Ws enables *runtime* support for lib-sdnotify provided that USE_SYSTEMD
was enabled at build time. Otherwise it emits an error saying that this
mode is not supported.

- unit files making use of type=notify will call haproxy with -Ws. They
have to be modified anyway to change the type. Let's change both the
type and the startup argument accordingly. If haproxy is built without
USE_SYSTEMD, it will report the error saying that systemd support was
not built in, as you wanted.

- older unit files, those using type=fork, development etc can simply
continue to use -W to match the current behaviour, regardless of the
presence of the build option.

This way there is no confusion, and the working method is not guessed
(with all the frustration this causes when it doesn't work as expected)
but instead enforced. If a unit file references one mode, it must be
consistent with itself and pass the appropriate option as well.

Regards,
Willy

[Tim Düsterhus]

From: Tim Duesterhus <***@bastelstu.be>

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.

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 <assert.h>
#endif
+#if defined(USE_SYSTEMD)
+#include <systemd/sd-daemon.h>
+#endif

#include <common/base64.h>
#include <common/cfgparse.h>
@@ -404,6 +407,9 @@ static void usage(char *name)
" -V enters verbose mode (disables quiet mode)\n"
" -D goes daemon ; -C changes to <dir> 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 (%d)\n"
@@ -635,6 +641,10 @@ static void mworker_reload()

mworker_block_signals();
mworker_unregister_signals();
+#if defined(USE_SYSTEMD)
+ if (global.tune.options & GTUNE_USE_SYSTEMD)
+ sd_notify(0, "RELOADING=1");
+#endif
setenv("HAPROXY_MWORKER_REEXEC", "1", 1);

/* compute length */
@@ -698,6 +708,11 @@ static void mworker_wait()

restart_wait:

+#if defined(USE_SYSTEMD)
+ if (global.tune.options & GTUNE_USE_SYSTEMD)
+ sd_notifyf(0, "READY=1\nMAINPID=%lu", (unsigned long)getpid());
+#endif
+
mworker_register_signals();
mworker_unblock_signals();

@@ -710,6 +725,11 @@ restart_wait:
/* should reach there only if it fail */
goto restart_wait;
} else {
+#if defined(USE_SYSTEMD)
+ if ((global.tune.options & GTUNE_USE_SYSTEMD) && (sig == SIGUSR1 || sig == SIGTERM)) {
+ sd_notify(0, "STOPPING=1");
+ }
+#endif
Warning("Exiting Master process...\n");
mworker_kill(sig);
mworker_unregister_signals();
@@ -1342,6 +1362,15 @@ static void init(int argc, char **argv)
arg_mode |= MODE_CHECK;
else if (*flag == 'D')
arg_mode |= MODE_DAEMON;
+ else if (*flag == 'W' && flag[1] == 's') {
+ arg_mode |= MODE_MWORKER;
+#if defined(USE_SYSTEMD)
+ global.tune.options |= GTUNE_USE_SYSTEMD;
+#else
+ Alert("master-worker mode with systemd support (-Ws) requested, but not compiled. Use master-worker mode (-W) if you are not using Type=notify in your unit file or recompile with USE_SYSTEMD=1.\n\n");
+ usage(progname);
+#endif
+ }
else if (*flag == 'W')
arg_mode |= MODE_MWORKER;
else if (*flag == 'q')

[Lukas Tribus]

Hello Tim,

Post by Tim Düsterhus
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

[Willy Tarreau]

Hi Lukas,

Post by Lukas Tribus

Post by Tim Düsterhus
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
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

[Lukas Tribus]

Hello,

Post by Willy Tarreau
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=
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

[Willy Tarreau]

Hi Lukas,

Post by Lukas Tribus

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 300000
backends and 600000 servers and took several seconds to process).

Post by Lukas Tribus
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 :-/

Post by Lukas Tribus
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.

Post by Lukas Tribus
I will send an RFC patch shortly.

Thank you!
Willy

[Lukas Tribus]

Hello,

Post by Willy Tarreau

Post by Lukas Tribus
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 300000
backends and 600000 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

[Willy Tarreau]

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.

Willy

[Tim Düsterhus]

Lukas,

Post by Lukas Tribus
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.

Yes. Or you can set `NotifyAccess=all` in the unit file (with the
security implications that every child process could also send messages

Post by Lukas Tribus
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.

I agree that adding `-db` is a better option than opening up the notify
socket (and I also see that this thread evolved to just force foreground
mode for the master-worker with systemd support option).

Best regards
Tim Düsterhus

[Willy Tarreau]

Coming along is a patch that implements your suggested changes.
- I was unsure whether to renumber the GTUNE_* constants. I opted not to.

Not needed. It might be a cleanup for later. However I moved your decalaration
(1<<10) after (1<<9) to avoid the risk that someone else spots the empty slot
and reuses it.

- I was unsure whether `#if defined(...)` or `#ifdef` is the variant to use
for the USE_SYSTEMD option. You use both and I was unable to find out
whether there is a scheme you follow.

The two are valid. Most often ifdef is used, but when you want to apply
combinations, if defined() often becomes more convenient.

Thanks for your patience and really helpful feedback so far :-)

You're welcome, thanks for working on this. I've applied your patch with
William's blessing :-)

Willy

[Tim Düsterhus]

William,

Post by Tim Düsterhus
+KillSignal=USR1

In my opinion this part is a problem, it won't stop the process immediatly
but wait for session to be finished. It will cause the restart to behave like
the reload but with a PID change.

Thinking about this: Agree, I will take that out again. I'll hold off
sending the updated patch, until the discussion in the sibling thread is
resolved.

Best regards
Tim Düsterhus