services/content: fix reading a blob which is smaller than the read buffer#2221
Conversation
AkihiroSuda
force-pushed
the
content-small-blob
branch
2 times, most recently
from
c677e80 to
b02850f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2221 +/- ##
==========================================
+ Coverage 41.03% 41.14% +0.11%
==========================================
Files 66 66
Lines 7755 7755
==========================================
+ Hits 3182 3191 +9
+ Misses 4071 4060 -11
- Partials 502 504 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Why is this test different than all the other checks? Does this need to be exported?
Please follow the existing conventions:
- This does not need a new file. Please put with the other checks.
- Don't export the identifiers.
There was a problem hiding this comment.
Exposed tests inprovideringester.go is expected to be used for a type that implements ProviderIngester but does not fully implement content.Store interface.
Updated the code comment for clarity.
There was a problem hiding this comment.
You can read this in https://github.com/containerd/containerd/blob/master/api/services/content/v1/content.proto#L143. It is the total size of the read.
There was a problem hiding this comment.
How can that read be satisfied? If I ask for 10 bytes and you only provide 5 bytes, we can fulfill the request. The size needs to be correct to 5 bytes. Are you sure there isn't a bug in the readerAtReader?
There was a problem hiding this comment.
Updated the code to let size = oi.Size - offset when offset+size > oi.Size.
Are you sure there isn't a bug in the readerAtReader?
Please refer to the below comment
|
@AkihiroSuda I think the problem with the client code is that it is trying to read more than exists. If you correctly use If you checkout the implementation in https://golang.org/src/io/io.go?s=15311:15368#L471, you can see that the buffer is sliced to the correct size. |
There was a problem hiding this comment.
I suspect this test will pass if you use a SectionReader.
There was a problem hiding this comment.
Even with SectionReader, it fails unless the exact blob size is passed.
…uffer.
The newly added test fails without this fix in services/content/service.go:
$ go test -c . && sudo ./containerd.test -test.v -test.root -test.run TestContentClient
...
--- FAIL: TestContentClient/SmallBlob (0.02s)
provideringester.go:62: rpc error: code = OutOfRange desc = read
past object length 6 bytes
helpers.go:67: drwx------ 4096
/tmp/content-suite-ContentClient-286788688
FAIL
Signed-off-by: Akihiro Suda <[email protected]>
AkihiroSuda
force-pushed
the
content-small-blob
branch
from
b02850f to
7b323b1
Compare
|
LGTM I pushed another commit to fix the tests. |
There was a problem hiding this comment.
This comment no longer applies
Signed-off-by: Stephen J Day <[email protected]>
stevvooe
force-pushed
the
content-small-blob
branch
from
c8a0fc5 to
a76f230
Compare
|
LGTM I am going to open up another issue to fix the remote reader at. In reviewing this we did figure out that the reader at interface should be returning the number of bytes read AND an EOF in this case where more bytes are requested than exists. |
4 participants
The newly added test fails without this fix in services/content/service.go:
Signed-off-by: Akihiro Suda [email protected]
Split from #2135