Dockerfile: Remove VOLUME instruction #20
Conversation
This instruction is usually useless e.g. when using novolume-plugin. In cloud environments volumes must be mounted and there's no point in keeping Prometheus data temporary volume dir only during life-time of a container. Another downside of using VOLUME instruction is it hard to undone it in child images and one would need to re-write the Dockerfile.
openshift-ci-robot
added
the
size/S
|
Hi @aramalipoor. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
needs-ok-to-test
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
| @@ -19,10 +19,10 @@ COPY --from=builder ${FROM_DIRECTORY}/consoles/ /usr | |||
|
|
|||
| RUN ln -s /usr/share/prometheus/console_libraries /usr/share/prometheus/consoles/ /etc/prometheus/ | |||
| RUN mkdir -p /prometheus && \ | |||
| chown -R nobody:nobody etc/prometheus /prometheus | |||
| chgrp -R 0 /etc/prometheus /prometheus && \ | |||
| chmod -R g=u /etc/prometheus /prometheus | |||
|
|
|||
There was a problem hiding this comment.
What's the reason for changing the ownership here? I'm not sure who is the default owner of the /prometheus directory, but wouldn't this change cause prometheus to no longer be able to write to the /prometheus dir?
There was a problem hiding this comment.
These two commands are recommended by official OKD docs to be set on directories you'd need to write in runtime. https://docs.okd.io/3.10/creating_images/guidelines.html#use-uid
Probably since VOLUME was used before permissions where working OK but these commands explicitly will set root group and allows it to write to this directory, even if user ID is randomly set.
|
|
||
| USER nobody | ||
| EXPOSE 9090 | ||
| VOLUME [ "/prometheus" ] |
There was a problem hiding this comment.
I'd like to keep the configuration in sync with upstream prometheus as much as possible, so I'll probably wait for prometheus#5050 to be merged first.
In general it seems that this case is a bit different than kubernetes/kube-state-metrics#471, since the /prometheus directory contains data that should normally be persisted between container restarts.
There was a problem hiding this comment.
What this change will help is to avoid creating temporary volumes (i.e. within /var/lib/docker) while the user deliberately does not want to persist data and also helps with deploying this image on cloud providers who have restricted using VOLUME instruction / temporary volumes on host disk.
|
/lgtm |
Following up on prometheus#5050.