fix: Dockerfile - VOLUME directive is an anti-pattern#2948
fix: Dockerfile - VOLUME directive is an anti-pattern#2948polarathene wants to merge 2 commits intokanidm:masterfrom
Dockerfile - VOLUME directive is an anti-pattern#2948Conversation
This directive provides no value to an image. It only causes problems.
|
Side-note, the link that follows the mentioned warning is invalid. You should probably update that 👍 |
yaleman
left a comment
yaleman
left a comment
There was a problem hiding this comment.
Since you noted it and while you're there, could you please update the broken link to https://kanidm.github.io/kanidm/master/server_configuration.html 😄
| RUN chmod +x /sbin/kanidmd | ||
|
|
||
| EXPOSE 8443 3636 | ||
| VOLUME /data |
There was a problem hiding this comment.
Deleting it will break the container's default config as the folder doesn't exist, please change it to :
RUN mkdir /data
There was a problem hiding this comment.
could you please update the broken link
Link to source file and line and sure 👍
as the folder doesn't exist
Would it make sense to default to the location instead of / when running the ENTRYPOINT?
EDIT: Oh you only have CMD set (/sbin/kanidmd server -c /data/server.toml), that is still absolute path so a WORKDIR directive would create the location (not sure if that avoids an extra layer or not, I think it might).
Alternatively, I could add && mkdir /data in an earlier RUN?
There was a problem hiding this comment.
The
WORKDIRdoesn't apply to the final container, note theFROM reposon line 65.
Sorry I don't follow? That is the last FROM directive in this file, and given the remainder of the file it looks like the final stage? Are you doing something else to the image?
Separate Dockerfile:
FROM alpine
WORKDIR /datacompose.yaml that extends that image with a custom Dockerfile:
services:
example:
# The `image` setting now represents the tag for the local build configured below:
image: local/workdir-extended:latest
# Local build (no need to try pull `image` remotely):
pull_policy: build
# The original image was extended, the directory from WORKDIR still exists.
# NOTE: If this were `FROM scratch`, obviously you'd need to COPY `/data` over
# but that's no different to how you approach creating `/data` prior.
build:
dockerfile_inline: |
FROM local/workdir:latest
RUN touch hello.txt
CMD pwd && ls# Build the separate image:
$ docker build --tag local/workdir .
# Build and run the variant image that extends it:
$ docker compose run --rm example
/data
hello.txt
# Also verify image metadata still has the workdir set:
$ docker image inspect local/workdir-extended:latest | jq -r '.[].Config.WorkingDir'
/dataSeems to work perfectly fine no?
There was a problem hiding this comment.
could you please update the broken link
Link to source file and line and sure 👍
That is not the line you wanted changed? 🤷♂️
I've used that to identify the intended line and correct the link with the one you've provided, so thanks 👍
There was a problem hiding this comment.
I thought you were talking about the existing workdir command, not adding one.
There was a problem hiding this comment.
the existing workdir command, not adding one.
That's in the builder stage and wouldn't make sense to replace an existing one with a different path.
- If it makes sense to run the container from
/dataI can add aWORKDIRdirective. I assumed/datawas arbitrary for documentation, and not hard-coded/expected anywhere. - If that assumption is correct, you could just use an existing directory for documentation and anyone who wants to use a custom location like
/datacan do so via config + volume?
How would you like to proceed?
WORKDIRorRUN?/dataor an existing location like/tmp?
EDIT: Or sure... ignore responding and do the work again via a separate PR without any appreciation beyond a link ref to this PR to automate closing it. Way to encourage contributions 🙄
2 participants
The
VOLUMEdirective provides no value to an image. It only causes problems.For detailed justification, please see below context or my past writeup on this subject: ory/hydra#3683
Checklist
cargo fmthas been runcargo clippyhas been runcargo testhas been run and passesbook chapter included (if relevant)design document included (if relevant)Context
Reproduction Example
After running the above
compose.yamlwith the config update todomain+origin, the container fails unexpectedly:Specifically the problem is identified at this log line:
Problem triggered in relation to
VOLUMEdirectiveI am aware of the requirements for changing the configuration, but this failure was not expected as I was using
docker compose up --force-recreate.Typically that should be equivalent to
docker run --rm ..., ensuring a fresh container instance is created, discarding any internal state. Nor diddocker compose downhelp./datasince I was only in an evaluation stage and wanting to quickly get the bare minimum up and running.kanidmmay write to/datain the container was expected to have been discarded for a clean slate.VOLUMEdirective. They will initialize by copying any of the existing content at the mount point, unlike a bind mount volume which replaces.docker run --rm ..., each container instance started will create a new anonymous volume. Without the--rm, these will accumulate pointlessly, the user may not be aware of it.Thus I had to remove the actual anonymous volume manually to get a fresh container like the first time I started
kanidm, which would then not attempt to re-use the unwanted prior database in/data.Conclusion
There is no reason to use
VOLUMEinDockerfile. It only causes issues.Your database in
/datawill persist with the internal container state for the lifecycle of that container. If a user wants to persist it externally, then they can do so with an explicit volume (be that anonymous/named/bind/etc).Please avoid this anti-pattern, thanks!