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.

Sign up for GitHub

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

Jump to bottom

Override load balancer alg view config map #673

Merged
merged 1 commit into from
Apr 29, 2017
Merged

Override load balancer alg view config map #673

merged 1 commit into from
Apr 29, 2017

Conversation

jeffpearce
Copy link
Contributor

@jeffpearce jeffpearce commented Apr 28, 2017 •
edited
Loading

@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 28, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 28, 2017
@jeffpearce
Copy link
Contributor Author

@aledbf looks like a bunch of lint errors, not all related to my changes. Should I fix them?

@aledbf
Copy link
Member

aledbf commented Apr 28, 2017

@jeffpearce no, you need to fix just this one

controllers/nginx/pkg/config/config.go:271: struct field tag `json:"load-balance",omitempty` not compatible with reflect.StructTag.Get: key:"value" pairs not separated by spaces

@aledbf
Copy link
Member

aledbf commented Apr 28, 2017

@jeffpearce you can check this in you pc/laptop running make test

aledbf
aledbf reviewed Apr 28, 2017
View reviewed changes
controllers/nginx/pkg/config/config.go Outdated
@@ -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`
Copy link
Member

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"

aledbf
aledbf reviewed Apr 28, 2017
View reviewed changes
controllers/nginx/pkg/config/config.go Outdated
@@ -301,6 +304,7 @@ func NewDefault() Configuration {
SSLSessionTimeout: sslSessionTimeout,
UseGzip: true,
WorkerProcesses: strconv.Itoa(runtime.NumCPU()),
LoadBalanceAlgorithm: "least_conn;",
Copy link
Member

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

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 think it needs to be empty to specify the default, ngnix doesn't like leaving just a ; on that line

aledbf
aledbf reviewed Apr 28, 2017
View reviewed changes
controllers/nginx/pkg/config/config.go Outdated
@@ -301,6 +304,7 @@ func NewDefault() Configuration {
SSLSessionTimeout: sslSessionTimeout,
UseGzip: true,
WorkerProcesses: strconv.Itoa(runtime.NumCPU()),
LoadBalanceAlgorithm: "least_conn;",
Copy link
Member

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

@aledbf aledbf self-assigned this Apr 28, 2017
silasbw
silasbw reviewed Apr 28, 2017
View reviewed changes
controllers/nginx/pkg/template/configmap.go Outdated
} else if to.LoadBalanceAlgorithm == "round_robin" {
to.LoadBalanceAlgorithm = ""
} else if !strings.HasSuffix(to.LoadBalanceAlgorithm, ";") {
to.LoadBalanceAlgorithm += ";"
Copy link

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.

controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl Outdated
@@ -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 }}
Copy link

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.

Copy link
Contributor Author

@jeffpearce jeffpearce Apr 28, 2017 •
edited
Loading

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?

Copy link

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.

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'll change it to do that

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 47.119% when pulling e9efc40 on jeffpearce:jeffpearce/lb into f5d27bf on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 47.048% when pulling 85e4675 on jeffpearce:jeffpearce/lb into f5d27bf on kubernetes:master.

aledbf
aledbf reviewed Apr 29, 2017
View reviewed changes
controllers/nginx/pkg/template/configmap_test.go Outdated
@@ -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
Copy link
Member

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

@aledbf
Copy link
Member

aledbf commented Apr 29, 2017

@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

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 47.078% when pulling 548b674 on jeffpearce:jeffpearce/lb into f5d27bf on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Apr 29, 2017

@jeffpearce please squash the commits

@aledbf
Copy link
Member

aledbf commented Apr 29, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2017
@jeffpearce
Copy link
Contributor Author

jeffpearce commented Apr 29, 2017 •
edited
Loading

Commits squashed. Thanks for the super quick turnaround for review.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.007%) to 47.063% when pulling a5d58cc on jeffpearce:jeffpearce/lb into f5d27bf on kubernetes:master.

@aledbf aledbf merged commit 6bf5d75 into kubernetes:master Apr 29, 2017
@jeffpearce jeffpearce deleted the jeffpearce/lb branch April 29, 2017 17:15
@aledbf aledbf mentioned this pull request May 1, 2017
Closed
@schancel schancel mentioned this pull request Mar 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aledbf aledbf aledbf left review comments

@silasbw silasbw silasbw left review comments

Assignees

@aledbf aledbf

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jeffpearce @k8s-ci-robot @k8s-reviewable @aledbf @coveralls @silasbw @jxpearce-godaddy