docker pull: warn when pulled single-arch image does not match --platform by thaJeztah · Pull Request #42325 · moby/moby

thaJeztah · 2021-04-26T10:58:08Z

This takes the same approach as was implemented on docker build, where a warning
is printed if FROM --platform=... is used (added in 3996953 / #41873)

Before:

docker rmi armhf/busybox
docker pull --platform=linux/s390x armhf/busybox

Using default tag: latest
latest: Pulling from armhf/busybox
d34a655120f5: Pull complete
Digest: sha256:8e51389cdda2158935f2b231cd158790c33ae13288c3106909324b061d24d6d1
Status: Downloaded newer image for armhf/busybox:latest
docker.io/armhf/busybox:latest

With this change:

docker rmi armhf/busybox
docker pull --platform=linux/s390x armhf/busybox

Using default tag: latest
latest: Pulling from armhf/busybox
d34a655120f5: Pull complete
Digest: sha256:8e51389cdda2158935f2b231cd158790c33ae13288c3106909324b061d24d6d1
Status: Downloaded newer image for armhf/busybox:latest
WARNING: image with reference armhf/busybox was found but does not match the specified platform: wanted linux/s390x, actual: linux/arm64
docker.io/armhf/busybox:latest

And daemon logs print:

WARN[2021-04-26T11:19:37.153572667Z] ignoring platform mismatch on single-arch image  error="image with reference armhf/busybox was found but does not match the specified platform: wanted linux/s390x, actual: linux/arm64" image=armhf/busybox

When pulling without specifying `--platform, no warning is currently printed (but we can add a warning in future);

docker rmi armhf/busybox
docker pull armhf/busybox

Using default tag: latest
latest: Pulling from armhf/busybox
d34a655120f5: Pull complete
Digest: sha256:8e51389cdda2158935f2b231cd158790c33ae13288c3106909324b061d24d6d1
Status: Downloaded newer image for armhf/busybox:latest
docker.io/armhf/busybox:latest

- Description for the changelog

Print a warning when pulling a single-arch image with `docker pull --platform` and  the pulled image's platform does not match the specified `--platform`.

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2021-04-26T10:59:14Z

@cpuguy83 @tianon @tonistiigi ptal

Actually considering if this should be an error (but we can do this in a follow-up)

thaJeztah · 2021-04-26T11:10:31Z

Let me fix this log, as it's mentioning manifest list which is not the case here

Oh, actually, err already contains all info we need;

image with reference armhf/busybox was found but does not match the specified platform: wanted linux/s390x, actual: linux/arm64

We can probably just use the error

thaJeztah · 2021-04-26T15:53:38Z

Hmmm... flaky test? Not enough space on the CI machine?


[2021-04-26T11:42:25.900Z] === FAIL: github.com/docker/docker/integration/build TestBuildWCOWSandboxSize (232.07s)
[2021-04-26T11:42:25.900Z]     build_test.go:571: assertion failed: value "{\"stream\":\"Step 1/8 : FROM busybox AS intermediate\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e 46998f50ff5b\\n\"}\r\n{\"stream\":\"Step 2/8 : WORKDIR C:\\\\\\\\stuff\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in d8acfbc856bc\\n\"}\r\n{\"stream\":\"Removing intermediate container d8acfbc856bc\\n\"}\r\n{\"stream\":\" ---\\u003e 5ebaa94a5c35\\n\"}\r\n{\"stream\":\"Step 3/8 : RUN fsutil file createnew C:\\\\\\\\stuff\\\\\\\\bigfile_0.txt 22548578304 \\u0026\\u0026 del bigfile_0.txt\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in 3c70187a55af\\n\"}\r\n{\"stream\":\"File C:\\\\stuff\\\\bigfile_0.txt is created\\r\\n\"}\r\n{\"stream\":\"Removing intermediate container 3c70187a55af\\n\"}\r\n{\"stream\":\" ---\\u003e 42fe6fa61721\\n\"}\r\n{\"stream\":\"Step 4/8 : RUN fsutil file createnew C:\\\\\\\\stuff\\\\\\\\bigfile_1.txt 7516192768\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in 5072b6636314\\n\"}\r\n{\"stream\":\"File C:\\\\stuff\\\\bigfile_1.txt is created\\r\\n\"}\r\n{\"stream\":\"Removing intermediate container 5072b6636314\\n\"}\r\n{\"stream\":\" ---\\u003e 78a4277cd66d\\n\"}\r\n{\"stream\":\"Step 5/8 : RUN fsutil file createnew C:\\\\\\\\stuff\\\\\\\\bigfile_2.txt 7516192768\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in b3ebb880a716\\n\"}\r\n{\"stream\":\"File C:\\\\stuff\\\\bigfile_2.txt is created\\r\\n\"}\r\n{\"stream\":\"Removing intermediate container b3ebb880a716\\n\"}\r\n{\"stream\":\" ---\\u003e a5cbf2aa2d32\\n\"}\r\n{\"stream\":\"Step 6/8 : RUN fsutil file createnew C:\\\\\\\\stuff\\\\\\\\bigfile_3.txt 7516192768\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in bf00c6863eb4\\n\"}\r\n{\"stream\":\"File C:\\\\stuff\\\\bigfile_3.txt is created\\r\\n\"}\r\n{\"stream\":\"Removing intermediate container bf00c6863eb4\\n\"}\r\n{\"errorDetail\":{\"message\":\"re-exec error: exit status 1: output: write \\\\\\\\?\\\\D:\\\\CI\\\\PR-42325\\\\2\\\\daemon\\\\tmp\\\\hcs889963371\\\\Files\\\\stuff\\\\bigfile_3.txt: There is not enough space on the disk.\"},\"error\":\"re-exec error: exit status 1: output: write \\\\\\\\?\\\\D:\\\\CI\\\\PR-42325\\\\2\\\\daemon\\\\tmp\\\\hcs889963371\\\\Files\\\\stuff\\\\bigfile_3.txt: There is not enough space on the disk.\"}\r\n" does not match regexp "Successfully built|COPY --from=intermediate"

tianon · 2021-04-26T16:27:08Z

IMO there are enough weird edge cases in the wild to where I think this makes sense to keep as a warning (not error). 😅

Does this still warn for things that match the "vector"? (ala, docker pull --platform linux/amd64 i386/hello-world or even docker pull --platform linux/arm/v7 arm32v7/hello-world:linux which will be linux/arm but won't have a variant in the image metadata)

thaJeztah · 2021-04-28T11:16:55Z

IMO there are enough weird edge cases in the wild to where I think this makes sense to keep as a warning (not error). 😅

A bit on the fence; this error/warning would only be produced when explicitly setting --platform. I'm leaning toward "an error" is ok in that situation (the user asked for a specific platform, if it doesn't match, that's an error).

That said; l'd be ok with not doing that for the existing 20.10 release. If there are corner-cases where the image should be valid, but we don't handle it correctly, those are bugs we must fix.

Does this still warn for things that match the "vector"? (ala, docker pull --platform linux/amd64 i386/hello-world or even docker pull --platform linux/arm/v7 arm32v7/hello-world:linux which will be linux/arm but won't have a variant in the image metadata)

Good question; let me give that a try on this branch

thaJeztah · 2021-04-28T12:27:31Z

Looks like i386/hello-world is a multi-arch image with a single variant;

docker pull --platform linux/amd64 i386/hello-world
Using default tag: latest
latest: Pulling from i386/hello-world
no matching manifest for linux/amd64 in the manifest list entries

# without --platform
docker pull i386/hello-world
Using default tag: latest
latest: Pulling from i386/hello-world
no matching manifest for linux/amd64 in the manifest list entries

Interestingly, it has 386 (not i386) in the manifest-list; is that correct?

from docker manifest inspect i386/hello-world

{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
   "manifests": [
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 525,
         "digest": "sha256:cb55d8f7347376e1ba38ca740904b43c9a52f66c7d2ae1ef1a0de1bc9f40df98",
         "platform": {
            "architecture": "386",
            "os": "linux"
         }
      }
   ]
}

Pulling with either linux/386 or linux/i386 works;

docker pull --platform=linux/386 i386/hello-world
Using default tag: latest
latest: Pulling from i386/hello-world
87e8d6e253e0: Pull complete
Digest: sha256:eb3c7d0a9879a72341421e5b0d9de21faf442201b0643823eab6786af8834d72
Status: Downloaded newer image for i386/hello-world:latest
docker.io/i386/hello-world:latest

docker rmi i386/hello-world
docker pull --platform=linux/i386 i386/hello-world
Using default tag: latest
latest: Pulling from i386/hello-world
Digest: sha256:eb3c7d0a9879a72341421e5b0d9de21faf442201b0643823eab6786af8834d72
Status: Image is up to date for i386/hello-world:latest
docker.io/i386/hello-world:latest

And docker run i386/hello-world works after that

For linux/arm/v7, it looks to work as well (so normalizing works correctly)

docker pull --platform linux/arm/v7 arm32v7/hello-world:linux
linux: Pulling from arm32v7/hello-world
4ee5c797bcd7: Pull complete
Digest: sha256:50b8560ad574c779908da71f7ce370c0a2471c098d44d1c8f6b513c5a55eeeb1
Status: Downloaded newer image for arm32v7/hello-world:linux
docker.io/arm32v7/hello-world:linux

And some other variations;

docker rmi arm32v7/hello-world:linux
docker pull --platform linux/arm/v8 arm32v7/hello-world:linux
linux: Pulling from arm32v7/hello-world
Digest: sha256:50b8560ad574c779908da71f7ce370c0a2471c098d44d1c8f6b513c5a55eeeb1
Status: Image is up to date for arm32v7/hello-world:linux
docker.io/arm32v7/hello-world:linux

docker rmi arm32v7/hello-world:linux
docker pull --platform linux/arm64 arm32v7/hello-world:linux
linux: Pulling from arm32v7/hello-world
4ee5c797bcd7: Pull complete
Digest: sha256:50b8560ad574c779908da71f7ce370c0a2471c098d44d1c8f6b513c5a55eeeb1
Status: Downloaded newer image for arm32v7/hello-world:linux
docker.io/arm32v7/hello-world:linux

docker rmi arm32v7/hello-world:linux
docker pull --platform linux/arm/v8 arm32v7/hello-world:linux
linux: Pulling from arm32v7/hello-world
4ee5c797bcd7: Pull complete
Digest: sha256:50b8560ad574c779908da71f7ce370c0a2471c098d44d1c8f6b513c5a55eeeb1
Status: Downloaded newer image for arm32v7/hello-world:linux
docker.io/arm32v7/hello-world:linux

tianon

Looks like i386/hello-world is a multi-arch image with a single variant;

Doh, I missed that I'd referenced that one too -- you notice I fixed the arm32v7 example to use :linux explicitly for that exact reason before I posted. 😂

Thanks for the additional testing! LGTM 👍

(totally agree that warning for 20.10 is good and I won't object to error in the next release 😇)

tianon · 2021-04-30T15:38:57Z

CI failure seems unrelated?

=== FAIL: amd64.integration.service TestCreateServiceSecretFileMode (8.21s)
    create_test.go:291: assertion failed: 2 (int) != 1 (int)

thaJeztah · 2021-05-03T08:12:47Z

Triggered CI, and it's green now 👍

@AkihiroSuda @cpuguy83 @tonistiigi ptal

cpuguy83

Is the purpose here just to go the extra mile and warn that the fetched image does not match the platform?

cpuguy83 · 2021-05-12T18:57:51Z

I'm not sure I understand this line.

LOL, I literally took that from your PR #41873 😅

But to answer your question; err is not "local" to this if, so we need to reset it as we only want a warning to be printed, and not make the pull fail.

Alternatively I could use a local err (e.g. iErr) if you think that's better

A local iErr makes more sense to me as it's harder to miss when changing the code in the future.

I updated this code, and now use an early return, as well as added more comments to describe what we're doing

…form This takes the same approach as was implemented on `docker build`, where a warning is printed if `FROM --platform=...` is used (added in 3996953) Before: docker rmi armhf/busybox docker pull --platform=linux/s390x armhf/busybox Using default tag: latest latest: Pulling from armhf/busybox d34a655120f5: Pull complete Digest: sha256:8e51389cdda2158935f2b231cd158790c33ae13288c3106909324b061d24d6d1 Status: Downloaded newer image for armhf/busybox:latest docker.io/armhf/busybox:latest With this change: docker rmi armhf/busybox docker pull --platform=linux/s390x armhf/busybox Using default tag: latest latest: Pulling from armhf/busybox d34a655120f5: Pull complete Digest: sha256:8e51389cdda2158935f2b231cd158790c33ae13288c3106909324b061d24d6d1 Status: Downloaded newer image for armhf/busybox:latest WARNING: image with reference armhf/busybox was found but does not match the specified platform: wanted linux/s390x, actual: linux/arm64 docker.io/armhf/busybox:latest And daemon logs print: WARN[2021-04-26T11:19:37.153572667Z] ignoring platform mismatch on single-arch image error="image with reference armhf/busybox was found but does not match the specified platform: wanted linux/s390x, actual: linux/arm64" image=armhf/busybox When pulling without specifying `--platform, no warning is currently printed (but we can add a warning in future); docker rmi armhf/busybox docker pull armhf/busybox Using default tag: latest latest: Pulling from armhf/busybox d34a655120f5: Pull complete Digest: sha256:8e51389cdda2158935f2b231cd158790c33ae13288c3106909324b061d24d6d1 Status: Downloaded newer image for armhf/busybox:latest docker.io/armhf/busybox:latest Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2021-06-10T23:02:34Z

All green again; @cpuguy83 @samuelkarp ptal

cpuguy83

LGTM

samuelkarp

LGTM

thaJeztah added area/distribution Image Distribution status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. process/cherry-pick labels Apr 26, 2021

thaJeztah force-pushed the warn_on_non_matching_platform branch from c61c0ff to d3c6134 Compare April 26, 2021 10:58

thaJeztah mentioned this pull request Apr 26, 2021

docker pull --platform=linux/arm64 silently falls back to linux/amd64 image docker/for-mac#5625

Closed

3 tasks

thaJeztah commented Apr 26, 2021

View reviewed changes

thaJeztah force-pushed the warn_on_non_matching_platform branch from d3c6134 to 79f6265 Compare April 26, 2021 11:25

tianon approved these changes Apr 28, 2021

View reviewed changes

cpuguy83 reviewed May 12, 2021

View reviewed changes

thaJeztah added this to the 21.xx milestone May 19, 2021

thaJeztah force-pushed the warn_on_non_matching_platform branch from 79f6265 to 424c0eb Compare June 7, 2021 21:09

cpuguy83 approved these changes Jun 10, 2021

View reviewed changes

samuelkarp approved these changes Jun 10, 2021

View reviewed changes

samuelkarp added status/4-merge and removed status/2-code-review labels Jun 10, 2021

samuelkarp merged commit 17bf621 into moby:master Jun 10, 2021

thaJeztah deleted the warn_on_non_matching_platform branch June 11, 2021 06:34

thaJeztah mentioned this pull request Jul 13, 2021

[20.10 backport] docker pull: warn when pulled single-arch image does not match --platform #42633

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Jul 13, 2021

Uh oh!

Conversation

thaJeztah commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Apr 26, 2021

Uh oh!

thaJeztah Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 26, 2021

Uh oh!

tianon commented Apr 26, 2021

Uh oh!

thaJeztah commented Apr 28, 2021

Uh oh!

thaJeztah commented Apr 28, 2021

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

tianon commented Apr 30, 2021

Uh oh!

thaJeztah commented May 3, 2021

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 May 12, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 19, 2021

Choose a reason for hiding this comment

Uh oh!

samuelkarp May 30, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 10, 2021

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah commented Apr 26, 2021 •

edited

Loading