compression: add support for estargz #1351
Conversation
6f35425 to
76e9aa4
Compare
76e9aa4 to
5f14638
Compare
There was a problem hiding this comment.
How large is the added dependency?
Through some lack of foresight and carelessness, we have ended up with various components that really shouldn’t care, like c/image/docker, depending on this subpackage.
At some point, we need to resolve that, either by a significant reimplementation of those dependency chains, or by making the new compression formats optional.
In this case at least, where we don’t need any new implementation to decompress the data, and where compression using this format can only ever happen by an explicit user request (i.e. types.SystemContext.CompressionFormat providing a *compressiontypes.Algorithm), it might make sense to move this into a subpackage. And then, to allow simple implementations of CLIs, we might need a transports/alltransports-like split of the algorithm registry and the algorithm dependencies.
pkg/compression/compression.go
Outdated
| @@ -35,13 +37,17 @@ var ( | |||
| // Zstd:chunked compression. | |||
| ZstdChunked = internal.NewAlgorithm(types.ZstdChunkedAlgorithmName, types.ZstdAlgorithmName, /* Note: InternalUnstableUndocumentedMIMEQuestionMark is not ZstdChunkedAlgorithmName */ | |||
| nil, ZstdDecompressor, compressor.ZstdCompressor) | |||
| // Estargz compression. | |||
| Estargz = internal.NewAlgorithm(types.EstargzChunkedAlgorithmName, types.GzipAlgorithmName, /* Note: InternalUnstableUndocumentedMIMEQuestionMark is not ZstdChunkedAlgorithmName */ | |||
There was a problem hiding this comment.
The comment is wrong.
(Also, please actually define and document the semantics of that field?)
|
@ktock do you think we can move some of the logic to github.com/containerd/stargz-snapshotter? |
|
@giuseppe I think we can move |
pkg/compression/compression.go
Outdated
| defer blob.Close() | ||
|
|
||
| c.metadata["containerd.io/snapshot/stargz/toc.digest"] = string(blob.TOCDigest()) | ||
| c.metadata["io.containers.estargz.uncompressed-size"] = fmt.Sprintf("%v", st.Size()) |
There was a problem hiding this comment.
This needs to be the uncompressed size of the result estargz blob (not the source tar).
pkg/compression/compression.go
Outdated
| return nil, err | ||
| } | ||
| // Unlink immediately the file so we won't leak it. | ||
| os.Remove(file.Name()) |
There was a problem hiding this comment.
Can we return err here?
add support for the estargz compression format from https://github.com/containerd/stargz-snapshotter Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
5f14638 to
9c77027
Compare
|
I've moved the implementation to |
docker/docker_image_src.go
Outdated
| // streams as it would have been done with 206. | ||
| streams := make(chan io.ReadCloser) | ||
| errs := make(chan error) | ||
| go func() { |
There was a problem hiding this comment.
This is becoming rather large (e.g. how far down the “unknown res.StatusCode case is); please split the two streams-producing goroutines into two separate functions.
Unit tests also look worthwhile, at least for those streams producers; without testing actual pulls of this format with high frequency (the way Skopeo and Podman tests trigger at least the default pull paths), we are pretty unlikely to notice breakage.
a65c7b4 to
9c77027
Compare
add support for the estargz compression format from
https://github.com/containerd/stargz-snapshotter
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com