Touch-up some errors for missing platforms#48631
Conversation
thaJeztah
added
status/2-code-review
kind/bugfix
| switch len(presentMatchingManifests) { | ||
| case 0: | ||
| return ocispec.Descriptor{}, errdefs.NotFound(fmt.Errorf("no suitable image manifest found for platform %s", *platform)) | ||
| return ocispec.Descriptor{}, errdefs.NotFound(fmt.Errorf("no suitable image manifest found for platform %s", platforms.FormatAll(*platform))) |
There was a problem hiding this comment.
We may be able to improve these errors further, as they're still somewhat "technical". Perhaps improve them to more clearly describe "the image was found, but not for the given platform variant"
I'll keep that for a future change though (will open a ticket for improving)
There was a problem hiding this comment.
I went looking for it as I was sure I'd seen something like this, and realized we do have https://github.com/docker/moby/blob/810c7c1dce5bbf76af4ed5c6bac8c47fead4f6c6/daemon/containerd/image.go#L27
Can we use that (with some modifications if needed)?
Without containerd store enabled, we were discarding underlying errors,
and instead informing the user that `--platform` is not suported;
docker pull --quiet --platform=linux/riscv64 alpine:latest
docker image push --platform=linux/amd64 alpine:latest
Error response from daemon: graphdriver backed image store doesn't support multiplatform images
However, that's not the case; platform filtering works, but if the image
is not the right platform, the push fails (which is the same as would
happen with the containerd image store enabled).
docker image push --platform=linux/amd64 alpine:latest
Error response from daemon: image with reference docker.io/library/alpine:latest was found but does not match the specified platform: wanted linux/amd64, actual: linux/riscv64
When specifying the platform and that platform matches, it finds the image,
and the push continue;
docker image push --platform=linux/riscv64 alpine:latest
The push refers to repository [docker.io/library/alpine]
3fd4750fd687: Layer already exists
...
(The above example obviously fails because I don't have permissions to
push official images).
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Slightly touching up the error message, because the "wanted/actual" output
felt too much like a assertion failure in a test-case.
- Format the image-reference using "familiar" format, which shows the
image's short name for official images.
- Move the actual and requested platforms to be a part of the sentence,
but within braces.
Before this patch:
docker image push --platform=linux/amd64 alpine:latest
Error response from daemon: image with reference docker.io/library/alpine:latest was found but does not match the specified platform: wanted linux/amd64, actual: linux/riscv64
With this patch:
docker image push --platform=linux/amd64 alpine:latest
Error response from daemon: image with reference alpine:latest was found but its platform (linux/riscv64) does not match the specified platform (linux/amd64)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
- Changed "match" to "provide", in an attempt to indicate that the image is
a multi-platform image that doesn't contain the given platform.
- Remove the "wanted" and instead make the requested platforms to be a part
of the sentence, but within braces.
Before this patch:
docker pull --quiet --platform=linux/riscv64 alpine:latest
docker image history --platform=linux/amd64 alpine
Error response from daemon: image with reference alpine:latest was found but does not match the specified platform: wanted linux/nosuch
With this patch:
docker pull --quiet --platform=linux/riscv64 alpine:latest
docker image history --platform=linux/amd64 alpine
Error response from daemon: image with reference alpine:latest was found but does not provide the specified platform (linux/amd64)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
…rrors
The platform was printed in its raw format, which didn't produce a very
readable output;
Before this change:
$ docker image save --platform=linux/amd64 -o alpine_amd64.tar alpine:latest
Error response from daemon: no suitable export target found for platform linux/amd64: no suitable image manifest found for platform {amd64 linux [] }
After this change:
$ docker image save --platform=linux/amd64 -o alpine_amd64.tar alpine:latest
Error response from daemon: no suitable export target found: image with reference alpine:latest was found but does not provide the specified platform (linux/amd64)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
improve_multiarch_errors
branch
from
ad24262 to
cd551b9
Compare
thaJeztah
changed the title
|
@laurazard @dmcgowan I updated the error with #48631 (comment), and touched up some other errors; updated the PR description (and each commit should outline the changes I made); PTAL Also opened a tracking ticket for some further improvements we could make; |
|
This one also randomly got stuck and timed out after 2 hours (but should've taken just a few minutes); |
|
Thanks! I'll have a look how much of this can be backported; possibly for some of the errors I may have to temporarily keep it to the original "fix" only (formatting the platform-string), and keep the remaining parts for v28.0.0; #48632 (comment)
|
| if ref, err := reference.ParseNamed(refOrID); err == nil { | ||
| imgName = reference.FamiliarString(ref) | ||
| } | ||
| retErr = errdefs.NotFound(errors.Errorf("image with reference %s was found but its platform (%s) does not match the specified platform (%s)", imgName, platforms.Format(imgPlat), platforms.Format(p))) |
There was a problem hiding this comment.
It occurred to me that we're using platforms.Format() here, and not platforms.FormatAll(). The latter also includes "os-version" if it's set. While we don't use that / document that (yet) in the CLI documentation, it may become more relevant with Windows images.
I don't think it's problematic to have it here, but I'll open a tracking ticket to make a pass at looking for places where we could replace Format with FormatAll.
|
Thanks for review, everyone! Let me bring this one in, and start looking if we can backport (parts of) this |
5 participants
--platformflag to history, save and load docker/cli#5331PushImage: remove misleading error about --platform without containerd
Without containerd store enabled, we were discarding underlying errors,
and instead informing the user that
--platformis not suported;However, that's not the case; platform filtering works, but if the image
is not the right platform, the push fails (which is the same as would
happen with the containerd image store enabled).
When specifying the platform and that platform matches, it finds the image,
and the push continue;
(The above example obviously fails because I don't have permissions to
push official images).
images: GetImage: touch-up error message for missing platform
Slightly touching up the error message, because the "wanted/actual" output
felt too much like a assertion failure in a test-case.
image's short name for official images.
but within braces.
Before this patch:
With this patch:
daemon/containerd: touch-up errPlatformNotFound error
a multi-platform image that doesn't contain the given platform.
of the sentence, but within braces.
Before this patch:
With this patch:
daemon/containerd: getPushDescriptor: fix formatting of platform in errors
The platform was printed in its raw format, which didn't produce a very
readable output;
Before this change:
After this change: