-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add defaults and env control of the fin timeouts in the router #14220
Conversation
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)
I'll prep the docs PR if this is not contentious. |
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.
Insignificant nit: Should we just take the timeout value in 's'econds? 'h|d' sounds malicious.
LGTM though
@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). |
Same defaults as haproxy or lower/higher? |
@openshift/networking PTAL |
@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. |
Agree.
…On Fri, May 19, 2017 at 12:17 PM, Ben Bennett ***@***.***> wrote:
@smarterclayton <https://github.com/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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14220 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pzvGY0B_-cjxP1qygIKyrjpsqNAbks5r7cB_gaJpZM4NczWo>
.
|
[merge] |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to 3939a73 |
Docs PR openshift/openshift-docs#4465 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1647/) (Base Commit: c02131d) |
[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. |
Evaluated for origin merge up to 3939a73 |
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) |
Set defaults for the fin timeouts and expose them as environment
variable controls to the user.
The new environment variables are: