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

compression: add support for the zstd algorithm #41759

Merged
merged 1 commit into from Sep 17, 2021

Conversation

giuseppe
Copy link
Contributor

@giuseppe giuseppe commented Dec 8, 2020

zstd is a compression algorithm that has a very fast decoder, while
providing also good compression ratios. The fast decoder makes it
suitable for container images, as decompressing the tarballs is a very
expensive operation.

opencontainers/image-spec#788 added support
for zstd to the OCI image specs.

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

- What I did

added read support for layers compressed with zstd

- How I did it

added a new zstd compression format to pkg/archive

- How to verify it

try pulling an image compressed with zstd, e.g. docker pull gscrivano/zstd-chunked:fedora33

- Description for the changelog

support zstd compressed layers

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

@rhatdan
Copy link
Contributor

rhatdan commented Dec 8, 2020

@thaJeztah PTAL

@AkihiroSuda
Copy link
Member

Can we exec C implementation? I assume it is faster than Go.

@giuseppe
Copy link
Contributor Author

giuseppe commented Dec 9, 2020

Can we exec C implementation? I assume it is faster than Go.

wouldn't that be a problem to require the zstd compression tool to be installed? Should the Go version be used as fallback?

@thaJeztah
Copy link
Member

Thanks for opening this PR! So, I definitely would like to see support for zstd (it can be a huge space-saver!), I still have my concerns about the current approach in the image-spec. I realise this is not something "new", and not your fault, but something that should be looked at, to prevent being painted into a corner.

Also see the more detailed discussion on opencontainers/image-spec#803. I did discuss this (and related) PRs internally (also with our Hub team), and I see that @justincormack has added this topic to Today's OCI call agenda. I may not be able to join that call myself, but looking forward to the outcome of that, hoping that we can move support for zstd (and other compressions) forward.

@giuseppe
Copy link
Contributor Author

giuseppe commented Dec 9, 2020

I see that @justincormack has added this topic to Today's OCI call agenda

Same here, I may not be able to join the meeting. Is it possible to keep the discussion asynchronous first?

@thaJeztah thaJeztah added area/distribution area/images impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/1-design-review labels Dec 10, 2020
@tonistiigi
Copy link
Member

I think we should move on with this. I (mostly) read through opencontainers/image-spec#803. It doesn't seem to be going anywhere and we shouldn't let it block us.

We already support non-gzip blobs, so this is nothing new or breaking in that perspective. It follows the same rules used previously.

The only real current issue in opencontainers/image-spec#803 is that mediatype is not in OCI release. But given that Moby implementation doesn't validate mediatypes at all and many other tools already use this, I don't see a chance for any breaking changes there.

All the discussion about possible better ways to migrate between compression formats are separate from this and optional. If it involves a schema change, then everything will be broken anyway. If some kind of manifest list extension, then no client implement that currently, and that is a different component.

One thing to note is that this same function is used in Dockerfile ADD command extraction. I don't mind supporting zstd there as well, but we need to make sure the change is consistent when we do a release(eg. that this is vendored to buildkit).

Can we exec C implementation? I assume it is faster than Go.

I created issue for the same thing in buildkit moby/buildkit#2345 . It looks like there isn't a good package with cgo/no-cgo switch atm(or exec), and the existing cgo packages are not ideal either. I agree with providing the fastest implementation but it doesn't need to be in the first PR.

LGTM from me after rebase

@giuseppe
Copy link
Contributor Author

I think we should move on with this. I (mostly) read through opencontainers/image-spec#803. It doesn't seem to be going anywhere and we shouldn't let it block us.

thanks for your review. I've just rebased it and pushed a new version

@AkihiroSuda
Copy link
Member

Can we have an integration test to run a zstd image? Just “echo hello” would be fine.

@giuseppe giuseppe force-pushed the zstd-compression branch 2 times, most recently from a249d37 to 71b28ef Compare September 16, 2021 14:08
zstd is a compression algorithm that has a very fast decoder, while
providing also good compression ratios.  The fast decoder makes it
suitable for container images, as decompressing the tarballs is a very
expensive operation.

opencontainers/image-spec#788 added support
for zstd to the OCI image specs.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the zstd-compression branch 2 times, most recently from 9606e2a to e187eb2 Compare September 16, 2021 15:03
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

discussing with @tonistiigi and @tianon and integration tests for (for example) xz is also missing, so we're ok with looking at integration tests for all of them (with some test images) in a follow up

@tonistiigi
Copy link
Member

For the follow-up test I pushed tonistiigi/hello-world:zstd that could be added to the frozen images list. It is a modified version of library/hello-world built from https://gist.github.com/tonistiigi/239ed89a2cc70c086141def545890f44

@giuseppe
Copy link
Contributor Author

thanks everyone for the review.

Is there anything more left before this can be merged?

@AkihiroSuda AkihiroSuda merged commit 6014c1e into moby:master Sep 17, 2021
@thaJeztah thaJeztah added this to the 21.xx milestone Sep 17, 2021
giuseppe added a commit to giuseppe/image that referenced this pull request Sep 17, 2021
Allow to use the zstd compression for docker images now that Docker
supports it: moby/moby#41759

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe added a commit to giuseppe/image that referenced this pull request Sep 17, 2021
Allow to use the zstd compression for docker images now that Docker
supports it: moby/moby#41759

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/distribution area/images impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/1-design-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants