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

Make --init optional when using --docker-run #481

Closed
CMajeri opened this Issue Feb 28, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@CMajeri
Contributor

CMajeri commented Feb 28, 2018

Hi,

I'm running full system containers that run their own init, and I noticed that my init gets switched in favor of docker's --init option.

Could it be possible to make passing that flag optional? Possibly by having the user have to specify --init themselves?
If that is considered a big change for existing users, maybe consider a --no-init parameter that will just remove that flag?

There doesn't seem to be a need for that flag for telepresence itself, since it can seemingly run on older versions of docker that do not have that flag available.

I can participate to the pull request, depending on the option you guys prefer!

Cheers!

@ark3 ark3 added the enhancement label Feb 28, 2018

@ark3

This comment has been minimized.

Contributor

ark3 commented Feb 28, 2018

I would love to drop the automatic --init, but that would most certainly be a breaking change. On the other hand, I am not excited about adding yet another option (--no-init) to the Telepresence command line given how complicated things already are (see #455). What do other folks think?

@CMajeri

This comment has been minimized.

Contributor

CMajeri commented Mar 1, 2018

Does kubernetes even support passing the --init flag to containers? In my very biased opinion, since the --docker-run option is meant more as a dev tool to connect a container to a cluster, I'd expect the container to run about the same as they would in the cluster, just so I could catch bugs more easily.

@CMajeri

This comment has been minimized.

Contributor

CMajeri commented Mar 1, 2018

For now, the workaround I'm using is passing --init=false to the command, which works but depends on docker run --init --init=false parsing the arguments correctly, which it does, at least on my version of docker, but that's not the most reliable thing.

A way I see that doesn't affect current users and lets people turn that feature off would be to look for the --init flag in the user specified arguments. If there's a --init, maybe drop the automatic --init, and just pass the args as is. Such a check wouldn't be too hard to add, and can still be kept somewhat clean, as long as it is clearly documented and we limit the number of such cases. What do you guys think?
This would also prevent stuff like docker run --init --init from happening (which again, depends on docker doing the right thing, although that's not too far fetched for this case).

@ark3

This comment has been minimized.

Contributor

ark3 commented Mar 1, 2018

You are correct: adding --init automatically makes the container behave differently from how it would act in the cluster. Telepresence does this to handle signals better (Ctrl-C, a signal from your IDE, a kill command, etc.), which improves its responsiveness when restarting a session repeatedly. This scenario cannot occur in the Kubernetes cluster. We felt it was okay to make things behave a bit differently in exchange for having a more comfortable developer experience.

Telepresence already examines the arguments being sent to Docker, specifically to detect --publish and possibly other arguments that need to be part of the sshuttle container docker command. Your suggestion to have Telepresence notice --init and act accordingly is a fair way to preserve the current signal handling behavior while providing the flexibility you need.

@CMajeri

This comment has been minimized.

Contributor

CMajeri commented Mar 1, 2018

I just noticed the --publish in the parser now that you mentioned it!

Since I'm the only one who seems to need the --init feature, and it's such a small part to change and test, I can prepare a PR going with this approach.
Or would you rather handle this internally?

@ark3

This comment has been minimized.

Contributor

ark3 commented Mar 1, 2018

A pull request would be great. Thank you!

@CMajeri

This comment has been minimized.

Contributor

CMajeri commented Mar 1, 2018

The implementation itself feels "wonky" in that I can't use argparse for parsing purposes. That's because --init can be specified both with and without arguments, such as --init, --init=false or --init=true. This works with the golang parser, but not argparse. So I had to manually check if something looking like --init is specified, if so, it's split in the same spot as its "optional" argument...

This also makes tests more dragged out, due to the unnecessary distinction between --init and --init=true. I'm open to comments and criticism, I mostly just wanted to have the PR done before going to bed so I could get it to you. Time zones... 👍

@ark3

This comment has been minimized.

Contributor

ark3 commented Mar 1, 2018

Thanks for the PR! But let's discuss the PR code in the discussion there.

@ark3 ark3 closed this in 435630d Mar 5, 2018

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