[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
Re: [PATCH] MINOR: mworker: do not store child pid anymore in the pidfile
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 signature.asc Description: OpenPGP digital signature
Re: [PATCH] MINOR: mworker: do not store child pid anymore in the pidfile
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
Re: [PATCH] MINOR: mworker: do not store child pid anymore in the pidfile
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 signature.asc Description: OpenPGP digital signature
Re: [PATCH] MINOR: mworker: do not store child pid anymore in the pidfile
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
Re: [PATCH] MINOR: mworker: do not store child pid anymore in the pidfile
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 signature.asc Description: OpenPGP digital signature
