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

Use Docker ENTRYPOINT #4776

Closed
SuperQ opened this Issue Oct 23, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@SuperQ
Copy link
Member

SuperQ commented Oct 23, 2018

Proposal

Use Docker ENTRYPOINT0 rather than CMD for setting the default command line flag values.

When CMD is used, any additional flags cause the default flags to be dropped.

This makes starting up Prometheus with a simple additional flag like --storage.tsdb.retention=15d to need the full list of flags added.

Environment

Docker

@DrakeW

This comment has been minimized.

Copy link
Contributor

DrakeW commented Oct 27, 2018

@simonpasquier I would like to help with this issue :)

@simonpasquier

This comment has been minimized.

Copy link
Member

simonpasquier commented Oct 29, 2018

@DrakeW sure, feel free to submit a PR. IIUC @SuperQ's suggestion is to "merge" the CMD flags with ENTRYPOINT command.

prometheus/Dockerfile

Lines 18 to 22 in fe0f0da

ENTRYPOINT [ "/bin/prometheus" ]
CMD [ "--config.file=/etc/prometheus/prometheus.yml", \
"--storage.tsdb.path=/prometheus", \
"--web.console.libraries=/usr/share/prometheus/console_libraries", \
"--web.console.templates=/usr/share/prometheus/consoles" ]

@DrakeW

This comment has been minimized.

Copy link
Contributor

DrakeW commented Oct 31, 2018

@simonpasquier PR submitted, let me know if there's anything else that you would like me to add

@AndorCS

This comment has been minimized.

Copy link

AndorCS commented Nov 22, 2018

Guys, this change brokes deployment which was works for years:

Error parsing commandline arguments: flag 'config.file' cannot be repeated

and

Error parsing commandline arguments: flag 'storage.tsdb.path' cannot be repeated

I have this in my deployment manifest:

          args: # copy-pasted from official Dockerfile and adjusted to current flags
            - '--config.file=/etc/prometheus-config/prometheus.yml'
            - '--storage.tsdb.path=/data'
            - '--storage.tsdb.retention=21d'
            - '--web.console.libraries=/etc/prometheus/console_libraries'
            - '--web.console.templates=/etc/prometheus/consoles'
            - '--web.external-url=http://{prometheus.hostname}'
            - '--web.enable-lifecycle'
@SuperQ

This comment has been minimized.

Copy link
Member Author

SuperQ commented Nov 23, 2018

@AndorCS You will have to update your configuration.

@AndorCS

This comment has been minimized.

Copy link

AndorCS commented Nov 23, 2018

@SuperQ sure I can do it, but I really don't see any real reason for breaking what works for years.

@SuperQ

This comment has been minimized.

Copy link
Member Author

SuperQ commented Nov 23, 2018

This was done to make deployment easier for everyone. You don't have to copy-and-paste all flags to add one additional flag change.

@AndorCS

This comment has been minimized.

Copy link

AndorCS commented Nov 23, 2018

But I want to change the path to the config file, and now I should change the whole entrypoint because of that.

@SuperQ

This comment has been minimized.

Copy link
Member Author

SuperQ commented Nov 23, 2018

You change the path of the config file via -v/--volume. The path inside the container doesn't need to be changed.

@AndorCS

This comment has been minimized.

Copy link

AndorCS commented Nov 23, 2018

In this case, it's better for me to override entrypoint in case you will break something else in the future releases.

I'm not against this change, but I don't like you kinda breaking what works for a long time even it was not the best way to run the application. Especially not for major version change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.