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

Replace grimes with tini for default init shipped in Docker #28037

Merged
merged 2 commits into from Nov 9, 2016

Conversation

Projects
None yet
@crosbymichael
Member

crosbymichael commented Nov 3, 2016

There is no reason to duplicate efforts and tini is well built and
better than grimes. It is a much stronger option for the default init
and @krallin has done a great job maintaining it and helping make
changes so that it will work with Docker using it as the default.

The only build change required is that we add cmake to the dockerfile and vim-common because it has tools for embedding the licence in tini that is part of its build process.

You can see the tini repo here:

https://github.com/krallin/tini

The init change has not be released so changing from grimes to tini does not impact anyone out of master.

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

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Nov 3, 2016

@crosbymichael ooooh, there are much more files where you need to change that :)

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 3, 2016

@LK4D4 what do you mean?

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 3, 2016

ahh, yes, the other dockerfiles

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Nov 3, 2016

@crosbymichael and also rpm and deb stuff :/ and don't forget about specs there, too

@crosbymichael crosbymichael force-pushed the crosbymichael:tini branch from 2ae999b to 1a81fb8 Nov 3, 2016

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 3, 2016

@LK4D4 i think I found all the references let me know if I missed anything specifically

@@ -74,6 +75,7 @@ RUN apt-get update && apt-get install -y \
python-websocket \
ubuntu-zfs \
xfsprogs \
vim-common \

This comment has been minimized.

@LK4D4

LK4D4 Nov 3, 2016

Contributor

does this dep of tini?

This comment has been minimized.

@crosbymichael

crosbymichael Nov 3, 2016

Member

Yes, its used during the build an provides features to embed the license of the project into the strings of the binary.

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Nov 3, 2016

LGTM
CI is little dead "though".
Also, I'd love to see CI with --init enabled.

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 3, 2016

Lets see about setting up CI for this before merging

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 3, 2016

Anything to make @LK4D4 happy ;)

@crosbymichael crosbymichael force-pushed the crosbymichael:tini branch from 1a81fb8 to 537befc Nov 3, 2016

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Nov 3, 2016

ping @crosbymichael
experimental failure looks valid:

21:30:55 FAIL: docker_cli_daemon_test.go:2733: DockerDaemonSuite.TestDaemonRestartSaveContainerExitCode
21:30:55 
21:30:55 [d3909b3aae006] waiting for daemon to start
21:30:55 [d3909b3aae006] daemon started
21:30:55 docker_cli_daemon_test.go:2742:
21:30:55     c.Assert(out, checker.Contains, runError)
21:30:55 ... obtained string = "[FATAL tini (5)] Executing child process 'toto' failed: 'No such file or directory'\n"
21:30:55 ... substring string = "exec: \\\"toto\\\": executable file not found in $PATH"
21:30:55 
21:30:55 [d3909b3aae006] exiting daemon

dunno who is toto, though, but probably he is not in Kansas anymore.

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 3, 2016

@LK4D4 ya, i'm working on fixing it. @tiborvass explained to me what toto was one time

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Nov 3, 2016

@LK4D4 it's the signature of a French guy debugging software. It's the equivalent of foo. It's also the main character of the stupidest jokes ever.

@justincormack

This comment has been minimized.

Contributor

justincormack commented Nov 4, 2016

This may have been no different with grimes, but please (at least on platforms with support) can we compile with Musl libc not glibc.

Compiling tini-static on Alpine it is 31440 bytes, vs 764208 compiled static with glibc.

Alpine does not support all the platforms we do yet, but it should soon, and at least the common ones can have a small init.

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 4, 2016

@justincormack whats the best way to do this inside our normal build process?

@crosbymichael crosbymichael force-pushed the crosbymichael:tini branch 3 times, most recently from afb3a84 to 5dd6f51 Nov 4, 2016

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 8, 2016

This PR is ready to be reviewed again.

The change I made in daemon/start.go is to make the error checking coming from the container on a start failure case-insensitive which I think is a good overall change to make if we are comparing error strings vs actual exit codes.

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Nov 8, 2016

LGTM

crosbymichael added some commits Nov 3, 2016

Replace grimes with tini
There is no reason to duplicate efforts and tini is well built and
better than grimes.  It is a much stronger option for the default init
and @krallin has done a great job maintaining it and helping make
changes so that it will work with Docker.

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
Convert err description to lower
Convert this to lower before checking the message of the error.

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

@crosbymichael crosbymichael force-pushed the crosbymichael:tini branch from 5dd6f51 to 47637b4 Nov 8, 2016

@vdemeester

LGTM 🐸

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Nov 9, 2016

LGTM

@cpuguy83 cpuguy83 merged commit 93e837d into moby:master Nov 9, 2016

3 checks passed

experimental Jenkins build Docker-PRs-experimental 26395 has succeeded
Details
janky Jenkins build Docker-PRs 34967 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 5879 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.13.0 milestone Nov 9, 2016

@crosbymichael crosbymichael deleted the crosbymichael:tini branch Nov 9, 2016

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 9, 2016

nice, @krallin fyi ;)

@krallin

This comment has been minimized.

Contributor

krallin commented Nov 9, 2016

Awesome! Looking forward to trying this out in Docker 1.13 😄

@justincormack

This comment has been minimized.

Contributor

justincormack commented Nov 9, 2016

Will look at how best to do Musl build within the normal build...

On 9 Nov 2016 4:52 p.m., "Thomas Orozco" notifications@github.com wrote:

Awesome! Looking forward to trying this out in Docker 1.13 😄


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28037 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAdcPH5mAuLDpd7vKQGjQr2I6Xf8ei13ks5q8ewngaJpZM4Koq9I
.

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Nov 9, 2016

@justincormack thanks. I don't know the best way to do it with our current build process

@dnephin

This comment has been minimized.

Member

dnephin commented Nov 9, 2016

#27359 (with dobi) would make this pretty easy.

We'd have one task for building tini on alpine, and another that depends on it, to build the binary:

image=tini-builder:
  image: docker-tini-dev
  context: dockerfiles/tini/

job=tini:
  use: tini-builder
  mounts: [tini-source]

job=binary:
  use: builder
  depends: [tini]

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

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

vieux added a commit to vieux/docker that referenced this pull request Jan 25, 2017

do not require custom build of tini
krallin/tini#55 (comment)
krallin/tini#55 (comment)
moby#28037

Signed-off-by: Elan Ruusamäe <glen@delfi.ee>
(cherry picked from commit d7df731)
Signed-off-by: Victor Vieux <vieux@docker.com>

@WyseNynja WyseNynja referenced this pull request Mar 25, 2017

Closed

tini isn't needed anymore #3

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

jakirkham referenced this pull request in dask/dask-docker Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment