On Thu, Apr 13, 2017 at 03:06:47PM +0200, Conrad Hoffmann wrote: > > > On 04/13/2017 02:28 PM, Olivier Houchard wrote: > > On Thu, Apr 13, 2017 at 12:59:38PM +0200, Conrad Hoffmann wrote: > >> On 04/13/2017 11:31 AM, Olivier Houchard wrote: > >>> On Thu, Apr 13, 2017 at 11:17:45AM +0200, Conrad Hoffmann wrote: > >>>> Hi Olivier, > >>>> > >>>> On 04/12/2017 06:09 PM, Olivier Houchard wrote: > >>>>> On Wed, Apr 12, 2017 at 05:50:54PM +0200, Olivier Houchard wrote: > >>>>>> On Wed, Apr 12, 2017 at 05:30:17PM +0200, Conrad Hoffmann wrote: > >>>>>>> Hi again, > >>>>>>> > >>>>>>> so I tried to get this to work, but didn't manage yet. I also don't > >>>>>>> quite > >>>>>>> understand how this is supposed to work. The first haproxy process is > >>>>>>> started _without_ the -x option, is that correct? Where does that > >>>>>>> instance > >>>>>>> ever create the socket for transfer to later instances? > >>>>>>> > >>>>>>> I have it working now insofar that on reload, subsequent instances are > >>>>>>> spawned with the -x option, but they'll just complain that they can't > >>>>>>> get > >>>>>>> anything from the unix socket (because, for all I can tell, it's not > >>>>>>> there?). I also can't see the relevant code path where this socket > >>>>>>> gets > >>>>>>> created, but I didn't have time to read all of it yet. > >>>>>>> > >>>>>>> Am I doing something wrong? Did anyone get this to work with the > >>>>>>> systemd-wrapper so far? > >>>>>>> > >>>>>>> Also, but this might be a coincidence, my test setup takes a huge > >>>>>>> performance penalty just by applying your patches (without any > >>>>>>> reloading > >>>>>>> whatsoever). Did this happen to anybody else? I'll send some numbers > >>>>>>> and > >>>>>>> more details tomorrow. > >>>>>>> > >>>>>> > >>>>>> Ok I can confirm the performance issues, I'm investigating. > >>>>>> > >>>>> > >>>>> Found it, I was messing with SO_LINGER when I shouldn't have been. > >>>> > >>>> <removed code for brevity> > >>>> > >>>> thanks a lot, I can confirm that the performance regression seems to be > >>>> gone! > >>>> > >>>> I am still having the other (conceptual) problem, though. Sorry if this > >>>> is > >>>> just me holding it wrong or something, it's been a while since I dug > >>>> through the internals of haproxy. > >>>> > >>>> So, as I mentioned before, we use nbproc (12) and the systemd-wrapper, > >>>> which in turn starts haproxy in daemon mode, giving us a process tree > >>>> like > >>>> this (path and file names shortened for brevity): > >>>> > >>>> \_ /u/s/haproxy-systemd-wrapper -f ./hap.cfg -p /v/r/hap.pid > >>>> \_ /u/s/haproxy-master > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> \_ /u/s/haproxy -f ./hap.cfg -p /v/r/hap.pid -Ds > >>>> > >>>> Now, in our config file, we have something like this: > >>>> > >>>> # expose admin socket for each process > >>>> stats socket ${STATS_ADDR} level admin process 1 > >>>> stats socket ${STATS_ADDR}-2 level admin process 2 > >>>> stats socket ${STATS_ADDR}-3 level admin process 3 > >>>> stats socket ${STATS_ADDR}-4 level admin process 4 > >>>> stats socket ${STATS_ADDR}-5 level admin process 5 > >>>> stats socket ${STATS_ADDR}-6 level admin process 6 > >>>> stats socket ${STATS_ADDR}-7 level admin process 7 > >>>> stats socket ${STATS_ADDR}-8 level admin process 8 > >>>> stats socket ${STATS_ADDR}-9 level admin process 9 > >>>> stats socket ${STATS_ADDR}-10 level admin process 10 > >>>> stats socket ${STATS_ADDR}-11 level admin process 11 > >>>> stats socket ${STATS_ADDR}-12 level admin process 12 > >>>> > >>>> Basically, we have a dedicate admin socket for each ("real") process, as > >>>> we > >>>> need to be able to talk to each process individually. So I was wondering: > >>>> which admin socket should I pass as HAPROXY_STATS_SOCKET? I initially > >>>> thought it would have to be a special stats socket in the haproxy-master > >>>> process (which we currently don't have), but as far as I can tell from > >>>> the > >>>> output of `lsof` the haproxy-master process doesn't even hold any FDs > >>>> anymore. Will this setup currently work with your patches at all? Do I > >>>> need > >>>> to add a stats socket to the master process? Or would this require a list > >>>> of stats sockets to be passed, similar to the list of PIDs that gets > >>>> passed > >>>> to new haproxy instances, so that each process can talk to the one from > >>>> which it is taking over the socket(s)? In case I need a stats socket for > >>>> the master process, what would be the directive to create it? > >>>> > >>> > >>> Hi Conrad, > >>> > >>> Any of those sockets will do. Each process are made to keep all the > >>> listening sockets opened, even if the proxy is not bound to that specific > >>> process, justly so that it can be transferred via the unix socket. > >>> > >>> Regards, > >>> > >>> Olivier > >> > >> > >> Thanks, I am finally starting to understand, but I think there still might > >> be a problem. I didn't see that initially, but when I use one of the > >> processes existing admin sockets it still fails, with the following > >> messages: > >> > >> 2017-04-13_10:27:46.95005 [WARNING] 102/102746 (14101) : We didn't get the > >> expected number of sockets (expecting 48 got 37) > >> 2017-04-13_10:27:46.95007 [ALERT] 102/102746 (14101) : Failed to get the > >> sockets from the old process! > >> > >> I have a suspicion about the possible reason. We have a two-tier setup, as > >> is often recommended here on the mailing list: 11 processes do (almost) > >> only SSL termination, then pass to a single process that does most of the > >> heavy lifting. These process use different sockets of course (we use > >> `bind-process 1` and `bind-process 2-X` in frontends). The message above is > >> from the first process, which is the non-SSL one. When using an admin > >> socket from any of the other processes, the message changes to "(expecting > >> 48 got 17)". > >> > >> I assume the patches are incompatible with such a setup at the moment? > >> > >> Thanks once more :) > >> Conrad > > > > Hmm that should not happen, and I can't seem to reproduce it. > > Can you share the haproxy config file you're using ? Are the number of > > socket > > received always the same ? How are you generating your load ? Is it > > happening > > on each reload ? > > > > Thanks a lot for going through this, this is really appreciated :) > > I am grateful myself you're helping me through this :) > > So I removed all the logic and backends from our config file, it's still > quite big and it still works in our environment, which is unfortunately > quite complex. I can also still reliably reproduce the error with this > config. The number seem consistently the same (except for the difference > between the first process and the others). > > I am not sure if it makes sense for you to recreate the environment we have > this running in, the variables used in the config file are set to the > following values: >
Ah ! Thanks to your help, I think I got it (well really Willy got it, but let's just pretend it's me). The attached patch should hopefully fix that, so that you can uncover yet another issue :). Thanks again ! Olivier
>From ea58ec0a314d8974680a12341e160b6fbceb7e8c Mon Sep 17 00:00:00 2001 From: Olivier Houchard <ohouch...@haproxy.com> Date: Thu, 13 Apr 2017 15:44:48 +0200 Subject: [PATCH 11/11] MINOR: listener: Don't close sockets with the "process" directive. Binding on process can be done both proxy-wide, or listener-wide. Previously the seamless reload code only tracked bind-process per-proxy directive. Introduce a new "LI_ZOMBIE" state for listener, and use that for "process" per-bind directives. --- include/types/listener.h | 1 + src/cli.c | 9 +++++---- src/haproxy.c | 2 +- src/listener.c | 11 ++++++++--- src/proxy.c | 12 ++++++------ 5 files changed, 21 insertions(+), 14 deletions(-) diff --git a/include/types/listener.h b/include/types/listener.h index 227cc28..2b8f5fe 100644 --- a/include/types/listener.h +++ b/include/types/listener.h @@ -47,6 +47,7 @@ enum li_state { LI_INIT, /* all parameters filled in, but not assigned yet */ LI_ASSIGNED, /* assigned to the protocol, but not listening yet */ LI_PAUSED, /* listener was paused, it's bound but not listening */ + LI_ZOMBIE, /* The listener doesn't belong to the process, but is kept opened */ LI_LISTEN, /* started, listening but not enabled */ LI_READY, /* started, listening and enabled */ LI_FULL, /* reached its connection limit */ diff --git a/src/cli.c b/src/cli.c index 533f792..55baee3 100644 --- a/src/cli.c +++ b/src/cli.c @@ -1065,10 +1065,11 @@ static int _getsocks(char **args, struct appctx *appctx, void *private) struct listener *l; list_for_each_entry(l, &px->conf.listeners, by_fe) { - /* Only transfer IPv4/IPv6 sockets */ - if (l->proto->sock_family == AF_INET || + /* Only transfer IPv4/IPv6/UNIX sockets */ + if (l->state >= LI_ZOMBIE && + (l->proto->sock_family == AF_INET || l->proto->sock_family == AF_INET6 || - l->proto->sock_family == AF_UNIX) + l->proto->sock_family == AF_UNIX)) tot_fd_nb++; } px = px->next; @@ -1119,7 +1120,7 @@ static int _getsocks(char **args, struct appctx *appctx, void *private) list_for_each_entry(l, &px->conf.listeners, by_fe) { int ret; /* Only transfer IPv4/IPv6 sockets */ - if (l->state >= LI_LISTEN && + if (l->state >= LI_ZOMBIE && (l->proto->sock_family == AF_INET || l->proto->sock_family == AF_INET6 || l->proto->sock_family == AF_UNIX)) { diff --git a/src/haproxy.c b/src/haproxy.c index 54a457f..2b1db00 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -1676,7 +1676,7 @@ void deinit(void) * because they still hold an opened fd. * Close it and give the listener its real state. */ - if (p->state == PR_STSTOPPED && l->state >= LI_LISTEN) { + if (p->state == PR_STSTOPPED && l->state >= LI_ZOMBIE) { close(l->fd); l->state = LI_INIT; } diff --git a/src/listener.c b/src/listener.c index a38d05d..a99e4c0 100644 --- a/src/listener.c +++ b/src/listener.c @@ -59,7 +59,12 @@ void enable_listener(struct listener *listener) /* we don't want to enable this listener and don't * want any fd event to reach it. */ - unbind_listener(listener); + if (!(global.tune.options & GTUNE_SOCKET_TRANSFER)) + unbind_listener(listener); + else { + unbind_listener_no_close(listener); + listener->state = LI_LISTEN; + } } else if (listener->nbconn < listener->maxconn) { fd_want_recv(listener->fd); @@ -95,7 +100,7 @@ void disable_listener(struct listener *listener) */ int pause_listener(struct listener *l) { - if (l->state <= LI_PAUSED) + if (l->state <= LI_ZOMBIE) return 1; if (l->proto->pause) { @@ -149,7 +154,7 @@ int resume_listener(struct listener *l) return 0; } - if (l->state < LI_PAUSED) + if (l->state < LI_PAUSED || l->state == LI_ZOMBIE) return 0; if (l->proto->sock_prot == IPPROTO_TCP && diff --git a/src/proxy.c b/src/proxy.c index 9e3f901..dc70213 100644 --- a/src/proxy.c +++ b/src/proxy.c @@ -995,10 +995,10 @@ void soft_stop(void) if (p->state == PR_STSTOPPED && !LIST_ISEMPTY(&p->conf.listeners) && LIST_ELEM(p->conf.listeners.n, - struct listener *, by_fe)->state >= LI_LISTEN) { + struct listener *, by_fe)->state >= LI_ZOMBIE) { struct listener *l; list_for_each_entry(l, &p->conf.listeners, by_fe) { - if (l->state >= LI_LISTEN) + if (l->state >= LI_ZOMBIE) close(l->fd); l->state = LI_INIT; } @@ -1082,13 +1082,13 @@ void zombify_proxy(struct proxy *p) listeners--; jobs--; } - if (!first_to_listen && l->state >= LI_LISTEN) - first_to_listen = l; /* * Pretend we're still up and running so that the fd * will be sent if asked. */ - l->state = oldstate; + l->state = LI_ZOMBIE; + if (!first_to_listen && oldstate >= LI_LISTEN) + first_to_listen = l; } /* Quick hack : at stop time, to know we have to close the sockets * despite the proxy being marked as stopped, make the first listener @@ -1096,7 +1096,7 @@ void zombify_proxy(struct proxy *p) * parse the whole list to be sure. */ if (first_to_listen && LIST_ELEM(p->conf.listeners.n, - struct listener *, by_fe)) { + struct listener *, by_fe) != first_to_listen) { LIST_DEL(&l->by_fe); LIST_ADD(&p->conf.listeners, &l->by_fe); } -- 2.9.3