Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to speed up deploy #56

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Conversation

yosifkit
Copy link
Member

Now that we have rate limits under control, we should always try a HEAD request on the indexes & manifests before the PUT request (since the PUT is much slower).

Some local testing for riscv64, arm32v6, and s390x using docker-library/meta@6c3188a shows some significant speed increases. I chose these arches since they were fully pushed and would only do head requests (and my Docker Hub user doesn't have access to push to the arch namespaces anyway).

$ jq -L ../meta-scripts/ 'include "deploy"; arch_tagged_manifests("riscv64") | deploy_objects[]' builds.json > deploy.json
$ time ../meta-scripts/bin/deploy < deploy.json
...
real    0m6.736s
user    0m0.065s
sys     0m0.032s
$ # compared to Jenkins: ~ 2min 49s - 4min 49s

$ jq -L ../meta-scripts/ 'include "deploy"; arch_tagged_manifests("arm32v6") | deploy_objects[]' builds.json > deploy.json
$ time ../meta-scripts/bin/deploy < deploy.json
...
real    0m32.444s
user    0m0.053s
sys     0m0.035s
$ # compared to Jenkins: ~ 5min 16s - 8min 58s

$ jq -L ../meta-scripts/ 'include "deploy"; arch_tagged_manifests("s390x") | deploy_objects[]' builds.json > deploy.json
$ time ../meta-scripts/bin/deploy < deploy.json
...
real    3m12.348s
user    0m0.278s
sys     0m0.252s
$ # compared to Jenkins: ~ 19min 36s - 24min 22s

Always try HEAD request before the PUT request
@yosifkit yosifkit requested a review from tianon as a code owner June 25, 2024 00:19
Copy link
Collaborator

@whalelines whalelines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this only removes the size check for manifests? Are all our requests still properly accounted for when considering pull and abuse limits?

@yosifkit
Copy link
Member Author

yosifkit commented Jun 25, 2024

So this only removes the size check for manifests? Are all our requests still properly accounted for when considering pull and abuse limits?

Yup, the rate limiting is active on every request to Docker Hub using this registry client:

clientOptions.Transport = &rateLimitedRetryingRoundTripper{

@tianon
Copy link
Member

tianon commented Jun 25, 2024

Because of the way that's implemented, it technically applies to requests to auth.docker.io too (any HTTP request that's in the path to our actual lookups gets accounted individually, even including redirects), so it's technically a little tiny bit overaggressive, but I think it's probably better to limit too much than too little. 😅

@tianon tianon merged commit cab9251 into docker-library:main Jun 25, 2024
1 check passed
@tianon tianon deleted the try-for-speed branch June 25, 2024 22:13
@tianon
Copy link
Member

tianon commented Jun 25, 2024

Confirmed, deploy-riscv64 went from ~3m15s down to ~13s 😂

@tianon
Copy link
Member

tianon commented Jun 25, 2024

For unrelated reasons, deploy-arm32v7 has been failing for a little bit, so it had a definitely load-bearing run at ~15m9s with a prior successful run at ~27m, so definitely very net positive. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants