From 9ae8b38549128ea5d3112d1d8d07dc12080a1944 Mon Sep 17 00:00:00 2001 From: Peter Bourgon Date: Wed, 7 Oct 2015 15:39:45 +0200 Subject: [PATCH] Review feedback --- README.md | 32 ++++++++++++++++++-------------- example/Makefile | 8 ++++---- main.go | 20 +++++++++++++------- zombietest/Makefile | 8 ++++---- zombietest/test.bash | 9 +++++---- 5 files changed, 44 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index bf03389..5d72b0e 100644 --- a/README.md +++ b/README.md @@ -1,19 +1,27 @@ # runsvinit [![Circle CI](https://circleci.com/gh/peterbourgon/runsvinit.svg?style=svg)](https://circleci.com/gh/peterbourgon/runsvinit) -If you have a Docker container that's a collection of runit-supervised daemons, -this process is suitable for use as the ENTRYPOINT. +If you have a Docker container that's a collection of runit-supervised daemons, this process is suitable for use as the ENTRYPOINT. See [the example](https://github.com/peterbourgon/runsvinit/tree/master/example). -**Why not just exec runsvdir?** +**Why not use runit(8) directly?** -`docker stop` issues SIGTERM (or, in a future version of Docker, perhaps another custom signal) -but if runsvdir receives a signal, -it doesn't wait for its supervised processes to exit before returning. -If you don't care about graceful shutdown of your daemons, no problem, you don't need this tool. +[runit(8)](http://smarden.org/runit/runit.8.html) is designed to be used as process 1. +And, if you provide an `/etc/service/ctrlaltdel` script, it will be executed when runit receives the INT signal. +So, we could use that hook to gracefully terminate our services. +But Docker only sends TERM on `docker stop`. -**Why not wrap runsvdir in a simple shell script?** +Note that the container stop signal [will become configurable](https://github.com/docker/docker/pull/15307) in Docker 1.9. +Once Docker 1.9 ships, this utility will be obsolete. -This works great: +**Why not just exec runsvdir(8) directly?** + +If [runsvdir(8)](http://smarden.org/runit/runsvdir.8.html) receives a signal, it doesn't wait for its supervised processes to exit before returning. + +**Why not wrap runsvdir(8) in a simple shell script?** + +Process 1 has the additional responsibility of [reaping orphaned child processes](https://blog.phusion.nl/2015/01/20/docker-and-the-pid-1-zombie-reaping-problem/). +To the best of my knowledge, there is no sane way to do this with a shell script. +Otherwise, indeed, this would work great: ```sh #!/bin/sh @@ -30,14 +38,10 @@ trap "sv_stop; exit" SIGTERM wait ``` -...except it doesn't [reap orphaned child processes](https://blog.phusion.nl/2015/01/20/docker-and-the-pid-1-zombie-reaping-problem/) -and is therefore unsuitable for being PID 1. - **Why not use my_init from [phusion/baseimage-docker](https://github.com/phusion/baseimage-docker)?** -That works great — if you're willing to add python3 to your Docker images :) +my_init depends on Python 3, which might be a big dependency for such a small responsibility. **So this is just a stripped-down my_init in Go?** Basically, yes. - diff --git a/example/Makefile b/example/Makefile index 5629007..2bd5392 100644 --- a/example/Makefile +++ b/example/Makefile @@ -2,9 +2,9 @@ docker: runsvinit docker build -t runsvinit-example . -runsvinit: runsvinit-linux-amd64.tgz - tar zxf $< +runsvinit: $GOPATH/bin/runsvinit + cp $< $@ -runsvinit-linux-amd64.tgz: - wget --quiet https://github.com/peterbourgon/runsvinit/releases/download/v2.0.0/runsvinit-linux-amd64.tgz +$GOPATH/bin/runsvinit: + go get -u github.com/peterbourgon/runsvinit diff --git a/main.go b/main.go index 5942e6a..b043d11 100644 --- a/main.go +++ b/main.go @@ -8,6 +8,7 @@ import ( "os/signal" "path/filepath" "strings" + "sync" "syscall" ) @@ -121,7 +122,7 @@ func shutdown(sv string, s signaler) { return } - var stopped []string + var wg sync.WaitGroup for _, match := range matches { fi, err := os.Stat(match) if err != nil { @@ -134,14 +135,19 @@ func shutdown(sv string, s signaler) { } service := filepath.Base(match) stop := cmd(sv, "stop", service) - if err := stop.Run(); err != nil { - infof("%s: %v", strings.Join(stop.Args, " "), err) - continue - } - stopped = append(stopped, service) + debugf("stopping %s...", service) + wg.Add(1) + go func() { + defer wg.Done() + if err := stop.Run(); err != nil { + infof("%s: %v", strings.Join(stop.Args, " "), err) + } else { + debugf("stopped %s", service) + } + }() } + wg.Wait() - debugf("stopped %d: %s", len(stopped), strings.Join(stopped, ", ")) debugf("stopping supervisor with signal %s...", sig) if err := s.Signal(sig); err != nil { info(err) diff --git a/zombietest/Makefile b/zombietest/Makefile index c1d9b19..6e2c9a1 100644 --- a/zombietest/Makefile +++ b/zombietest/Makefile @@ -1,6 +1,6 @@ -GO?=go -SUDO?= -RM?=--rm +GO ?= go +SUDO ?= +RMCONTAINER ?= --rm .PHONY: test clean @@ -15,7 +15,7 @@ runsvinit: ../*.go env GOOS=linux GOARCH=amd64 $(GO) build -o $@ github.com/peterbourgon/runsvinit zombie: .build.uptodate - $(SUDO) docker run $(RM) -v $(shell pwd):/mount zombietest-build cc -Wall -Werror -o /mount/zombie /zombie.c + $(SUDO) docker run $(RMCONTAINER) -v $(shell pwd):/mount zombietest-build cc -Wall -Werror -o /mount/zombie /zombie.c .build.uptodate: build/zombie.c build/Dockerfile $(SUDO) docker build -t zombietest-build build/ diff --git a/zombietest/test.bash b/zombietest/test.bash index f6339d8..bf4f613 100755 --- a/zombietest/test.bash +++ b/zombietest/test.bash @@ -11,14 +11,15 @@ function zombies() { } function stop_rm() { - docker stop $1 - docker rm $1 + docker stop $1 >/dev/null + #docker logs $1 + docker rm $1 >/dev/null } SLEEP=1 RC=0 -C=$(docker run -d zombietest /runsvinit -reap=false) +C=$(docker run -d zombietest /runsvinit -reap=false -debug) sleep $SLEEP NOREAP=$(zombies) echo -n without reaping, we have $NOREAP zombies... @@ -31,7 +32,7 @@ else fi stop_rm $C -C=$(docker run -d zombietest /runsvinit) +C=$(docker run -d zombietest /runsvinit -debug) sleep $SLEEP YESREAP=$(zombies) echo -n with reaping, we have $YESREAP zombies...