-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Balancing mode UTILIZATION -> RATE #251
Conversation
I'll need to get an LGTM and push the image before kubernetes/kubernetes#41037 goes in |
controllers/gce/backends/backends.go
Outdated
if err := b.cloud.CreateBackendService(backend); err != nil { | ||
return nil, err | ||
errs := []string{} | ||
for _, bm := range []BalancingMode{Rate, Utilization} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are basically first trying to set it to RATE mode right? If fail, then try to set on UTILIZATION mode.
Can you comment? it is not obvious what are you trying to do with the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the comment around where I handle the error, I'll move it up
Just curious. Why do you prefer RATE mode over UTILIZATION mode? |
Basically it's the only balancing mode that allows people to use ILB (https://cloud.google.com/compute/docs/load-balancing/internal/). Without this anyone using HTTP Lb through ingress cannot use ILB, even if they want to setup manually (b/35102911), because we can't mix modes. There are a few more small reasons described in the comparison between modes https://github.com/kubernetes/ingress/pull/251/files#diff-7c0fa9e36d708bfcad29923173e23fe2R43 (like query distribution across multi IG/zone clusters). |
LGTM |
81f6240
to
bc70efb
Compare
bc70efb
to
1e6cced
Compare
Added a comment and a unittest, @freehan inherit LGTM? |
yeah |
Thanks, I'm merging once travis is green |
1e6cced
to
9b305f1
Compare
Fixed build, I wish coveralls would show me test coverage again post my unittests. Lets see if I can get it to re-poll prs. |
aha coverage +0.3% up to a rounded 45, merging |
Tested manually
Created ingress with version 0.8.0:
Updated to 0.9.1 and created another ingress, backends all end up in UTILIZATION
Then recreted them all, backends end up in RATE