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

[TEST] skopeo: test zstd:chunked #1111

Closed
wants to merge 2 commits into from

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Nov 19, 2020

skopeo version for testing zstd:chunked

It needs:

I've prepared some zstd:chunked images here: https://hub.docker.com/repository/docker/gscrivano/zstd-chunked

using a command like: skopeo copy containers-storage:fedora:33 docker://docker.io/gscrivano/zstd-chunked:fedora33 on a Fedora 33 host shows:

Getting image source signatures
Copying blob 787bfd9c5211 done [===========+++++++++++++++++++++++++++] 68.4MiB / 68.4MiB (skipped: 48.2MiB = 70.51%)
Copying config 7985d34067 done  
Writing manifest to image destination
Storing signatures

Then using a squashed image pushed to a local registry, the image is created as:

$ cat Dockerfile 
FROM fedora:33
RUN echo hello > /greet
$ podman build --squash-all -t squashed . 

we get:

# skopeo  copy  --src-tls-verify=false docker://192.168.125.1:5001/squashed containers-storage:squashed
Getting image source signatures
Copying blob 8853985fae1c done [++++++++++++++++++++++++++++++++++++++] 68.5MiB / 68.5MiB (skipped: 68.1MiB = 99.48%)
Copying config 05885c35a9 done  
Writing manifest to image destination
Storing signatures

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@giuseppe giuseppe force-pushed the zstd-chunked branch 13 times, most recently from fef713d to f12358b Compare November 25, 2020 11:16
@giuseppe giuseppe force-pushed the zstd-chunked branch 17 times, most recently from fa17adc to 8973014 Compare December 3, 2020 11:07
@giuseppe giuseppe force-pushed the zstd-chunked branch 4 times, most recently from 7033859 to 77e50c9 Compare December 9, 2020 09:39
@giuseppe giuseppe force-pushed the zstd-chunked branch 2 times, most recently from a22bb4b to 9ad85c8 Compare February 17, 2021 13:08
giuseppe added 2 commits April 1, 2021 12:13
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@github-actions
Copy link

github-actions bot commented Jun 4, 2021

A friendly reminder that this PR had no activity for 30 days.

@mtrmac
Copy link
Contributor

mtrmac commented Jul 26, 2021

@giuseppe What do you plan to do with this PR? Was it just to provide a proof of concept? Or do you expect every caller of c/image to set FetchPartialBlobs? (In which case, why not enable it by default? Except that now it is a part of the stable API…).

@giuseppe
Copy link
Member Author

@giuseppe What do you plan to do with this PR? Was it just to provide a proof of concept? Or do you expect every caller of c/image to set FetchPartialBlobs? (In which case, why not enable it by default? Except that now it is a part of the stable API…).

I've opened this PR just as a PoC but I can still rebase it on top of containers/image. Should we just make it part of containers.conf?

I've not enabled it by default because 1) it is an experimental feature yet 2) and it cost an extra request for images that are not seekable (so pulls for current images will just be more expensive)

@mtrmac
Copy link
Contributor

mtrmac commented Jul 27, 2021

@giuseppe What do you plan to do with this PR? Was it just to provide a proof of concept? Or do you expect every caller of c/image to set FetchPartialBlobs? (In which case, why not enable it by default? Except that now it is a part of the stable API…).

I've opened this PR just as a PoC but I can still rebase it on top of containers/image. Should we just make it part of containers.conf?

From a Skopeo perspective, if that happened, this PR would become empty, wouldn’t it?

WRT containers.conf, when would users make a global decision one way or the other? Are there actually distinct groups that would want to set the option in a particular way?


I've not enabled it by default because 1) it is an experimental feature yet 2) and it cost an extra request for images that are not seekable (so pulls for current images will just be more expensive)

For all images, even those that don’t carry the annotations?

@giuseppe
Copy link
Member Author

I've not enabled it by default because 1) it is an experimental feature yet 2) and it cost an extra request for images that are not seekable (so pulls for current images will just be more expensive)

For all images, even those that don’t carry the annotations?

You are right. I forgot that the implementation was changed to try only when the annotation is explicitly set, so it should not be more expensive.

I am fine to enable it by default so containers/image users don't have to change anything.

Do we need to mark FetchPartialBlobs as obsolete? I think it should be fine to just drop it as I guess there are no users of it yet and it was documented as Experimental

	// FetchPartialBlobs indicates whether to attempt to fetch the blob partially.  Experimental.
	FetchPartialBlobs bool

@giuseppe giuseppe closed this Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants