When downloading zstd:chunked image from docker registry backed by s3 bucket, downloading manifest causes downloading the entire layer (effectively the full layer blob is downloaded twice)

[kbr-]

• AWS S3, and also minio, don't support multi-part range queries
• PutBlobPartial function (in storage/storage_dest.go in containers/image) relies on HTTP multi-part range queries to obtain some metadata from the end of the blob. It performs a multi-part range query with 2 ranges (perhaps to get tar-split data and chunk manifest data)

func (s *dockerImageSource) GetBlobAt(ctx context.Context, info types.BlobInfo, chunks []private.ImageSourceChunk) (chan io.ReadCloser, chan error, error) {
	headers := make(map[string][]string)

	rangeVals := make([]string, 0, len(chunks))
	lastFound := false
	for _, c := range chunks {
		if lastFound {
			return nil, nil, fmt.Errorf("internal error: another chunk requested after an util-EOF chunk")
		}
		// If the Length is set to -1, then request anything after the specified offset.
		if c.Length == math.MaxUint64 {
			lastFound = true
			rangeVals = append(rangeVals, fmt.Sprintf("%d-", c.Offset))
		} else {
			rangeVals = append(rangeVals, fmt.Sprintf("%d-%d", c.Offset, c.Offset+c.Length-1))
		}
	}

	headers["Range"] = []string{fmt.Sprintf("bytes=%s", strings.Join(rangeVals, ","))}

• docker registry started against s3/minio returns a 200 response instead of 206
• this causes the library to download the full layer blob (silently, without any warning 👌), just to discard 99.9% of it in order to obtain the metadata

	switch res.StatusCode {
	case http.StatusOK:
		// if the server replied with a 200 status code, convert the full body response to a series of
		// streams as it would have been done with 206.
		streams := make(chan io.ReadCloser)
		errs := make(chan error)
		go splitHTTP200ResponseToPartial(streams, errs, res.Body, chunks)

• then it calculates missing chunks and downloads a large part of the blob again

Possibly related: containers/storage#1928

---

To reproduce, you can use the following docker compose setup.
docker-compose.yml:

networks:
  registry_net:
    driver: bridge

services:
  minio:
    image: minio/minio
    container_name: minio
    ports:
      - "9000:9000"
      - "9001:9001"
    volumes:
      - ./minio-data:/data
    environment:
      - MINIO_ROOT_USER=minioadmin
      - MINIO_ROOT_PASSWORD=minioadmin
    command: server /data --console-address ":9001"
    networks:
      registry_net:
        aliases:
          - minio.localhost

  registry:
    image: registry:2
    container_name: minio-registry
    ports:
      - "5000:5000"
    volumes:
      - ./registry-config.yml:/etc/docker/registry/config.yml
    command: serve /etc/docker/registry/config.yml
    depends_on:
      - minio
    networks:
      - registry_net

registry-config.yml:

version: 0.1
storage:
  s3:
    accesskey: minioadmin
    secretkey: minioadmin
    region: us-east-1
    regionendpoint: http://minio.localhost:9000
    bucket: docker-registry
    secure: false
    rootdirectory: /
  delete:
    enabled: true
http:
  addr: :5000

create the directory ./minio-data
docker compose up, then use minio client mc to create the bucket docker-registry inside minio
(after setting mc alias reg):
mc mb reg/docker-registry

pick any image. Pull it to local storage with buildah. Then push it to the registry while compressing with zstd:chunked:

buildah push --tls-verify=false --compression-format zstd:chunked <image-id> docker://localhost:5000/img

then remove it from local storage since we're gonna pull it next

buildah rmi <image-id>

Now I was testing this with limited bandwidth using tc, then the behavior is visible to the naked eye: when pulling the image, at the end buildah gets "stuck" for a while at 0% (but download is happening behind the scenes, e.g. visible in slurm), that's most likely the part where it's downloading the manifests (actually downloading the whole blob 🥲)

here's the script to limit bandwidth, run it with sudo in the same directory where docker-compose.yml is

#!/bin/bash

# Need to create and start virtual interface first:
# sudo ip link add name ifb0 type ifb
# sudo ip link set dev ifb0 up
#
# virual interface used to turn $interface ingress traffic into egress traffic for ifb0

interface="$(docker compose config --format json | jq .networks.registry_net.name | xargs -I {} docker network ls -q --filter name={} --format br-\{\{.ID\}\})"

echo "Interface used by docker compose: $interface"

download_limit=100mbps

tc qdisc del dev "$interface" ingress 2>/dev/null
tc qdisc del dev "ifb0" root 2>/dev/null

# It's not possible to shape ingress, only police it
# So instead we reroute all ingress traffic into virtual interface "ifb0" on which it becomes egress
tc qdisc add dev "$interface" ingress
tc filter add dev "$interface" parent ffff: protocol ip u32 match u32 0 0 action mirred egress redirect dev ifb0

tc qdisc add dev ifb0 root handle 1: htb default 10
tc class add dev ifb0 parent 1: classid 1:1 htb rate "$download_limit"
tc class add dev ifb0 parent 1:1 classid 1:10 htb rate "$download_limit"

echo To remove: tc qdisc del dev "$interface" ingress
echo To remove: tc qdisc del dev "ifb0" root

(note that script requires ifb0 virtual interface to exist, see comment inside the script)
for my tests I was limiting to 100mbps and using ~700MB image (compressed in the registry)

$ skopeo inspect --tls-verify=false docker://localhost:5000/img --raw | jq -r '([.layers.[].size] | add)/1024/1024'
691.4367246627808

adjust the limits appropriately based on size of the image you're using for testing.

Now pull (also remember to set enable_partial_images to true)

time ~/go/bin/buildah --log-level trace --tls-verify=false pull docker://localhost:5000/img

with vanilla buildah this took for me:

real    0m17.138s
user    0m12.764s
sys     0m9.426s

now we can apply a workaround which translates multi-part range requests into a sequence of single-part range request (courtesy of @psarna):

diff --git a/vendor/github.com/containers/image/v5/storage/storage_dest.go b/vendor/github.com/containers/image/v5/storage/storage_dest.go
index 0af4523ff..360273633 100644
--- a/vendor/github.com/containers/image/v5/storage/storage_dest.go
+++ b/vendor/github.com/containers/image/v5/storage/storage_dest.go
@@ -323,20 +323,32 @@ type zstdFetcher struct {
 
 // GetBlobAt converts from chunked.GetBlobAt to BlobChunkAccessor.GetBlobAt.
 func (f *zstdFetcher) GetBlobAt(chunks []chunked.ImageSourceChunk) (chan io.ReadCloser, chan error, error) {
-       newChunks := make([]private.ImageSourceChunk, 0, len(chunks))
-       for _, v := range chunks {
-               i := private.ImageSourceChunk{
-                       Offset: v.Offset,
-                       Length: v.Length,
+       errs := make(chan error)
+       rc := make(chan io.ReadCloser)
+       logrus.Debug("chunks: ", chunks)
+       go func() {
+               for _, v := range chunks {
+                       i := private.ImageSourceChunk{
+                               Offset: v.Offset,
+                               Length: v.Length,
+                       }
+                       logrus.Debug("special careful single GetBlobAt to make docker registry work: ", i, " ", f.blobInfo)
+                       blobRc, blobErrs, blobErr := f.chunkAccessor.GetBlobAt(f.ctx, f.blobInfo, []private.ImageSourceChunk{i})
+                       if blobErr != nil {
+                               errs <- blobErr
+                       }
+                       // channels transplant
+                       for r := range blobRc {
+                               rc <- r
+                       }
+                       for e := range blobErrs {
+                               errs <- e
+                       }
                }
-               newChunks = append(newChunks, i)
-       }
-       rc, errs, err := f.chunkAccessor.GetBlobAt(f.ctx, f.blobInfo, newChunks)
-       if _, ok := err.(private.BadPartialRequestError); ok {
-               err = chunked.ErrBadRequest{}
-       }
-       return rc, errs, err
-
+               close(rc)
+               close(errs)
+       }()
+       return rc, errs, nil
 }
 
 // PutBlobPartial attempts to create a blob using the data that is already present

redo the experiment. I saw:

real    0m9.759s
user    0m12.078s
sys     0m8.066s

yay, 2x speedup

[kbr-]

Regarding S3 not supporting multi-part range requests
it's mentioned here: https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html

and for minio, they decided not to implement it until AWS S3 supports it 🫠
minio/minio#5285

[kbr-]

Note: in my experiment, for some reason buildah actually in the end pulled the layer using full pull, because it claimed there's no tar-split metadata (even though there is according to skopeo) -- I might open another issue about that

we still paid for the full blob when getting the manifest though...

I wonder if the bug above won't cause full blob downloads later down the line too (when we're actually pulling chunks), causing even larger slowdown...

[kbr-]

Note that this workaround doesn't give a great performance, the chunks are being pulled sequentially, with HTTP GET in-between each chunk. For layers with many chunks this will not utilize bandwidth well. A better strategy in complete fix would be to issue the GETs with certain concurrency.

[mtrmac]

Thanks for your report!

Cc: @giuseppe

• In the initial metadata-only request, we clearly don’t want to accept a HTTP 200 response with all data.
• If we managed to fetch only the right ranges for the metadata, the subsequent partial pull of data would almost certainly be multi-part, and thus also result in a HTTP 200. In that case the effect might not be that much of a multiple compared to only returning the requested ranges, but, anyway, there would be no benefit to doing a partial pull; a full pull would be only a single HTTP 200, and with a simpler/faster implementation.
• Issuing the range requests one at a time might work; the trouble with that is that I don’t know of a way to tell that a registry which returns 200 on a multi-range request would actually handle a single-item range request. I guess we would need the user to opt in? That’s plausible in principle but I wonder how many users would ever realize they might need that.

So I’m inclined to just fail: if we see a 200 on the initial metadata chunk request, we should fall back to a full pull. (Reissuing the HTTP GET, and closing the connection for the initial one, but that’s hopefully not too bad. Although, with the pipelined Go HTTP stack, we might need to do something a bit non-obvious to ensure the request body is not fully read.)

OTOH once we start a partial pull, if we see a 200 when pulling content chunks, we should just go with it; we don’t have a way to abort and revert to a full pull anyway, and the existing content chunk loader already has a loop trying to merge range requests, ultimately into a single range over all required data.

So, the c/storage chunked.ImageSourceSeekable interface would need to be extended replaced to let the caller specify whether to fail on a HTTP 200. … and if we are changing/replacing that interface, we might as well modify the new GetBlobAt to return a single chan streamOrErr, the way c/storage is currently converting in chunked.getBlobAt, and avoid the getBlobAtConverterGoroutine; we can leave that in only for backward compatibility.

[mtrmac]

I guess we would need the user to opt in?

Also, even if we did add this option, we would still need to implement a default behavior for users who do nothing = fall back to full pull. So that needs to be built either way.

[giuseppe]

the partial pull code path has some other advantages though. For example the storage deduplication part, and when convert_images is set, we need to force this code path anyway.

If the server returns a 200 status for the first request, we should probably store the result somewhere and always refer to that file for the partial pull.

@mtrmac what do you think?

[kbr-]

Just accepting the 200 and fetching the entire blob is throwing the baby out with bathwater, I think.

S3 backend supports single-part range requests and I think every sensible backend does.

So if the backend doesn't support multi-part range request, the code could fallback to use a set of single-part range requests. To achieve good performance and utilize network bandwidth, the range requests would have to be pipelined (send request for next N ranges while we're still fetching data from current range), but that shouldn't be so complex.

Issuing the range requests one at a time might work; the trouble with that is that I don’t know of a way to tell that a registry which returns 200 on a multi-range request would actually handle a single-item range request.

But it's easy to check -- just send the single-part range request and check if it returns 206.
If neither multi-part range nor single-part range request return 206, that's when we can fallback to full pull.

[giuseppe]

single part is too expensive. It would make sense only if there are just a few files changed so we are limited to few requests; otherwise it is a lot of overhead and no clear benefits

[mtrmac]

the partial pull code path has some other advantages though. For example the storage deduplication part, and when convert_images is set, we need to force this code path anyway.

(compare containers/storage#2180 .)

If the server returns a 200 status for the first request, we should probably store the result somewhere and always refer to that file for the partial pull.

Locally: That sounds reasonable — if we fall back to a full pull, we do a full on-disk copy in putBlobToPendingFile anyway. We’d need some kind of c/image + c/storage interaction to manage the file and its deletion on errors/aborts/unexpected reboots, I don’t immediately have an API structure in mind, but I’m sure that’s solvable.

More generally: I’m a bit unhappy how different the partial pull and full pull code paths are, and I’d like them to converge over time. (See also today’s conversation about creating layers outside of locks — we should have an equivalent of PrepareStagedLayer for full pulls.) We probably don’t want to solve all of this just for this issue, but if it happened to be easy, great.

(Completely hypothetically, it would be elegant if we had the same code path store the full file on disk, and then we decided whether to process it as a full pull or partial, from the same file. But that’s almost certainly too much complexity just for style points.)

---

single part is too expensive. It would make sense only if there are just a few files changed so we are limited to few requests; otherwise it is a lot of overhead and no clear benefits

I’d be curious to see a benchmark. Intuitively, I’d expect that a single request with 500 ranges, getting a 500-item response, or 500 pipelined requests sent over a single connection, getting 500 HTTP pipelined responses, would principally differ only in the 500*(request+response header) extra size, and a constant number of extra round-trips, so it might be still worthwhile to do zstd:chunked.

But, also, I wouldn’t be at all surprised to hear that the Go standard library client and/or the servers don’t implement it that way, and that it doesn’t work.

(I’m rather more skeptical that if we added an option to do one-range request, that users would opt in. Would we need a heuristic, then? That would get messy.)

[giuseppe]

I’d be curious to see a benchmark. Intuitively, I’d expect that a single request with 500 ranges, getting a 500-item response, or 500 pipelined requests sent over a single connection, getting 500 HTTP pipelined responses, would principally differ only in the 500*(request+response header) extra size, and a constant number of extra round-trips, so it might be still worthwhile to do zstd:chunked.

But, also, I wouldn’t be at all surprised to hear that the Go standard library client and/or the servers don’t implement it that way, and that it doesn’t work.

(I’m rather more skeptical that if we added an option to do one-range request, that users would opt in. Would we need a heuristic, then? That would get messy.)

the biggest problem are the too many roundtrips. It will make the pull extremely slow (it is similar to how ostree works, without deltas) as we need a request+response for each chunk

[kbr-]

single part is too expensive. It would make sense only if there are just a few files changed so we are limited to few requests; otherwise it is a lot of overhead and no clear benefits

Why is it expensive?

Suppose you have a 1GB layer and you can avoid downloading half of it if you use range requests, even single-part requests, potentially many of them. It still seems like a win.

Of course to make any absolute statements tests and benchmarks would be required. Even the most basic benchmarks can show interesting stuff and destroy our assumptions.

BTW Are there any publicly available benchmarks for containers/ libraries? Or some data from performance testing? For example I read about partial pulls on this post: https://www.redhat.com/en/blog/faster-container-image-pulls which claims that they are faster, but the post provides no data, no graphs, no reproducible experiment that I could perform and see for myself. And it took me maybe 1 day of creating and running a very basic benchmarking setup to notice that we're downloading full layer blobs twice when using partial pulls, and open this issue. (And I didn't even know that this library existed a week ago.)

For my use case performance is critical. We're preparing thousands of containers every second.

[mtrmac]

We don’t run benchmarks in CI against any goal, at least. Most things I have seen are one-offs.

[kbr-]

the biggest problem are the too many roundtrips. It will make the pull extremely slow (it is similar to how ostree works, without deltas) as we need a request+response for each chunk

Will that be a problem even if we do the roundtrips concurrently?

[giuseppe]

the biggest problem are the too many roundtrips. It will make the pull extremely slow (it is similar to how ostree works, without deltas) as we need a request+response for each chunk

Will that be a problem even if we do the roundtrips concurrently?

podman can deal with that, but I am not sure the registry will be very happy to see so many requests from the same client at the same time