makefiles/docker.inc.mk: always pull to keep image up-to-date by mguetschow · Pull Request #20470 · RIOT-OS/RIOT

mguetschow · 2024-03-14T14:57:58Z

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 · 2024-03-14T15:07:03Z

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 · 2024-03-14T15:12:57Z

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 · 2024-03-14T15:46:27Z

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 · 2024-03-14T19:35:48Z

makefiles/docker.inc.mk

 DOCKER_USER ?= $$(id -u)
 DOCKER_USER_OPT = $(if $(_docker_is_podman),--userns keep-id,--user $(DOCKER_USER))
-DOCKER_RUN_FLAGS ?= --rm --tty $(DOCKER_USER_OPT)
+DOCKER_RUN_FLAGS ?= --pull=always --rm --tty $(DOCKER_USER_OPT)


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.

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).

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

kaspar030 · 2024-03-15T07:57:12Z

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

kaspar030 · 2024-03-15T07:59:15Z

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 · 2024-03-15T08:13:55Z

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 · 2024-03-15T09:59:44Z

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 · 2024-03-25T11:45:59Z

Can this be a draft?

mguetschow · 2024-07-02T17:42:46Z

Obsolete since #20472 has been merged.

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

85da593

github-actions bot added the Area: build system Area: Build system label Mar 14, 2024

mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 14, 2024

maribu previously approved these changes Mar 14, 2024

View reviewed changes

maribu reviewed Mar 14, 2024

View reviewed changes

maribu mentioned this pull request Mar 15, 2024

makefiles/docker.inc.mk: Pin riotbuild version with BUILD_IN_DOCKER=1 #20472

Merged

1 task

mguetschow marked this pull request as draft March 25, 2024 11:48

mguetschow closed this Jul 2, 2024

mguetschow deleted the docker-pull branch July 2, 2024 17:42

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

Conversation

mguetschow commented Mar 14, 2024

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

riot-ci commented Mar 14, 2024

Murdock results

Artifacts

Uh oh!

maribu commented Mar 14, 2024

Uh oh!

mguetschow commented Mar 14, 2024

Uh oh!

maribu Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

mguetschow Mar 15, 2024

Choose a reason for hiding this comment

Uh oh!

mguetschow Mar 15, 2024

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Mar 15, 2024

Uh oh!

kaspar030 commented Mar 15, 2024

Uh oh!

maribu commented Mar 15, 2024

Uh oh!

maribu commented Mar 15, 2024

Uh oh!

Teufelchen1 commented Mar 25, 2024

Uh oh!

mguetschow commented Jul 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants