makefiles/docker.inc.mk: always pull to keep image up-to-date

[mguetschow]

Contribution description

On current master, BUILD_IN_DOCKER might fail just because the local docker image is outdated. Since the image is automatically pulled upon the first build with docker, users (including me) probably would expect RIOT to keep the image updated automatically.

This PR changes the docker run --pull argument from the default missing to always.

Testing procedure

• Have an outdated version of riot/riotbuild locally (in my case it was ~5 months old).
• BUILD_IN_DOCKER=1 make -C examples/hello-world all
• see how the docker image gets updated automatically

---

• Have an up-to-date version of riot/riotbuild locally.
• BUILD_IN_DOCKER=1 make -C examples/hello-world all
• see that the image is only updated if it was outdated:

  latest: Pulling from riot/riotbuild
  Digest: sha256:66d026e6f33763b8e016354853b15f4a71ffdd62a3c92fbd0f5e9eb8166ef87f
  Status: Image is up to date for riot/riotbuild:latest 
  

Issues/PRs references

Issue was discussed on matrix, where @maribu proposed to automatically pull on each build.

[maribu]

Thx for addressing this. Just a week ago the issue with an outdated container popped up in an issue, so this improves quality of life a lot.

[riot-ci]

Murdock results

✔️ PASSED

85da593 makefiles/docker.inc.mk: always pull to keep image up-to-date

Success	Failures	Total	Runtime
10009	0	10009	06m:47s

Artifacts

[maribu]

Ah, two corner cases: What happens when sitting in a train in Brandenburg? (For the non-Germans: There is no Internet connectivity in Brandenburg.)

What happens when a local docker image is passed to be used instead of the default one?

[mguetschow]

Brandenburg

docker: Error response from daemon: Get "https://registry-1.docker.io/v2/": dial tcp: lookup registry-1.docker.io on [::1]:53: read udp [::1]:42183->[::1]:53: read: connection refused.
See 'docker run --help'.
make: *** [/home/mikolai/TUD/Code/taler-riot/RIOT/makefiles/docker.inc.mk:352: ..in-docker-container] Error 125
make: Leaving directory '/home/mikolai/TUD/Code/taler-riot/RIOT/examples/hello-world'

DOCKER_IMAGE = local:latest

docker: Error response from daemon: pull access denied for local, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.
make: *** [/home/mikolai/TUD/Code/taler-riot/RIOT/makefiles/docker.inc.mk:352: ..in-docker-container] Error 125
make: Leaving directory '/home/mikolai/TUD/Code/taler-riot/RIOT/examples/hello-world'

As you can see, very good points! Should we have an environment/Makefile variable to explicitly disable pulling?

[maribu]

Suggested change

	DOCKER_RUN_FLAGS ?= --pull=always --rm --tty $(DOCKER_USER_OPT)
	DOCKER_RUN_FLAGS ?= --pull=newer --rm --tty $(DOCKER_USER_OPT)

With podman this does the trick. It does take a bit of time to timeout the check in the Brandenburg case.

But maybe the best would be to just allow users to overwrite this via environment variable, e.g. by setting RIOT_DOCKER_PULL_POLICY or whatever.

[mguetschow]

Unfortunately this flag is not available for docker: https://docs.docker.com/reference/cli/docker/container/run/#pull

I've tried adding it and it will just be silently ignored (i.e., the docker image is not updated).

[mguetschow]

So we could have different defaults for docker (--pull=always) and podman (--pull=newer) and have DOCKER_SKIP_PULL to disable pulling altogether?

[kaspar030]

Doesn't this touch the network every time, taking time and possibly failing when in trains? I would definitely want to disable this.

[kaspar030]

I think it would make more sense to fix thix first in riotdocker by tagging & building semver releases, then pin a semver compatible release in the RIOT branch. and then, only pull if the current release is not semver-compatible. or, just bail out in that case, as it wouldn't happen too often.

[maribu]

I think it would make more sense to fix thix first in riotdocker by tagging & building semver releases

Agreed. But who is going to do that? This would fix problems people are having right now. (Two people reported issues caused by an outdated docker container just within the last 7 days, likely this is frequent.)

And with the --pull=newer as default it would only try to update and just run the local one when no update can be retrieved, because sitting in a train in Brandenburg.

Let's go with the --pull=newer and a variable to control the pull behavior for now. Once someone does semantic versioning for the docker container, we could switch to the better solution.

[maribu]

OK, since --pull=newer is not portable to docker, I guess auto-updates will do more harm then good.

How about pinning the docker image instead to a list of known good versions: #20472

this is probably not the solution we want -> make this a little less green

[Teufelchen1]

Can this be a draft?

[mguetschow]

Obsolete since #20472 has been merged.