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 <[email protected]>
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
