[PATCH] MINOR: mworker: do not store child pid anymore in the pidfile

The parent process supervises itself the children, we don't need to
store the children pids anymore in the pidfile in master-worker mode.
---
src/haproxy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/haproxy.c b/src/haproxy.c
index bcbbad4a1..4d4bd3b26 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2649,7 +2649,7 @@ int main(int argc, char **argv)
else if (ret == 0) /* child breaks here */
break;
children[proc] = ret;
- if (pidfd >= 0) {
+ if (pidfd >= 0 && !(global.mode & MODE_MWORKER)) {
char pidstr[100];
snprintf(pidstr, sizeof(pidstr), "%d\n", ret);
shut_your_big_mouth_gcc(write(pidfd, pidstr, strlen(pidstr)));
--
2.13.6

On 06/11/2017 11:16 πμ, William Lallemand wrote:
> The parent process supervises itself the children, we don't need to
> store the children pids anymore in the pidfile in master-worker mode.

I have a small objection against this. Having PIDs in a file allows external tools to monitor the
workers. If those PIDs aren't written in a the pidfile then those tools have to use pgrep to find
them, unless the master process can provide them in some way(stats socket?).

My 2cents,
Pavlos

On Mon, Nov 06, 2017 at 12:11:13PM +0100, Pavlos Parissis wrote:
> On 06/11/2017 11:16 πμ, William Lallemand wrote:
> > The parent process supervises itself the children, we don't need to
> > store the children pids anymore in the pidfile in master-worker mode.
>
> I have a small objection against this. Having PIDs in a file allows external tools to monitor the
> workers. If those PIDs aren't written in a the pidfile then those tools have to use pgrep to find
> them, unless the master process can provide them in some way(stats socket?).
>
> My 2cents,
> Pavlos
>

Hi Pavlos,

This patch was made to prevent scripts to send signals to the children directly
instead of sending them to the master which will forward them. That could end
in reporting a wrong exit code in the master for example.

One of the problem of the pidfile, is that only the latest children were
written, so you wouldn't have the remaining PID in this list on a reload.
With pgrep -P you will have the full list of processes attached to the master.

Unfortunately there is no stats socket on the master (yet?), so it's not
possible to do what you suggest.

However, we can maybe add an option to write all new PIDs in the pidfile if
it's easier for supervision.

--
William Lallemand

On 06/11/2017 01:35 μμ, William Lallemand wrote:
> On Mon, Nov 06, 2017 at 12:11:13PM +0100, Pavlos Parissis wrote:
>> On 06/11/2017 11:16 πμ, William Lallemand wrote:
>>> The parent process supervises itself the children, we don't need to
>>> store the children pids anymore in the pidfile in master-worker mode.
>>
>> I have a small objection against this. Having PIDs in a file allows external tools to monitor the
>> workers. If those PIDs aren't written in a the pidfile then those tools have to use pgrep to find
>> them, unless the master process can provide them in some way(stats socket?).
>>
>> My 2cents,
>> Pavlos
>>
>
> Hi Pavlos,
>
> This patch was made to prevent scripts to send signals to the children directly
> instead of sending them to the master which will forward them. That could end
> in reporting a wrong exit code in the master for example.
>
> One of the problem of the pidfile, is that only the latest children were
> written, so you wouldn't have the remaining PID in this list on a reload.
> With pgrep -P you will have the full list of processes attached to the master.
>
> Unfortunately there is no stats socket on the master (yet?), so it's not
> possible to do what you suggest.
>
> However, we can maybe add an option to write all new PIDs in the pidfile if
> it's easier for supervision.
>

That will be very much appreciated as it will allow us to have a smooth migration to the new master
process model.

Cheers,
Pavlos

Hi Pavlos,

On Mon, Nov 06, 2017 at 03:09:10PM +0100, Pavlos Parissis wrote:
> That will be very much appreciated as it will allow us to have a smooth
> migration to the new master process model.

In fact the current behaviour is to continue to dump the pids if you're
not working in master-worker. If you need to perform some adaptations
for master-worker, I think it would be easier to do them only once to
support the single pid instead of having a temporary period where you
have to deal with N+1 pids and be careful not to touch the workers by
accident.

Just my two cents,
Willy

On 06/11/2017 03:19 μμ, Willy Tarreau wrote:
> Hi Pavlos,
>
> On Mon, Nov 06, 2017 at 03:09:10PM +0100, Pavlos Parissis wrote:
>> That will be very much appreciated as it will allow us to have a smooth
>> migration to the new master process model.
>
> In fact the current behaviour is to continue to dump the pids if you're
> not working in master-worker. If you need to perform some adaptations
> for master-worker, I think it would be easier to do them only once to
> support the single pid instead of having a temporary period where you
> have to deal with N+1 pids and be careful not to touch the workers by
> accident.
>

Valid point. So, no objections from me about this commit:-)

Cheers,
Pavlos