distribution: match manifest list resolution with containerd#43675
Merged
Conversation
tonistiigi
commented
Jun 2, 2022
| id, _, err = p.pullSchema1(ctx, manifestRef, v, &platform) | ||
| if err != nil { | ||
| for _, match := range manifestMatches { | ||
| if err := checkImageCompatibility(match.Platform.OS, match.Platform.OSVersion); err != nil { |
Member
Author
There was a problem hiding this comment.
This is a windows-only check. Logically it should probably also just continue rather than error. I left it as-is for now as it seems to be a very specific old case that is intentionally set to error.
thaJeztah
added
area/distribution
thaJeztah
reviewed
Jun 2, 2022
thaJeztah
reviewed
Jun 2, 2022
| platform specs.Platform | ||
| } | ||
|
|
||
| func (e noMatchesErr) Error() string { |
Member
There was a problem hiding this comment.
For a follow-up, we should check what status code this produces at the API level (and if needed, use a suitable errdef
cpuguy83
requested changes
Jun 2, 2022
Member
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
distribution/pull_v2.go:1068:66: packge is a misspelling of package (misspell)
Make finding the correct runtime image from image index more compliant with OCI spec and match containerd implementation. Changes: - Manifest list is allowed to contain manifest lists - Unknown mediatype inside manifest list is skipped instead of causing an error - Platform in descriptor is optional Signed-off-by: Tonis Tiigi <[email protected]>
tonistiigi
force-pushed
the
make-pull-match-containerd
branch
from
0c15e10 to
9adad26
Compare
cpuguy83
approved these changes
Jun 2, 2022
Member
|
All green; only failure is a known flaky test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are slight differences on how Docker and containerd find a specific image config/rootfs for running a container from a multi-platform image index.
Containerd logic for these edge cases seems to be according to spec https://github.com/opencontainers/image-spec although some cases are a bit vague there.
This PR matches Docker logic with what containerd is doing. With containerd storage integration, the Moby implementation will be replaced with containerd's anyway in the next release and this would be the future behavior. With this PR we can move to correct logic sooner and avoid behavior changes with containerd integration.
There is also a discussion going on at OCI Reference Types working group opencontainers/wg-reference-types#50 that may choose to use nested Image indexes as a way to attach signatures to images. As nested indexes are currently handled differently in Moby we should get to matching implementation sooner to avoid breakage.
Changes:
The algorithm for finding the best match is as follows:
OCI mediatypes and Docker mediatypes work the same way. Obviously, the specs are slightly different but the intention in here is to replicate containerd's behavior not to redefine new behavior for the loosely defined areas.
Signed-off-by: Tonis Tiigi [email protected]