Sane default and configurable Marathon request timeouts #3286
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
m3co-code
changed the title
ldez
added
status/2-needs-review
kind/bug/fix
timoreimann
reviewed
May 8, 2018
There was a problem hiding this comment.
Definitely worthwhile.
Two questions:
- Do we need to make the timeouts configurable? That is, could there be users that whose workflow breaks with the new, more strict timeouts?
- Should this PR go into the release branch as a regular bugfix? Personally I'd say so iff the answer to the first question is "no".
|
|
Sounds reasonable on both fronts. 👍 |
m3co-code
force-pushed
the
sane-marathon-connection-timeouts
branch
from
f1f6db5
to
c8f7ccd
Compare
m3co-code
changed the title
m3co-code
changed the title
|
I updated the PR accordingly. PTAL. |
ldez
added
kind/enhancement
dtomcej
suggested changes
May 14, 2018
provider/marathon/marathon.go
Outdated
| @@ -55,7 +55,9 @@ type Provider struct { | |||
| MarathonLBCompatibility bool `description:"Add compatibility with marathon-lb labels" export:"true"` | |||
| FilterMarathonConstraints bool `description:"Enable use of Marathon constraints in constraint filtering" export:"true"` | |||
| TLS *types.ClientTLS `description:"Enable TLS support" export:"true"` | |||
| DialerTimeout flaeg.Duration `description:"Set a non-default connection timeout for Marathon" export:"true"` | |||
| DialerTimeout flaeg.Duration `description:"Set a non-default dialer timeout for Marathon" export:"true"` | |||
There was a problem hiding this comment.
Do we need to use the phrase "non-default"? Isn't it kind of assumed that defaults will be used unless specified?
There was a problem hiding this comment.
Good point, thanks! I removed the "non-default".
A dial timeout of 60 seconds is overly excessive for a DNS request and probably as good as no timeout at all. Therefore, I changed the default value to 5 seconds as this is more reasonable.
The new timeout values are matching the one that go-marathon would set by itself for the default HTTP client that it uses for SSE subscriptions.
traefiker
force-pushed
the
sane-marathon-connection-timeouts
branch
from
e0b2db3
to
86e865f
Compare
mmatur
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Set sane timeouts for requests to Marathon and make them configurable.
Motivation
Before the timeouts were either overly excessive or not existent at all. For requests done by go-marathon there was no timeout, which means when there was temporarily a network problem which doesn't immediately show a failure (e.g. when the packets are dropped by a firewall) Traefik will never be able to create a connection to Marathon, as this one request will be running basically forever. I used the same timeouts as go-marathon does by default. We have to do this in Traefik as we pass in our own http.Client.
More