-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Changed the router to default to roundrobin when weights are used #12752
Changed the router to default to roundrobin when weights are used #12752
Conversation
67f448b
to
686d7b6
Compare
@openshift/networking PTAL |
[test] |
pkg/router/template/router.go
Outdated
key := fmt.Sprintf("%s/%s", route.Namespace, route.Spec.To.Name) | ||
if route.Spec.To.Weight == nil { | ||
serviceUnits[key] = 0 | ||
} else { | ||
serviceUnits[key] = *route.Spec.To.Weight | ||
hasWeights = true |
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.
Shouldn't this check be that there is more than one service? A route with weight 100 and one backend should behave exactly the same as a route with weight 1 and one backend.
686d7b6
to
58af634
Compare
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.
LGTM
change the e2e test for weighted router to remove the balance annotations |
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.
LGTM
58af634
to
f39b6a7
Compare
If the route is associated with multiple services then we will set the default load balance policy to RoundRobin if no policy is set with an annotation or as a global default with an environment variable. Without this change the user would need to both set the services, weights, and then set an annotation to change the default balancing algorithm... which people almost always forgot to do. For bug 1416869 (https://bugzilla.redhat.com/show_bug.cgi?id=1416869)
f39b6a7
to
0ddebc1
Compare
@smarterclayton Good call. I was contemplating the tests this morning, and that's a clean way to do it. |
In retrospect we probably could have caught that the review. It did not
jump out at me at the time.
On Feb 2, 2017, at 9:46 AM, OpenShift Bot <[email protected]> wrote:
continuous-integration/openshift-jenkins/test Running (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13507/) (Base
Commit: cfb50d9
<cfb50d9>
)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12752 (comment)>,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p1YcXhBvjXkqjiZn8PFqrOCUhZ3kks5rYewXgaJpZM4Lz_b2>
.
|
[test] last failure was due to subscription-manager not getting a subscription |
[test] last was (now resolved) flake #12736 |
Evaluated for origin test up to 0ddebc1 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13561/) (Base Commit: e2e5d9e) |
[merge] |
Evaluated for origin merge up to 0ddebc1 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13572/) (Base Commit: a1c66f9) (Image: devenv-rhel7_5847) |
If the route has weights set for the services it is associated with,
then we will set the default load balance policy to RoundRobin if no
policy is set with an annotation or as a global default with an
environment variable. Without this change the user would need to both
set the weights, and then set an annotation to change the default
balancing algorithm... which people almost always forgot to do.
For bug 1416869 (https://bugzilla.redhat.com/show_bug.cgi?id=1416869)