Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add init process for zombie fighting and signal handling #26061

Merged
merged 1 commit into from Sep 20, 2016

Conversation

@crosbymichael
Copy link
Member

crosbymichael commented Aug 26, 2016

This adds a small C binary for fighting zombies. It is mounted under
/dev/init and is prepended to the args specified by the user. You
enable it via a daemon flag, dockerd --init, as it is disable by
default for backwards compat.

You can also override the daemon option or specify this on a per
container basis with docker run --init=true|false.

You can test this by running a process like this as the pid 1 in a
container and see the extra zombie that appears in the container as it
is running.

int main(int argc, char ** argv) {
    pid_t pid = fork();
    if (pid == 0) {
        pid = fork();
        if (pid == 0) {
            exit(0);
        }
        sleep(3);
        exit(0);
    }
    printf("got pid %d and exited\n", pid);
    sleep(20);
}

Fixes #3793
Fixes #24284

Signed-off-by: Michael Crosby crosbymichael@gmail.com

@tonistiigi

This comment has been minimized.

Copy link
Member

tonistiigi commented Aug 26, 2016

# dockerd -s overlay -D --reaper 2>/daemon.log &
# docker run busybox /nosuchfile
error: No such file or directory
# echo $?
1

# dockerd -s overlay -D 2>/daemon.log &
# docker run busybox /nosuchfile
docker: Error response from daemon: oci runtime error: exec: "/nosuchfile": stat /nosuchfile: no such file or directory.
# echo $?
127

We should add an option to run integration-cli with both modes. I'm sure we have a test for this case.

@crosbymichael crosbymichael force-pushed the crosbymichael:reaper branch from 30db3f9 to b7f2306 Aug 26, 2016

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Aug 30, 2016

Looks like this will resolve #3793

Destination: "/dev/init",
Type: "bind",
Source: path,
Options: []string{"bind", "rw"},

This comment has been minimized.

@AkihiroSuda

@crosbymichael crosbymichael force-pushed the crosbymichael:reaper branch from b7f2306 to 3388119 Aug 30, 2016

@crosbymichael crosbymichael changed the title Add init process for zombie fighting Add init process for zombie fighting and signal handling Aug 30, 2016

@crosbymichael crosbymichael force-pushed the crosbymichael:reaper branch 3 times, most recently from 4b6aefe to 393515d Aug 30, 2016

@crosbymichael

This comment has been minimized.

Copy link
Member Author

crosbymichael commented Aug 31, 2016

Ok, so as far as flags go, @tonistiigi and I talked about this some. We are not totally sold on what the flag name should be but we have a good idea on how it should work.

On the daemon we will have a bool --init flag. This will be false by default, and if set to true, will run the custom docker init in every container.

On run, we will also have a --init flag that is a nullable bool. If it is not set, we use the daemon default. If it is set to true, we use the docker init for the container, if it is --init=false we will not use the init. This allows us to have bot a daemon wide option and the ability to enable/disable per container, without changing anything for users that don't have this set.

If anyone has a better idea for a flag name let me know.

@crosbymichael crosbymichael force-pushed the crosbymichael:reaper branch from 393515d to 22c3f4d Aug 31, 2016

@crosbymichael

This comment has been minimized.

Copy link
Member Author

crosbymichael commented Aug 31, 2016

Updated this to use --init for now on both the daemon and run. You can try out all the various combinations.

@icecrime

This comment has been minimized.

Copy link
Contributor

icecrime commented Aug 31, 2016

Moving this to code review: I think there no big controversy on the design 👍 I agree with everything in that comment #26061 (comment).

@@ -2402,30 +2402,6 @@ func (s *DockerSuite) TestRunExposePort(c *check.C) {
c.Assert(out, checker.Contains, "invalid range format for --expose")
}

func (s *DockerSuite) TestRunUnknownCommand(c *check.C) {
out, _, _ := dockerCmdWithStdoutStderr(c, "create", "busybox", "/bin/nada")

This comment has been minimized.

@tonistiigi

tonistiigi Aug 31, 2016

Member

What if we do a lookup and mode check for the entrypoint before actually executing to maintain this behavior?

This comment has been minimized.

@crosbymichael

crosbymichael Aug 31, 2016

Author Member

Like in the docker daemon?

This comment has been minimized.

@tonistiigi

tonistiigi Aug 31, 2016

Member

in daemon or containerd

This comment has been minimized.

@crosbymichael

crosbymichael Aug 31, 2016

Author Member

won't work because lookups have to happen inside the container's mount ns after all volumes are mounted because PATH and binaries could live inside them

This comment has been minimized.

@tonistiigi

tonistiigi Aug 31, 2016

Member

We already have same hack for cp.

This comment has been minimized.

@crosbymichael

crosbymichael Sep 6, 2016

Author Member

yes, "hack"

@@ -75,40 +75,6 @@ func (s *DockerSuite) TestEventsUntag(c *check.C) {
}
}

func (s *DockerSuite) TestEventsContainerFailStartDie(c *check.C) {

This comment has been minimized.

@icecrime

icecrime Aug 31, 2016

Contributor

Why is this change related to the PR?

This comment has been minimized.

@crosbymichael

crosbymichael Sep 8, 2016

Author Member

If we run the tests with this feature enabled these two will fail because they check that the cmd passed does not exist and it will always exist because we prepend our init.

It will still exit with a 127 but not at start time. I tried looking through the history to see why these these were added but I couldn't find anything.

We went over this same discussion in the runc change for 1.11 and I said that if we were to ever solve the zombie problem this is not going to work. We get the same outcome, the containers fails with 127 exit status but it does not return the error on start.

This comment has been minimized.

@icecrime

icecrime Sep 9, 2016

Contributor

I see, thanks 👍

@@ -2402,30 +2402,6 @@ func (s *DockerSuite) TestRunExposePort(c *check.C) {
c.Assert(out, checker.Contains, "invalid range format for --expose")
}

func (s *DockerSuite) TestRunUnknownCommand(c *check.C) {

This comment has been minimized.

@icecrime

icecrime Aug 31, 2016

Contributor

Why is this change related to the PR?

@icecrime

This comment has been minimized.

Copy link
Contributor

icecrime commented Aug 31, 2016

Small questions regarding tests, otherwise LGTM 👍


// only add the custom init if it is specified and the container is running in its
// own private pid namespace. It does not make sense to add if it is running in the
// host namespace or another container's pid namespace were we already have an init

This comment has been minimized.

@justincormack

justincormack Sep 19, 2016

Contributor

typo "were" not "where"

@rogaha

This comment has been minimized.

Copy link
Contributor

rogaha commented Oct 6, 2016

+1, thanks for working on it @crosbymichael! That's awesome!

@thaJeztah thaJeztah referenced this pull request Oct 30, 2016

Open

PID 0 zombie reap #120

@krallin krallin referenced this pull request Nov 2, 2016

Closed

tini flags and `--` #55

@glensc

This comment has been minimized.

Copy link
Contributor

glensc commented Nov 14, 2016

is the /dev abuse really a good idea?

i recall discussion about /run because vendors abused /dev for their early boot dirty work:
https://lists.fedoraproject.org/pipermail/devel/2011-March/150031.html

perhaps just use file named docker-init (or .docker-init) in root filesystem or /lib or /sbin?

glensc added a commit to pld-linux/docker that referenced this pull request Nov 15, 2016

add init reaper via tini
moby/moby#26061
moby/moby#28037

own copy (instead of distro package)
because it requires tini with no args build
krallin/tini#55
@ndeloof

This comment has been minimized.

Copy link
Contributor

ndeloof commented Nov 15, 2016

That's a nice improvement to get docker handle this PID1 issue. But I'm concerned this moves the responsibility to run with --init option into end users hands. For sample, the jenkins docker image uses tiny for same purpose, but end user don't have to even know what a zombie process is.

I suggest one could use some metadata in image to let runtime know such init process is required, for sample by adding to Dockerfile: LABEL io.docker.init='true'

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Nov 15, 2016

I suggest one could use some metadata in image to let runtime know such init process is required, for sample by adding to Dockerfile: LABEL io.docker.init='true'

In cases where a zombie reaper is required (in most cases, it should not even be needed), it should be added by the image author. I think this PR should be seen as a "last effort" if the image is not under control of the user.

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Nov 15, 2016

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jan 16, 2017

Add version annotation to various flags added in 1.13
Pull request moby#27745 added support for the
client to talk to older versions of the daemon. Various flags were added to
docker 1.13 that are not compatible with older daemons.

This PR adds annotations to those flags, so that they are automatically hidden
if the daemon is older than docker 1.13 (API 1.25).

Not all new flags affect the API (some are client-side only). The following
PR's added new flags to docker 1.13 that affect the API;

- moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime`
- moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm`
- moby#27702 added `--network` to `docker build`
- moby#25962 added `--attachable` to `docker network create`
- moby#27998 added `--compose-file` to `docker stack deploy`
- moby#22566 added `--stop-timeout` to `docker run` and `docker create`
- moby#26061 added `--init` to `docker run` and `docker create`
- moby#26941 added `--init-path` to `docker run` and `docker create`
- moby#27958 added `--cpus` on `docker run` / `docker create`
- moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create`
- moby#27596 added `--force` to `docker service update`
- moby#27857 added `--hostname` to `docker service create`
- moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update`
- moby#28076 added `--tty` on `docker service create` / `docker service update`
- moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update`
- moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update`

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@WyseNynja WyseNynja referenced this pull request Mar 25, 2017

Closed

tini isn't needed anymore #3

andrewhsu pushed a commit to docker/docker-ce that referenced this pull request May 19, 2017

Add version annotation to various flags added in 1.13
Pull request moby/moby#27745 added support for the
client to talk to older versions of the daemon. Various flags were added to
docker 1.13 that are not compatible with older daemons.

This PR adds annotations to those flags, so that they are automatically hidden
if the daemon is older than docker 1.13 (API 1.25).

Not all new flags affect the API (some are client-side only). The following
PR's added new flags to docker 1.13 that affect the API;

- moby/moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime`
- moby/moby#27800 / moby/moby#25317 added `--group` / `--group-add` / `--group-rm`
- moby/moby#27702 added `--network` to `docker build`
- moby/moby#25962 added `--attachable` to `docker network create`
- moby/moby#27998 added `--compose-file` to `docker stack deploy`
- moby/moby#22566 added `--stop-timeout` to `docker run` and `docker create`
- moby/moby#26061 added `--init` to `docker run` and `docker create`
- moby/moby#26941 added `--init-path` to `docker run` and `docker create`
- moby/moby#27958 added `--cpus` on `docker run` / `docker create`
- moby/moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create`
- moby/moby#27596 added `--force` to `docker service update`
- moby/moby#27857 added `--hostname` to `docker service create`
- moby/moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update`
- moby/moby#28076 added `--tty` on `docker service create` / `docker service update`
- moby/moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update`
- moby/moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update`

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db
Component: cli

srust added a commit to srust/moby that referenced this pull request Nov 30, 2017

Add version annotation to various flags added in 1.13
Pull request moby#27745 added support for the
client to talk to older versions of the daemon. Various flags were added to
docker 1.13 that are not compatible with older daemons.

This PR adds annotations to those flags, so that they are automatically hidden
if the daemon is older than docker 1.13 (API 1.25).

Not all new flags affect the API (some are client-side only). The following
PR's added new flags to docker 1.13 that affect the API;

- moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime`
- moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm`
- moby#27702 added `--network` to `docker build`
- moby#25962 added `--attachable` to `docker network create`
- moby#27998 added `--compose-file` to `docker stack deploy`
- moby#22566 added `--stop-timeout` to `docker run` and `docker create`
- moby#26061 added `--init` to `docker run` and `docker create`
- moby#26941 added `--init-path` to `docker run` and `docker create`
- moby#27958 added `--cpus` on `docker run` / `docker create`
- moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create`
- moby#27596 added `--force` to `docker service update`
- moby#27857 added `--hostname` to `docker service create`
- moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update`
- moby#28076 added `--tty` on `docker service create` / `docker service update`
- moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update`
- moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update`

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

rdallman added a commit to fnproject/fn that referenced this pull request Sep 4, 2018

use tini to run every container
fixes #1101

additional context:

* this was introduced in docker 1.13 (1/2017), we require docker 17.10
(10/2017), this should not have any issues dependency-wise, as `docker-init`
is in the docker install from that point in time. unless explicitly removed,
it should be in the dind container we use as well...
* the PR that introduced this to docker is
moby/moby#26061 for additional context
* it may be wise to put this through some paces, if anybody has any...
interesting... function containers. the tests seem to work fine, however, and
this shouldn't be something users have to think about (?) at all, just
something that we are doing. this isn't the default in docker for
compatibility reasons, which is maybe a yellow flag but I am not sure tbh

rdallman added a commit to fnproject/fn that referenced this pull request Sep 4, 2018

use tini to run every container (#1195)
fixes #1101

additional context:

* this was introduced in docker 1.13 (1/2017), we require docker 17.10
(10/2017), this should not have any issues dependency-wise, as `docker-init`
is in the docker install from that point in time. unless explicitly removed,
it should be in the dind container we use as well...
* the PR that introduced this to docker is
moby/moby#26061 for additional context
* it may be wise to put this through some paces, if anybody has any...
interesting... function containers. the tests seem to work fine, however, and
this shouldn't be something users have to think about (?) at all, just
something that we are doing. this isn't the default in docker for
compatibility reasons, which is maybe a yellow flag but I am not sure tbh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.