Welcome! Log In Create A New Profile

Advanced

[PATCH] MINOR: systemd: consider exit status 143 as successful

Posted by Vincent Bernat 
The master process will exit with the status of the last worker. When
the worker is killed with SIGTERM, it is expected to get 143 as an
exit status. Therefore, we consider this exit status as normal from a
systemd point of view. If it happens when not stopping, the systemd
unit is configured to always restart, so it has no adverse effect.

This has mostly a cosmetic effect. Without the patch, stopping HAProxy
leads to the following status:

● haproxy.service - HAProxy Load Balancer
Loaded: loaded (/lib/systemd/system/haproxy.service; disabled; vendor preset: enabled)
Active: failed (Result: exit-code) since Fri 2018-06-22 20:35:42 CEST; 8min ago
Docs: man:haproxy(1)
file:/usr/share/doc/haproxy/configuration.txt.gz
Process: 32715 ExecStart=/usr/sbin/haproxy -Ws -f $CONFIG -p $PIDFILE $EXTRAOPTS (code=exited, status=143)
Process: 32714 ExecStartPre=/usr/sbin/haproxy -f $CONFIG -c -q $EXTRAOPTS (code=exited, status=0/SUCCESS)
Main PID: 32715 (code=exited, status=143)

After the patch:

● haproxy.service - HAProxy Load Balancer
Loaded: loaded (/lib/systemd/system/haproxy.service; disabled; vendor preset: enabled)
Active: inactive (dead)
Docs: man:haproxy(1)
file:/usr/share/doc/haproxy/configuration.txt.gz
---
contrib/systemd/haproxy.service.in | 1 +
1 file changed, 1 insertion(+)

diff --git a/contrib/systemd/haproxy.service.in b/contrib/systemd/haproxy..service.in
index 7a8b6bead2df..74e66e302065 100644
--- a/contrib/systemd/haproxy.service.in
+++ b/contrib/systemd/haproxy.service.in
@@ -10,6 +10,7 @@ [email protected]@/haproxy -f $CONFIG -c -q
ExecReload=/bin/kill -USR2 $MAINPID
KillMode=mixed
Restart=always
+SuccessExitStatus=143
Type=notify

# The following lines leverage SystemD's sandboxing options to provide
--
2.18.0
On Fri, Jun 22, 2018 at 08:57:03PM +0200, Vincent Bernat wrote:
> The master process will exit with the status of the last worker. When
> the worker is killed with SIGTERM, it is expected to get 143 as an
> exit status. Therefore, we consider this exit status as normal from a
> systemd point of view. If it happens when not stopping, the systemd
> unit is configured to always restart, so it has no adverse effect.
>
> This has mostly a cosmetic effect. Without the patch, stopping HAProxy
> leads to the following status:
>
> ● haproxy.service - HAProxy Load Balancer
> Loaded: loaded (/lib/systemd/system/haproxy.service; disabled; vendor preset: enabled)
> Active: failed (Result: exit-code) since Fri 2018-06-22 20:35:42 CEST; 8min ago
> Docs: man:haproxy(1)
> file:/usr/share/doc/haproxy/configuration.txt.gz
> Process: 32715 ExecStart=/usr/sbin/haproxy -Ws -f $CONFIG -p $PIDFILE $EXTRAOPTS (code=exited, status=143)
> Process: 32714 ExecStartPre=/usr/sbin/haproxy -f $CONFIG -c -q $EXTRAOPTS (code=exited, status=0/SUCCESS)
> Main PID: 32715 (code=exited, status=143)
>
> After the patch:
>
> ● haproxy.service - HAProxy Load Balancer
> Loaded: loaded (/lib/systemd/system/haproxy.service; disabled; vendor preset: enabled)
> Active: inactive (dead)
> Docs: man:haproxy(1)
> file:/usr/share/doc/haproxy/configuration.txt.gz
> ---
> contrib/systemd/haproxy.service.in | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/contrib/systemd/haproxy.service.in b/contrib/systemd/haproxy.service.in
> index 7a8b6bead2df..74e66e302065 100644
> --- a/contrib/systemd/haproxy.service.in
> +++ b/contrib/systemd/haproxy.service.in
> @@ -10,6 +10,7 @@ [email protected]@/haproxy -f $CONFIG -c -q
> ExecReload=/bin/kill -USR2 $MAINPID
> KillMode=mixed
> Restart=always
> +SuccessExitStatus=143
> Type=notify
>
> # The following lines leverage SystemD's sandboxing options to provide


Hm that's interesting, but isn't it better to consider that the SIGTERM is a
"normal" status code, and let the master leaves with zero, instead of changing
systemd configuration?

We could do the same with 130 (SIGINT). In fact those two exits codes are
already handled differently in the master.

--
William Lallemand
Vincent Bernat
[PATCH] MINOR: mworker: exit with 0 on successful exit
June 22, 2018 10:10PM
Without this patch, when killing the master process, the SIGTERM
signal is forwarded to all children. Last children will likely exit
with "killed by signal SIGTERM" status which would be converted by an
exit with status 143 of the master process.

With this patch, the master process takes note it is requesting its
children to stop and will convert "killed by signal SIGTERM" to an
exit status of 0. Therefore, the master process will exit with status
0 if everything happens as expected.

Killing a worker process with SIGTERM will still trigger an exit of
the master process with a status of 143.
---
src/haproxy.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/haproxy.c b/src/haproxy.c
index 11d1d47ceb41..2f11ad124873 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -723,6 +723,7 @@ static void mworker_wait()
{
int exitpid = -1;
int status = 0;
+ int exiting = 0;

restart_wait:

@@ -749,6 +750,7 @@ restart_wait:
}
#endif
ha_warning("Exiting Master process...\n");
+ exiting = 1;
mworker_kill(sig);
mworker_unregister_signals();
}
@@ -763,8 +765,13 @@ restart_wait:

if (WIFEXITED(status))
status = WEXITSTATUS(status);
- else if (WIFSIGNALED(status))
- status = 128 + WTERMSIG(status);
+ else if (WIFSIGNALED(status)) {
+ if (exiting && (WTERMSIG(status) == SIGINT ||
+ WTERMSIG(status) == SIGTERM))
+ status = 0;
+ else
+ status = 128 + WTERMSIG(status);
+ }
else if (WIFSTOPPED(status))
status = 128 + WSTOPSIG(status);
else
@@ -776,7 +783,7 @@ restart_wait:
/* check if exited child was in the current children list */
if (current_child(exitpid)) {
ha_alert("Current worker %d exited with code %d\n", exitpid, status);
- if (status != 0 && status != 130 && status != 143
+ if (status != 0
&& !(global.tune.options & GTUNE_NOEXIT_ONFAILURE)) {
ha_alert("exit-on-failure: killing every workers with SIGTERM\n");
mworker_kill(SIGTERM);
--
2.18.0
Vincent Bernat
Re: [PATCH] MINOR: mworker: exit with 0 on successful exit
June 22, 2018 10:20PM
❦ 22 juin 2018 22:03 +0200, Vincent Bernat <[email protected]> :

> if (current_child(exitpid)) {
> ha_alert("Current worker %d exited with code %d\n", exitpid, status);

This is a lie, but I don't think it matters much. We could (mentally)
translate "with code 0" by "with success".
--
Choose a data representation that makes the program simple.
- The Elements of Programming Style (Kernighan & Plauger)
Vincent Bernat
Re: [PATCH] MINOR: mworker: exit with 0 on successful exit
July 12, 2018 04:20PM
❦ 22 juin 2018 22:03 +0200, Vincent Bernat <[email protected]> :

> Without this patch, when killing the master process, the SIGTERM
> signal is forwarded to all children. Last children will likely exit
> with "killed by signal SIGTERM" status which would be converted by an
> exit with status 143 of the master process.
>
> With this patch, the master process takes note it is requesting its
> children to stop and will convert "killed by signal SIGTERM" to an
> exit status of 0. Therefore, the master process will exit with status
> 0 if everything happens as expected.

I think this patch may have slipped through the cracks!
--
Be careful of reading health books, you might die of a misprint.
-- Mark Twain
William Lallemand
Re: [PATCH] MINOR: mworker: exit with 0 on successful exit
July 12, 2018 04:30PM
On Thu, Jul 12, 2018 at 04:14:34PM +0200, Vincent Bernat wrote:
> ❦ 22 juin 2018 22:03 +0200, Vincent Bernat <[email protected]> :
>
> > Without this patch, when killing the master process, the SIGTERM
> > signal is forwarded to all children. Last children will likely exit
> > with "killed by signal SIGTERM" status which would be converted by an
> > exit with status 143 of the master process.
> >
> > With this patch, the master process takes note it is requesting its
> > children to stop and will convert "killed by signal SIGTERM" to an
> > exit status of 0. Therefore, the master process will exit with status
> > 0 if everything happens as expected.
>
> I think this patch may have slipped through the cracks!
> --


Hi Vincent,

Sorry I forgot to reply to this mail. I'm currently reworking the code of the
master so I don't want to rebase everything on top of your patch :-)

Maybe we could take your first patch for the unit file and backport it in 1.8,
and then make the appropriate changes for 1.9 once the master was redesigned.

What do you think?


--
William Lallemand
Vincent Bernat
Re: [PATCH] MINOR: mworker: exit with 0 on successful exit
July 12, 2018 04:50PM
❦ 12 juillet 2018 16:25 +0200, William Lallemand <[email protected]> :

> Maybe we could take your first patch for the unit file and backport it in 1.8,
> and then make the appropriate changes for 1.9 once the master was
> redesigned.

Yes, no problem. The first patch should apply without any change on 1.8.
I am using it in Debian packages and so far, nobody complained.
--
Hell is empty and all the devils are here.
-- Wm. Shakespeare, "The Tempest"
William Lallemand
Re: [PATCH] MINOR: mworker: exit with 0 on successful exit
July 12, 2018 05:50PM
On Thu, Jul 12, 2018 at 04:42:01PM +0200, Vincent Bernat wrote:
> ❦ 12 juillet 2018 16:25 +0200, William Lallemand <[email protected]> :
>
> > Maybe we could take your first patch for the unit file and backport it in 1.8,
> > and then make the appropriate changes for 1.9 once the master was
> > redesigned.
>
> Yes, no problem. The first patch should apply without any change on 1.8.
> I am using it in Debian packages and so far, nobody complained.

Okay, thanks!

@Willy, could you apply the first patch "[PATCH] MINOR: systemd: consider exit
status 143 as successful"?

Thanks.

--
William Lallemand
On Thu, Jul 12, 2018 at 05:38:34PM +0200, William Lallemand wrote:
> On Thu, Jul 12, 2018 at 04:42:01PM +0200, Vincent Bernat wrote:
> > ? 12 juillet 2018 16:25 +0200, William Lallemand <[email protected]> :
> >
> > > Maybe we could take your first patch for the unit file and backport it in 1.8,
> > > and then make the appropriate changes for 1.9 once the master was
> > > redesigned.
> >
> > Yes, no problem. The first patch should apply without any change on 1.8.
> > I am using it in Debian packages and so far, nobody complained.
>
> Okay, thanks!
>
> @Willy, could you apply the first patch "[PATCH] MINOR: systemd: consider exit
> status 143 as successful"?

Sure, now done, thank you guys!

Willy
Sorry, only registered users may post in this forum.

Click here to login