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

Add support for adjusting ELB idle timeout for apiserver #1886

Merged
merged 7 commits into from Feb 17, 2017
Merged

Add support for adjusting ELB idle timeout for apiserver #1886

merged 7 commits into from Feb 17, 2017

Conversation

zacblazic
Copy link
Contributor

@zacblazic zacblazic commented Feb 14, 2017 •
edited by k8s-reviewable

Addresses #1883 by adding an idleTimeoutSeconds to the api.loadBalancer in the cluster spec:

spec:
  api:
    loadBalancer:
      type: Public
      idleTimeoutSeconds: 600 # I'd like it a bit longer than the default

The default timeout value is 5 minutes, and can be adjusted by editing the cluster spec.

  • Add documentation
  • Ensure tests pass locally

P.S. First time contributor, let me know if I've missed something.

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @zacblazic. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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

Once you've signed, please reply here (e.g. "I signed it!") 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 Feb 14, 2017
@zacblazic
Copy link
Contributor Author

I signed it!

@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 Feb 14, 2017
@chrislovecnm
Copy link
Contributor

@zacblazic can we rebase this? Flannel integration test will throw an error on merging this into master.

chrislovecnm
chrislovecnm suggested changes Feb 14, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please run make apimachinery to generate the files.

@chrislovecnm
Copy link
Contributor

@justinsb additions to the API, could I get a review :)

@zacblazic zacblazic force-pushed the optional-apiserver-elb-timeout branch from 5eecfb6 to ce643d0 Compare February 14, 2017 19:53
@sstarcher
Copy link
Contributor

sstarcher commented Feb 15, 2017 •
edited

I was about to open an issue for this 👍

sstarcher
sstarcher reviewed Feb 15, 2017
View reviewed changes
docs/cluster_spec.md Outdated
@@ -24,11 +24,15 @@ spec:
When configuring a LoadBalancer, you can also choose to have a public ELB or an internal (VPC only) ELB. The `type`
field should be `Public` or `Internal`.

Additionally, you can increase idle timeout of the load balancer by setting its `idleTimeoutSeconds`. The default idle timeout is 5 minutes, with a maximum of 1200 seconds (20 minutes) being allowed by AWS.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AWS maximum is 3600. http://docs.aws.amazon.com/elasticloadbalancing/latest/classic/config-conn-drain.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 7eee6f2

@sstarcher
Copy link
Contributor

LGTM

@chrislovecnm
Copy link
Contributor

@k8s-bot ok to test

@chrislovecnm
Copy link
Contributor

/lgtm

Waiting on e2e

justinsb
justinsb approved these changes Feb 17, 2017
View reviewed changes
Copy link
Member

@justinsb justinsb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - nice fix!

pkg/apis/kops/cluster.go
@@ -260,7 +260,8 @@ const (
)

type LoadBalancerAccessSpec struct {
Type LoadBalancerType `json:"type,omitempty"`
Type LoadBalancerType `json:"type,omitempty"`
IdleTimeoutSeconds *int64 `json:"idleTimeoutSeconds,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int64 might be overkill here, but we can plan for the (very distant) future ;-)

@justinsb justinsb merged commit 45cfd8a into kubernetes:master Feb 17, 2017
@zacblazic zacblazic mentioned this pull request Feb 28, 2017
Closed
@aknysh aknysh mentioned this pull request Nov 14, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sstarcher sstarcher sstarcher left review comments

@justinsb justinsb justinsb approved these changes

@chrislovecnm chrislovecnm chrislovecnm approved these changes

Assignees

@justinsb justinsb

@chrislovecnm chrislovecnm

Labels
blocks-next cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zacblazic @k8s-ci-robot @chrislovecnm @sstarcher @justinsb