[s3 cache] Fix upload of downloaded layer

[bpaquet]

see #5584.

Seems this is a regression related to #4551, which happen when buildkit need to export a S3 layer directly from a downloaded S3 layer.

With the new wrapper, there is no exception, and buildkit download and re upload them without any issue. Checksum and size of layers are identical.

[bpaquet]

@tonistiigi I'm not sure why buildkit need to download the layers a second time.
Layers are downloaded to be loaded into the docker daemon, but they are downloaded again to be uploaded to the s3 target cache. Is it expected?

[tonistiigi]

Layers are downloaded to be loaded into the docker daemon, but they are downloaded again to be uploaded to the s3 target cache. Is it expected?

If the layers exist already in the same cache location, then they should not be uploaded. It should check that the layer is already available at the target and skip it(including skip downloading it). This is how it works in the registry backend, for example, I'm not sure about s3.

[bpaquet]

Layers are downloaded to be loaded into the docker daemon, but they are downloaded again to be uploaded to the s3 target cache. Is it expected?

If the layers exist already in the same cache location, then they should not be uploaded. It should check that the layer is already available at the target and skip it(including skip downloading it). This is how it works in the registry backend, for example, I'm not sure about s3.

No my question was not clear (and I confirm that the s3 remote cache will not upload the layers if they already exists).

To reproduce the issue #5584., we launch a build which

• start from a fresh builder
• load the image to docker, so download the cache from S3 (expected)
• export the cache to another directory on S3. Yes, we can probably do a simple CopyObject in this case, but this is not really the question here.
  The question is why the export triggers a second download on s3? The layers were already downloaded to be loaded into docker.

[tonistiigi]

If you export cache with mode=max (this may be the same for existing layers also in mode=min) then more cache is exporter than your build result. If you get a cache match for your result only the layers required for it are downloaded, not everything that could be part of the cache. The most typical in here would be a cache for an intermediate stage in multi-stage builds.

Another cases that you may be seeing is 1) layers remaining lazy on initial cache match because nothing accesses their contents (I don't think this is the case if you say image was loaded to docker) 2) something specific to s3 if you are seeing the same layer being downloaded again.

[bpaquet]

If you export cache with mode=max (this may be the same for existing layers also in mode=min) then more cache is exporter than your build result. If you get a cache match for your result only the layers required for it are downloaded, not everything that could be part of the cache. The most typical in here would be a cache for an intermediate stage in multi-stage builds.

Another cases that you may be seeing is 1) layers remaining lazy on initial cache match because nothing accesses their contents (I don't think this is the case if you say image was loaded to docker) 2) something specific to s3 if you are seeing the same layer being downloaded again.

@tonistiigi: Moved this side conversation here: #5598, with a full reproduction on remote cache registry.

Can you review this PR? Thx

[tonistiigi]

Can you explain this change more(or add comments)? On first look it isn't entirely obvious what the difference is between this custom Read implementation and the io.SectionReader it is replacing.

[tonistiigi]

Is the issue that the ReaderAt coming from dgstPair.Provider.ReaderAt is not really a ReaderAt and can only expect specific offsets. And using SectionReader exposes the Seek function that doesn't work? If so, can this be fixed from the ReaderAt side, and if not, then I guess a smaller option than this custom Read function would be to wrap SectionReader into struct so the Seek method is not available anymore.