Remove Docker VOLUME instruction from Dockerfile for more flexibility

[alexpovel]

This is branched off master, hopefully that's right (didn't see anything else in the README).

I noticed this running v0.104.3 via Docker. The scenario this happened in is a bit contrived, but I think this is still a change that's useful for all sorts of use cases.

Expected behaviour

The expected behaviour I'm trying to achieve with this PR is:

Inheriting FROM adguard/adguardhome and then baking files into /opt/adguardhome/conf in a derived Dockerfile should work.

Actual behaviour

However, it doesn't work that way, because the base image declares volumes at two points, one of which is /opt/adguardhome/conf:

AdGuardHome/scripts/make/Dockerfile

Line 38 in 7fab31b

VOLUME ["/opt/adguardhome/conf", "/opt/adguardhome/work"]

As such, any e.g. COPY instructions will get overriden once the container is run. For example, take this Dockerfile:

FROM adguard/adguardhome

# The base image has a VOLUME at `/opt/adguardhome/conf`

# Allowed but ignored:
RUN echo "Hello World 1!" > /opt/adguardhome/conf/test
# Allowed but also ignored:
# (the fact that it doesn't error out is dangerous and misleading imho)
COPY somefile /opt/adgurdhome/conf/somefile

# This works as expected:
RUN mkdir -p /somewhere/else
RUN echo "Hello World 2!" > /somewhere/else/test && echo "This works fine, as usual."
# It's present at image build time:
RUN cat /somewhere/else/test

# ATTENTION: The following causes
# `cat: can't open '/opt/adguardhome/conf/test': No such file or directory`.
# The file doesn't exist because it was created in what is declared as a VOLUME in the
# base image.
# As a user, this is not transparent and can be hard to debug.
RUN cat /opt/adguardhome/conf/test || echo "No such file found!"

I think the echos and comments sufficiently explain the weird and potentially confusing behaviour this has. This is a good compilation of reasons not to use the VOLUME instruction in Dockerfiles (in fact some people say to remove VOLUME entirely). Note that running COPY into a directory, then declaring it as a VOLUME afterwards works. But this is not available to child images. In the Dockerfile reference, it says:

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.

---

The worst and essentially breaking part here is that you cannot reset any of these properties in child images, see moby/moby/issues/3465 (7 year old topic...). If that were possible, I wouldn't need and have made this PR.

In the primary guide for running AdGuard Home in Docker, you use --volume instructions anyway. This renders the VOLUME instructions void, as far as I can see. To persist the working directory, you wouldn't even need the --volume option, since that is a VOLUME. Docker would create an anonymous volume and data would be persisted. But it's better to be explicit and use --volume imho, which you do -- but then, VOLUME can just be discarded. This would allow for more flexibility for advanced use cases.

Use case

The reason I even stumbled over this is trying a setup with Ansible. A controlling machine copies over the relevant files, like Dockerfile, docker-compose.yml, AdGuardHome.yaml etc. Those then get built and run by Ansible as well, using its docker_compose module. This is very straightforward for basically all stuff out there, except for DNS servers!

As stated in the wiki, we have to stop the container before changing the config files:

Upon the first execution, a file named AdGuardHome.yaml will be created, with default values written in it. You can modify the file while your AdGuard Home service is not running. Otherwise, any changes to the file will be lost because the running program will overwrite them.

I tried without stopping the container first, can confirm it doesn't work. So the order goes, from the Ansible play:

1. Stop service (DNS gone!)
2. Copy over config files
3. Start service up again
4. Wait for DNS to be up (e.g. wait for dig to succeed)
5. Update the image

This is stupid and confusing. I also want to update the image in the process, but in step 3, we have to do a simple docker-compose up, no pulling, since no DNS is up at that point.

However, the much graver issue is that steps 2 and 3 can fail. You will be left naked, without a DNS server running (assuming it's the only one). I also had no DHCP server at that point. Ugly situation.

This wouldn't happen if instead of manipulating the "live" config files that get mounted into the container were baked into the image in the first place. From Ansible, it is very easy and not much of a difference to just build a new image every time, using a Dockerfile similar to:

FROM adguard/adguardhome

COPY adguard.yml /opt/adguardhome/conf/AdGuardHome.yaml

This occurred to me and is a very simple solution to implement. It solves all of the above problems and simplifies the process to:

1. Copy over config files
2. Recreate the service (basically docker-compose up --build)

This updates the service and there is never any dangerous downtime. If the pull fails, the Dockerfile is invalid etc. etc., Docker will simply not recreate the service and leave the current one running. If everything builds and updates fine, it quickly recreates the service. Basically no downtime, and essentially no risk of ending up with no DNS server (except for when the new config files are somehow spectacularly corrupted).

Of course, this doesn't work because we COPY into VOLUME. The above COPY just silently fails. The new YAML config is never put into place, and upon container startup, the hidden, anonymous Docker volume is used again. This can lead to very surprising behaviour.

For reference, I have a named volume for the working directory, and had planned on simply not using a volume for the conf directory, thinking I was smart. But Docker creates and uses one anyway:

$ docker volume ls
DRIVER    VOLUME NAME
local     5cff82c265d979b52701a2a192ad470c15a7a2b975e4290dc0a432eddc5ab8f9
local     adguard_work

Conclusion

All guides seem to use explicit --volume instructions anway. These do not benefit or even use the base image's VOLUME instruction, they override it. Beginners will follow the guide and more advanced users will know to declare volumes, and also refer to the guide to see where to mount to. Even if you had no VOLUME instruction and forgot to use --volume, there isn't much to be lost if you lose your AGH's state.

Advanced use cases that require inheriting from the base image are however hampered, and irretrievably so, since you cannot undo VOLUME instructions.

I therefore see this change as a net gain with very little possibility of effing up.

We really rely on this being done upstream. As an alternative I looked into just taking the current Dockerfile verbatim, removing the VOLUME line and building. But that Dockerfile isn't a multi-stage file that compiles the binary itself, it just copies it in. So the Dockerfile is very heavily coupled to the surrounding CI (good stuff though!) and end users cannot easily take and modify it. Our best bet is to inherit from the built base image.

Maybe a similar point could be made about the EXPOSE instruction, see also OxalisCommunity/oxalis/issues/440. They call it Dockerfile generalization, which is very fitting.

[ainar-g]

@alexpovel, thanks! We'll probably review and merge this shortly.

@ameshkov, I've been thinking. Why do we have the two directories in the first place? Why not just have everything in /opt/AdGuardHome?

[alexpovel]

@ameshkov, I've been thinking. Why do we have the two directories in the first place? Why not just have everything in /opt/AdGuardHome?

As far as that goes, I think it's good style to have a separation of:

• user is interested/provides these files -> conf. These should be visible and accessible
• user doesn't really care/it's an implementation detail that just happens to need persistence through volumes -> work. A query log database needs to be persistent, but can be tucked away, a user never needs to interact with it directly (can do so via CLI/Web)

On top of that, as it stands now, /opt/AdGuardHome would also contain the main binary itself, which is very much part of the image/container and should definitely not be part of a volume. You wouldn't be able to pull a new image and update that way if the old binary is mounted back in. So when going that route, the binary would have to move.

[ainar-g]

Merged in abf8f65. Thanks!

[alexpovel]

Thanks, this now works using v0.105.0, after some tweaking. Despite updating to the working version, Docker kept using the old, anonymous volume, so the old behaviour was still there. I fixed this running docker-compose up with --renew-anon-volumes once. That seems to have unlinked the old, no longer used VOLUME and allowed the new behavior to work.