Consider making Consul act as a viable "pid 1" in containers

[vidarh]

More background in this ticket: gliderlabs/docker-consul#45

Basically, because of the consul support for executing health checks etc., if consul is run under Docker as pid 1, and a health check in effect double forks, say because the health check is a shell script shelling out to the docker client or curl and then gets killed when it exceeds the interval, the "double-forked" processes will end up as zombies until the container is restarted.

This is not a bug in Consul, as in a "normal" Linux environment init will reap those zombies. One solution is simply to run Consul under an init. However it is also desirable to make containers as simple as possible, and an alternative approach would be to include something similar to this when starting the Consul server if it is started as pid 1 (it shouldn't do any harm to do it unconditionally):

go func() {
    var wstatus syscall.WaitStatus

    for {
        pid, err := syscall.Wait4(-1, &wstatus, 0, nil)
        if err != nil {
            log.Println(err)
        } else {
            log.Println("Reaping child", pid)
        }
    }
}()

Thoughts?

[vidarh]

To expand on the motivation: Even many small inits ends up at hundreds of KB when compiled with gcc. Including musl in the build process reduces the size drastically at the cost of pulling in additional stuff in the build process. A standalone minimal Go init ended up at 2.9MB.

Busybox is an alternative, but then requires an inittab and additional hassle to achieve the equivalent of a proper entrypoint that can take arguments as its init can't take arguments on the command line.

Adding the few lines above takes away all of the hassle for users that wish to build a reliable containerised Consul.

[aidanhs]

Consul is 18MB. Seems excessive to worry about a few hundred KB?

[aidanhs]

And fwiw, you can compile a static tini in under a minute with no requirement other than docker, so correct my 'about a few hundred KB' to 'about 30KB'.

~ $ time docker run --rm alpine:3.2 sh -c '(apk -U add build-base cmake git && git clone https://github.com/krallin/tini.git && cd tini && cmake . && make) >&2 && cat /tini/tini-static' > tini-static
Unable to find image 'alpine:3.2' locally
3.2: Pulling from alpine
[...]
fetch http://dl-4.alpinelinux.org/alpine/v3.2/main/x86_64/APKINDEX.tar.gz
(1/42) Upgrading musl (1.1.9-r2 -> 1.1.9-r5)
[...]
(42/42) Installing git (2.4.1-r0)
Executing busybox-1.23.2-r0.trigger
Executing ca-certificates-20141019-r2.trigger
OK: 170 MiB in 55 packages
Cloning into 'tini'...
-- The C compiler identification is GNU 4.9.2
[...]
Linking C executable tini-static
[100%] Built target tini-static

real    0m36.680s
user    0m0.044s
sys 0m0.011s
$ du -sh tini-static
32K tini-static
$ ldd tini-static
    not a dynamic executable
$ chmod +x tini-static
$ ./tini-static -h
tini-static (version 0.5.0 - 98311ca)
Usage: tini-static [OPTIONS] PROGRAM -- [ARGS]

Execute a program under the supervision of a valid init process (tini-static)

  -h: Show this help message and exit.
  -v: Generate more verbose output. Repeat up to 4 times.

[vidarh]

@aidanhs It's not just the "few hundred KB" that's the issue, though I on principle don't like unnecessarily adding lots of unnecessary bloat to avoid this small amount of code. The bigger issue is that it complicates building a container for Consul by requiring pulling in and building packages for another Init, or setting up an inittab for something like Busybox, when the issue can be avoided entirely with <10 lines of code in Consul.

This is a step that, by searching for "consul" on Dockerhub and checking the Dockerfiles, almost nobody are aware they need to take: At least 8 out of the first 10 available Consul containers I checked has this problem.

Which means a lot of people are going to run into slowly building zombies, and potentially containers locking up when the process table fills.

Fixing that either requires educating everyone who builds these containers, or making Consul resilient to it. And while the ideal situation might be to educate everyone about it anyway, I'm a big fan of defensive programming when the cost is low...

[vidarh]

@aidanhs That's great, but you're paying that cost to avoid 6-7 lines of code in Consul. Seems like a really bad tradeoff to me. Especially given that to fix this issue it'd still be necessary to convince everyone building Consul containers to do it right in every Dockerfile. And it clearly in that case needs to be documented, as it's clear it's totally non-obvious to pretty much everyone who tries to package Consul..

(edit: I really appreciate the example of using an Alpine container to build stuff against musl, though - I'm definitively stealing that)

[slackpad]

Talked about this internally and we will go ahead and add the child reaping to Consul itself. Seems like an easy way to reduce the complexity of running inside a container.