Hi again,
On Thu, Apr 11, 2019 at 06:24:47PM +0200, Willy Tarreau wrote:
> Hi Patrick,
>
> On Thu, Apr 11, 2019 at 12:18:14PM -0400, Patrick Hemmer wrote:
> > With haproxy 1.9.6 the `stats bind-process` directive is not working. Every
> > connection to the socket is going to a random process:
> >
> > Here's a simple reproduction:
> > Config:
> > global
> > nbproc 3
> > stats socket /tmp/haproxy.sock level admin
> > stats bind-process 1
> >
> >
> > Testing:
> > # for i in {1..5}; do socat - unix:/tmp/haproxy.sock <<< "show info" |
> > grep Pid: ; done
> > Pid: 33371
> > Pid: 33373
> > Pid: 33372
> > Pid: 33373
> > Pid: 33373
>
> This must be pretty annoying. I don't have memories of anything changed
> regarding the bind-process stuff between 1.8 and 1.9 (the threads have
> moved a lot however). It could be a side effect of some of these changes
> though.
>
> However I'm seeing that adding "process 1" on the "stats socket" line
> itself fixes the problem. I suspect the issue is located in the propagation
> of the frontend's mask to the listener, I'll look at this.
So I've found the problem, it's caused by this line (that I commented) in
the master-worker code :
--- a/src/cli.c
+++ b/src/cli.c
@@ -2556,7 +2556,7 @@ int mworker_cli_sockpair_new(struct mworker_proc
*mworker_proc, int proc)
bind_conf->level |= ACCESS_LVL_ADMIN; /* TODO: need to lower the rights
with a CLI keyword*/
bind_conf->bind_proc = 1UL << proc;
- global.stats_fe->bind_proc = 0; /* XXX: we should be careful with that,
it can be removed by configuration */
+ //global.stats_fe->bind_proc = 0; /* XXX: we should be careful with
that, it can be removed by configuration>
if (!memprintf(&path, "sockpair@%d", mworker_proc->ipc_fd[1])) {
ha_alert("Cannot allocate listener.\n");
I *think* it does this to make sure that there can be one master-worker
socket per process between the master and each worker. But as you've
found, it also breaks the "stats bind-process" directive. At this point
I'm confused and don't really know what to do since it appears that the
same config element is needed for two different things. We might be able
to try to emulate the behaviour by storing the "stats bind-process"
setting somewhere else, but it would just push the problem elsewhere until
it's rediscovered and I don't really like this.
I'm leaning towards what I'd consider a cleaner and more future-proof
option consisting in deprecating "stats bind-process" in favor of "process"
on the stats line (which binds the listener itself, not the whole frontend)
and emitting a warning at startup time if master-worker + this option are
set together, explaining what to do instead. I think it could be a reasonably
acceptable change for 1.9/2.0 given that the overlap between master-worker
and "stats bind-process" should not be high.
Would you see any issue with this if the warning gives all the indications
on what to do ?
Thanks,
Willy
