-
Notifications
You must be signed in to change notification settings - Fork 812
Feat/pull image with multi arch #2883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/pull image with multi arch #2883
Conversation
|
This PR has multiple commits, and the default merge method is: squash. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zuoxuesong-worker The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @zuoxuesong-worker, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the image pulling functionality by introducing support for multi-architecture image selection. Previously, the system might pull all available architectures for an image, which is inefficient, especially when creating offline packages for specific environments. The new implementation allows users to specify desired architectures, ensuring that only the necessary image variants are downloaded and stored, leading to more streamlined and efficient image management. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
dcc4bd6 to
78efccb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a feature to pull specific architectures of a multi-arch container image. The implementation involves manually fetching the image index, filtering manifests based on the desired platforms, and then reconstructing a new index with only the selected architectures.
The overall approach is sound, but I have identified a few areas for improvement regarding correctness, robustness, and code maintainability. My main concerns are a potential silent failure if no matching architectures are found, and an imprecise architecture matching logic that could lead to bugs. I've also suggested replacing some magic strings with predefined constants to improve readability. Please see my detailed comments below.
pkg/modules/image.go
Outdated
| // some arm architecture is arm64/v7 or arm68/v8 , support all of then | ||
| for _, arch := range platform { | ||
| if strings.Contains(manifest.Platform.Architecture, arch) { | ||
| manifest.SourceRepo = src | ||
| filteredManifests = append(filteredManifests, manifest) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of strings.Contains for matching architectures is too broad and can lead to incorrect behavior. For example, if a user requests the arm architecture, this will also match arm64, which is likely not the intended behavior. A direct equality check (==) should be used for a precise match.
Additionally, the comment on line 217 is confusing and contains typos (arm68, then). The arm64/v7 combination is also not standard. I've suggested a replacement with a clearer comment and the corrected logic.
| // some arm architecture is arm64/v7 or arm68/v8 , support all of then | |
| for _, arch := range platform { | |
| if strings.Contains(manifest.Platform.Architecture, arch) { | |
| manifest.SourceRepo = src | |
| filteredManifests = append(filteredManifests, manifest) | |
| break | |
| } | |
| } | |
| // Match architecture precisely. | |
| for _, arch := range platform { | |
| if manifest.Platform.Architecture == arch { | |
| manifest.SourceRepo = src | |
| filteredManifests = append(filteredManifests, manifest) | |
| break | |
| } | |
| } |
pkg/modules/image.go
Outdated
| if len(filteredManifests) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When len(filteredManifests) is 0, it means no images were found for the specified architectures. The function currently returns nil, indicating success, which can be misleading as no images were actually pulled. This should return an error to inform the user that the requested architectures were not found for the given image.
| if len(filteredManifests) == 0 { | |
| return nil | |
| } | |
| if len(filteredManifests) == 0 { | |
| return errors.Errorf("no manifests found for image %q and platforms %v", img, platform) | |
| } |
| if desc.MediaType == imagev1.MediaTypeImageIndex || | ||
| desc.MediaType == "application/vnd.docker.distribution.manifest.list.v2+json" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string literal "application/vnd.docker.distribution.manifest.list.v2+json" is used here. To improve code readability and maintainability, it's better to use the predefined constant images.MediaTypeDockerSchema2ManifestList from the github.com/containerd/containerd/images package, which is already imported.
| if desc.MediaType == imagev1.MediaTypeImageIndex || | |
| desc.MediaType == "application/vnd.docker.distribution.manifest.list.v2+json" { | |
| if desc.MediaType == imagev1.MediaTypeImageIndex || | |
| desc.MediaType == images.MediaTypeDockerSchema2ManifestList { |
| if manifestDesc.MediaType != imagev1.MediaTypeImageManifest && | ||
| manifestDesc.MediaType != "application/vnd.docker.distribution.manifest.v2+json" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string literal "application/vnd.docker.distribution.manifest.v2+json" is used here. To improve code readability and maintainability, it's better to use the predefined constant images.MediaTypeDockerSchema2Manifest from the github.com/containerd/containerd/images package, which is already imported.
| if manifestDesc.MediaType != imagev1.MediaTypeImageManifest && | |
| manifestDesc.MediaType != "application/vnd.docker.distribution.manifest.v2+json" { | |
| if manifestDesc.MediaType != imagev1.MediaTypeImageManifest && | |
| manifestDesc.MediaType != images.MediaTypeDockerSchema2Manifest { |
| } else if desc.MediaType == imagev1.MediaTypeImageManifest || | ||
| desc.MediaType == "application/vnd.docker.distribution.manifest.v2+json" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string literal "application/vnd.docker.distribution.manifest.v2+json" is used here. To improve code readability and maintainability, it's better to use the predefined constant images.MediaTypeDockerSchema2Manifest from the github.com/containerd/containerd/images package, which is already imported.
| } else if desc.MediaType == imagev1.MediaTypeImageManifest || | |
| desc.MediaType == "application/vnd.docker.distribution.manifest.v2+json" { | |
| } else if desc.MediaType == imagev1.MediaTypeImageManifest || | |
| desc.MediaType == images.MediaTypeDockerSchema2Manifest { |
6521082 to
931209f
Compare
…ine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]>
931209f to
744fd6f
Compare
# Conflicts: # docs/zh/modules/image.md # pkg/modules/image.go
7093fae to
1248e74
Compare
…ine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]>
1248e74 to
e3cd061
Compare
…ine them to one image Signed-off-by: [email protected] <[email protected]>
# Conflicts: # pkg/modules/image.go
438237b to
2e7784b
Compare
…ine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]> feat: support user pull one or more arch from image registry and combine them to one image Signed-off-by: [email protected] <[email protected]>
2e7784b to
17b2d91
Compare
|
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: d63e737abf03f912109db600fe45a931e167bf83 |



What type of PR is this?
/kind feature
What this PR does / why we need it:
when make offline package , we maybe only need amd64 and arm64 image
but some image has other arch which we dont need
change pull func , we can pull image with arch we want
Which issue(s) this PR fixes:
Fixes #
Special notes for reviewers:
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: