add cli option to download all manifests#3127
add cli option to download all manifests#3127erain wants to merge 2 commits intocontainerd:masterfrom
Conversation
- Add `all-manifests` option to both `ctr content fetch` and `ctr images pull`. By default it is false. - This option ties to `AppendDistributionSourceLabel` in client. Signed-off-by: Yu Yi <[email protected]>
|
If we agree to change default behavior, then this test would also have to be modified: |
| Usage: "pull content from all platforms", | ||
| }, | ||
| cli.BoolFlag{ | ||
| Name: "all-manifests", |
There was a problem hiding this comment.
if the only use of downloading all the manifests is AppendDistributionSourceLabel, we can have the flag with a more related name, like store-distribution-source.
Otherwise, does it make sense to add distribution source without having all the manifests? If so, can they have independent flags that can somehow be combined?
@dmcgowan what's your recommendation?
There was a problem hiding this comment.
I don't have strong opinion on this and will hold on for the suggestion of @dmcgowan
There was a problem hiding this comment.
Flags are tricky, we don't actually support these so it isn't hugely important we get it right the first time. There are a few different "abilities" to keep in mind around pulling: single images vs all images and pulling layers vs shallow pulling metadata. It would be nice if the flag when used on fetch could handle the "shallow" case, that is don't pull tar blobs for any platform but pull all metadata (manifests/configs).
There was a problem hiding this comment.
Thanks for the feedback!
So my understanding right now:
- for
ctr content fetchwhenall-manifestsis used, containerd downloads all manifests/configs but not layers. - for
ctr images pullwhenall-manifestsis used, containerds downloads all manifests/configs + the layers for the matched platform.
Did I miss anything? If not I will have a new commit to implement the mentioned behavior.
Signed-off-by: Yu Yi <[email protected]>
|
@estesp Thanks for the reminder and this is my first PR so that I was not aware some tests would not run without root. Sorry about that! I've reverted the behavior in:506b815#diff-51565ad50d7426213c2f33579f6f0e71 and the test passes now. |
| // Filter manifests by platforms but allow to handle manifest | ||
| // and configuration for not-target platforms | ||
| childrenHandler = remotes.FilterManifestByPlatformHandler(childrenHandler, rCtx.PlatformMatcher) | ||
| if rCtx.AppendDistributionSourceLabel { |
There was a problem hiding this comment.
I think this is worthy of a new option for all manifests or all image metadata. We haven't finalized cross repository push so it is possible we want to append the distribution labels without pulling each manifest.
|
Had an offline discussion with @alculquicondor and it seems we need two flags here:
@dmcgowan Do you think my understanding is right? |
|
carried |
[carry #3127] Update pull default to skip all platform manifests
4 participants
all-manifestsoption to bothctr content fetchandctr images pull. By default it is false.AppendDistributionSourceLabelinRemoteContextwill only works when this option is set.Fixes #3126
Signed-off-by: Yu Yi [email protected]