fixbug: blob for schemav1 could be uncompressed#2263
fixbug: blob for schemav1 could be uncompressed#2263estesp merged 1 commit intocontainerd:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2263 +/- ##
==========================================
+ Coverage 41.26% 44.98% +3.72%
==========================================
Files 66 92 +26
Lines 7850 9400 +1550
==========================================
+ Hits 3239 4229 +990
- Misses 4103 4488 +385
- Partials 508 683 +175
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
log.G(ctx).WithError(err).Errorf("failed to fetch blob %v", desc.Digest)
|
For schema1, uncompressed blobs wasn't really ever well supported, but it does work in docker. I would say this change is fine as long as the following steps are taken:
cc @dmcgowan |
|
Change looks reasonable, please use the decompression code in |
|
@stevvooe @dmcgowan thanks for your advice, i have changed and commit as your suggestion, PTAL. I donn't real know what does this suggestion mean "Once the compression is detected, the converted manifest should reflect that in the media types. In this case, the gzip needs to be dropped." , if this step is a must ,please give me more tips, appreciate your help. |
The issue is that after the conversion takes place, a manifest is created which has layers described as having the media type |
|
@dmcgowan thanks very much for your patient explanation, and i change the media type of layer from |
There was a problem hiding this comment.
This function should return Compression. Maybe a signature like GetCompression(r. io.Reader) Compression
|
@dmcgowan thanks for you review, I have change code according to your suggestion. |
There was a problem hiding this comment.
nit: break the comment into 2 lines. Also instead of the reader r -> the given reader, the variable name is not relevant to the caller.
There was a problem hiding this comment.
Does the caller not properly handle or log this error?
There was a problem hiding this comment.
you are right, the caller will handle this error, I'll get rid of this log
There was a problem hiding this comment.
Use WithField here, also the message could be more descriptive
log.G(ctx).WithField("d", desc.Digest).Debuf("changed media type for uncompressed schema1 layer blob")
|
A few nits, otherwise LGTM |
|
@dmcgowan appreciate your advice, all the nits you mentioned has been fixed. |
There was a problem hiding this comment.
What if instead readCloseWrapper stored the Compression that was used? Otherwise this will have to be updated in tandem with DecompressStream (Assuming at some point DecompressStream supports more than gzip).
|
@cpuguy83 the design you suggested is more elegant, I have change implement according to your suggestion, PTAL |
|
I think CI fail isn't because of the new commit, is a url cannot be connected. |
Signed-off-by: frank yang <[email protected]>
|
@cpuguy83 PTAL? |
|
LGTM |
7 participants
blob for schemav1 could be uncompressed, which can be pulled by docker but cannot pull by ctr, because when docker downloading a blob layer it will check if it is compressed or not.
in our company we have a image registry which use schemaV1 and has uncompressed layers, these layers can be download by docker but cannot download by ctr, after dig a while i found that maybe we should check the downloading stream in advance, and if it is uncompressed we don't use the gzip reader to decompress it.
PTAL, and thanks.
Signed-off-by: frank yang [email protected]