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

[maribu]

Contribution description

We had a number of issues being reported that in the end were caused by building with a version of riotbuild incompatible with the source.

With this PR the docker container version is pinned in the RIOT repo code. Running with BUILD_IN_DOCKER=1 will fetch that container, if not locally present.

In addition an CI check was added that makes sure that after a new container was published, no PR other one that updates the pinning can get merged. This check contains a helpful message with exact instructions on how to update the pinning, to which value, and where.

Testing procedure

Compile an app with BUILD_IN_DOCKER=1. It should be have as follows:

1. The pinned image is not available or not in the version pinned
   • It should pull the pinned image automatically and build with that
2. The pinning image is available in the version pinned. Possibly other versions are also available, which may also be more recent
   • It should build with the local image in the pinned version

Issues/PRs references

Alternative to #20470

Depends on and includes:

• dist/tools/buildsystem_sanity_check: make shellcheck happy #20721

[mguetschow]

LGTM in general, although a proper semver versioning as suggested by @kaspar030 would probably be the cleaner solution. Having a hardcoded hash in here requires us to remember updating it whenever breaking changes happen to RIOT sources.

In any case, I would add a sentence to the documentation explicitly stating this behavior for the default docker image.

[riot-ci]

Murdock results

✔️ PASSED

17dcb97 dist/tools/buildsystem_sanity_check: check pinned docker version

Success	Failures	Total	Runtime
10161	0	10161	27m:21s

Artifacts

• Documentation preview

[mguetschow]

The Docker container got updated with the last merge, so we would need to update the whitelist already: https://hub.docker.com/r/riot/riotbuild/tags. I'm wondering if it wouldn't be better to enforce a single specific Docker image at any point in time to avoid having an ever-growing whitelist that would somehow need to be checked for backwards compatibility?

@maribu would you mind adding a sentence to the documentation so we can get this in before the next release?

[mguetschow]

A suggestion on how to move this forward:

1. only have a single, tested and guaranteed-to-work riotbuild docker image version for every RIOT commit (i.e., change COMPATIBLE_RIOTBUILD_VERSIONS to a single TESTED_RIOTBUILD_VERSION)
2. do not abort the build when the downloaded docker image does not match (no breakage in trains), but instead issue a prominent warning, suggesting docker pull docker.io/riot/riotbuild:sha256@b6af289bf59df73766d685d0df8b8aa98d4fd07b19457699a8a946d204a3405d (see docker docs) and mentioning that things may break with this docker image version

How does that sound to you @maribu ? If you don't find the time for this right now, I can also take over the PR if you want me to.

[maribu]

I wonder if it is just better to set the docker image to docker.io/riot/riotbuild:<hash> instead of docker.io/riot/riotbuild:latest and document in riotbuild to update that hash in RIOT on every bump.

Recalling the last few bumps of riotbuild there was only a single one (adding python-serial) would not have needed to update the docker image to remain bug-for-bug and overflow-for-overflow compatible with the CI.

[mguetschow]

I wonder if it is just better to set the docker image to docker.io/riot/riotbuild:<hash> instead of docker.io/riot/riotbuild:latest and document in riotbuild to update that hash in RIOT on every bump.

Sounds good to me, but I still think the proposed behavior changes of this PR (no automatic download on first use, warning on wrong riotbuild version) make sense.

(I think it needs to be docker.io/riot/riotbuild:sha256@<hash> as written above)

[maribu]

but I still think the proposed behavior changes of this PR (no automatic download on first use, warning on wrong riotbuild version) make sense.

I think the new behavior is even better: When the required docker image is locally available, it will never pull. But if not, it will pull automatically. That would allow bisecting with BUILD_IN_DOCKER=1 to work like a charm even if toolchain and source need a matching version.

Note: In a train in Brandenburg without the docker image available, one could still run make DOCKER_IMAGE=docker.io/riot/riotbuild:latest to just go with the latest locally available instead of a compatible one and hope for the best.

[mguetschow]

Thanks for picking this up again!

I've just tried the current changeset locally, and it seems docker run (...) $image:sha256@$hash doesn't work with my setup (Docker version 20.10.24+dfsg1, build 297e128). Instead, it should be docker run (...) $hash or docker run sha256:$hash, see here: docker/cli#2815

This makes the build fail with the not very user-friendly error message:

docker: Error response from daemon: No such image: sha256:f5951bc41dfface6cac869181d703e62cbdd3b7976b0946130a38f2e658000b2.

Edit: It does work as you describe it with podman version 4.3.1. Actually, it doesn't either, also prints

Error: invalid reference format

[maribu]

No such image

That makes sense. The DROP ME!!! TESTING CI ONLY commit changes the hash to something invalid, so that I can see that the static test would indeed complain when someone would update the CI but forget to bump the hash in makefiles/docker.inc.mk.

[mguetschow]

No such image

That makes sense. The DROP ME!!! TESTING CI ONLY commit changes the hash to something invalid, so that I can see that the static test would indeed complain when someone would update the CI but forget to bump the hash in makefiles/docker.inc.mk.

Yeah sorry, this also happens when using the right hash with 3 at the end. The point is just that docker tries to find the image locally, but when failing to do so, it will just report that it doesn't exist instead of pulling it (since it's missing the docker image location information).

[kfessel]

maybe you can make the Brandenburg case a little more accessible ( and have it produce a warning message)

[mguetschow]

Apparently there are two different hashes involved: The local image ID which can be used to refer to the image, and the repo digest which is the one that is shown on Dockerhub.

One can see both with docker inspect <image> in as in docker inspect --format='{{index .RepoDigests 0}}' riot/riotbuild:latest and docker inspect --format='{{index .Id}}' riot/riotbuild:latest.

[maribu]

Indeed! I now have just added both digest to the Makefile and let the CI verify both are up to date.

It is IMO pretty odd to have one ID to identify an image to run, and an unrelated ID to identify the same image for downloading. But anyway, seems to work now.

[kfessel]

it seems like digest needs to be sha256:75dec511ba26424987a26bdee5ac2f94d5f4928d79b627d1620b9d2391aab3e1

to receive sha256:f5951bc41dfface6cac869181d703e62cbdd3b7976b0946130a38f2e658000b3

$ ./dockery.sh riot/riotbuild manifests/latest
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
  "config": {
    "mediaType": "application/vnd.docker.container.image.v1+json",
    "size": 78827,
    "digest": "sha256:f5951bc41dfface6cac869181d703e62cbdd3b7976b0946130a38f2e658000b3"
  },
  ...

$./dockery.sh riot/riotbuild manifests/latest
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
  "config": {
    "mediaType": "application/vnd.docker.container.image.v1+json",
    "size": 78827,
    "digest": "sha256:f5951bc41dfface6cac869181d703e62cbdd3b7976b0946130a38f2e658000b3"
  },

$ ./dockery.sh riot/riotbuild manifests/sha256:75dec511ba26424987a26bdee5ac2f94d5f4928d79b627d1620b9d2391aab3e1
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
  "config": {
    "mediaType": "application/vnd.docker.container.image.v1+json",
    "size": 78827,
    "digest": "sha256:f5951bc41dfface6cac869181d703e62cbdd3b7976b0946130a38f2e658000b3"
  },
  
  ...

dockery.sh is for dancing around that token thing that docker hub put in front of the OCI container API

it is a stripped version of this

https://stackoverflow.com/questions/57316115/get-manifest-of-a-public-docker-image-hosted-on-docker-hub-using-the-docker-regi

#!/bin/sh

repo=$1
q=$2
api="application/vnd.docker.distribution.manifest.v2+json"
apil="application/vnd.docker.distribution.manifest.list.v2+json"
token=$(curl -s "https://auth.docker.io/token?service=registry.docker.io&scope=repository:${repo}:pull" \
        | jq -r '.token')
curl -H "Accept: ${api}" -H "Accept: ${apil}" \
     -H "Authorization: Bearer $token" \
     -s "https://registry-1.docker.io/v2/${repo}/${q}" | jq .

[maribu]

@kfessel Yes. This is already implemented in this PR.

[mguetschow]

LGTM then, thanks for your effort! 🤗

But let's hold this back until #20721 is merged.

[maribu]

Thanks a bunch! 😄 🎉