This repository was archived by the owner on Aug 19, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 11
Use ENTRYPOINT instead of CMD in Dockerfile #39
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a good idea. This will prevent us from running this container in debug mode (launch a shell with this image via
docker run IMAGE:TAG /bin/bashand executing commands in that environment).If you really want to extend the command via the deployment config, we could instead add an environment expansion into the
CMDline (e.g.,CMD /opt/stackdriver/metadata/sbin/metadatad $ARGS) or via a file mounted into the container (e.g.,CMD /opt/stackdriver/metadata/sbin/metadatad $(cat /var/run/agent-args)).Note that the only command-line arguments accepted by the metadata agent today are
-vand--configfile(the latter also happens to be the first positional argument).Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you do
docker run IMAGE:TAG --entrypoint /bin/bash? It's a rare case, and the current version forces every user to figure out what the default CMD is before they can override flags.Details at https://www.ctl.io/developers/blog/post/dockerfile-entrypoint-vs-cmd/
I don't like the suggestion to introduce an extra file or environment variable for flags, it's unnatural and it makes the API even more complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could override the entrypoint as well.
The blog you pointed to says that
I'm not certain that this is the contract we want to have for all agents. However, we could try it and see.
That said, though, what arguments would you want to provide? As I said above, the agent executable only accepts
-vor a filename, and the latter has to be mounted into the container anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to use -v to debug problems like the one I just reported. Any additional step (such as figuring out the correct base command) makes it less likely someone will do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the benefit of making that change. I'll drop this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for bothering you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chiming in here after talking offline with @igorpeshansky about this issue. I'm writing this strictly to have it documented and to serve as a talking point for the future.
I believe that leveraging the
ENTRYPOINTas opposed to theCMDas @jkohen has requested will be the right course of action to take in the future, however, I would not recommend setting theENTRYPOINTto be themetadatadbinary directly.The primary concern we have by setting
metadatadas theENTRYPOINTis that it gives the user full control over all flags thatmetadatadsupports, even though some of those flags will break Docker idioms such as passing in the name of a configuration file, as passing the flag does not simultaneously mount the file into the Docker container.It is considered to be a viable best practice to add a helper script that is maintained by us called
docker-entrypoint.shthat abstracts around the binary that is intended to be ran,metadatadin our case. When we create thedocker-entrypoint.shscript, flags such as-vshould be allowed to pass through without incident. Extraneous flags such as ones that specify configuration files should be processed by thedocker-entrypoint.shand possibly be ignored / result in a warning providing a clear error message to the user as to what expectations have been violated.Once we are ready to support a
docker-entrypoint.shwe will have a solution that is the best of both worlds. It will provide users like @jkohen with a mechanism of easily passing in-vto obtain verbose output, but it will also prevent users from shooting themselves in the foot by preventing other flags such as-cwhich will override the expected location of the configuration files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the background, Bryan. I still don't understand why we'd want to prevent users from passing -c. The mention of docker-entrypoint.sh I found in the official docs is for creating images that have multiple responsibilities, such as starting Progres, but also passing commands to Postgres. Maybe you have a similar plan for the Metadata agent, in which case your concern makes sense; otherwise it sounds like a theoretical problem getting in the way of practicality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The end goal is to try to reduce the amount of user angst and increase the overall UX. I think this PR has the right goal in mind, I've created another PR which essentially duplicates what you've done here but through a
docker-entrypoint.sh, this way we afford ourselves the ability to make changes using thedocker-entrypoint.shas the contract in the event that we did wrap around additional functionality in the future. I hope that this can meet somewhere in the middle. A nice byproduct ofdocker-entrypoint.shis that it serves as "some" documentation around the functionality we support within the metadata agent. I'd like to be clear with @igorpeshansky that my change "functionally" is no different that what you've suggested here, it simply secures us by making a public statement that our entrypoint is a wrapper, not the binary itself. #40