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.

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

Enable TCP Keep-Alive in Docker client #415

Merged
merged 1 commit into from
Aug 3, 2017

Conversation

jlhawn
Copy link
Contributor

@jlhawn jlhawn commented Aug 2, 2017

Some network environments may have NATs, proxies, or gateways which
kill idle connections. There are many Docker API operations which may
be idle for long periods of time (such as ContainerWait and ContainerAttach)
and may result in unexpected connection closures or hangs if TCP keepalives
are not used.

This patch updates the default HTTP transport used by the Docker client
package to enable TCP Keep-Alive with a keep-alive interval of 30 seconds.
It also sets a connect timeout of 30 seconds.

This PR complements the change to the moby client package here: moby/moby#34359

Some network environments may have NATs, proxies, or gateways which
kill idle connections. There are many Docker API operations which may
be idle for long periods of time (such as ContainerWait and ContainerAttach)
and may result in unexpected connection closures or hangs if TCP keepalives
are not used.

This patch updates the default HTTP transport used by the Docker client
package to enable TCP Keep-Alive with a keep-alive interval of 30 seconds.
It also sets a connect timeout of 30 seconds.

Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)
@thaJeztah
Copy link
Member

ugh circleCI keeps having problems, now its a memory issue (possibly);

./scripts/build/cross
Exited with code 137

Hint: Exit code 137 typyically means the process is killed because it was running out of memory
Hint: Check if you can optimize the memory usage in your app
Hint: Max memory usage of this container is 58572800
 according to /sys/fs/cgroup/memory/memory.max_usage_in_bytes

@codecov-io
Copy link

codecov-io commented Aug 3, 2017

Codecov Report

Merging #415 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
- Coverage   46.26%   46.25%   -0.02%     
==========================================
  Files         193      193              
  Lines       16093    16097       +4     
==========================================
  Hits         7445     7445              
- Misses       8259     8263       +4     
  Partials      389      389

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

not sure what's with CircleCI

never mind, it's green now 👍

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit f7b78dc into docker:master Aug 3, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.08.0 milestone Aug 3, 2017
@jlhawn jlhawn deleted the client_dial_enable_tcp_keepalive branch August 3, 2017 17:48
@@ -214,6 +216,10 @@ func newHTTPClient(host string, tlsOptions *tlsconfig.Options) (*http.Client, er
}
tr := &http.Transport{
TLSClientConfig: config,
DialContext: (&net.Dialer{
KeepAlive: 30 * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

For now this can help keep connections alive over the NAT timeout issue we've been facing, but would really like to see more knobs to turn in a configurable manner much like curl: https://curl.haxx.se/libcurl/c/CURLOPT_TCP_KEEPALIVE.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants