S3 Cache: Remove custom readerAt to use the standard one #3993
Conversation
Signed-off-by: Bertrand Paquet <[email protected]>
bpaquet
force-pushed
the
remove_s3_reader_at
branch
from
428a522 to
cbc5e3a
Compare
| @@ -400,7 +392,8 @@ func (s3Client *s3Client) getManifest(ctx context.Context, key string, config *v | |||
| return true, nil | |||
| } | |||
|
|
|||
| func (s3Client *s3Client) getReader(ctx context.Context, key string) (io.ReadCloser, error) { | |||
| func (s3Client *s3Client) Fetch(ctx context.Context, desc ocispecs.Descriptor) (io.ReadCloser, error) { | |||
There was a problem hiding this comment.
In order for FromFetcher to be efficient, this io.ReadCloser needs to support either ReadAt or io.Seeker as well. If it does support it then better to make it clear in the exported type.
There was a problem hiding this comment.
Hmm. I dig around on this interesting question :)
- In integration test, the
io.ReadCloserdoes not supportio.Seeker. Seems the AWS SDK is using https://go.dev/src/net/http/transport.go?h=bodyEOFSignal. - The current code is buggy. https://github.com/moby/buildkit/blob/master/cache/remotecache/s3/s3.go#L460. The offset parameter is not used ...
- Nobody complains, meaning it's working anyway :) Even if in production the reader is not an
http.bodyEOFSignal.
If my analysis is right, I see multiple solutions
- Fix the actual code by handling the offset properly, and keep a dedicated
ReaderAt - Go forward with my PR. The line https://github.com/moby/buildkit/blob/master/util/contentutil/fetcher.go#L51 will throw an error if the offset sent as parameter is not the good one.
WDYT? I'm more in favor of 1/, even if I feel we have some duplication in the code base.
There was a problem hiding this comment.
Nobody complains, meaning it's working anyway :)
This would not matter in the default case. Starting from the previous offset would only be required for resuming a previous incomplete pull etc.
If my analysis is right, I see multiple solutions
We should do a better solution than throwing an error for the resuming case. Either by fixing the old ReaderAt or implementing/type-checking Seeker in the new variant.
bpaquet
force-pushed
the
remove_s3_reader_at
branch
4 times, most recently
from
7c44356 to
60e8e24
Compare
Signed-off-by: Bertrand Paquet <[email protected]>
bpaquet
force-pushed
the
remove_s3_reader_at
branch
from
60e8e24 to
070a226
Compare
|
@tonistiigi done :) |
|
@tonistiigi ping :) |
Tested with
TESTFLAGS="-run TestIntegration/TestBasicS3CacheImportExport/worker=containerd-rootles"