-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Override load balancer alg view config map #673
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@aledbf looks like a bunch of lint errors, not all related to my changes. Should I fix them? |
@jeffpearce no, you need to fix just this one
|
@jeffpearce you can check this in you pc/laptop running |
@@ -266,6 +266,9 @@ type Configuration struct { | |||
// Defines the number of worker processes. By default auto means number of available CPU cores | |||
// http://nginx.org/en/docs/ngx_core_module.html#worker_processes | |||
WorkerProcesses string `json:"worker-processes,omitempty"` | |||
|
|||
// Defines the load balancing algorithm to use. The deault is round-robin | |||
LoadBalanceAlgorithm string `json:"load-balance",omitempty` |
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.
this should be json:"load-balance,omitempty"
@@ -301,6 +304,7 @@ func NewDefault() Configuration { | |||
SSLSessionTimeout: sslSessionTimeout, | |||
UseGzip: true, | |||
WorkerProcesses: strconv.Itoa(runtime.NumCPU()), | |||
LoadBalanceAlgorithm: "least_conn;", |
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.
don't leave ;
as part of the setting. That character must be present in the template instead
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 think it needs to be empty to specify the default, ngnix doesn't like leaving just a ; on that line
@@ -301,6 +304,7 @@ func NewDefault() Configuration { | |||
SSLSessionTimeout: sslSessionTimeout, | |||
UseGzip: true, | |||
WorkerProcesses: strconv.Itoa(runtime.NumCPU()), | |||
LoadBalanceAlgorithm: "least_conn;", |
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.
also make "least_conn" a const so you can reference the value
} else if to.LoadBalanceAlgorithm == "round_robin" { | ||
to.LoadBalanceAlgorithm = "" | ||
} else if !strings.HasSuffix(to.LoadBalanceAlgorithm, ";") { | ||
to.LoadBalanceAlgorithm += ";" |
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.
Should we assume no ;
and add it in the template? That way LoadBalanceAlgorithm
is the algorithm instead of the algorithm + the semi-colon nginx config requires.
@@ -206,7 +206,8 @@ http { | |||
{{ if eq $upstream.SessionAffinity.AffinityType "cookie" }} | |||
sticky hash={{$upstream.SessionAffinity.CookieSessionAffinity.Hash}} name={{$upstream.SessionAffinity.CookieSessionAffinity.Name}} httponly; | |||
{{ else }} | |||
least_conn; | |||
# Load balance algorithm; empty for round robin, which is the default | |||
{{ $cfg.LoadBalanceAlgorithm }} |
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'd add a colon here.
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.
round_robin isn't actually valid here - to get the default behavior we need to leave this line empty, without even a ;
I added round_robin
as a string you can specify in the config map, but it needs to be translated to empty string. Does that make sense?
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.
yeah it does, but #673 (review) seems like a good suggestion, yeah?
Another option is to use conditionals in the template and only set the load balancing policy if it's not round_robin
. E.g., similar to what the session affinity check above does.
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'll change it to do that
@@ -5,7 +5,7 @@ Licensed under the Apache License, Version 2.0 (the "License"); | |||
you may not use this file except in compliance with the License. | |||
You may obtain a copy of the License at | |||
|
|||
http://www.apache.org/licenses/LICENSE-2.0 | |||
http://www.apache.org/licenses/LICENSE-2.0 |
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.
don not change this line
@jeffpearce this looks good. Just add a comment about the configuration here https://github.com/kubernetes/ingress/blob/master/controllers/nginx/configuration.md#allowed-parameters-in-configuration-configmap and we can merge this |
Coverage increased (+0.008%) to 47.078% when pulling 548b6745475f117aada638674a8fedb62ad0a3fe on jeffpearce:jeffpearce/lb into f5d27bf on kubernetes:master. |
@jeffpearce please squash the commits |
/lgtm |
548b674
to
a5d58cc
Compare
Commits squashed. Thanks for the super quick turnaround for review. |
#656