Skip to content
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

Imagetools multiple repositories #1137

Merged
merged 4 commits into from
Aug 1, 2022

Conversation

jedevc
Copy link
Collaborator

@jedevc jedevc commented May 18, 2022

Fixes #486

This PR introduces support for multiple repositories: this allows creating manifest lists with manifests pulled from multiple other locations.

This is split into two commits:

  • The first refactors surrounding code to ensure that multiple repositories are recognized - this has changed some of the API surface, making the private src struct public to allow keeping the oci descriptor next to where it can be fetched from.
  • The second to actually copy the manifests around to the new repository, and enable the multiple repositories feature

@jedevc jedevc added the kind/enhancement New feature or request label May 18, 2022
@tonistiigi
Copy link
Member

Could we add some progress logs so the user can see what is going on when layer transfers happen? Ideally, we could just reuse the build progressbar and show that transfers the same way we see on image pulls during build. For now, just printing logs like "copying blob sha256: from .. to .. & done` would do as well.

I don't this currently does cross-repo mounting but it should be quite easy to add support for it. Supposedly the label for the origin of the blob gets missing somewhere. https://github.com/containerd/containerd/blob/main/remotes/handlers.go#L330

@jedevc jedevc force-pushed the imagetools-multiple-repositories branch from 7e004d2 to 07b2bec Compare May 24, 2022 09:06
@jedevc
Copy link
Collaborator Author

jedevc commented May 25, 2022

Have got the cross-repo mounting working now in a wip commit on top - I think the cleanest solution is to add an annotateDistributionSourceHandler to contentutil.CopyChain in buildkit, by having it copy the annotation attached to the top level descriptor to the lower level descriptors. Then, the resolver's Copy function should just be able to attach it to the manifest descriptor.

Let me know, and I can actually open a PR on buildkit, instead of hacking around with the vendor/ directory 🙈

@tonistiigi
Copy link
Member

Yes, go ahead with the PR to buildkit.

@crazy-max
Copy link
Member

We should review this example in build-push-action repo when 0.9 is out with this PR: https://github.com/docker/build-push-action/blob/master/docs/advanced/copy-between-registries.md

@jedevc jedevc force-pushed the imagetools-multiple-repositories branch from 26d9f7a to 0602be0 Compare July 20, 2022 15:01
@jedevc
Copy link
Collaborator Author

jedevc commented Jul 20, 2022

Have rebased onto master, this should be ready to review/merge.

@jedevc jedevc requested review from crazy-max and removed request for crazy-max July 25, 2022 16:31
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

var repo string
for r := range repos {
repo = r
var defaultRepo *string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repos map looks unused now. You can just use the earlier range tags/srcs to determine the defaultRepo

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repos is only used to determine the defaultRepo. I think some sort of collection of all the repo names is required, since we only derive a default if all the other repo names present are the same.

We could potentially have a bool to track if we found a different repo that doesn't match the previously selected default during iteration, but I think that's potentially more complex than just tracking all the found repo names and then having a len().

eg.Go(func() error {
return progress.Wrap(fmt.Sprintf("pushing %s", t.String()), pw.Write, func(sub progress.SubLogger) error {
eg2, _ := errgroup.WithContext(ctx)
for _, s := range srcs {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this only happen when src and destination repo are actually different?

This patch modifies the existing combining code in imagetools create to
provide better support for multiple repositories down the road.
Specifically, the code should no longer rely on a single repository
being used for all sources and tags, and should resolve descriptors in
their relevant repositories.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the imagetools-multiple-repositories branch from 0602be0 to f1a9f91 Compare July 29, 2022 13:30
@jedevc jedevc requested a review from tonistiigi July 29, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

imagetools create should support copying images between repositories
3 participants