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

Remove VOLUME instructions #118

Closed
jjlin opened this issue Sep 19, 2020 · 15 comments · Fixed by #220
Closed

Remove VOLUME instructions #118

jjlin opened this issue Sep 19, 2020 · 15 comments · Fixed by #220

Comments

@jjlin
Copy link

jjlin commented Sep 19, 2020

The current Dockerfiles specify

VOLUME /config
VOLUME /data

The problem with this is it ends up creating anonymous volumes that aren't cleaned up when the Caddy container exits, even when the container is deleted. You can see this by running docker run caddy true followed by docker volume ls; repeat this a few times and note that new volumes keep building up.

From #104 (comment), the stated rationale for having these VOLUME instructions is "to prevent [config and data] files from being written to the container, and hence lost during a container restart". However, even without VOLUME, these files actually aren't lost during a container restart; they're just stored in the container's writable layer, which will exist as long as the container isn't actually deleted. Given that these anonymous volumes wouldn't be reused when creating a new Caddy container, this doesn't seem to have much advantage over just using the writable layer for storage.

The docs already suggest creating named volumes caddy_data and caddy_config anyway, though this might be simpler as just a single caddy volume mounted at /caddy (or /srv/caddy), with XDG_CONFIG_HOME=/caddy/config and XDG_DATA_HOME=/caddy/data.

@hairyhenderson
Copy link
Contributor

Hi @jjlin, thanks for logging this issue.

Given that these anonymous volumes wouldn't be reused when creating a new Caddy container, this doesn't seem to have much advantage over just using the writable layer for storage.

This is a good point that I hadn't considered. I think I had conflated things when I made that comment originally 😉

Also, good point about a single volume - that would be simpler. However changing the values now would break existing users, and I'm not too excited about that. In many deployments only the data directory really needs to be persisted. As far as I understand, while it's useful to persist the config directory, it's less critical to TLS operations.

I think how I'd like to proceed is to add some logic that prints a warning message if XDG_CONFIG_HOME/XDG_DATA_HOME aren't present on mounted volumes (i.e. use the output of df or /proc/mounts to find a matching entry), and then remove the VOLUME instructions. Does that make sense?

@jjlin
Copy link
Author

jjlin commented Sep 23, 2020

Adding the XDG_CONFIG_HOME/XDG_DATA_HOME check seems a little overkill, but I don't feel that strongly about it. However, if you do add that, I'd suggest only checking for XDG_DATA_HOME, since there are probably lots of users who don't care to mount anything in for config, and then the warning message is just an annoyance.

But as far as I can see, just removing the VOLUME instructions basically maintains the status quo (except without dangling volumes left over), which is fine by me.

@OCram85
Copy link

OCram85 commented Feb 9, 2022

I think removing the volume instructions would really help many users in many use cases.
Alternatively - Whats about publishing caddy without the volumes in an additional tag?

If you just wan't to use caddy to serve static files or in a cluster scenario, you don't want to define multiple empty volumes.
Unfortunately you can't mount /dev/null:/caddy ^^

Here is a sample config file for the described caddy use case:

@hairyhenderson
Copy link
Contributor

Hi @OCram85 - I'm open to reviewing a PR for this - see my comment in #118 (comment) - I still think a warning message of some sort may be useful.

@OCram85
Copy link

OCram85 commented Feb 17, 2022

So maybe an additional image variant would help without breaking anything for the existing users / use cases.
I didn't had much time getting familiar with the code base (build pipeline) in this repo, so a PR could take some time 😿

@polarathene
Copy link

Is it documented anywhere that users should expect the image to implicitly persist /data or /config when no volume is provided? (and that they can rely on that, such that it's supported officially?)

  • The user is advised to persist to external volumes explicitly on DockerHub, especially for /data (A Dockerfile also can't extend the image to provide custom /config, while providing an alternate location doesn't help prevent a redundant anonymous volume from being potentially created repeatedly over time).

Dropping VOLUME directives seems appropriate / beneficial?

  • No problem for users that explicitly mount or rely on unmodified defaults, only when changes were made since the container start/creation; where data is persisted to undocumented anonymous volumes (instead of just using the containers CoW layer).

Let me know if I misunderstood something 😅

This can be ignored - Earlier response that was revised above.

However changing the values now would break existing users, and I'm not too excited about that.

How exactly would it break anything for existing users by removing the anonymous volumes? These are created new each time the container is recreated / instanced, and as mentioned, the containers own writable layer will persist data between start/stop operations on the container. (EDIT: Misunderstood, your response was regarding a breaking change to use a single volume instead of multiple)

This was a long-standing issue with nginx-proxy image that I resolved with a PR and discussion late last year, to avoid the anonymous container buildup or requirement to add a dummy volume to workaround that. In their case the original author IIRC misunderstood why they were adding VOLUME directive.

The DockerHub Caddy instructions mention the /data mount-point for the user to explicitly persist a volume for, while the image implicitly creates an anonymous volume otherwise (not documented?). Doesn't sound like it would break any expectations by dropping the VOLUME and just using mkdir / WORKDIR or checking if the directory exists and if it doesn't exiting with an error message about that? (it is nice to be able to run the image without providing volumes, but VOLUME directives aren't required for that functionality either..)


Notes on VOLUME

Below are some notes (Verbose Notes section) from researching the topic for pro/cons of using VOLUME. TLDR covers keypoints.

TL;DR

VOLUME doesn't seem to serve any real benefit for this image

If users want data to persist, they should explicitly specify a named or bind-mounted volume rather than rely on implicit behaviour (or mounting over the VOLUME anyway..).

Allowing those that do not provide explicit volumes to avoid unexpected surprises with:

  • extending this image via their own Dockerfile.
  • anonymous volume pollution buildup on the host (user is not made aware that this can happen).

Multi-stage build makes sense as a workaround

Just have a final build layer that adds VOLUME directives if you are really concerned about breakage (existing users that update the image without checking release notes, perhaps relying on a :latest tag?).

Users could then clone a Dockerfile and build an image skipping that last stage (not as straight-forward for this project however..?):

  • no forking/maintenance woes
  • publishing extra tag variants is not required (although worse UX vs deprecating VOLUME and removal in future release).

Verbose notes (for reference, no expectation to read any of this)

Click to expand

Official Docker docs on Dockerfile VOLUME directive:

Changing the volume from within the Dockerfile: If any build steps change the data within the volume after it has been declared, those changes will be discarded.

This is problematic for any Dockerfile that extends an image to bake in files for a given VOLUME path, as the directive will discard any contents during build before/after that declaration from the resulting image. This includes metadata changes such as ownership/permissions of the directory (example of a project having problems with an upstream base image having different permissions set for a VOLUME directory).

The alternative is to maintain a fork of the Dockerfile, often by multiple users instead of an official upstream variant, which isn't great for either party involved.

The behaviour can be surprising and hard to troubleshoot as a cause for those unaware of this, as noted by a user here where several hours was lost trying to identify the cause. Another example of users citing notable time lost to figure out why data was not persisting.

The prior comment on that thread expresses that the Dockerfile should be focused on build time, while the CLI or configs like docker-compose.yml are focused on what is relevant for runtime use such as ports and volumes for persistence. Declaring volumes in the Dockerfile meant for runtime seems ill-advised.

Additionally, if an image is not straight forward to run without looking through the documentation for anything else that must be configured, is the VOLUME directive really providing any benefit vs being a documented volume the user should configure (or copy/paste + modify with any existing CLI command or docker-compose config) to run successfully? (this comment from the same thread brings up this concern)


There's numerous discussions about this with popular Dockerfile images using a VOLUME directive without decent justification.

One is mysql where the best defense for it is docker-compose implicit behaviour to retain the volume, even when forcing recreation of the container (which imo is a little confusing/unexpected to not reset an internal anonymous volume so that you get a container in a clean state like the first time).

A reproducible example of that behaviour is provided in this comment. Although it's probably more common to use docker-compose down with docker-compose up -d, where that behaviour differs and the anonymous volume lingers/accumulates as that sequence is repeated over time, similar to any docker run usage without --rm, which is mentioned in this comment.

That then has user rebuttals arguing that any data persistence should be clearly documented for the image and it being reasonable to expect users that care about data persistence to be explicit about declaring volumes for such rather than relying on hidden implicit behaviour (which may vary across container runtimes?).

At the end of that same referenced issue, a comment mentions an official Oracle image dropping the VOLUME directive.

The opening comment of that linked discussion thread also details the following Pro/cons to using VOLUME:

  • (+) Users that did not specify host mount on startup have a chance to recover their data

  • (-) Anonymous volume is terribly hard to search for when container is gone and you have literally hundreds of them

  • (-) Volumes consume free space at dramatic speeds. Because htere is no GC. Because they were made to not to be garbage collected, and that means they should not be created automagically, because, in turn, that means they would require garbage collection.

  • (-) Users that persist their data will certainly do a regular host mount that renders volume useless.

  • (-) Users that do tons of CI builds a day and thought they can finally forget about data bloat when using containers, they, well, are highly annoyed when they discover the consequences.

  • (-) You can add a volume later, but you literally can't cancel declared volume. And if you mount it to your host - well, if that's remote host, you have no chance at cleaning volume at the end of the build.


A separate issue for the official WordPress image also questions the value of VOLUME:

The only thing the VOLUME directive does is: a) make it clear this is where application data is stored. b) explicitly decouple the state of the specified directory.

In case of b, if you want to persist, you can mount a volume regardless. If you don't mount a volume, you'll end up with an unnamed volume (exactly my problem), for which I can not come up with a use case.

One of the maintainers of official images responds to explain use of VOLUME as:

The volume is the only reliable way to let users know where the mutable state is stored.

WordPress was not designed for containers and, as such, considers all of its installation mutable state (and applies security updates automatically). So we designed the WordPress image to reflect that; the initial start will fill the volume (whether a bind-mount or named/unnamed docker volume) with the install and WordPress would then take care of updates. You can even update the container around WordPress to get newer PHP without it effecting the WordPress install.

In the early days of docker, volumes did not exist without a contianer and so tools like fig/docker-compose used tricks to keep volumes for new containers and still does so for named or unnamed volumes.


Grafana also removed VOLUME from their Dockerfile, and it was also previously an issue for CoreOS developers using that grafana issue in the past.


  • Legitimate reasons for removing VOLUME are brought up on the official mongo db image regarding seeding a DB as a base image. The maintainer advises a more verbose workaround that still results in creating redundant anonymous volumes without a dummy bind/data volume workaround.
  • One workaround to this problem when using multiple images that have this bad practice is to schedule cleanup of unattached volumes with docker system prune --all --volumes --force, that may not always be desirable though?
  • On the official Postgres image, a related issue has two comments, one about local CI runs for each commit/PR amassing anonymous volumes on host, and the comment after that about incompatibility with Kubernetes (albeit this is from 2018, I do not use k8s myself, so cannot verify if that's still a valid concern).
  • In 2019 behaviour changes when using BuildKit, I haven't checked if this has become the default builder yet but I recall that being planned in the near future if not already done. Some old servers may run older versions of Docker though, thus behaviour could be mixed. Related issue describing difference between builders.
  • Another discussion noting that VOLUME had worse UX/DX for data management than being more explicit at runtime/config with bind/named volumes instead of anonymous volumes.
  • This StackOverflow answer notes that historically prior to Docker Engine 1.9 (2015Q4 release), anonymous volumes were used prior to named volumes being introduced, although bind mount volumes still seemed available as an option.. The answer covers some of the issues mentioned here, considering VOLUME only valid for special cases.

Official Docker docs best practice advice for VOLUME:

The VOLUME instruction should be used to expose any database storage area, configuration storage, or files/folders created by your docker container. You are strongly encouraged to use VOLUME for any mutable and/or user-serviceable parts of your image.

The maintainers for official Docker images also cite a 2016 comment for why and when to use VOLUME. That remains the case at least in 2020 during review of making Caddy an official Docker image.

One of the other official Docker image maintainers in 2018 chimed in on a VOLUME issue answering that the anonymous volume was preferable than the containers CoW layer via OverlayFS as the first modification to a file requires making a copy of the immutable file from the image. The quotes from the referenced docs in that comment remain accurate in current 2022 docs.

This being notably important for write-heavy workloads on large files such as databases.... however this is moot if you would mount a volume to persist data explicitly anyway.. as is common practice for such I/O usage.

@hairyhenderson
Copy link
Contributor

So maybe an additional image variant would help without breaking anything for the existing users / use cases.

I'd rather not have an additional variant just to remove the instruction.

Again, I'm open to this, and am willing to review a PR for this (see above)!

@OCram85
Copy link

OCram85 commented Feb 23, 2022

So then I create PR which just removes the VOLUME instructions in the Dockerfiles.
Additionally, I would add a section in the docs about the volumes. Can you point me the repo from which the docs are generated?

@hairyhenderson
Copy link
Contributor

Can you point me the repo from which the docs are generated?

@OCram85 that's listed in the README. But let's focus on the PR itself first.

@plashenkov
Copy link

Yup. Volumes are really superfluous. You have to declare them explicitly all the time to avoid getting "garbage" names in the volume list.

And every derivative image (which inherits from caddy) will produce such volumes, you simply have no method to get rid of them. So, if I want to wrap my service into caddy and provide it to users, they have to do the same every time: declare volumes or get "garbage" names.

So I still have to use nginx in such cases, but I would like to use one tool everywhere — caddy.

@polarathene
Copy link

I'm open to this, and am willing to review a PR for this (see above)!

@hairyhenderson I'm not sure what the PR status was in Feb, it seems to need some rebasing now (@OCram85 ?); but any chance of providing that review sometime? :)

@OCram85
Copy link

OCram85 commented Apr 7, 2022

@polarathene: Just rebased the PR 🧨 🔥 🧯 🚒 😄

@OCram85
Copy link

OCram85 commented Apr 7, 2022

@polarathene: I can't speak for others, but my personal opinion to this is clear:
I think the caddy community doesn't need or want implicitly created VOLUMES. If I interpret your explanations correctly, implicit volume definitions shouldn't be used either?

@polarathene
Copy link

If I interpret your explanations correctly, implicit volume definitions shouldn't be used either?

Correct.

My earlier comment covered that pretty well, even with only the terse version I thought.. but I'll try again.

  • I didn't notice any documentation or official support for VOLUME, but instead explicit volume mounts were encouraged.
  • VOLUME causes more problems than benefits, and the small (if any) amount of users that want it for this image should extend the image to add it.
  • Don't pollute user systems with implicit anonymous volumes.
  • Grafana and Oracle acknowledged VOLUME had become bad practice and dropped it on their images.
  • It rarely makes sense for an official image to use by default when an explicit volume mount accomplishes the same more clearly and consistently.

I am of the opinion VOLUME is bad practice for the majority of images.


Original verbose/detailed attempt, before making terse version above
  • I did not get a response from @hairyhenderson regarding my first point about official documentation (which only advises explicit volume mounts to persist data).
  • Dropping VOLUME avoids a bunch of surprises/problems, especially when extending the image.
  • If there is a legitimate reason to retain VOLUME (what % of users are actually relying on that and why?), use a multi-stage build so the official image can be easily built or extended without the VOLUME. I don't believe there is users depending on it with this image, and if there was it seems illogical to pollute user systems for such a minority.. Let those users extend the image to add VOLUME back if they have a niche use-case requiring it?

More reasons

  • Grafana and Oracle have both dropped VOLUME from their images. Other notable organizations have issues about it, but the discussion by maintainers in support of it never really seems to get anywhere most of the time to justify it, as opposed to valid reasons to drop it.
  • VOLUME directive is effectively legacy imo. There is little benefit from being implicit, not reliable across container environments (Issues with k8s at runtime, and differences with building images via BuildKit, which will eventually become Dockers new default AFAIK).
  • Main benefit of VOLUME is to avoid the containers CoW (OverlayFS) layer creating a new layer off the containers image base data to write to. For large data and heavy I/O, a data volume defers to storage driver for better performance/efficiency, which may make sense for databases, but not so much for a couple of files that are not written to at high frequency.
  • An explicit bind mount accomplishes the same and is better, along with more consistent across environments. The user needs to do a bit more work upfront perhaps by explicitly configuring a volume mount, but that's a non-issue when other configuration is required. Good documentation / examples for configuring the container should make any need for VOLUME redundant and simplify technical support (by not running into issues that VOLUME can be responsible for).
  • Without any real reason for VOLUME, it just pollutes user systems (unless they configure an explicit volume anyway), or they regularly perform cleanup on dangling volumes.
  • Likewise, VOLUME can cause friction for extending the docker image.

@hairyhenderson
Copy link
Contributor

Sorry for the radio silence on this one, folks. My goal is to get #220 merged real soon and released with the 2.5.0 image ~today-ish

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants