Implement more fine-grained mutexes in registryCache #44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In short, this keeps the "data mutex" as-is, but only acquires it when we have some data to shove in (and only for the limited instructions necessary to update the data), and adds a new mutex per-tag or per-digest to prevent concurrent requests for the exact same upstream content (which is the main reason for this cache in the first place, so feels appropriate).
Without this, our current mutex means that any other efforts to parallelize will ultimately bottleneck on our registry cache.
In order to test this effectively, I've added a `--parallel` flag to `lookup` which just hyper-aggressively runs every single lookup in parallel inside a goroutine.
My first test of this was doing a lookup of the first tag of every DOI repository (so ~147 concurrent lookups in total), and I found it was consistently ~1m57s both with and without this change. Our Hub rate limiter is pegged at ~100/min, which seems consistent with that result. If I increase that limit to ~200/min, I was able to achieve a small speedup with this change (~43s down to ~30s).
In other words, for this to actually be effective as a speedup against Docker Hub if we implement parallel deploy, for example, we'll *also* have to increase our rate limit (which I think is fairly safe now that we handle 429 by explicitly emptying the limiter).
In order to *really* test this effectively, I took a different approach. I spun up a local registry (`docker run -dit -p 127.0.0.1:5000:5000 -p [::1]:5000:5000 --name registry registry`), and copied `hello-world:linux` into it 10,000 times (something like `crane cp hello-world:linux localhost:5000/hello && echo localhost:5000/hello:{1..10000} | xargs -rtn1 -P$(nproc) crane cp localhost:5000/hello` -- could also be done with `jq` and `deploy` if you are ambitious 馃憖).
Then I used `time ./bin/lookup --parallel $(crane ls --full-ref localhost:5000/hello) > /dev/null` to establish some benchmarks.
Without this change, it's pretty consistently in the 15-20s range on my local system. With this change, it drops down an order of magnitude to be in the 3-6s range.
|
Just for giggles, here's the less speedy (but more fun) version of that (I don't know if we'd want to implement |
whalelines
reviewed
May 3, 2024
Correct -- the existing coverage includes all these cases already (and makes our percentages slightly better because the implementations are now longer so the error cases are a less meaningful percentage of overall code 馃檭). |
yosifkit
approved these changes
May 10, 2024
tianon
commented
May 10, 2024
| // TODO this does *not* need to lock the entire cache during the upstream push (but it *would* be good to block pushing to this specific digest) | ||
| rc.mu.Lock() | ||
| defer rc.mu.Unlock() | ||
| // TODO technically we should also be able to safely imply that "fromRepo" has digest here too (assuming MountBlob success), but need to double check whether the contract of the MountBlob API in OCI is such that it's legal for it to return success if "toRepo" already has "digest" (even if "fromRepo" doesn't) |
There was a problem hiding this comment.
At least for Docker Hub, we sure could assume this!
$ crane blob tianon/test@sha256:25be82253336f0b8c4347bc4ecbbcdc85d0e0f118ccf8dc2e119c0a47a0a486e > /dev/null
$ crane blob tianon/scratch@sha256:25be82253336f0b8c4347bc4ecbbcdc85d0e0f118ccf8dc2e119c0a47a0a486e > /dev/null
Error: fetching blob tianon/scratch@sha256:25be82253336f0b8c4347bc4ecbbcdc85d0e0f118ccf8dc2e119c0a47a0a486e: GET https://index.docker.io/v2/tianon/scratch/blobs/sha256:25be82253336f0b8c4347bc4ecbbcdc85d0e0f118ccf8dc2e119c0a47a0a486e: BLOB_UNKNOWN: blob unknown to registry; sha256:25be82253336f0b8c4347bc4ecbbcdc85d0e0f118ccf8dc2e119c0a47a0a486e
$ curl -H "$(crane auth token --header --push --mount tianon/scratch tianon/test)" 'https://registry-1.docker.io/v2/tianon/test/blobs/uploads/?mount=sha256:25be82253336f0b8c4347bc4ecbbcdc85d0e0f118ccf8dc2e119c0a47a0a486e&from=tianon/scratch'
{"errors":[{"code":"BLOB_UPLOAD_UNKNOWN","message":"blob upload unknown to registry","detail":"blob upload unknown"}]}(but whether it's generally safe to assume is not guaranteed by the OCI specification, and in fact the opposite might actually be true -- from is allowed to be optional 馃槃 https://github.com/opencontainers/distribution-spec/blob/v1.1.0/spec.md#mounting-a-blob-from-another-repository)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In short, this keeps the "data mutex" as-is, but only acquires it when we have some data to shove in (and only for the limited instructions necessary to update the data), and adds a new mutex per-tag or per-digest to prevent concurrent requests for the exact same upstream content (which is the main reason for this cache in the first place, so feels appropriate).
Without this, our current mutex means that any other efforts to parallelize will ultimately bottleneck on our registry cache.
In order to test this effectively, I've added a
--parallelflag tolookupwhich just hyper-aggressively runs every single lookup in parallel inside a goroutine.My first test of this was doing a lookup of the first tag of every DOI repository (so ~147 concurrent lookups in total), and I found it was consistently ~1m57s both with and without this change. Our Hub rate limiter is pegged at ~100/min, which seems consistent with that result. If I increase that limit to ~200/min, I was able to achieve a small speedup with this change (~43s down to ~30s).
In other words, for this to actually be effective as a speedup against Docker Hub if we implement parallel deploy, for example, we'll also have to increase our rate limit (which I think is fairly safe now that we handle 429 by explicitly emptying the limiter).
In order to really test this effectively, I took a different approach. I spun up a local registry (
docker run -dit -p 127.0.0.1:5000:5000 -p [::1]:5000:5000 --name registry registry), and copiedhello-world:linuxinto it 10,000 times (something likecrane cp hello-world:linux localhost:5000/hello && echo localhost:5000/hello:{1..10000} | xargs -rtn1 -P$(nproc) crane cp localhost:5000/hello-- could also be done withjqanddeployif you are ambitious 馃憖).Then I used
time ./bin/lookup --parallel $(crane ls --full-ref localhost:5000/hello) > /dev/nullto establish some benchmarks.Without this change, it's pretty consistently in the 15-20s range on my local system. With this change, it drops down an order of magnitude to be in the 3-6s range.