exporter: support creating blobs with zstd compression#2344
exporter: support creating blobs with zstd compression#2344AkihiroSuda merged 2 commits intomoby:masterfrom
Conversation
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
ktock
left a comment
There was a problem hiding this comment.
TestGetRemote in manager_test.go needs to be updated to also create zstd.
| ) | ||
|
|
||
| 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. |
ktock
left a comment
There was a problem hiding this comment.
Thank you for pointing the issue. I'll work on estargz decompressor that provides the original tar contents.
zstd changes LGTM.
| 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