Rate limit crane.Copy() operations #771
Conversation
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
the
kind/feature
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: puerco The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
area/artifacts
|
🤔 |
internal/promoter/image/sign.go
Outdated
| @@ -96,6 +103,7 @@ func (di *DefaultPromoterImplementation) CopySignatures( | |||
| ) error { | |||
| // Cycle all signedEdges to copy them | |||
| logrus.Infof("Precopying %d previous signatures", len(signedEdges)) | |||
| rtripper := ratelimit.NewRoundTripper(rateLimitEvents) | |||
There was a problem hiding this comment.
This object is thread safe, maybe a global until we can more easily plumb through a single shared instance? (So we can ensure the rate limit is maintained throughout)
There was a problem hiding this comment.
I was just moving it to a global :) And also added the rate limiter to the promotion crane calls.
| func (rt *RoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { | ||
| // only rate limit read type calls, not writes | ||
| if r.Method == http.MethodGet || r.Method == http.MethodHead || | ||
| r.Method == http.MethodPut || r.Method == http.MethodPost { |
There was a problem hiding this comment.
Put / Post should be write calls?
There was a problem hiding this comment.
I removed the comment but I was wondering if we should sniff at all 🤔
There was a problem hiding this comment.
Don't we only want to limit read calls? Put / Post should count as Write and have a different rate limit (none atm).
There was a problem hiding this comment.
Ah gotcha I thought they had the same limit, reverted to the original.
There was a problem hiding this comment.
aha. docs: https://cloud.google.com/artifact-registry/quotas
60000 requests per minute in each region or multi-region.
18000 write or delete requests per minute in each region or multi-region.
these are the shared baselines that can be raised by request (and we have raised some of them), then we have the per-user/per-ip read quota configured but not the per-user write quota (since kpromo should be the only thing performing writes/deletes anyhow)
There was a problem hiding this comment.
kubernetes/registry.k8s.io#153 (comment)
Requests per project per user per minute per user
Write requests per project per user per minute per user
kubernetes/registry.k8s.io#153 (comment)
Requests per project per user per minute per user set to 5000.
internal/promoter/image/sign.go
Outdated
| @@ -46,6 +47,12 @@ const ( | |||
| TestSigningAccount = "k8s-infra-promoter-test-signer@k8s-cip-test-prod.iam.gserviceaccount.com" | |||
| ) | |||
|
|
|||
| var rtripper *ratelimit.RoundTripper | |||
There was a problem hiding this comment.
Maybe move both to round trip package so there's a single process-scoped global?
most ideal would be instancing once and passing to everywhere but that may require breaking changes etc...
There was a problem hiding this comment.
Unless the process authenticates with multiple accounts or something
There was a problem hiding this comment.
OK I've moved them to a ratelimiter.Limiter global 👀
This commit adds a ratelimiter packaged based on geranos' roundtripper: https://github.com/kubernetes/registry.k8s.io/blob/main/cmd/geranos/ratelimitroundtrip.go Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
Calls to crane.Copy() when signing are now rate limited to avoid hitting the AR quotas. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
This commit uses the ratelimit package to limit the rate copy operations at image promotion time. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
This commit moves the default max events we feed to the rate limiter to a constant in the package. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
puerco
force-pushed
the
roundtriper
branch
2 times, most recently
from
3d19ed7
to
0364542
Compare
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
There was a problem hiding this comment.
/lgtm
TODO: see if we can pass the crane transport (ratelimit.RoundTripper) around instead of global, but hopefully the global enables us to quickly integrate this everywhere in crane calls.
we went through that transition in geranos as well. quicker to get in place with the global, refactor later.
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds a new
ratelimiterpackaged based on geranos' roundtripper.Based on it, we now rate limit the crane operations in the signature package to ensure
they are under the AR quotas set up by k8s infra.
Update: I've added the rate limit to the image promotion copy operations too.
(Thanks @BenTheElder ! )
Which issue(s) this PR fixes:
Part of kubernetes/release#2962
Special notes for your reviewer:
/cc @BenTheElder @xmudrii @palnabarun @ameukam
More context in this slack thread
Does this PR introduce a user-facing change?