Docker archive auto compression #481
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
This was referenced Jul 14, 2018
mtrmac
force-pushed
the
docker-archive-auto-compression
branch
from
03cb7b4
to
e3dfc8a
Compare
|
@runcom @rhatdan PTAL. Correponding skopeo tests are in containers/skopeo#525 . |
rhatdan
reviewed
Jul 17, 2018
docker/tarfile/src.go
Outdated
| @@ -373,6 +373,12 @@ func (s *Source) GetBlob(ctx context.Context, info types.BlobInfo) (io.ReadClose | |||
| if err != nil { | |||
| return nil, 0, err | |||
| } | |||
| succeeded := false | |||
| defer func() { | |||
| if !succeeded { | |||
There was a problem hiding this comment.
Why not define return err error on the call and then just check if err != nil?
There was a problem hiding this comment.
That’s elegant :) This particular method has another successful exit path (info.Digest == s.configDigest), so a bit risky.
For the same reason, the succeeded variable name is inaccurate, so I’ve at least renamed it to be right.
|
NIT: on error otherwise LGTM |
… paths Signed-off-by: Miloslav Trmač <mitr@redhat.com>
compression.DecompressionFunc now returns a ReadCloser; update all its users. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
For quite a while we were blocked on support xz decompression because it effectively required shelling out to "unxz" (which is just bad in general). However, there is now a library -- github.com/ulikunitz/xz -- which has implemented LZMA decompression in pure Go. It isn't as featureful as liblzma (and only supports 1.0.4 of the specification) but it is an improvement over not supporting xz at all. And since we aren't using its writer implementation, we don't have to worry about sub-par compression. Signed-off-by: Aleksa Sarai <asarai@suse.de> (Updated to return io.ReadCloser) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Most users of DetectCompression will use the decompressorFunc immediately afterwards (the only exception is the copy package). Wrap up this boilerplate in AutoDecompress, so it can be used elsewhere. Signed-off-by: Aleksa Sarai <asarai@suse.de> (Updated primarily to handle closing the compressed stream.) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This matches how "docker load" deals with compressed images, as well as being a general quality-of-life improvement over the previous error messages we'd give. This also necessarily removes the previous special-cased gzip handling, and adds support for auto-decompression for streams as well. Signed-off-by: Aleksa Sarai <asarai@suse.de> (Updated primarily to handle closing the decompressed streams.) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Tools like umoci will always compress layers, and in order to work around some *lovely* issues with DiffIDs, tarfile.GetBlob would always decompress them. However the BlobInfo returned from tarfile.GetBlob would incorrectly give the size of the *compressed* layer because the size caching code didn't actually check the layer size, resulting in "skopeo copy" failing whenever sourcing umoci images. As an aside, for some reason the oci: transport doesn't report errors when the size is wrong... Signed-off-by: Aleksa Sarai <asarai@suse.de> (Updated primarily to handle closing the decompressed stream.) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
force-pushed
the
docker-archive-auto-compression
branch
from
e3dfc8a
to
6353fa5
Compare
|
|
LGTM. Thanks for carrying this @mtrmac. :D |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DO NOT MERGE YET: Completely untested in practice.This is based on #427 , primarily adding the code to
.Close()agzip.Reader; also with a few more tests, and some stylistic touch-ups.