registry/storagedriver S3 Walk optimization

[CollinShoop]

Upstream PR #3480

Objective

blobstore enumeration with S3 storage driver (and possibly others with follow up effort) can be optimized by several orders of magnitude in most cases by offloading more work to the S3 API. In some cases this gives identical performance but in extreme cases, eg thousands of blobs in separate folders, this gives a huge performance boost.

Changes

• Use ListObjectsV2PagesWithContext without Delimiter, giving all objects of subpaths in batches up to 1000
• Infer directories (no longer listed without Delimiter & recursive implementation) by comparing subsequent object paths of different subdirectories
• Keep track of skipped directory and manually skip over any objects under that directory. Note: I acknowledge this could bel less efficient in some extreme cases. The usage patterns in the context of registry should be such that we get an overall performance increase in all cases, at least that I'm aware of.
• Added tests for S3 Walk implementation
• Added tests for fallback Walk implementation

Bug Fix
While testing, I noticed that WalkFallback does not handle ErrSkipDir as documented for non-directory.

• Expected: WalkFallback should stop when ErrSkipDir is returned for a non-directory, as documented WalkFallback
• Actual: WalkFallback handles ErrSkipDir for non-directory by skipping the file and does not stop. This is tested with the added case TestWalkFallback/stop early

Run S3 Tests

#export REGION_ENDPOINT=sfo3.digitaloceanspaces.com
export AWS_REGION=us-east-1
export AWS_ACCESS_KEY=<key>
export AWS_SECRET_KEY=<secret>
export S3_BUCKET=<bucket>
go test -run TestWalk -v ./registry/storage/driver/s3-aws/...

Performance

On a few test registries, I performed a rough benchmark using BlobEnumerator::enumerate twice: Once before making these chances & again with the changes. I used a few local changes to keep track of the number of objects / folders enumerated and API calls made.

Test 1 (medium) ~300 blobs

• Before
  • Took 53.9 seconds
  • 479 API calls
• After
  • Took 0.128 seconds
  • 1 API call

Results

• Reduce runtime by factor of 421
• Reduce API calls by a factor of 479

Test 2 (large) ~50k blobs
Only the first 5 minutes of Walk are recorded and extrapolated, which I think is fair to get the point across

• Before - partial results
  • Took 5 minutes
  • 2680 API calls
• Before - extrapolated (x18)
  • Took 89.74 minutes
  • 48100 API calls
• After
  • Took 9.85 seconds
  • 49 API call

Results

• Reduce runtime by factor of 546
• Reduce API calls by a factor of 981.

[waynr]

Looks good to me, other than the nit about explaining the principle of the new doWalk implementation.

[CollinShoop]

Finding through further testing, the current impl does not work for scan repositories so addressing that now.

[waynr]

Are you familiar with the filepath package? Specifically, filepath.Rel. Along with the the filepath.SplitList subcommand we should be able to simplify the loop/logic constructing parents and eliminate the sort.Sort by looping over the split directory names to construct the parents one-by-one. Not totally clear to me if that would make this function overall more efficient, but if it so it should be worth the effort since it looks like we run this for every filepath we see.

[CollinShoop]

Will take a look at that 👍🏻

[CollinShoop]

I'm not sure how filepath.Rel and SplitList would help here, though I can replace sort.Sort with something to reverse the list ordering, as the way this is done generates a list in reverse order compared to how we want them to be walked. If you have an idea of an alternate implementation, do you mind writing it up?

[CollinShoop]

Using a simple reverse function now and all the unit tests still pass 👍🏻

[waynr]

I'm not sure how filepath.Rel and SplitList would help here

The main benefit in my view would be improved readability and remove the need for the sort (or reverse now i guess).

[adamwg]

This looks great, though it took me a couple of read-throughs (and some S3 doc reading) to understand the correctness. I've left a couple of comment suggestions that would help make it more obvious for the reader.