Add a retroactive one-pager for rate limiting#5415
Conversation
Signed-off-by: Nic Cope <nicc@rk0n.org>
cab334e to
9615979
Compare
|
@turkenh two thoughts come to mind after writing this up. First, there's not really a need for Second, I'm not sure why the global rate limiter would affect the |
| the middleware `Reconciler`, subject to its token bucket reconciler. If there | ||
| are sufficient tokens available in the bucket, the reconcile is passed to the | ||
| wrapped (inner) `Reconciler` immediately. If there aren't sufficient tokens | ||
| available, the reconcile is returned to the tail of the work queue by returning |
There was a problem hiding this comment.
Wouldn't it be more fair if we put it back to the head of the queue? Why do we change the ordering in the queue?
Maybe I misunderstood something but, as an analogy, I am waiting in a bank queue and when it is my turn I am told by the officer to go to the tail of the queue because he processed enough for that given hour 😅
There was a problem hiding this comment.
Wouldn't it be more fair if we put it back to the head of the queue?
It would.
Why do we change the ordering in the queue?
No reason, really. 🙂 It just seems to be how all the Kubernetes work queue rate limiting works, and the topic didn't come up until now.
The queueing is all using https://pkg.go.dev/k8s.io/client-go/util/workqueue. When we return RequeueAfter I
believe that translates to an AddAfter, i.e. "add back to the end of the queue after duration D":
https://github.com/crossplane/crossplane-runtime/blob/v1.15.1/pkg/ratelimiter/reconciler.go#L51
We could potentially improve on this by using a RateLimitingQueue inside our ratelimiter.Reconciler. This way there would be two levels of workqueue:
- Outer queue handles per-object exponential backoff. Requests sent to back of outer queue.
- Inner queue handles global max-reconcile-rate. Requests sent to back of inner queue.
I think if we wanted to support requests being sent to the front of the queue after a certain duration we'd need to implement our own work queue.
There's talk of allowing users to supply their own work queue (not only their own rate limiter) in controller-runtime in kubernetes-sigs/controller-runtime#857. If we want to optimize as best as possible, it might be worth pursuing that upstream.
|
@turkenh Given that this is just documenting how things work today, WDYT about approving so I can merge? We can then discuss how it could be better. |
turkenh
left a comment
There was a problem hiding this comment.
Thanks for taking time for writing this down 🙌
Description of your changes
I spent a bunch of time these evening trying to remember how Crossplane's rate limiting works, and why it works that way. I figured I'd write it all down for next time.
I have:
make reviewableto ensure this PR is ready for review.Added or updated unit tests.Added or updated e2e tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.