Support base image layer compressed with zstd #3717
Conversation
| @@ -117,7 +150,7 @@ public void testWriteUncompressed() throws IOException { | |||
|
|
|||
| CachedLayer cachedLayer = cacheStorageWriter.writeUncompressed(uncompressedLayerBlob, selector); | |||
|
|
|||
| verifyCachedLayer(cachedLayer, uncompressedLayerBlob); | |||
| verifyCachedLayer(cachedLayer, uncompressedLayerBlob, compress(uncompressedLayerBlob)); | |||
There was a problem hiding this comment.
The test is about uncompressed layers, but I noticed the verifyCachedLayer actually uses compression for asserts.
What is even more strange is that this test seems dependant on the gzip compression settings: it passes with JDK GZIPOutputStream, but not with common-compress GzipCompressorOutputStream.
There was a problem hiding this comment.
Seems like a convenience use... @chanseokoh you had a comment in #1678 saying it's a common but unnecessary pattern to "wrap anything in a Blob only for the sake of writing it to an output stream or compute a digest". Do you recall if that convenience shortcut is the reason verifyCachedLayer happens to compress the Blob before getting a descrptor out of it?
There was a problem hiding this comment.
I apologize for the late response. Jib's general approach was to minimize the number of external dependencies, unless they are absolutely needed. Supporting zstd is useful, though, so there is a balance there. What are the alternatives? Apache Commons compress seems to have something.
|
No worries. Apache Commons compress does rely on optional dependency zstd-jni (https://commons.apache.org/proper/commons-compress/examples.html#Zstandard, https://github.com/apache/commons-compress/blob/aeb1defce5676b5c15a1dc377960fdcf119cddb4/pom.xml#L120 ), which is a binding around the reference C implementation. There is also a pure Java port listed on https://facebook.github.io/zstd/#other-languages, but I don't know its status. Checking the other implementations, moby and podman use the Golang port https://pkg.go.dev/github.com/klauspost/compress/zstd#section-readme (https://github.com/giuseppe/docker/blob/e187eb2bb5f0c3f899fe643e95d1af8c57e89a73/pkg/archive/archive.go#L25 and https://github.com/containers/podman/blob/67e7b2d6e3788c3560a852156a3505f0e2b6c8be/vendor/modules.txt#L485) A short-term option could be to keep the transitive dependency to zstd-jni as optional in jib-core (i.e. test scope), and ask users to add it as a direct dependency if they have to deal with "application/vnd.oci.image.layer.v1.tar+zstd" layers, and re-assess when these layers become widely used... |
|
Hi,
From what I understand Red Hat plans (in podman) are to enable it by default, when Docker 22.06 will be out (first non beta docker release that will support zstd layers) and spread enough among the different installations. Cheers, |
rquinio
force-pushed
the
feature/zstd-decompression
branch
2 times, most recently
from
9ba5b87
to
ee6a8e6
Compare
|
Ok, I've tried the Gradle feature, which I understand will translate to an optional dependency in Maven pom. |
There was a problem hiding this comment.
Functionality looks perfect; great FAQ entry -- just do something about the test coverage.
By the way, this was an interesting anecdata point in favor of zstd getting adopted quickly: https://twitter.com/adrianco/status/1560854827810361345
| @@ -117,7 +150,7 @@ public void testWriteUncompressed() throws IOException { | |||
|
|
|||
| CachedLayer cachedLayer = cacheStorageWriter.writeUncompressed(uncompressedLayerBlob, selector); | |||
|
|
|||
| verifyCachedLayer(cachedLayer, uncompressedLayerBlob); | |||
| verifyCachedLayer(cachedLayer, uncompressedLayerBlob, compress(uncompressedLayerBlob)); | |||
There was a problem hiding this comment.
Seems like a convenience use... @chanseokoh you had a comment in #1678 saying it's a common but unnecessary pattern to "wrap anything in a Blob only for the sake of writing it to an output stream or compute a digest". Do you recall if that convenience shortcut is the reason verifyCachedLayer happens to compress the Blob before getting a descrptor out of it?
- Use commons-compress automatic algorithm detection via magic-bytes - Use optional dependency zstd-jni from commons-compress to support application/vnd.oci.image.layer.v1.tar+zstd layers
rquinio
force-pushed
the
feature/zstd-decompression
branch
from
ee6a8e6
to
0e20258
Compare
|
To reach 80% on 6 lines changed I had to cover the 2 lines dealing with exception ^^ Should now be 100% |
|
Yeah, Sonar gets really weird on small changes. |
|
Kudos, SonarCloud Quality Gate passed! |
This is a proposal to resolve #3714
Changes:
Before filing a pull request, make sure to do the following:
This helps to reduce the chance of having a pull request rejected.