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

tini flags and -- #55

Closed
crosbymichael opened this issue Nov 1, 2016 · 17 comments
Closed

tini flags and -- #55

crosbymichael opened this issue Nov 1, 2016 · 17 comments

Comments

@crosbymichael
Copy link
Contributor

Because tini has flags the ability to inject init as the init in many containers is not possible without having to change the container. Is there a way that we can remove these options so that this can be a drop in replacement as a container init for something like Docker?

Also would you be interested in having tini the default init for container containers provided by the daemon? I wrote a very similar thing and instead of having two inits I'm interested in using tini instead but the requirement for -- is a deal breaker at the moment and would require changes for what we want to do.

Thanks!

@krallin
Copy link
Owner

krallin commented Nov 2, 2016

Hey @crosbymichael, thanks for reaching out!

Because tini has flags the ability to inject init as the init in many containers is not possible without having to change the container. Is there a way that we can remove these options so that this can be a drop in replacement as a container init for something like Docker?

Would it make sense if Tini behaved like Docker itself does in docker run, and ignored any flags found after the program (image in Docker's case) it's meant to run? For example:

  • If you run tini -v foo, then Tini reads the -v flag, and runs foo.
  • If you run tini foo -v, then Tini runs foo -v.
  • Finally, if you tini -v foo bar -qux, then Tini reads the -v flag, then runs foo bar -qux.

Now, obviously, this won't work if the command to run collides with a Tini flag. This is arguably pretty unlikely, but is it an acceptable tradeoff here?

Note that this would be a very trivial change in Tini (one I should be doing anyway, since right now it means Tini otherwise behaves differently depending on the libc it was compiled with: musl already implements the behavior I'm describing above); I'd just need to add + in front of the opt strings 😄

(One upside of this approach is that curious folks could run /dev/init -h to get an idea of what this thing is)

Another (perhaps safer) solution would be to look at whether /proc/self/exe points to /dev/init and disable option processing altogether in this case. Whether or not this is a good idea probably depends on whether you plan on /dev/init being an implementation detail or not. Let me know!

(One downside of this approach is that /dev/init -h would try to run -h and fail)

Finally, one last (perhaps the simplest) option is of course a compile flag to disable option processing altogerher. I'm not sure how you'd plan to distribute Tini as a container init for Docker, so this may or may not be a good option. But let me know! (under this option, /dev/init -h wouldn't work, of course)

Also would you be interested in having tini the default init for container containers provided by the daemon? I wrote a very similar thing and instead of having two inits I'm interested in using tini instead but the requirement for -- is a deal breaker at the moment and would require changes for what we want to do.

I would, naturally; I'm very flattered that you're considering Tini for this! 😄

(though I must say I'm also a little surprised having followed moby/moby#26061 😉)

I'm happy to make any of all 3 suggestions I listed above. Let me know if any of these are acceptable for Docker's purposes!

Cheers,

@crosbymichael
Copy link
Contributor Author

@krallin thanks and sorry that you were surprised following the PR, I didn't look into this project until after I started working on that PR but now there is no need to duplicate efforts and this project is built very well.

We would be distributing tini in the docker packages so we would also be compiling it inside our build process. I think a flag to disable tini flag parsing would work best for us as its an easy option and we are not abusing flag parsing.

With the large number of applications that are run, flag collisions can happen often especially with the -v, -h flags.

@krallin
Copy link
Owner

krallin commented Nov 2, 2016

@krallin thanks and sorry that you were surprised following the PR, I didn't look into this project until after I started working on that PR but now there is no need to duplicate efforts and this project is built very well.

Ha, no worries at all; I think I was a little unclear here: I actually meant that I was a little surprised by this issue, not by the Docker PR. Sorry about the confusion!

We would be distributing tini in the docker packages so we would also be compiling it inside our build process. I think a flag to disable tini flag parsing would work best for us as its an easy option and we are not abusing flag parsing.

Sounds great! I'll get that in shortly (480fed1 - most of the changes there are for CI, though).

I'm using cmake, so you'd want to do something along the lines of:

cmake -B"${BUILD_DIR}" -H"${SOURCE_DIR}" -DNO_ARGS=ON

Does that work?

(if using cmake is a problem, you could compile Tini compiler directly, but if so let me know, there are a couple things I could do to make the build process simpler without Cmake)

Cheers,

@crosbymichael
Copy link
Contributor Author

Let me check that we have cmake in our buildchain but I don't think it will be an issue.

@crosbymichael
Copy link
Contributor Author

crosbymichael commented Nov 2, 2016

We currently don't have cmake but I'll add it to our builds so that works for me.

@krallin krallin mentioned this issue Nov 2, 2016
@krallin
Copy link
Owner

krallin commented Nov 2, 2016

Hey @crosbymichael, I just released Tini 0.11.0, which includes the new flag.

A couple notes summarizing the build:

  • The build uses xxd to inject the license file into the binary (to expose via a -l flag... but also for strings — more conversation in here: Include license in strings #46), for some reason, this is usually included in a vim-common package or some such.
  • You can access the new "no flags" compile option via: cmake -DNO_ARGS=ON

I should mention that I left in ENV var parsing (since these are prefixed with TINI_) even when the option is set, but I'm happy to ifdef that out too if needed (there's only one: TINI_SUBREAPER).

Finally, if that's useful as a reference, here are the package files for a couple distros that have packaged Tini:

Thanks again!

@crosbymichael
Copy link
Contributor Author

@krallin thanks. The env stuff seems ok to me since it has the TINI_ prefix.

Thanks and i'll try integrating your 0.11.0 release and ping you on the docker PR.

@krallin
Copy link
Owner

krallin commented Nov 2, 2016

Sounds great; thanks @crosbymichael!

@crosbymichael
Copy link
Contributor Author

I opened the docker pr here moby/moby#28037

@krallin
Copy link
Owner

krallin commented Nov 3, 2016

Awesome; thanks @crosbymichael! 😄


As an aside, I noticed moby/moby#27955 might require a way to extract the version from the resulting binary (which won't work when Tini consumes no args at all). I'm happy to make some tweaks to accommodate this (perhaps by consuming --version if it's the only argument; or via an ENV var); just let me know if that's indeed needed!

@crosbymichael
Copy link
Contributor Author

@krallin ahh yes, i'm sure @mlaventure would greatly appreciate that

@mlaventure
Copy link

yes, I would very much like to have --version working so we can ascertain what version is actually used, thanks! :)

krallin added a commit that referenced this issue Nov 3, 2016
krallin added a commit that referenced this issue Nov 3, 2016
@krallin
Copy link
Owner

krallin commented Nov 3, 2016

Sure; how's this @mlaventure @crosbymichael: #58?

@glensc
Copy link

glensc commented Nov 15, 2016

if the sole reason for -DMINIMAL=ON was this PR, why not actually implement -- argument? this way i can package in distro version of tini which docker can just use instead of having custom builds of tini.

/sbin/tini --tiny-arg -- command --command-args

it's pretty standard to stop command line args parsing on --, maybe it's even in C library

glensc added a commit to pld-linux/docker that referenced this issue Nov 15, 2016
moby/moby#26061
moby/moby#28037

own copy (instead of distro package)
because it requires tini with no args build
krallin/tini#55
@crosbymichael
Copy link
Contributor Author

@glensc it is implemented but we want to inject an init in every container (if you have this feature turned on) without everyone having to rewrite their dockerfiles to add -- to their ENTRYPOINT or CMD.

@glensc
Copy link

glensc commented Nov 15, 2016

@crosbymichael i mean dockerd mounts /dev/init and executes /dev/init original-entrypoint requiring custom build of tini.

but i do not see why can't dockerd execute:

/dev/init -- original-entrypoint

as for shell in CMD "echo 123" it would execute:

/dev/init -- sh -c 'echo "123"'

i.e in the actual code:

s.Process.Args = append([]string{"/dev/init", "--", c.Path}, c.Args...)

@glensc
Copy link

glensc commented Nov 15, 2016

the patch just works, should i send PR to docker/docker?

host# docker run --init --rm -it glen/pld bash -l
bash-4.4# ls -l /proc/1/exe
lrwxrwxrwx 1 root root 0 Nov 15 03:33 /proc/1/exe -> /dev/init

bash-4.4# /dev/init -- sh -c 'echo "kala" "maja"; cat /proc/$$/cmdline|tr "\0" "\n">cmdline'
[WARN  tini (39)] Tini is not running as PID 1 and isn't registered as a child subreaper.
Zombie processes will not be re-parented to Tini, so zombie reaping won't work.
To fix the problem, use the -s option or set the environment variable TINI_SUBREAPER to register Tini as a child subreaper, or run Tini as PID 1.
kala maja
bash-4.4# cat cmdline
sh
-c
echo "kala" "maja"; cat /proc/$$/cmdline|tr "\0" "\n">cmdline
bash-4.4#

--- docker-1.13.0-rc1/daemon/oci_linux.go~      2016-11-11 12:27:33.000000000 +0200
+++ docker-1.13.0-rc1/daemon/oci_linux.go       2016-11-15 02:42:25.660635864 +0200
@@ -593,7 +593,7 @@
        if c.HostConfig.PidMode.IsPrivate() {
                if (c.HostConfig.Init != nil && *c.HostConfig.Init) ||
                        (c.HostConfig.Init == nil && daemon.configStore.Init) {
-                       s.Process.Args = append([]string{"/dev/init", c.Path}, c.Args...)
+                       s.Process.Args = append([]string{"/dev/init", "--", c.Path}, c.Args...)
                        var path string
                        if daemon.configStore.InitPath == "" && c.HostConfig.InitPath == "" {
                                path, err = exec.LookPath(DefaultInitBinary)

glensc added a commit to glensc/moby that referenced this issue Nov 15, 2016
vieux pushed a commit to vieux/docker that referenced this issue Jan 25, 2017
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants