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
CloseIdleConnections of wrapped Transport RoundTrippers #104844
Conversation
this is not working, maybe I'm missing some wrapper 🤔 |
/assign @liggitt |
f1e09e1
to
0558d26
Compare
/assign @wojtek-t |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
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.
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.
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.
fae9591
to
15ea255
Compare
/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 |
@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 |
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>
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>
[ 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>
[ 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>
/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 theRoundTripperWrapper
interface.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.