Skip to content

c8d/pull: Handle pull all tags #46516

Closed
vvoland wants to merge 3 commits intomoby:masterfrom
vvoland:c8d-pull-all-tags
Closed

c8d/pull: Handle pull all tags #46516
vvoland wants to merge 3 commits intomoby:masterfrom
vvoland:c8d-pull-all-tags

Conversation

@vvoland
Copy link
Copy Markdown
Contributor

@vvoland vvoland commented Sep 20, 2023

distribution: Export NewRepository

Make the function public

distribution: Extract PullEndpoints from Pull

Extract the function to iterate over available pull endpoints for the
given image reference.

c8d/pull: Handle pull all tags

Use the distribution code to query the remote repository for tags and
pull them sequentially just like the non-c8d pull.

Note: This uses the distribution code to query the remote registry for available tags. Theoretically these endpoints could be a little bit different than what the containerd pull would use as we don't pass these (and have no ability to do so) to the containerd pull.

- How to verify it

$ docker pull -a docker/doodle
-Error response from daemon: failed to resolve reference "docker.io/docker/doodle": object required
+birthday: Pulling from docker/doodle
+ecaf003f35ec: Download complete
+Digest: sha256:4a203cbea73f32a4885f8d9ee05233246445aa95dd4362887058ec663a7b8088
+Status: Downloaded newer image for docker/doodle:birthday
+birthday2019: Pulling from docker/doodle
+Digest: sha256:4a203cbea73f32a4885f8d9ee05233246445aa95dd4362887058ec663a7b8088
+Status: Downloaded newer image for docker/doodle:birthday2019
(...)
+summer: Pulling from docker/doodle
+6fe738df11e6: Download complete
+Digest: sha256:f7fdfd4a030d16a619f32f183307c9f60ddd9206dc92b1c3b1e1c2f1450567fc
+Status: Downloaded newer image for docker/doodle:summer
+summer2019: Pulling from docker/doodle
+Digest: sha256:f7fdfd4a030d16a619f32f183307c9f60ddd9206dc92b1c3b1e1c2f1450567fc
+Status: Downloaded newer image for docker/doodle:summer2019
+docker.io/docker/doodle

- Description for the changelog

- containerd image store: Implement pulling all tags (`docker pull -a`)

- A picture of a cute animal (not mandatory but encouraged)

Make the function public

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Extract the function to iterate over available pull endpoints for the
given image reference.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Use the distribution code to query the remote repository for tags and
pull them sequentially just like the non-c8d pull.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland added area/distribution Image Distribution status/2-code-review area/images Image Distribution containerd-integration Issues and PRs related to containerd integration labels Sep 20, 2023
@vvoland vvoland added this to the 25.0.0 milestone Sep 20, 2023
@rumpl
Copy link
Copy Markdown
Member

rumpl commented Sep 20, 2023

Just one comment before I read the code, it's nice to have 3 independent commits but if I read only the first commit message I have no idea what is going in, it's missing a "why", at least an explanation for our future selves. I've been digging a lot into the history lately and pretty much the majority of the messages on commits like these say "this is doing this and that", but none say "we want it like this because..."

func (p *puller) pull(ctx context.Context, ref reference.Named) (err error) {
// TODO(tiborvass): was ReceiveTimeout
p.repo, err = newRepository(ctx, p.repoInfo, p.endpoint, p.config.MetaHeaders, p.config.AuthConfig, "pull")
p.repo, err = NewRepository(ctx, p.repoInfo, p.endpoint, p.config.MetaHeaders, p.config.AuthConfig, "pull")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I need to have a closer look at the changes, but wanted to leave a "global" comment; the code in the distribution package is "not great". There's a lot of complexity involved, and some weird constructs (having to create objects just to know "is this for docker hub, or another registry"). I know I tried to un-export as much as possible for that reason.

I guess my comment here is; if we can avoid exposing more, we should (perhaps internal if there's currently no other way, which would buy us time to work on better implementations)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried to avoid exposing too much, but I don't think we can unexpose more here. We can't even move it to internal, because we would need to reference private things from distribution, which we would need to make public anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, I could make a function like distribution.GetAllTags(registry.Service) ([]string, error) so we'd avoid exposing NewRepository and PullEndpoints.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Opened: #46618 as alternative

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/distribution Image Distribution area/images Image Distribution containerd-integration Issues and PRs related to containerd integration status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

3 participants