Add support for zstd compressed layers by cpuguy83 · Pull Request #40820 · moby/moby

cpuguy83 · 2020-04-15T21:30:01Z

This allows docker to decompress layers that are compressed with
zstd (https://github.com/facebook/zstd).
This is only for layer decompression.

Since zstd support is part of the OCI spec, we should probably handle it
here.

Closes #28394

cpuguy83 · 2020-04-15T21:30:23Z

I still need to test this with a real image.

cpuguy83 · 2020-04-15T21:31:38Z

Also, should we use a zstd lib like I'm doing here or depend on distro zstd packages?

AkihiroSuda · 2020-04-15T21:34:03Z

Also, should we use a zstd lib like I'm doing here or depend on distro zstd packages?

I guess the latter one is faster

thaJeztah · 2020-04-15T22:03:02Z

seeing compile failures;

Can't run linter unused: buildssa: analysis skipped: errors in package: [/go/src/github.com/docker/docker/pkg/archive/archive.go:28:2: could not import github.com/valyala/gozstd (/go/src/github.com/docker/docker/vendor/github.com/valyala/gozstd/dict.go:27:8: could not import C (cgo preprocessing failed))]

and

gcc: error: vendor/github.com/valyala/gozstd/libzstd_linux_amd64.a: No such file or directory

cpuguy83 · 2020-04-15T22:20:21Z

Oh nice. I tested this manually at the package level.
Indeed it seems we should use zstd CLI.

thaJeztah · 2020-04-15T22:29:12Z

does shelling out have security implications? guess we already do so for pigz

we may want to add it to our "recommends" in the packages?

This allows docker to decompress layers that are compressed with zstd (https://github.com/facebook/zstd). This is only for layer decompression. Since zstd support is part of the OCI spec, we should probably handle it here. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2020-04-16T16:39:24Z

we may want to add it to our "recommends" in the packages?

Yep!

I went ahead and updated this to use the zstd CLI.

thaJeztah · 2020-04-17T11:57:09Z

does shelling out have security implications? guess we already do so for pigz

Had a quick chat with @justincormack and he "trusts zstd more than pigz" 😂 so I think we should be ok there.

cpuguy83 · 2020-04-17T22:37:06Z

Ok, I tested this by saving debian:buster, compressing the layer.tar with zstd, updating the layer SHA in the image config, and loading back into docker.

cpuguy83 · 2020-04-17T22:39:31Z

Removed WIP

thaJeztah · 2020-04-19T19:04:39Z

Ok, I tested this by saving debian:buster, compressing the layer.tar with zstd, updating the layer SHA in the image config, and loading back into docker.

Do you think it would make sense to push a minimal image to docker hub, and include it in our "frozen images" ? https://github.com/moby/moby/blob/master/Dockerfile#L90-L95, so that we can do a test with that? (I'm ok with doing so in a follow-up if it needs more work)

cpuguy83 · 2020-04-19T19:12:05Z

I was thinking about it, but the image will have the wrong media types, and fairly dependent on the image staying the same.

cpuguy83 · 2020-04-19T19:17:18Z

The later I guess would not be a problem.

thaJeztah

Left some comments inline; in addition: with this change, will we actually be able to pull such images? While I see that the image-spec accepted a PR to add layers with +zstd, those media-types are not yet in the current (v1.0.1) specification. I see containerd worked around this by implementing a "generic" media-type handling (separating "compression" from the media-type), and containers/image is vendoring a image-spec version from "master".

I opened opencontainers/image-spec#803 for discussion; a bit unsure what the best way forward is for this 😅 (open for input!)

thaJeztah · 2020-04-20T07:34:39Z

pkg/archive/archive.go

 		Bzip2: {0x42, 0x5A, 0x68},
 		Gzip:  {0x1F, 0x8B, 0x08},
 		Xz:    {0xFD, 0x37, 0x7A, 0x58, 0x5A, 0x00},
+		Zstd:  {0x28, 0xB5, 0x2F, 0xFD},


I noticed that the original PR implementing this (#36754) had a much longer list; did this change, or was the original implementation incorrect?

These are probably legacy forms, I'll investigate.

thaJeztah · 2020-04-20T07:38:25Z

pkg/archive/archive.go

 	return Uncompressed
 }

+func zstdDecompress(ctx context.Context, archive io.Reader) (io.ReadCloser, error) {


The original PR (#36754) seems to have some error handling for "compressing" (indicating compression is not supported), and handling of .zstd file-extensions; should we include those changes in this PR as well?

re: zstd extension, this would be for ADD foo.zst. We could add it, though it's not really my intention here.

Ah, yes, forgot that the other one also implemented exporting / saving (and for that used .tar.zst file-extension for the exported file)

thaJeztah · 2020-04-23T18:42:01Z

From a discussion on slack to get an image with this compression;

You can use Skopeo to create images using zstd: skopeo copy --dest-compress-format=zstd docker://alpine oci:alpine-dir

~ $ file alpine-dir/blobs/sha256/*
alpine-dir/blobs/sha256/1c0ce5587a33a2e4e3814a2cf4c7f401947f5c0df4f71d11bd2bcc7447f2136e: JSON data
alpine-dir/blobs/sha256/839e93492e74d48e2405032557ea3c67cce742c2f992409709cf2184a2b0ee88: JSON data
alpine-dir/blobs/sha256/c88dfc0e0cf4364bf3c8a5fe14d65e18a7864abe77eb5cc08590a024c988abae: Zstandard compressed data (v0.8+), Dictionary ID: None

cpuguy83 · 2020-04-29T22:43:17Z

What was the consensus on this for 20.0x?

thaJeztah · 2020-04-30T17:58:31Z

I think we were ok, but let's discuss again in the maintainers meeting to be sure

thaJeztah · 2020-04-30T18:58:24Z

test / document that ADD <file> will now also decompress zstd files
pulling an image with zstd compression and pushing it again will result in a regular (gzip) image

@tonistiigi had some other comments 😅

cpuguy83 · 2020-05-21T18:31:28Z

Closing for now. We'd like to gain zstd support by switching to containerd for image distribution/management.

mrueg · 2020-09-04T23:25:30Z

@cpuguy83 is there an issue for switching to containerd for image management? If so, could this issue be linked here and added to v21.x?

cpuguy83 · 2020-09-05T00:18:38Z

@mrueg Thanks, yep it's on the list :)

tonistiigi · 2020-09-05T00:20:08Z

related containerd/containerd#4263

sjackman · 2021-10-28T20:29:19Z

$ skopeo copy --dest-compress-format=zstd docker://hello-world docker://ghcr.io/sjackman/hello-world-zstd
…
$ skopeo inspect --raw docker://ghcr.io/sjackman/hello-world-zstd | jq -r '.layers[].mediaType'
application/vnd.oci.image.layer.v1.tar+zstd
$ docker pull ghcr.io/sjackman/hello-world-zstd
latest: Pulling from sjackman/hello-world-zstd
91a477ba2178: Extracting  2.531kB/2.531kB
failed to register layer: Error processing tar file(exit status 1): archive/tar: invalid tar header
$ docker --version
Docker version 20.10.8, build 3967b7d

This containerd PR was merged:

Generic layer support containerd/containerd#3649

Docker version 20.10.8 does not yet seem to support zstd compressed layers. Might this feature be included in Docker version 21?

tianon · 2021-10-28T21:17:43Z

See #41759 (and #42862)

sjackman · 2021-10-28T23:42:42Z

Woohoo! Thank you, Tianon!

Unrelated, a neat GitHub hack that I learned recently is that by listing issue numbers in a list, GitHub expands the description of the issues:

cpuguy83 requested a review from dmcgowan April 15, 2020 21:30

cpuguy83 mentioned this pull request Apr 15, 2020

support zstd as archive format #28394

Closed

cpuguy83 force-pushed the add_zstd_support branch from 1cb0e3f to 47ac254 Compare April 16, 2020 16:38

thaJeztah added area/distribution Image Distribution impact/changelog status/2-code-review labels Apr 16, 2020

cpuguy83 changed the title ~~[WIP] Add support for zstd compressed layers~~ Add support for zstd compressed layers Apr 17, 2020

AkihiroSuda approved these changes Apr 18, 2020

View reviewed changes

thaJeztah mentioned this pull request Apr 20, 2020

Formalize support for zstd compression: v1.1.0 ? opencontainers/image-spec#803

Open

thaJeztah requested changes Apr 20, 2020

View reviewed changes

cpuguy83 closed this May 21, 2020

Conversation

cpuguy83 commented Apr 15, 2020

Uh oh!

cpuguy83 commented Apr 15, 2020

Uh oh!

cpuguy83 commented Apr 15, 2020

Uh oh!

AkihiroSuda commented Apr 15, 2020

Uh oh!

thaJeztah commented Apr 15, 2020

Uh oh!

cpuguy83 commented Apr 15, 2020

Uh oh!

thaJeztah commented Apr 15, 2020

Uh oh!

cpuguy83 commented Apr 16, 2020

Uh oh!

thaJeztah commented Apr 17, 2020

Uh oh!

cpuguy83 commented Apr 17, 2020

Uh oh!

cpuguy83 commented Apr 17, 2020

Uh oh!

thaJeztah commented Apr 19, 2020

Uh oh!

cpuguy83 commented Apr 19, 2020

Uh oh!

cpuguy83 commented Apr 19, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Apr 29, 2020

Uh oh!

thaJeztah commented Apr 30, 2020

Uh oh!

thaJeztah commented Apr 30, 2020

Uh oh!

cpuguy83 commented May 21, 2020

Uh oh!

mrueg commented Sep 4, 2020

Uh oh!

cpuguy83 commented Sep 5, 2020

Uh oh!

tonistiigi commented Sep 5, 2020

Uh oh!

sjackman commented Oct 28, 2021

Uh oh!

tianon commented Oct 28, 2021

Uh oh!

sjackman commented Oct 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cpuguy83 Apr 20, 2020 •

edited

Loading

thaJeztah commented Apr 23, 2020 •

edited

Loading