Skip to content

Add defaults and env control of the fin timeouts in the router #14220

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

Merged
merged 1 commit into from
May 24, 2017

Conversation

knobunc
Copy link
Contributor

@knobunc knobunc commented May 16, 2017

Set defaults for the fin timeouts and expose them as environment
variable controls to the user.

The new environment variables are:

  • ROUTER_CLIENT_FIN_TIMEOUT: Controls the fin timeout to the client of haproxy (default 1s)
  • ROUTER_DEFAULT_SERVER_FIN_TIMEOUT: Controls the fin timeout to the server haproxy contacts, i.e. in the cluster (default 1s)

Set defaults for the fin timeouts and expose them as environment
variable controls to the user.

The new environment variables are:
 - ROUTER_CLIENT_FIN_TIMEOUT: Controls the fin timeout to the client of
     haproxy (default 1s)
 - ROUTER_DEFAULT_SERVER_FIN_TIMEOUT: Controls the fin timeout to the
     server haproxy contacts, i.e. in the cluster (default 1s)
@knobunc
Copy link
Contributor Author

knobunc commented May 16, 2017

I'll prep the docs PR if this is not contentious.

Copy link
Contributor

@rajatchopra rajatchopra left a comment

Choose a reason for hiding this comment

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

Insignificant nit: Should we just take the timeout value in 's'econds? 'h|d' sounds malicious.

LGTM though

@knobunc
Copy link
Contributor Author

knobunc commented May 16, 2017

@rajatchopra good question, but all the other timeouts are specified the same way, so I'd go with consistency. Also, you can't really hurt other people with these. The cluster admin has to set the env, so they'd be shooting themselves in the foot (and someone can just put a large number of seconds to be an ass anyway).

I did have the ENV names different, one with DEFAULT in case we need to allow an annotation for the per-server connections, and the other without since there can be only one (well... one per frontend, but effectively one).

@smarterclayton
Copy link
Contributor

Same defaults as haproxy or lower/higher?

@knobunc
Copy link
Contributor Author

knobunc commented May 19, 2017

@openshift/networking PTAL

@knobunc
Copy link
Contributor Author

knobunc commented May 19, 2017

@smarterclayton the haproxy default seems to be TICK_ETERNITY. I went with the recommended timeout from this haproxy presentation: https://www.slideshare.net/haproxytech/haproxy-best-practice

I can't see any harm in dropping a connection shortly after we send the FIN. Worst case we miss a FIN ACK that is slow. (Stupid Byzantine generals). So I vote we go with the recommended low value.

@smarterclayton
Copy link
Contributor

smarterclayton commented May 19, 2017 via email

@knobunc
Copy link
Contributor Author

knobunc commented May 23, 2017

[merge]

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 3939a73

@knobunc
Copy link
Contributor Author

knobunc commented May 23, 2017

Docs PR openshift/openshift-docs#4465

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1647/) (Base Commit: c02131d)

@knobunc
Copy link
Contributor Author

knobunc commented May 24, 2017

[merge] Last failed due to 'FAILURE: Generated bootstrap bindata out of date. Please run hack/update-generated-bindata.sh' which is not related, and seems to be resolved.

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 3939a73

@openshift-bot
Copy link
Contributor

openshift-bot commented May 24, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/771/) (Base Commit: 67275e1) (Image: devenv-rhel7_6260)

@openshift-bot openshift-bot merged commit 61b1f2a into openshift:master May 24, 2017
@knobunc knobunc deleted the feature/add-fin-timeouts branch May 24, 2017 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants