On Mon, Oct 24, 2016 at 09:13:13PM +0200, Willy Tarreau wrote:
> BTW, that just makes me realize that we also have another possibility for this
> precisely using a pipe (which are more portable than mandatory locks). Let's
> see if that would work. The wrapper creates a pipe then forks. The child
> closes the read side, the parent the write side. Then the parent performs a
> read() on this fd and waits until it returns zero. The child execve() and
> calls the haproxy sub-processes. The FD is closed after the pidfile is updated
> (and in children). After the last close, the wrapper receives a zero on this
> pipe. If haproxy dies, the pipe is closed as well. We could even (ab)use it
> to let the wrapper know whether the process properly started or not, or pass
> the pids there (though that just needlessly complicates operations).
So before taking the road back home I decided to give it a try. Here comes the
patch. "It works for me"(TM). Instead of seeing my kill loop leave over 500
processes while spinning like mad, now I constantly have 10 for nbproc=10,
which is good. I also fixed a small bug by which the wrapper could get killed
by its own signal during the reexec where signals are unmasked.
The patch is ugly for now, it's just a proof of concept, it lacks any error
checking. I cannot make it fail anymore, so please try it, hammer it, torture
it. If it works fine I'm even willing to backport it to 1.6 given that it's
not much intrusive and will fix the problems for all victims of systemd.
Regards,
Willy
>From 50970eb815dabfab9443aa3e80ce710d8af99feb Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Mon, 24 Oct 2016 22:17:17 +0200
Subject: EXP: maintain a pipe between the wrapper and its kids to notify when
it's safe to reload
X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4
Using this trick, the wrapper refrains from performing new reloads until the
currently starting processes either succeed or die.
This command used to leave >500 kids for a nbproc=10 config involving SSL
(slower to start), now it leaves exactly 10 :
$ for i in {1..100}; do killall -USR2 haproxy-systemd-wrapper;done
It also fixes a bug where the newly re-executed wrapper could catch the
signal before intercepting it and die. Now we ignore it just before doing
the execve.
---
src/haproxy-systemd-wrapper.c | 29 +++++++++++++++++++++++++++++
src/haproxy.c | 13 +++++++++++++
2 files changed, 42 insertions(+)
diff --git a/src/haproxy-systemd-wrapper.c b/src/haproxy-systemd-wrapper.c
index d118ec6..b2065d9 100644
--- a/src/haproxy-systemd-wrapper.c
+++ b/src/haproxy-systemd-wrapper.c
@@ -66,16 +66,28 @@ static void spawn_haproxy(char **pid_strv, int nb_pid)
pid_t pid;
int main_argc;
char **main_argv;
+ int pipefd[2];
+ char fdstr[20];
+ int ret;
main_argc = wrapper_argc - 1;
main_argv = wrapper_argv + 1;
+ if (pipe(pipefd) != 0)
+ exit(1);
+
pid = fork();
if (!pid) {
/* 3 for "haproxy -Ds -sf" */
char **argv = calloc(4 + main_argc + nb_pid + 1, sizeof(char
*));
int i;
int argno = 0;
+
+ close(pipefd[0]); /* close the read side */
+
+ snprintf(fdstr, sizeof(fdstr), "%d", pipefd[1]);
+ setenv("HAPROXY_WRAPPER_FD", fdstr, 1);
+
locate_haproxy(haproxy_bin, 512);
argv[argno++] = haproxy_bin;
for (i = 0; i < main_argc; ++i)
@@ -96,6 +108,19 @@ static void spawn_haproxy(char **pid_strv, int nb_pid)
execv(argv[0], argv);
exit(0);
}
+
+ /* The parent closes the write side and waits for the child to close it
+ * as well. Also deal the case where the fd would unexpectedly be 1 or 2
+ * by silently draining all data.
+ */
+ close(pipefd[1]);
+
+ do {
+ char c;
+ ret = read(pipefd[0], &c, sizeof(c));
+ } while ((ret > 0) || (ret == -1 && errno == EINTR));
+ /* the child has finished starting up */
+ close(pipefd[0]);
}
static int read_pids(char ***pid_strv)
@@ -134,6 +159,10 @@ static void do_restart(int sig)
fprintf(stderr, SD_NOTICE "haproxy-systemd-wrapper: re-executing on
%s.\n",
sig == SIGUSR2 ? "SIGUSR2" : "SIGHUP");
+ /* don't let the other process take one of those signals by accident */
+ signal(SIGUSR2, SIG_IGN);
+ signal(SIGHUP, SIG_IGN);
+ signal(SIGINT, SIG_IGN);
execv(wrapper_argv[0], wrapper_argv);
}
diff --git a/src/haproxy.c b/src/haproxy.c
index b1c10b6..522fad0 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -1998,6 +1998,19 @@ int main(int argc, char **argv)
close(pidfd);
}
+ /* each child must notify the wrapper that it's ready by
closing the requested fd */
+ {
+ char *wrapper_fd;
+ int pipe_fd = -1;
+
+ wrapper_fd = getenv("HAPROXY_WRAPPER_FD");
+ if (wrapper_fd)
+ pipe_fd = atoi(wrapper_fd);
+
+ if (pipe_fd >= 0)
+ close(pipe_fd);
+ }
+
/* We won't ever use this anymore */
free(oldpids); oldpids = NULL;
free(global.chroot); global.chroot = NULL;
--
1.7.12.1
