CloseIdleConnections of wrapped Transport RoundTrippers #104844
Merged
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
k8s-ci-robot
added
release-note
k8s-ci-robot
added
area/apiserver
area/kubelet
sig/api-machinery
this is not working, maybe I'm missing some wrapper 🤔 |
|
/assign @liggitt |
k8s-ci-robot
added
the
triage/accepted
k8s-ci-robot
removed
the
needs-triage
caesarxuchao
reviewed
Sep 9, 2021
liggitt
reviewed
Sep 9, 2021
liggitt
reviewed
Sep 9, 2021
aojea
force-pushed
the
clientgo_close_idle
branch
from
f1e09e1
to
0558d26
Compare
|
/assign @wojtek-t |
wojtek-t
reviewed
Nov 16, 2021
There was a problem hiding this comment.
I see... so the expectation would be:
- kubelets using http/2 would use the http/2 ping timeout and this CloseIdleConnections mechanism
- kubelets that automatically negotiated back to http/1.1 would use only this CloseIdleConnections mechanism
- a kubelet could be forced to use http/1.1 and use the old tear-down-all-connections mechanism by explicitly setting DISABLE_HTTP2
That limits the exposure to regressions because of this new mechanism without an escape hatch to kubelets which:
- don't use client certificate rotation
- use http/2
That seems reasonable to me, though I'd like a sig-node reviewer to ack as well.
That seems reasonable to me as well and it seems we have an ACK from sig-node reviewer.
I just added a couple nits to the PR.
| enableHTTP2 bool | ||
| }{ | ||
| {"HTTP1", false}, | ||
| {"HTTP2", false}, |
There was a problem hiding this comment.
interestingly, the http1 client shows a different behavior here than the http2, in http2 the connection is not closed after the timeout, I think that is because of
https://datatracker.ietf.org/doc/html/rfc7540#section-9.1
HTTP/2 connections are persistent. For best performance, it is
expected that clients will not close connections until it is
determined that no further communication with a server is necessary
(for example, when a user navigates away from a particular web page)
or until the server closes the connection.
added 2 commits
November 16, 2021 15:39
It iterates over the wrapped transports until it finds one that implements the CloseIdleConnections method and executes it. add test for closeidle http1 connections add test for http1.1 reconnect with inflight request add test to reuse connection request add test for request connect after timeout add test for client-go request concurrency
Don't use a custom dialer for the kubelet if is not rotating certificates, so we can reuse TCP connections because we don't need a customer dialer. Kubelet needs to be able to recover from stale http connections. HTTP2 has a mechanism to detect broken connections by sending periodical pings. HTTP1 only can have one persistent connection, and it will close all Idle connections once the Kubelet heartbet fails. However, since there are many edge cases that we can't control, users can still opt-in to the previous behavior for closing the connections by setting the environment variable DISABLE_HTTP2.
aojea
force-pushed
the
clientgo_close_idle
branch
from
fae9591
to
15ea255
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, pacoxu, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@aojea Out of curiousity, how does the use of a custom dialer (before this PR) prevent re-use of connections? I've been looking at the code in this PR because you said in another thread that it might explain the issue I've been seeing. (#65954 (comment)) |
The client go constructor create a new transport if the dialer is customized Before #105490 each group version created a new transport |
christarazi
added a commit
to christarazi/cilium
that referenced
this pull request
Mar 31, 2022
Following up from cilium#19259, this commit implements the same logic as kubelet [1] to close all idle connections. We can rely on HTTP/2 to determine which connections are idle (HTTP/2 pings), instead of closing all connections indiscriminately (when the heartbeat fails) via the custom dialer in setDialer(). We can assume HTTP/2 support as it has been supported since K8s 1.20 [2]. Users can disable HTTP/2 by setting the DISABLE_HTTP2 environment variable, however this only really meant from very advanced users, hence no documentation. Kubernetes has been running with HTTP/2 as the default for a long time (~since Dec 2020) without major issues. CloseIdleConnectionsFor() was introduced in K8s 1.23 [3], hence this commit cannot be backported to any earlier version of Cilium which doesn't support K8s 1.23. [1]: kubernetes/kubernetes#104844 [2]: kubernetes/kubernetes#95981 [3]: kubernetes/kubernetes@b9d865a Suggested-by: André Martins <andre@cilium.io> Signed-off-by: Chris Tarazi <chris@isovalent.com>
pchaigno
pushed a commit
to cilium/cilium
that referenced
this pull request
Apr 4, 2022
Following up from #19259, this commit implements the same logic as kubelet [1] to close all idle connections. We can rely on HTTP/2 to determine which connections are idle (HTTP/2 pings), instead of closing all connections indiscriminately (when the heartbeat fails) via the custom dialer in setDialer(). We can assume HTTP/2 support as it has been supported since K8s 1.20 [2]. Users can disable HTTP/2 by setting the DISABLE_HTTP2 environment variable, however this only really meant from very advanced users, hence no documentation. Kubernetes has been running with HTTP/2 as the default for a long time (~since Dec 2020) without major issues. CloseIdleConnectionsFor() was introduced in K8s 1.23 [3], hence this commit cannot be backported to any earlier version of Cilium which doesn't support K8s 1.23. [1]: kubernetes/kubernetes#104844 [2]: kubernetes/kubernetes#95981 [3]: kubernetes/kubernetes@b9d865a Suggested-by: André Martins <andre@cilium.io> Signed-off-by: Chris Tarazi <chris@isovalent.com>
pchaigno
pushed a commit
to pchaigno/cilium
that referenced
this pull request
Apr 5, 2022
[ upstream commit 365c788 ] Following up from cilium#19259, this commit implements the same logic as kubelet [1] to close all idle connections. We can rely on HTTP/2 to determine which connections are idle (HTTP/2 pings), instead of closing all connections indiscriminately (when the heartbeat fails) via the custom dialer in setDialer(). We can assume HTTP/2 support as it has been supported since K8s 1.20 [2]. Users can disable HTTP/2 by setting the DISABLE_HTTP2 environment variable, however this only really meant from very advanced users, hence no documentation. Kubernetes has been running with HTTP/2 as the default for a long time (~since Dec 2020) without major issues. CloseIdleConnectionsFor() was introduced in K8s 1.23 [3], hence this commit cannot be backported to any earlier version of Cilium which doesn't support K8s 1.23. [1]: kubernetes/kubernetes#104844 [2]: kubernetes/kubernetes#95981 [3]: kubernetes/kubernetes@b9d865a Suggested-by: André Martins <andre@cilium.io> Signed-off-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Paul Chaignon <paul@cilium.io>
pchaigno
pushed a commit
to cilium/cilium
that referenced
this pull request
Apr 6, 2022
[ upstream commit 365c788 ] Following up from #19259, this commit implements the same logic as kubelet [1] to close all idle connections. We can rely on HTTP/2 to determine which connections are idle (HTTP/2 pings), instead of closing all connections indiscriminately (when the heartbeat fails) via the custom dialer in setDialer(). We can assume HTTP/2 support as it has been supported since K8s 1.20 [2]. Users can disable HTTP/2 by setting the DISABLE_HTTP2 environment variable, however this only really meant from very advanced users, hence no documentation. Kubernetes has been running with HTTP/2 as the default for a long time (~since Dec 2020) without major issues. CloseIdleConnectionsFor() was introduced in K8s 1.23 [3], hence this commit cannot be backported to any earlier version of Cilium which doesn't support K8s 1.23. [1]: kubernetes/kubernetes#104844 [2]: kubernetes/kubernetes#95981 [3]: kubernetes/kubernetes@b9d865a Suggested-by: André Martins <andre@cilium.io> Signed-off-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Paul Chaignon <paul@cilium.io>
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.
/kind bug
/kind cleanup
Fixes #100849
Add a function
func CloseIdleConnectionsFor(transport http.RoundTripper)that allow to close the idle connections that belong to the transport, if the transport implements theRoundTripperWrapperinterface.Use this new function to close the idle connections on kubelet and avoid using a custom dialer that forces the kubelet to create multiple TCP connections , instead of reusing a single one.