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

c8d/push: Fetch missing resources that can't be mounted #80

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

vvoland
Copy link
Collaborator

@vvoland vvoland commented Aug 30, 2022

Follow up to:

Lazily fetch content that is needed for push.
Small content like manifests/indexes/configs are always fetched, as they can't be cross-repo mounted and they are small json files.
Other blobs are not fetched if the push registry matches the source registry - in such case they can be cross-repo mounted and there is no need to fetch them.

This makes the pull-tag-push workflow work:

docker pull busybox
docker tag busybox pawelgronowski465/mybusybox0830:latest
docker push pawelgronowski465/mybusybox0830:latest

It also works with other registries than Hub (HTTPS only), but in this case all content is fetched from the source registry and then pushed to the target registry.

The progress is currently a bit wonky as it doesn't show the correct status for the lazily fetched content - will fix it in a follow up.

Signed-off-by: Paweł Gronowski pawel.gronowski@docker.com

fetchHandler := containerdimages.Handlers(
remotes.FetchHandler(store, fetcher),
appendDistributionSourceLabel,
appendLabelHandler(ctx, store, "docker.io/fetch.reason", "push"),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the label for debugging/testing purposes. I wanted to remove this for PR, but maybe this could be useful, eg. for prune?
We may want to use other label though.

}
log = log.WithField("ref", ref.String())

name, resolved, err := resolver.Resolve(ctx, ref.String())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fails currently for blobs that at over 4MiB due to a containerd bug.
I'll open a PR on containerd side to fix this upstream.
Anyway, I wonder if we even need to do the Resolve - looking at the Resolver implementation that we use the name returned is exactly the same as the ref.String(), so only the resolved descriptor is really filled with data from the registry.
However, we already have a descriptor that should have correct digest, mediaType and size, so I think we could drop the Resolve entirely.
Any thoughts on this?

return registry
}

func (source distributionSource) GetReference(dgst digest.Digest) (reference.Named, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

nit: does this method (and the Registry one) really need to be public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well since the struct itself is private then at this point it doesn't need to be public.
But since those functions are used "outside" of the struct itself then it made sense to me to make them public. It also could be extracted to some other package in future, for example if we'd need to also apply the lazy fetching to other operations.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Copy link
Owner

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

LGTM, we should merge this one and remove the debug things later?

@rumpl rumpl merged commit e13de6c into rumpl:c8d Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Won't Upstream
3 participants