Initial implementation for ingress rate limiting #148
Conversation
k8s-ci-robot
added
cncf-cla: yes
pkg/ratelimit/ratelimit.go
Outdated
| func constructRateLimitImpl(params []string) flowcontrol.RateLimiter, error { | ||
| // For now, only the "qps" type is supported. | ||
| rlType := params[0] | ||
| implArgs := params[1:] |
There was a problem hiding this comment.
Are implArgs required? You assert the length of params >= 2, then send [1:] to this func.
There was a problem hiding this comment.
If the length of the array passed in to constructRateLimitImpl is 1, then params[1:] will return empty. Is this what you are referring to?
There was a problem hiding this comment.
Yea, that's fine.
Can you add the expected format as a comment above the func - same thing you did for validateOperation.
pkg/flags/flags.go
Outdated
| @@ -85,6 +86,10 @@ the default backend.`) | |||
| external cloud resources as it's shutting down. Mostly used for testing. In | |||
| normal environments the controller should only delete a loadbalancer if the | |||
| associated Ingress is deleted.`) | |||
| flag.StringVar(&F.GCERateLimit, "gce-ratelimit", "", | |||
| `Optional, can be used to rate limit certain GCE API calls. Example usage: | |||
| --gce-ratelimit=ga.Addresses.Get,qps,30 (limit ga.Addresses.Get to maximum | |||
There was a problem hiding this comment.
Doesn't show an example using the semi-colon delimiter.
Doesn't show the burst.
There was a problem hiding this comment.
Let's also set the example to something logical: 1.25 or 1.5 QPS.
| if l.customRateLimiters != nil { | ||
| for _, c := range l.customRateLimiters { | ||
| if c.useLimiter(key) { | ||
| return c.rateLimiter.Accept(ctx, key) |
There was a problem hiding this comment.
As we discussed offline, I'm not seeing the immediate need of having a list over here and a map in ratelimit.go. Could it be simplified to a single map here?
There was a problem hiding this comment.
Done. Removed the usage of a map in ingress code, but left the list in vendored code. Still thinking about whether a map or list is better.
We can stick with the current list or we can use a map. In that case, AddRateLimiter would take in the cloud.RateLimiter and also the operation, version, service triple so that it can create a RateLimitKey from that which it would then use as a key in the map.
rramkumar1
force-pushed
the
rate-limit
branch
4 times, most recently
from
012e0e2
to
f9be41f
Compare
| @@ -510,6 +519,7 @@ func CreateGCECloud(config *CloudConfig) (*GCECloud, error) { | |||
| nodeInstancePrefix: config.NodeInstancePrefix, | |||
| useMetadataServer: config.UseMetadataServer, | |||
| operationPollRateLimiter: operationPollRateLimiter, | |||
| customRateLimiters: make([]CustomRateLimiter, 0), | |||
There was a problem hiding this comment.
Nit: the , 0 is unnecessary.
| // AddRateLimiter adds a custom cloud.RateLimiter implementation and a | ||
| // function that is used to check whether the custom rate limiter can be | ||
| // used on a given RateLimitKey. | ||
| func (g *GCECloud) AddRateLimiter(rl cloud.RateLimiter, useLimiter func(key *cloud.RateLimitKey) bool) { |
There was a problem hiding this comment.
Any reason to make this an Add... instead of SetRateLimiters? I'm not suggesting a change, just wondering.
There was a problem hiding this comment.
I figured that since this could be called multiple times, that Add... makes more sense.
cmd/glbc/main.go
Outdated
| @@ -69,6 +70,10 @@ func main() { | |||
| } | |||
|
|
|||
| cloud := app.NewGCEClient() | |||
| if err := ratelimit.ConfigureGCERateLimiting(cloud, flags.F.GCERateLimit); err != nil { | |||
There was a problem hiding this comment.
Should this be called within app.NewGCEClient()?
rramkumar1
force-pushed
the
rate-limit
branch
2 times, most recently
from
4b9d48b
to
0ad0a69
Compare
|
New changes are detected. LGTM label has been removed. |
|
New changes are detected. LGTM label has been removed. |
|
New changes are detected. LGTM label has been removed. |
|
New changes are detected. LGTM label has been removed. |
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Is there an issue associated with this PR? (If so, I might've missed it.)
You should have a higher level description of what you're doing somewhere, either in this PR or in an Issue. It's not clear from the title what is being rate-limited. Or what happens to the system when the limit is reached?
cmd/glbc/app/clients.go
Outdated
| @@ -102,6 +103,11 @@ func NewGCEClient() *gce.GCECloud { | |||
| // manually to re-create the client. | |||
| // TODO: why do we bail with success out if there is a permission error??? | |||
| if _, err = cloud.ListGlobalBackendServices(); err == nil || utils.IsHTTPErrorCode(err, http.StatusForbidden) { | |||
| rl, err := ratelimit.ConfigureGCERateLimiting(flags.F.GCERateLimit) | |||
| if err != nil { | |||
| glog.Infof("Error in configuring rate limiting: %v", err) | |||
There was a problem hiding this comment.
not an error? (either glog.Errorf() or returning nil)
There was a problem hiding this comment.
Good catch, fixed.
There was a problem hiding this comment.
Regarding your other point, I'll add some context to the PR.
pkg/ratelimit/ratelimit.go
Outdated
| ch := make(chan struct{}) | ||
| go func() { | ||
| // Call flowcontrol.RateLimiter implementation. | ||
| if l.rateLimitImpls[key] != nil { |
There was a problem hiding this comment.
This doesn't work -- the project ID will not be equal. You will have to write a routine that does the matching (can't use struct equality)
There was a problem hiding this comment.
Done. In order to the keep the rateLimitImpls field as a map, the way I did it was kind of ugly
If I change that to a slice, it can be a lot cleaner so let me know which way you prefer.
pkg/flags/flags.go
Outdated
| @@ -85,6 +86,11 @@ the default backend.`) | |||
| external cloud resources as it's shutting down. Mostly used for testing. In | |||
| normal environments the controller should only delete a loadbalancer if the | |||
| associated Ingress is deleted.`) | |||
| flag.StringVar(&F.GCERateLimit, "gce-ratelimit", "", | |||
There was a problem hiding this comment.
There is a way to handle multiple instances of the same flag and have them append to a list. Easier than having to parse the semicolon ';'
cmd/glbc/app/clients.go
Outdated
| @@ -102,6 +103,11 @@ func NewGCEClient() *gce.GCECloud { | |||
| // manually to re-create the client. | |||
| // TODO: why do we bail with success out if there is a permission error??? | |||
| if _, err = cloud.ListGlobalBackendServices(); err == nil || utils.IsHTTPErrorCode(err, http.StatusForbidden) { | |||
| rl, err := ratelimit.ConfigureGCERateLimiting(flags.F.GCERateLimit.Values()) | |||
There was a problem hiding this comment.
Can we reverse the conditions above move the ConfigureGCERateLimiting() call to the outer level. In other words, let's avoid nesting them.
pkg/ratelimit/ratelimit.go
Outdated
|
|
||
| // ConfigureGCERateLimiting parses the value for the --gce-ratelimit | ||
| // command line flag and returns a properly configured cloud.RateLimiter implementation. | ||
| func ConfigureGCERateLimiting(specs []string) (*ingressRateLimiter, error) { |
There was a problem hiding this comment.
Might be more idiomatic to name it NewGCERateLimiter and have the struct be GCERateLimiter?
While this implementation exists in ingress, there's nothing ingress-specific about it, right? We could potentially move it to a more global library and use it in different places.
pkg/ratelimit/ratelimit.go
Outdated
| } | ||
|
|
||
| // ConfigureGCERateLimiting parses the value for the --gce-ratelimit | ||
| // command line flag and returns a properly configured cloud.RateLimiter implementation. |
There was a problem hiding this comment.
I wouldn't mention the values coming in via flag. As far as this package knows, it's just a list of strings. You can give an example of the string format.
This reverts commit d51ce72.
rramkumar1
force-pushed
the
rate-limit
branch
4 times, most recently
from
b5b5afa
to
7c8e3ef
Compare
nicksardo
added
the
lgtm
Automatic merge from submit-queue (batch tested with PRs 61644, 61624, 61743, 61019, 61287). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add support for setting a custom rate limiter in gce cloud provider **What this PR does / why we need it**: This PR makes it possible to use a custom rate limiter for the GCE cloud provider layer. This is a copy of the raw vendor patch made in kubernetes/ingress-gce#148. Fixes: kubernetes/ingress-gce#154 **Special notes for your reviewer**: **Release note**: ```release-note None ``` /assign @bowei
This PR adds support for rate limiting certain GCE API call's that are made by the controller. Currently, the cloud provider layer does not rate limit any standard controller calls (such as BackendService Get). As a result, in some cases when API calls are frequent, we are blowing through project quota and getting rate limited on the GCE side. This could also cause the controller to slow down. By allowing for customization of what calls can be rate limited, we can prevent blowing through project quota and we can intelligently rate limit so that we do not experience slowdown.
Will add unit tests once the overall approach is LGTM'd.
/assign @nicksardo @bowei