Use ENTRYPOINT instead of CMD in Dockerfile

[jkohen]

This makes it easier to add command-line flags using 'args' in configs, such as Kubernetes YAML files.

[bmoyles0117]

LGTM

[igorpeshansky]

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/bash and 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 CMD line (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 -v and --configfile (the latter also happens to be the first positional argument).

[jkohen]

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.

[igorpeshansky]

Yes, we could override the entrypoint as well.
The blog you pointed to says that

the use of ENTRYPOINT sends a strong message that this container is only intended to run this one command.

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 -v or a filename, and the latter has to be mounted into the container anyway.

[jkohen]

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.

[jkohen]

Ping?

[jkohen]

I don't see the benefit of making that change. I'll drop this PR.

[jkohen]

Sorry for bothering you.

[bmoyles0117]

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 ENTRYPOINT as opposed to the CMD as @jkohen has requested will be the right course of action to take in the future, however, I would not recommend setting the ENTRYPOINT to be the metadatad binary directly.

The primary concern we have by setting metadatad as the ENTRYPOINT is that it gives the user full control over all flags that metadatad supports, 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.sh that abstracts around the binary that is intended to be ran, metadatad in our case. When we create the docker-entrypoint.sh script, flags such as -v should be allowed to pass through without incident. Extraneous flags such as ones that specify configuration files should be processed by the docker-entrypoint.sh and 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.sh we will have a solution that is the best of both worlds. It will provide users like @jkohen with a mechanism of easily passing in -v to obtain verbose output, but it will also prevent users from shooting themselves in the foot by preventing other flags such as -c which will override the expected location of the configuration files.

[jkohen]

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.

[bmoyles0117]

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 the docker-entrypoint.sh as 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 of docker-entrypoint.sh is 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

[igorpeshansky]

@bmoyles0117 Do my concerns make sense to you?

[igorpeshansky]

My concern is that -v is the only option that can be usefully specified in that manner. If you were to pass in a config file name, for example, it would break without additional docker options. This container is not really designed to be run as an executable.
Would -e VERBOSE=true address your needs? I'd be a lot more comfortable with adding an environment variable control than allowing arbitrary flags to be passed in.