Skip to content
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

Merged
merged 4 commits into from
Feb 9, 2017

Conversation

bprashanth
Copy link
Contributor

Tested manually

Created ingress with version 0.8.0:

$ kubectl get ing 
NAME      HOSTS     ADDRESS          PORTS     AGE
test      *         35.186.217.102   80, 443   2m

Updated to 0.9.1 and created another ingress, backends all end up in UTILIZATION

$ kubectl create -f ~/rtmp/ingress/ing.yaml 
ingress "echomap" created

$ kubectl get ing 
NAME      HOSTS     ADDRESS          PORTS     AGE
test      *         35.186.217.102   80, 443   12m
echomap   foo       35.186.236.70   80, 443   51s

$ for be in $(gcloud compute backend-services list | awk '{print $1}' | tail -n +2); do gcloud compute backend-services describe $be | grep -i balancingmode; done
- balancingMode: UTILIZATION
- balancingMode: UTILIZATION
- balancingMode: UTILIZATION

Then recreted them all, backends end up in RATE

$ kubectl delete ing --all
ingress "echomap" deleted
ingress "test" deleted

$ kubectl create -f test.yaml
ingress "test" created

$ kubectl get ing 
NAME      HOSTS     ADDRESS         PORTS     AGE
test      *         35.186.230.43   80, 443   1m

$ for be in $(gcloud compute backend-services list | awk '{print $1}' | tail -n +2); do gcloud compute backend-services describe $be | grep -i balancingm
- balancingMode: RATE
- balancingMode: RATE

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 8, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 44.407% when pulling 81f6240 on bprashanth:balancing_mode into 0b1fc38 on kubernetes:master.

@bprashanth
Copy link
Contributor Author

I'll need to get an LGTM and push the image before kubernetes/kubernetes#41037 goes in

if err := b.cloud.CreateBackendService(backend); err != nil {
return nil, err
errs := []string{}
for _, bm := range []BalancingMode{Rate, Utilization} {
Copy link
Contributor

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.

Copy link
Contributor Author

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

@freehan
Copy link
Contributor

freehan commented Feb 9, 2017

Just curious. Why do you prefer RATE mode over UTILIZATION mode?

@bprashanth
Copy link
Contributor Author

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).

@freehan
Copy link
Contributor

freehan commented Feb 9, 2017

LGTM

@bprashanth
Copy link
Contributor Author

Added a comment and a unittest, @freehan inherit LGTM?

@freehan
Copy link
Contributor

freehan commented Feb 9, 2017

yeah

@bprashanth
Copy link
Contributor Author

Thanks, I'm merging once travis is green

@bprashanth
Copy link
Contributor Author

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.

@bprashanth bprashanth added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 44.686% when pulling 9b305f1 on bprashanth:balancing_mode into 0b1fc38 on kubernetes:master.

@bprashanth
Copy link
Contributor Author

aha coverage +0.3% up to a rounded 45, merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants