Azure - ARM Read/Write rate limiting #59830
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
requested review from
andyzhangx,
ingvagabund,
lavalamp,
luxas and
pwittrock
k8s-github-robot
added
needs-rebase
k8s-ci-robot
added
the
approved
k8s-github-robot
removed
needs-rebase
khenidak
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@jackfrancis @brendanburns This is ready for review now. |
| az.rateLimiter.Accept() | ||
| err = createArmRateLimitErr(false, "VMGet") | ||
| if !az.rateLimiterReader.TryAccept() { | ||
| return |
There was a problem hiding this comment.
Educate me in golang. :)
By implicitly returning (simple return without params) both named return types, what are we returning as the value of result in the error case?
| @@ -181,7 +209,11 @@ func (az *azVirtualMachinesClient) Get(resourceGroupName string, VMName string, | |||
| } | |||
|
|
|||
| func (az *azVirtualMachinesClient) List(resourceGroupName string) (result compute.VirtualMachineListResult, err error) { | |||
| az.rateLimiter.Accept() | |||
| err = createArmRateLimitErr(false, "VMList") | |||
There was a problem hiding this comment.
Could we optimize by putting these err constructor invocations inside the failure condition if block (as they are only used if the failure condition is met)?
|
@khenidak Added a couple comments for thought. Overall lgtm, audited all the rate limit injections (esp. the write ones which require us to compose a zero value return value) and they look correct/sane. |
k8s-github-robot
added
the
needs-rebase
k8s-github-robot
removed
the
needs-rebase
|
@jackfrancis updated with your code comments PTAL |
|
@feiskyer if you're good with this implementation can you add the LGTM so we can get this merged? |
| @@ -33,6 +34,15 @@ import ( | |||
| "k8s.io/client-go/util/flowcontrol" | |||
| ) | |||
|
|
|||
| // Creates an error for rate limiting errors | |||
| func createArmRateLimitErr(isWrite bool, opName string) error { | |||
There was a problem hiding this comment.
Given that the purpose of this is to return this error in a channel, perhaps having this signature being:
Also nit, golang style says Arm should be ARM.
func createARMRateLimitError(isWrite bool, opName string) (err, chan error) {
There was a problem hiding this comment.
ah, I actually see that you use this elsewhere, perhaps two functions:
func createARMRateLimitError(...) error {
and
func createARMRateLimitErrChannel() chan error {
|
@brendanburns code comments addressed. Thanks |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, khenidak 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 |
|
Automatic merge from submit-queue (batch tested with PRs 59939, 59830). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Azure cloud provider currently runs with:
read [put/post/delete]andwriteoperations, while ARM provide [different rates for read/write] (https://docs.microsoft.com/en-us/azure/azure-resource-manager/resource-manager-request-limits). This causes write operation to stop even if there is available write request quotas.Accept()instead ofTryAccept()This causes control loop to wait for prolonged tikein case of no request quota availablefor all requests even for those does not require ARM interaction. A case for that theServicecontrol loop will wait for a prolonged time trying to createLoadBalancerservice even though it can fail and work on the next service which isClusterIP. This PR moves cloud provider tpTryAccept()Which issue(s) this PR fixes:
Fixes # #58770
Special notes for your reviewer:
n/aRelease note:
cc @jackfrancis (need your help carefully reviewing this one) @brendanburns @jdumars