BUG/MEDIUM: systemd: let the wrapper know that haproxy has completed or failed

Pierre Cheynier found that there's a persistent issue with the systemd
wrapper. Too fast reloads can lead to certain old processes not being
signaled at all and continuing to run. The problem was tracked down as
a race between the startup and the signal processing : nothing prevents
the wrapper from starting new processes while others are still starting,
and the resulting pid file will only contain the latest pids in this
case. This can happen with large configs and/or when a lot of SSL
certificates are involved.

In order to solve this we want the wrapper to wait for the new processes
to complete their startup. But we also want to ensure it doesn't wait for
nothing in case of error.

The solution found here is to create a pipe between the wrapper and the
sub-processes. The wrapper waits on the pipe and the sub-processes are
expected to close this pipe once they completed their startup. That way
we don't queue up new processes until the previous ones have registered
their pids to the pid file. And if anything goes wrong, the wrapper is
immediately released. The only thing is that we need the sub-processes
to know the pipe's file descriptor. We pass it in an environment variable
called HAPROXY_WRAPPER_FD.

It was confirmed both by Pierre and myself that this completely solves
the "zombie" process issue so that only the new processes continue to
listen on the sockets.

It seems that in the future this stuff could be moved to the haproxy
master process, also getting rid of an environment variable.

This fix needs to be backported to 1.6 and 1.5.

diff --git a/src/haproxy-systemd-wrapper.c b/src/haproxy-systemd-wrapper.c
index f4fcab106ea07960cc8fd1cf2e287b99b2da9bd6..b426f96c039c9991329cd97d7bb72f7df8bf69da 100644 (file)
--- a/src/haproxy-systemd-wrapper.c
+++ b/src/haproxy-systemd-wrapper.c
@@ -65,16 +65,30 @@ static void locate_haproxy(char *buffer, size_t buffer_size)
        return;
 }
 
+/* Note: this function must not exit in case of error (except in the child), as
+ * it is only dedicated the starting a new haproxy process. By keeping the
+ * process alive it will ensure that future signal delivery may get rid of
+ * the issue. If the first startup fails, the wrapper will notice it and
+ * return an error thanks to wait() returning ECHILD.
+ */
 static void spawn_haproxy(char **pid_strv, int nb_pid)
 {
        char haproxy_bin[512];
        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) {
+               fprintf(stderr, SD_NOTICE "haproxy-systemd-wrapper: failed to create a pipe, please try again later.\n");
+               return;
+       }
+
        pid = fork();
        if (!pid) {
                char **argv;
@@ -89,6 +103,15 @@ static void spawn_haproxy(char **pid_strv, int nb_pid)
                }
 
                reset_signal_handler();
+
+               close(pipefd[0]); /* close the read side */
+
+               snprintf(fdstr, sizeof(fdstr), "%d", pipefd[1]);
+               if (setenv("HAPROXY_WRAPPER_FD", fdstr, 1) != 0) {
+                       fprintf(stderr, SD_NOTICE "haproxy-systemd-wrapper: failed to setenv(), please try again later.\n");
+                       exit(1);
+               }
+
                locate_haproxy(haproxy_bin, 512);
                argv[argno++] = haproxy_bin;
                for (i = 0; i < main_argc; ++i)
@@ -113,6 +136,19 @@ static void spawn_haproxy(char **pid_strv, int nb_pid)
        else if (pid == -1) {
                fprintf(stderr, SD_NOTICE "haproxy-systemd-wrapper: failed to fork(), please try again later.\n");
        }
+
+       /* 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)

diff --git a/src/haproxy.c b/src/haproxy.c
index b1c10b66d09e5c215d0d78caae0db4e1e95cf467..8198e4efcbafa1ef2696f52616632ae92992df0d 100644 (file)
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -1962,6 +1962,7 @@ int main(int argc, char **argv)
                int ret = 0;
                int *children = calloc(global.nbproc, sizeof(int));
                int proc;
+               char *wrapper_fd;
 
                /* the father launches the required number of processes */
                for (proc = 0; proc < global.nbproc; proc++) {
@@ -1998,6 +1999,15 @@ int main(int argc, char **argv)
                        close(pidfd);
                }
 
+               /* each child must notify the wrapper that it's ready by closing the requested fd */
+               wrapper_fd = getenv("HAPROXY_WRAPPER_FD");
+               if (wrapper_fd) {
+                       int 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;