ResolveImageConfig: Only fetch best matching config #4311
Merged
Conversation
This file contains 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
ff5f785 to
6636bda
Compare
|
Some linting failures on receiver name. Also a small typo in the commit message (ReolveImageConfig -> ResolveImageConfig |
6636bda to
230339f
Compare
|
tentatively adding cherry pick label, but input welcome |
|
@cpuguy83 compile error; |
230339f to
ebeda12
Compare
tonistiigi
reviewed
Oct 9, 2023
vvoland
reviewed
Oct 10, 2023
ebeda12 to
24a76c2
Compare
rumpl
approved these changes
Oct 10, 2023
tonistiigi
reviewed
Oct 10, 2023
util/imageutil/config.go
Outdated
| } else { | ||
| descs = append(descs, index.Manifests...) | ||
| } | ||
| descs = append(descs, index.Manifests...) |
There was a problem hiding this comment.
platform.Match condition should still be here. LimitManifests does not call Match and if we skip this it could lead to wrong resolution (eg. for nested indexes).
There was a problem hiding this comment.
@cpuguy83 @tonistiigi I've pushed an update to add this block back in to keep the original condition.
There was a problem hiding this comment.
Sorry I totally missed this comment.
24a76c2 to
e17d488
Compare
Before this change, all platforms that loosely match the provided platform will be fetched even though we only care about 1 of them. As an example when linux/amd64 is requested it will also fetch linux/386 because it is a compatible architecture. This means extra round trips to the registry, potentially even for content that doesn't exist in the remote. This is especially a problem when resolve mode is prefer-local because we'll have the index locally but most likely only one manifest. In this case we'll end up reaching out to the registry to fetch the other manifests unncessarily. With this change instead of fetching all matching platforms it chooses only the best matching platform. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
e17d488 to
575cb10
Compare
jedevc
approved these changes
Oct 13, 2023
tonistiigi
approved these changes
Oct 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Before this change, all platforms that loosely match the provided platform will be fetched even though we only care about 1 of them. As an example when linux/amd64 is requested it will also fetch linux/386 because it is a compatible architecture.
This means extra round trips to the registry, potentially even for content that doesn't exist in the remote.
This is especially a problem when resolve mode is prefer-local because we'll have the index locally but most likely only one manifest. In this case we'll end up reaching out to the registry to fetch the other manifests unncessarily.
With this change instead of fetching all matching platforms it chooses only the best matching platform.
Related: moby/moby#46584