Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-05-06 Thread William Lallemand
On Sun, May 05, 2019 at 03:06:31PM +0200, Tim Düsterhus wrote:
> William,
> 
> Am 26.04.19 um 20:30 schrieb Tim Düsterhus:
> > William,
> > 
> > Am 26.04.19 um 14:56 schrieb William Lallemand:
> >> On Fri, Apr 26, 2019 at 12:15:37AM +0200, Tim Duesterhus wrote:
> >>>  [Service]
> >>> -Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid"
> >>> +Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid" 
> >>> "MASTER_SOCKET=/run/haproxy-master.sock"
> >>>  ExecStartPre=@SBINDIR@/haproxy -f $CONFIG -c -q
> >>> -ExecStart=@SBINDIR@/haproxy -Ws -f $CONFIG -p $PIDFILE
> >>> +ExecStart=@SBINDIR@/haproxy -Ws -S $MASTER_SOCKET -f $CONFIG -p $PIDFILE
> >>>  ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q
> >>
> >> In my opinion that's not a good idea to do it this way, because you can't
> >> disable the master CLI by unsetting the MASTER_SOCKET variable.
> >>
> >> It's probably better to have "-S /run/haproxy-master.sock" in an $OPTIONS
> >> variable at the end of the ExecStart line.
> >>
> > 
> > I'm not sure whether this is better. Yes you can disable the socket more
> > easily then, but you have to remember to add it back when editing the
> > 'OPTIONS' variable.
> > 
> > I believe it boils to to this question: Does the user usually want to
> > run with the socket enabled or not? My guess is that most users want to
> > have this socket (having is better than needing!), they might just want
> > to move it to a different location rather than outright disabling it.
> > During my tests it was only accessible to root, so there does not appear
> > a security issue in the default configuration either.
> > 
> > On an unrelated note: Debian patches the unit file to add in such a
> > variable, called 'EXTRAOPTS':
> > https://salsa.debian.org/haproxy-team/haproxy/blob/master/debian/patches/haproxy.service-use-environment-variables.patch
> > 
> > So if we want to go the 'OPTIONS' variable path we might want to re-use
> > that variable name. Any change will break their patch anyway so they
> > definitely notice.
> > 
> > Best regards
> > Tim Düsterhus
> > 
> 
> I'd like to bump my reply to you once more. As outlined previously I
> believe that the "default" expectation is having the socket, it can
> still be disabled by overriding the ExecStart= declaration.
> 
> Best regards
> Tim Düsterhus
> 

Hi Tim,

This socket gives full admin access to HAProxy without being in the
configuration, so it might surprise the user if it's there after an upgrade, it
should really be configurable. But I agree that it could be nice to have it
by default.

Regarding the overriding of ExecStart, I disagree. In my opinion this is a
confusing solution for the user, when doing that the user won't have the update
of the unit file in the package. Lots of people still prefer /etc/default/ and
/etc/sysconfig/ to add some options.

In this case we could indeed use the EXTRAOPTS variable, and add
"EXTRAOPTS=-S /run/haproxy-master.sock" at the end of the Environment line.

Regards,

-- 
William Lallemand



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-05-05 Thread Tim Düsterhus
William,

Am 26.04.19 um 20:30 schrieb Tim Düsterhus:
> William,
> 
> Am 26.04.19 um 14:56 schrieb William Lallemand:
>> On Fri, Apr 26, 2019 at 12:15:37AM +0200, Tim Duesterhus wrote:
>>>  [Service]
>>> -Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid"
>>> +Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid" 
>>> "MASTER_SOCKET=/run/haproxy-master.sock"
>>>  ExecStartPre=@SBINDIR@/haproxy -f $CONFIG -c -q
>>> -ExecStart=@SBINDIR@/haproxy -Ws -f $CONFIG -p $PIDFILE
>>> +ExecStart=@SBINDIR@/haproxy -Ws -S $MASTER_SOCKET -f $CONFIG -p $PIDFILE
>>>  ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q
>>
>> In my opinion that's not a good idea to do it this way, because you can't
>> disable the master CLI by unsetting the MASTER_SOCKET variable.
>>
>> It's probably better to have "-S /run/haproxy-master.sock" in an $OPTIONS
>> variable at the end of the ExecStart line.
>>
> 
> I'm not sure whether this is better. Yes you can disable the socket more
> easily then, but you have to remember to add it back when editing the
> 'OPTIONS' variable.
> 
> I believe it boils to to this question: Does the user usually want to
> run with the socket enabled or not? My guess is that most users want to
> have this socket (having is better than needing!), they might just want
> to move it to a different location rather than outright disabling it.
> During my tests it was only accessible to root, so there does not appear
> a security issue in the default configuration either.
> 
> On an unrelated note: Debian patches the unit file to add in such a
> variable, called 'EXTRAOPTS':
> https://salsa.debian.org/haproxy-team/haproxy/blob/master/debian/patches/haproxy.service-use-environment-variables.patch
> 
> So if we want to go the 'OPTIONS' variable path we might want to re-use
> that variable name. Any change will break their patch anyway so they
> definitely notice.
> 
> Best regards
> Tim Düsterhus
> 

I'd like to bump my reply to you once more. As outlined previously I
believe that the "default" expectation is having the socket, it can
still be disabled by overriding the ExecStart= declaration.

Best regards
Tim Düsterhus



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-30 Thread Patrick Hemmer




*From:* Willy Tarreau [mailto:w...@1wt.eu]
*Sent:* Monday, April 29, 2019, 23:55 EDT
*To:* William Lallemand 
*Cc:* Tim Düsterhus , Patrick Hemmer 
, haproxy@formilux.org

*Subject:* [PATCH] MINOR: systemd: Make use of master socket in systemd unit


On Tue, Apr 30, 2019 at 01:47:13AM +0200, William Lallemand wrote:

the systemd API
is limited and it's not possible to report a fail during a reload with 
sd_notify.

Anyway a service manager which requires daemons to stay in foreground,
does not allow processes to be replaced, and relies on asynchronous and
unidirectional signal delivery to perform critical actions such as config
updates is obviously misdesigned and can only result from the lack of
experience of production servers and people making them work every day.

I tend to think that the only longterm solution to work around all the
systemd imposed mess is to have haproxy become its own service manager.
It is a shame as it significantly complicates haproxy management on Linux
compared to sane operating systems, but I'm having a hard time accepting
that haproxy has become less reliable on Linux due to systemd. We've
already made progress in this direction with the master-worker model,
but we need to go further and make haproxy be able to serve as a client
to talk to an existing master and return relevant status codes. This
way we'll be able to emulate the start/reload/restart operations that
are performed on sane systems and gain reliability again, at the
expense of using a total of 3 processes talking to each other during
a reload instead of just one. The advantage is that it should work on
other operating systems too if needed.

In my opinion this is something we seriously need to work on for 2.1,
and address remaining limitations (doubled memory usage and CLI being
closed on reload).

By the way, does systemd support passing actions on stdin and getting
the result on stdout ? Given that the master process is forced to
stay in foreground, at least we could have the equivalent of the CLI
on stdio used as a "console" and not require a third process to talk
to it.

Willy
Not to start a systemd flame war, but you can always tell systemd to 
stop tracking the process, and you can then do whatever you want. Of 
course you won't get automatic restarts if it dies, but that's how all 
the other "sane operating systems" behave.

Just add to the service file:
Type=oneshot
RemainAfterExit=yes

Assuming we don't do that, and going back to the original issue at hand 
of putting the master socket in the systemd unit: do we want to do this? 
From all the discussion, whether or not we add a `haproxy reload` 
command to the haproxy binary, it still seems like we will need master 
socket support to make it work. So should this patch be adopted?


-Patrick



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-30 Thread Tim Düsterhus
Willy,

Am 30.04.19 um 05:55 schrieb Willy Tarreau:
> By the way, does systemd support passing actions on stdin and getting
> the result on stdout ? Given that the master process is forced to
> stay in foreground, at least we could have the equivalent of the CLI
> on stdio used as a "console" and not require a third process to talk
> to it.

stdin is usually closed, but can be connected to a TTY, socket or file:
https://www.freedesktop.org/software/systemd/man/systemd.exec.html#StandardInput=

stdout (right below stdin in the docs) goes to the journal by default
and I would not touch it (because you need to log with the journal API
instead of stdout then).

Best regards
Tim Düsterhus



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-29 Thread Willy Tarreau
On Tue, Apr 30, 2019 at 01:47:13AM +0200, William Lallemand wrote:
> the systemd API
> is limited and it's not possible to report a fail during a reload with 
> sd_notify.

Anyway a service manager which requires daemons to stay in foreground,
does not allow processes to be replaced, and relies on asynchronous and
unidirectional signal delivery to perform critical actions such as config
updates is obviously misdesigned and can only result from the lack of
experience of production servers and people making them work every day.

I tend to think that the only longterm solution to work around all the
systemd imposed mess is to have haproxy become its own service manager.
It is a shame as it significantly complicates haproxy management on Linux
compared to sane operating systems, but I'm having a hard time accepting
that haproxy has become less reliable on Linux due to systemd. We've
already made progress in this direction with the master-worker model,
but we need to go further and make haproxy be able to serve as a client
to talk to an existing master and return relevant status codes. This
way we'll be able to emulate the start/reload/restart operations that
are performed on sane systems and gain reliability again, at the
expense of using a total of 3 processes talking to each other during
a reload instead of just one. The advantage is that it should work on
other operating systems too if needed.

In my opinion this is something we seriously need to work on for 2.1,
and address remaining limitations (doubled memory usage and CLI being
closed on reload).

By the way, does systemd support passing actions on stdin and getting
the result on stdout ? Given that the master process is forced to
stay in foreground, at least we could have the equivalent of the CLI
on stdio used as a "console" and not require a third process to talk
to it.

Willy



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-29 Thread William Lallemand
On Mon, Apr 29, 2019 at 08:51:58PM +0200, Tim Düsterhus wrote:
> 
> One issue here is that this problem was not detected by the
> configuration check (for obvious reasons), most problems (fat fingered
> configuration) are detected using the check.
> 
> Regarding the sd_notify integration:
> 
> I guess this is a 'bug' within the sd_notify integration. HAProxy
> reports READY=1 back to systemd, even if the startup failed and it falls
> back to waitpid mode. Possibly the ERRNO= return should be used here (I
> did not check whether that actually does the right thing):
> https://www.freedesktop.org/software/systemd/man/sd_notify.html#ERRNO=%E2%80%A6
> 

Unfortunately this is not a bug, I made some test a while ago, the systemd API
is limited and it's not possible to report a fail during a reload with 
sd_notify.
Maybe I was doing it wrong, but the only way I found to do it is to return a
bad status code from the command executed by ExecReload.

The problem with launching an haproxy -c for that, is that the configuration is
parsed twice (once for the -c, once during the reload) which can be a problem
if you have a lot of SSL certificates for example. Another problem is that it's
possible that the -c will succeed and the reload will fail. The two commands
aren't atomics so there is a small window where you could have 2 differents
configuration files between the -c and the reload.

The solution I would implement in the future is a synchonous state on the
master CLI during a reload. We could have an `haproxy -reload` command, which
will connect to the master CLI, ask for a reload, and return the status code
corresponding to the state of the reload.

The current architecture of the master CLI doesn't allow that, because the
re-execution of the process close the connection to the master CLI.

-- 
William Lallemand



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-29 Thread Tim Düsterhus
Patrick,

Am 29.04.19 um 20:37 schrieb Patrick Hemmer:
> Yes, we use Type=notify. The problem though is that systemd is reporting
> reload success even if the reload fails.
> 
> Take the following example config:
> 
> global
> stats socket /tmp/foo
> 
> With the current systemd file provided by haproxy, I copied it as
> "haproxy-test.service" and modified one line to the following:
> 
> Environment="CONFIG=/tmp/haproxy.cfg" "PIDFILE=/run/haproxy-test.pid"
> 
> And started haproxy:
> 
> # systemctl start haproxy-test.service
> -- Logs begin at Thu 2019-04-11 15:13:39 EDT. --
> Apr 29 14:24:14 fll2albs01qa2 systemd[1]: Starting HAProxy Load Balancer...
> Apr 29 14:24:14 fll2albs01qa2 haproxy[27702]: [NOTICE] 118/142414
> (27702) : New worker #1 (27704) forked
> Apr 29 14:24:14 fll2albs01qa2 systemd[1]: Started HAProxy Load Balancer.
> 
> I then edited the config and changed the stats socket line to:
> 
> stats socket /notmp/foo
> 
> And then tried a reload:
> # systemctl reload haproxy-test.service
> -- Logs begin at Thu 2019-04-11 15:13:39 EDT. --
> Apr 29 14:24:47 fll2albs01qa2 haproxy[27702]: [WARNING] 118/142447
> (27702) : Reexecuting Master process
> Apr 29 14:24:47 fll2albs01qa2 haproxy[27702]: [ALERT] 118/142447 (27702)
> : Starting frontend GLOBAL: cannot bind UNIX socket [/notmp/foo]
> Apr 29 14:24:47 fll2albs01qa2 haproxy[27702]: [WARNING] 118/142447
> (27702) : Reexecuting Master process in waitpid mode
> Apr 29 14:24:47 fll2albs01qa2 haproxy[27702]: [WARNING] 118/142447
> (27702) : Reexecuting Master process
> Apr 29 14:24:47 fll2albs01qa2 systemd[1]: Reloaded HAProxy Load Balancer.
> 
> # systemctl status haproxy-test.service
> ● haproxy-test.service - HAProxy Load Balancer
>    Loaded: loaded (/etc/systemd/system/haproxy-test.service; disabled;
> vendor preset: disabled)
>    Active: active (running) since Mon 2019-04-29 14:24:14 EDT; 45s ago
>   Process: 28335 ExecReload=/bin/kill -USR2 $MAINPID (code=exited,
> status=0/SUCCESS)
>   Process: 28334 ExecReload=/usr/sbin/haproxy -f $CONFIG -c -q
> (code=exited, status=0/SUCCESS)
>   Process: 27700 ExecStartPre=/usr/sbin/haproxy -f $CONFIG -c -q
> (code=exited, status=0/SUCCESS)
>  Main PID: 27702 (haproxy)
>    Memory: 2.9M
>    CGroup: /system.slice/haproxy-test.service
>    ├─27702 /usr/sbin/haproxy -Ws -f /tmp/haproxy.cfg -p
> /run/haproxy-test.pid -sf 27704
>    └─27704 /usr/sbin/haproxy -Ws -f /tmp/haproxy.cfg -p
> /run/haproxy-test.pid
> 
> Note that the reload failed, but systemd reports success.

One issue here is that this problem was not detected by the
configuration check (for obvious reasons), most problems (fat fingered
configuration) are detected using the check.

Regarding the sd_notify integration:

I guess this is a 'bug' within the sd_notify integration. HAProxy
reports READY=1 back to systemd, even if the startup failed and it falls
back to waitpid mode. Possibly the ERRNO= return should be used here (I
did not check whether that actually does the right thing):
https://www.freedesktop.org/software/systemd/man/sd_notify.html#ERRNO=%E2%80%A6

Best regards
Tim Düsterhus

> With older versions of haproxy (<1.8) we had a script which checked the
> PID of the master process and that it changed after sending SIGUSR2.
> This strategy no longer works.
> 



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-29 Thread Patrick Hemmer




*From:* Tim Düsterhus [mailto:t...@bastelstu.be]
*Sent:* Friday, April 26, 2019, 15:03 EDT
*To:* Patrick Hemmer , William Lallemand 


*Cc:* haproxy@formilux.org, w...@1wt.eu
*Subject:* [PATCH] MINOR: systemd: Make use of master socket in systemd unit


Patrick,

Am 26.04.19 um 20:55 schrieb Patrick Hemmer:

One other consideration is a problem I've been looking to address, and
that's better reloads. Right now the systemd unit sends a signal for the
reload, but sending a signal gets no feedback on whether the reload
operation was successful or not.

Have you checked that you are using Type=notify in your unit file? It
uses systemd's sd_notify API to communicate the reload to systemd which
should do what you are searching for.

See this mailing list thread:
https://www.mail-archive.com/haproxy@formilux.org/msg27874.html

Best regards
Tim Düsterhus


Yes, we use Type=notify. The problem though is that systemd is reporting 
reload success even if the reload fails.


Take the following example config:

global
stats socket /tmp/foo

With the current systemd file provided by haproxy, I copied it as 
"haproxy-test.service" and modified one line to the following:


Environment="CONFIG=/tmp/haproxy.cfg" "PIDFILE=/run/haproxy-test.pid"

And started haproxy:

# systemctl start haproxy-test.service
-- Logs begin at Thu 2019-04-11 15:13:39 EDT. --
Apr 29 14:24:14 fll2albs01qa2 systemd[1]: Starting HAProxy Load Balancer...
Apr 29 14:24:14 fll2albs01qa2 haproxy[27702]: [NOTICE] 118/142414 
(27702) : New worker #1 (27704) forked

Apr 29 14:24:14 fll2albs01qa2 systemd[1]: Started HAProxy Load Balancer.

I then edited the config and changed the stats socket line to:

stats socket /notmp/foo

And then tried a reload:
# systemctl reload haproxy-test.service
-- Logs begin at Thu 2019-04-11 15:13:39 EDT. --
Apr 29 14:24:47 fll2albs01qa2 haproxy[27702]: [WARNING] 118/142447 
(27702) : Reexecuting Master process
Apr 29 14:24:47 fll2albs01qa2 haproxy[27702]: [ALERT] 118/142447 (27702) 
: Starting frontend GLOBAL: cannot bind UNIX socket [/notmp/foo]
Apr 29 14:24:47 fll2albs01qa2 haproxy[27702]: [WARNING] 118/142447 
(27702) : Reexecuting Master process in waitpid mode
Apr 29 14:24:47 fll2albs01qa2 haproxy[27702]: [WARNING] 118/142447 
(27702) : Reexecuting Master process

Apr 29 14:24:47 fll2albs01qa2 systemd[1]: Reloaded HAProxy Load Balancer.

# systemctl status haproxy-test.service
● haproxy-test.service - HAProxy Load Balancer
   Loaded: loaded (/etc/systemd/system/haproxy-test.service; disabled; 
vendor preset: disabled)

   Active: active (running) since Mon 2019-04-29 14:24:14 EDT; 45s ago
  Process: 28335 ExecReload=/bin/kill -USR2 $MAINPID (code=exited, 
status=0/SUCCESS)
  Process: 28334 ExecReload=/usr/sbin/haproxy -f $CONFIG -c -q 
(code=exited, status=0/SUCCESS)
  Process: 27700 ExecStartPre=/usr/sbin/haproxy -f $CONFIG -c -q 
(code=exited, status=0/SUCCESS)

 Main PID: 27702 (haproxy)
   Memory: 2.9M
   CGroup: /system.slice/haproxy-test.service
   ├─27702 /usr/sbin/haproxy -Ws -f /tmp/haproxy.cfg -p 
/run/haproxy-test.pid -sf 27704
   └─27704 /usr/sbin/haproxy -Ws -f /tmp/haproxy.cfg -p 
/run/haproxy-test.pid


Note that the reload failed, but systemd reports success.

With older versions of haproxy (<1.8) we had a script which checked the 
PID of the master process and that it changed after sending SIGUSR2. 
This strategy no longer works.


Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-26 Thread Tim Düsterhus
Patrick,

Am 26.04.19 um 20:55 schrieb Patrick Hemmer:
> One other consideration is a problem I've been looking to address, and
> that's better reloads. Right now the systemd unit sends a signal for the
> reload, but sending a signal gets no feedback on whether the reload
> operation was successful or not.

Have you checked that you are using Type=notify in your unit file? It
uses systemd's sd_notify API to communicate the reload to systemd which
should do what you are searching for.

See this mailing list thread:
https://www.mail-archive.com/haproxy@formilux.org/msg27874.html

Best regards
Tim Düsterhus



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-26 Thread Willy Tarreau
Hi Patrick,

On Fri, Apr 26, 2019 at 02:55:30PM -0400, Patrick Hemmer wrote:
> One other consideration is a problem I've been looking to address, and
> that's better reloads. Right now the systemd unit sends a signal for the
> reload, but sending a signal gets no feedback on whether the reload
> operation was successful or not.
> 
> I haven't thought about this a whole lot, but I'm thinking the way to
> address it would be some sort of inquiry to the master process, which means
> using the socket. So if the systemd unit file ensured that the master socket
> is available, then ExecReload could be adjusted to use it and get
> success/failure feedback.

A very long time ago I wanted to make haproxy itself be usable as a CLI
client. Nowadays with socat available everywhere it's pointless, but it
could at least be used to give a few orders to the master like "reload",
"stop" or any such thing. Just like you can reload squid using
"squid -k reconfigure" if my memory serves me right. The advantage here
is that it would require no other tool and would know its own protocol.

Willy



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-26 Thread Patrick Hemmer




*From:* Tim Düsterhus [mailto:t...@bastelstu.be]
*Sent:* Friday, April 26, 2019, 14:30 EDT
*To:* William Lallemand 
*Cc:* haproxy@formilux.org, w...@1wt.eu
*Subject:* [PATCH] MINOR: systemd: Make use of master socket in systemd unit


William,

Am 26.04.19 um 14:56 schrieb William Lallemand:

On Fri, Apr 26, 2019 at 12:15:37AM +0200, Tim Duesterhus wrote:

  [Service]
-Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid"
+Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid" 
"MASTER_SOCKET=/run/haproxy-master.sock"
  ExecStartPre=@SBINDIR@/haproxy -f $CONFIG -c -q
-ExecStart=@SBINDIR@/haproxy -Ws -f $CONFIG -p $PIDFILE
+ExecStart=@SBINDIR@/haproxy -Ws -S $MASTER_SOCKET -f $CONFIG -p $PIDFILE
  ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q

In my opinion that's not a good idea to do it this way, because you can't
disable the master CLI by unsetting the MASTER_SOCKET variable.

It's probably better to have "-S /run/haproxy-master.sock" in an $OPTIONS
variable at the end of the ExecStart line.


I'm not sure whether this is better. Yes you can disable the socket more
easily then, but you have to remember to add it back when editing the
'OPTIONS' variable.

I believe it boils to to this question: Does the user usually want to
run with the socket enabled or not? My guess is that most users want to
have this socket (having is better than needing!), they might just want
to move it to a different location rather than outright disabling it.
During my tests it was only accessible to root, so there does not appear
a security issue in the default configuration either.

On an unrelated note: Debian patches the unit file to add in such a
variable, called 'EXTRAOPTS':
https://salsa.debian.org/haproxy-team/haproxy/blob/master/debian/patches/haproxy.service-use-environment-variables.patch

So if we want to go the 'OPTIONS' variable path we might want to re-use
that variable name. Any change will break their patch anyway so they
definitely notice.

Best regards
Tim Düsterhus

One other consideration is a problem I've been looking to address, and 
that's better reloads. Right now the systemd unit sends a signal for the 
reload, but sending a signal gets no feedback on whether the reload 
operation was successful or not.


I haven't thought about this a whole lot, but I'm thinking the way to 
address it would be some sort of inquiry to the master process, which 
means using the socket. So if the systemd unit file ensured that the 
master socket is available, then ExecReload could be adjusted to use it 
and get success/failure feedback.


-Patrick


Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-26 Thread Tim Düsterhus
William,

Am 26.04.19 um 14:56 schrieb William Lallemand:
> On Fri, Apr 26, 2019 at 12:15:37AM +0200, Tim Duesterhus wrote:
>>  [Service]
>> -Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid"
>> +Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid" 
>> "MASTER_SOCKET=/run/haproxy-master.sock"
>>  ExecStartPre=@SBINDIR@/haproxy -f $CONFIG -c -q
>> -ExecStart=@SBINDIR@/haproxy -Ws -f $CONFIG -p $PIDFILE
>> +ExecStart=@SBINDIR@/haproxy -Ws -S $MASTER_SOCKET -f $CONFIG -p $PIDFILE
>>  ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q
> 
> In my opinion that's not a good idea to do it this way, because you can't
> disable the master CLI by unsetting the MASTER_SOCKET variable.
> 
> It's probably better to have "-S /run/haproxy-master.sock" in an $OPTIONS
> variable at the end of the ExecStart line.
> 

I'm not sure whether this is better. Yes you can disable the socket more
easily then, but you have to remember to add it back when editing the
'OPTIONS' variable.

I believe it boils to to this question: Does the user usually want to
run with the socket enabled or not? My guess is that most users want to
have this socket (having is better than needing!), they might just want
to move it to a different location rather than outright disabling it.
During my tests it was only accessible to root, so there does not appear
a security issue in the default configuration either.

On an unrelated note: Debian patches the unit file to add in such a
variable, called 'EXTRAOPTS':
https://salsa.debian.org/haproxy-team/haproxy/blob/master/debian/patches/haproxy.service-use-environment-variables.patch

So if we want to go the 'OPTIONS' variable path we might want to re-use
that variable name. Any change will break their patch anyway so they
definitely notice.

Best regards
Tim Düsterhus



Re: [PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-26 Thread William Lallemand
Hi Tim,

On Fri, Apr 26, 2019 at 12:15:37AM +0200, Tim Duesterhus wrote:
>  [Service]
> -Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid"
> +Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid" 
> "MASTER_SOCKET=/run/haproxy-master.sock"
>  ExecStartPre=@SBINDIR@/haproxy -f $CONFIG -c -q
> -ExecStart=@SBINDIR@/haproxy -Ws -f $CONFIG -p $PIDFILE
> +ExecStart=@SBINDIR@/haproxy -Ws -S $MASTER_SOCKET -f $CONFIG -p $PIDFILE
>  ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q

In my opinion that's not a good idea to do it this way, because you can't
disable the master CLI by unsetting the MASTER_SOCKET variable.

It's probably better to have "-S /run/haproxy-master.sock" in an $OPTIONS
variable at the end of the ExecStart line.

-- 
William Lallemand



[PATCH] MINOR: systemd: Make use of master socket in systemd unit

2019-04-25 Thread Tim Duesterhus
This commit adds the -S option to the ExecStart= line of
the provided systemd unit file.

This patch may be backported to 1.9.
---
 contrib/systemd/haproxy.service.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/systemd/haproxy.service.in 
b/contrib/systemd/haproxy.service.in
index 74e66e302..5a84fc45b 100644
--- a/contrib/systemd/haproxy.service.in
+++ b/contrib/systemd/haproxy.service.in
@@ -3,9 +3,9 @@ Description=HAProxy Load Balancer
 After=network.target
 
 [Service]
-Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid"
+Environment="CONFIG=/etc/haproxy/haproxy.cfg" "PIDFILE=/run/haproxy.pid" 
"MASTER_SOCKET=/run/haproxy-master.sock"
 ExecStartPre=@SBINDIR@/haproxy -f $CONFIG -c -q
-ExecStart=@SBINDIR@/haproxy -Ws -f $CONFIG -p $PIDFILE
+ExecStart=@SBINDIR@/haproxy -Ws -S $MASTER_SOCKET -f $CONFIG -p $PIDFILE
 ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q
 ExecReload=/bin/kill -USR2 $MAINPID
 KillMode=mixed
-- 
2.21.0