exporter: support creating blobs with zstd compression #2344
Conversation
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
| // UnknownCompression means not supported yet. | ||
| UnknownCompression Type = -1 | ||
| ) | ||
|
|
||
| const ( | ||
| mediaTypeDockerSchema2LayerZstd = images.MediaTypeDockerSchema2Layer + ".zstd" |
There was a problem hiding this comment.
Do we need to support application/vnd.docker.image.rootfs.diff.tar.zstd? Maybe we can forcibly turn on oci-mediatype=true when zstd.
There was a problem hiding this comment.
IIUC no application/vnd.docker.image.rootfs.diff.tar.zstd in https://github.com/distribution/distribution/blob/v2.7.1/docs/spec/manifest-v2-2.md#media-types ?
There was a problem hiding this comment.
I think this can provide more compatibility with registries. Not hard to support it and impossible for the spec to define something incompatible.
There was a problem hiding this comment.
I'd prefer to keep this OCI-only.
I guess non-OCI compliant registries are unlikely to support MediaTypeDockerSchema2Layer + ".zstd" too?
There was a problem hiding this comment.
Docker hub seems to support the docker mediatypes without any changes. I would guess at least Moby should always support them as well (after general zstd update).
There was a problem hiding this comment.
fwiw, zstd is not part of any standard atm. Even OCI has never released it. I guess if distribution/distribution would ever add this mediatype (I'm not sure if they are interested in maintaining this part) it wouldn't be before docker engine actually supports it.
Estargz support has been removed from this test as implementation does not guarantee digest stability and only reason it passed were the exceptions in the test via variant map that ignored cases where timing resulted the digest to go wrong. This needs to be addressed in the follow up if we want to keep estargz support. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
I updated the test but found a very problematic issue. I never realized that estagz messes up the uncompressed digest for a reference. That is a huge problem as these digests are meant to be stable for the ref and image. |
| @@ -1160,7 +1166,7 @@ func TestGetRemote(t *testing.T) { | |||
| eg, egctx := errgroup.WithContext(ctx) | |||
| for _, ir := range refs { | |||
| ir := ir.(*immutableRef) | |||
| for _, compressionType := range []compression.Type{compression.Uncompressed, compression.Gzip, compression.EStargz} { | |||
| for _, compressionType := range []compression.Type{compression.Uncompressed, compression.Gzip /*compression.EStargz,*/, compression.Zstd} { | |||
There was a problem hiding this comment.
Is EStargz intentionally omitted here? Could you add a comment line if so?
There was a problem hiding this comment.
Yes, see #2344 (comment) and the commit message. Also have "disabled" message in code (not on that line). In current form, it is a release blocker for v0.10 for me.
@ktock @fuweid
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com