Client-go: Client should expose a mechanism to close underlying TCP connections

[jfoy]

Re kubernetes/kubernetes#52176, kubernetes/kubernetes#56720, #342

(Short form: a stalled TCP connection to apiserver from kubelet or kube-proxy can cause ~15 minutes of disruption across a substantial number of nodes until the local kernel closes the socket.)

I believe we need a mechanism for requestCanceler.CancelRequest to invoke transport.CloseIdleConnections based on config. It looks like we could do this with a small http.RoundTripper built to purpose. I hope to submit a PR with this change shortly.

I'm not sure if that behavior should be activated on a case-by-case basis using config.WrapTransport (less invasive, narrower change) or if it should be part of the core config used in transport.HTTPWrappersForConfig (needed in multiple use cases per issues listed above). What's the convention here?

[lavalamp]

Is this failure mode unique to HTTP/2 connections?

[lavalamp]

Is the problem that the timeout is wrong, or that the client doesn't notice when connections have been half-closed?

[jfoy]

I can't tell if it's limited to HTTP/2, but it looks to me like clients maintain persistent TCP connections no matter the application protocol, so they would be hit by the same failure.

It's the latter problem - as far as we can tell, the load balancer is hanging up on decommissioned IP addresses without sending a FIN packet (or we never receive it), so the client thinks the connection is still open. The HTTP timeout is set correctly, but opening a new session appears to reuse the existing half-closed connection.

[stephbu]

Same issue happens also with NLB in AWS.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[warmchang]

/remove-lifecycle stale

[xh3b4sd]

We have situations in which pods using the client have to be restarted in order to work around this problem. What can we do to get some movement here?

[liggitt]

What can we do to get some movement here?

There is sufficient plumbing in the rest.Config to wrap the transport or set up a connection-tracking dialer yourself today, while this is considered as a built-in feature

[r7vme]

There is sufficient plumbing in the rest.Config to wrap the transport or set up a connection-tracking dialer yourself today, while this is considered as a built-in feature

Right, i experimented while ago with own Transport. Unfortunately problem is deeper on TCP level (at least for us). In Transport i can control via dialer some TCP keepalive settings (this was my initial idea for a fix), but this does not help. At least because by default transport TCP keepalive settings are already quite good in Go.

It turned out that our problem was/is with incorrect load-balancer settings. Following happens for us:

1. endpoint (k8s api) hardly killed (e.g. kill -9 on qemu process)
2. load balancer detects that backend died after some timeout
3. load balancer does not reset existing connection to the client and keeps answering heartbeats
4. on client we see connection stays ~15 min in ESTABLISHED state

What helps is to set quite low timeouts on load balancers.

[lavalamp]

How much overlap does this have with #65012? See: https://github.com/kubernetes/kubernetes/issues/65012#issuecomment-410320028

[szuecs]

@lavalamp It is not only h2, which is affected.

Basically there are some problems in the Go default Transport.
For example it caches DNS responses forever https://github.com/golang/go/issues/23427 .
The other as described in https://github.com/kubernetes/client-go/issues/374#issuecomment-407689186 is, that the loadbalancer (iptables/ipvs service for example) does not send TCP RST packet to the client, which will lead to a Linux kernel TCP timeout, because the current Timeout setting in client-go does not work.

I guess basically everywhere in Kubernetes and most of the 3rd party controllers, this is broken.
We saw it also in etcd-proxy, which we rewrote ourselves, because it was easier to build it, than to get a fix upstream merged.

[lavalamp]

@szuecs thank you, that was helpful. I think there are multiple reasons this class of issue has existed for a long time:

• There's actually multiple different issues with similar symptoms
  
  
  
  • DNS caching due to long connection retention time + apiserver IP changing in a HA environment
  
  
  • Long connection retention time + broken load balancer + apiserver restart in HA environment
  
  
  • Long connection retention time + shared connection fate due to HTTP/2 + apiserver restart in HA environment
  
  
• Most people don't hit any of them, so reports are rare (for a long time I thought this was 100% broken load balancers)
• Most reporters aren't clear about what exactly has gone on

A confounding factor is that we do need to reuse connection for performance.

It still seems like calling Ping() periodically for HTTP/2 connections (like I suggested in #65012) and then recommending that everyone use HTTP/2 should solve all of the problems-- e.g. DNS should get re-resolved if the connection is closed.

[szuecs]

H2 + Ping() is probably the safest bet we have, because you test end-to-end.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[george-angel]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[szuecs]

/remove-lifecycle stale

[riaan53]

Hi - We suspect that we hit this issue by simply shutting down an HA node or pulling out a nodes network cable. Initially though it was a ingress-nginx loadbalancer issue until we tried Traefik and experienced the same +/15min of failing requests. For more information please see https://github.com/kubernetes/ingress-nginx/issues/3985

[szuecs]

@riaan53 If you want to try https://github.com/zalando/skipper as ingress http router, it does not use client-go and is safe to use in this case. Docs you can find at https://opensource.zalando.com/skipper/kubernetes/ingress-controller/

[gaorong]

we encounter the same problem. when our LB upgrade, the existing TCP connection to apiserver can't transport data, and kubernetes client-go didn't seem to detect this failure. Later, some of our nodes turn to NotReady status because the heartbeat message to apiserver can't transmit. As a result, lots of pods were evicted. Until we restart kubelet, everything was ok. (another alternative is waiting for kernel to close this stalled connection and client-go make another new connection)

this seems to be a serious preblem because the disconnection to apiserver can cause cluser-level failure. In an edge case, all nodes turn to NotReady status at the same time.
To avoid this situation and increase the self-heal speed, maybe we can try to connect to apiserver by creating another new TCP connection when network failure occurs.

[hpdvanwyk]

I have had some success doing the following workaround in the effected network namespace:

sysctl -w net.ipv4.tcp_retries2=7
sysctl -w net.ipv4.tcp_keepalive_time=40
sysctl -w net.ipv4.tcp_keepalive_probes=3
sysctl -w net.ipv4.tcp_keepalive_intvl=5

This of course is not a proper fix and depending on the circumstances possibly not even a good idea but if you are desperately looking for a workaround it is worth a try.

[lavalamp]

This is a bad experience, I'll try to find a volunteer to implement what is described in https://github.com/kubernetes/client-go/issues/374#issuecomment-413923008.

Another approach that might help a little bit is to limit the number of streams multiplexed over a single HTTP/2 connection. By default it's in the 100s; here is a theory which I haven't checked at all: it means if you get a broken connection everything fails and the retries are frequent enough to keep trying new streams on that broken connection, which keeps the broken connection's streams from going to 0, which keeps it from being closed and a new one started? I believe there is an env variable for this that the go library respects. (if this theory is true, that seems like a bug in the go http client.)

[lavalamp]

@mml suggested that it would be good if we had a broken test first, otherwise it's hard to verify fixes. Maybe one could be built with the integration testing framework somehow?

[mml]

Not sure if we can do it at the integration lewel, but at e2e lewel... see https://github.com/kubernetes/kubernetes/blob/master/test/e2e/chaosmonkey/chaosmonkey.go

[roycaihw]

/assign

[gaorong]

After diving into this problem, I'd like to share some phenomena and proof I have met.

1. I wrote a little go application (source code can be seen here), which setups a web server and listens to clients' connection, then creates another goroutine requesting to this server periodically(to emulate heartbeat with server). when connection established between server and client, I use iptables rule to drop packages sent from client (to emulate some network failure, iptables -I OUTPUT -p tcp --sport 50746 -j DROP), At this moment, the network was broken between client and server, In HTTP/1.1 scenario, the client will create another new connection and heartbeat nearly doesn't affected by this failure, however, in HTTP/2.0 scenario, the connection can't recreate and will stay a long time to recover. this turn out HTTP/2.0 do need a ping frame to detect network failure, as @lavalamp said.
   
   
   
   1. I found client-go use HTTP/2.0 by default and can be disabled by DISABLE_HTTP2 environment variable, you can find this logic at here, so I make the above test twice (which use iptables to simulate network failure) using kubelet with DISABLE_HTTP2 env variable on and off respectively, then as expected, In HTTP/1.1 scenario the connection to apiserver can recover quickly, whereas in HTTP/2.0 not.
   
   
2. gRPC, a robust protocol, also use http/2.0's ping fame to keep connection alive and identify failed connections, as in this article described:

The less clean version is where the endpoint dies or hangs without informing the client. In this case, TCP might undergo retry for as long as 10 minutes before the connection is considered failed. Of course, failing to recognize that the connection is dead for 10 minutes is unacceptable. gRPC solves this problem using HTTP/2 semantics: when configured using KeepAlive, gRPC will periodically send HTTP/2 PING frames. These frames bypass flow control and are used to establish whether the connection is alive. If a PING response does not return within a timely fashion, gRPC will consider the connection failed, close the connection, and begin reconnecting (as described above).

[szuecs]

@gaorong Your test works as I expect. The problem is that in Kubernetes we have long poll watch endpoints (require ping frames) and GET requests (will timeout if Transport is correctly setup). How is the behavior based on the different type of requests?

I guess:

• long poll with http/1.1 will timeout in TCP
• long poll with h2 has to be changed that we actually send PING frames
• GET with http/1.1 or h2 will timeout in TCP

This should be IMO created as e2e test.

A fix for http/1.1 should create proper timeout handling in Transport (DialContext is TCP), http.Transport and the non optional CloseIdleConnections() loop if we want to support DNS failover.
H2 should be similar, but additionally use the mentioned PING frames.

[gaorong]

I try to fix this issue and find it's hard to fix merely by changing kubernetes' code, It's necessary to have support from underlying x/net/http2 standard library. The ClientConn in x/net/http2 package has supported to send ping frame to peer, but the related default clientConnPool doesn't support this feature and clientConnPool is an internal struct, we couldn't reuse this struct and add our own logic. I opened an issue in golang/go#31643 and there is an ongoing corresponding PR. If that PR in golang is merged in the future, I will follow up with a PR to support Ping frame in kubernetes. but before that, we may need a short-term workaround.

[roycaihw]

@gaorong I converted your test setup described in https://github.com/kubernetes/client-go/issues/374#issuecomment-485423115 into an integration test https://github.com/kubernetes/kubernetes/pull/77957, to show the behavior difference in kubernetes w/ and w/o http2 enabled.

I agree it's hard to get access to http2 transport and manipulate underlying connections without changing http2 stdlib. https://github.com/golang/go/issues/31643#issuecomment-487317046 looks like what we need

[szuecs]

You can also copy the code to internal/ if necessary to make local changes to x/net/http2, which are not upstream, yet.
I know there were quite some complications, if I looked into it, because Go itself does it in some parts of the h2 implementation.

[gaorong]

@szuecs thanks to your reply, I have found a regression in kubelet and fixed it by kubernetes/kubernetes#78016 , also backported this fix to v1.14, you can upgrade your cluster and have another try when the next version released.

[szuecs]

@gaorong thanks! We are running 1.13, we have to soon upgrade to fix the issue. :)

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[george-angel]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[george-angel]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[george-angel]

/remove-lifecycle stale

[caesarxuchao]

https://github.com/golang/net/pull/55 is an attempt to fix the dead connection problem in the http2 library.

[caesarxuchao]

https://go-review.googlesource.com/c/net/+/198040 has been merged. It added periodic connection health check. It will be in go 1.15. We need to set the ReadIdleTimeout to enable the feature.

[hakman]

@caesarxuchao Now that Go 1.15 was released and it's already used by most kubernetes projects, can this be addressed?

[duyanghao]

@caesarxuchao @hakman I have created a k8s.io/apimachinery PR and a relevant golang/net PR for this problem, please take a look at it.

[liuyuan10]

Can anybody help me understand why tcp keepalive is not the answer here? If the load balancer is doing the right thing (by healthchecking apiservers and reset existing sessions, or it's a simple natting LB using iptables), I think tcp keepalive should solve the problem. And golang already has it enabled by default (see Dailer.Keepalive. So I would expect a dead connection to be closed in 2-3 mins. Not the best but at least better than 10+ mins that people are reporting.

[hpdvanwyk]

@liuyuan10 I wondered the same thing a while ago. Turns out for tcp keepalive to trigger the connection cannot have anything in its transmit buffer. Since something is usually trying to use the client connection there will almost always be some untransmitted bytes causing keepalive to never trigger.

[liuyuan10]

@hpdvanwyk Thanks for sharing. Even in that case, because there is pending data in transmit buffer, TCP will always try to send something. To me it basically acts as keepalive probes. Are you saying TCP will not shutdown the session when ack is not received for more than 10mins?

[JensErat]

I must admit I'm not totally sure why TCP keepalive didn't help (but the http2 timeouts _do_), but at least in our case we assume connections getting lost "somewhere in the network", not on either client or server. Finding the root cause seems very difficult, in large setups this only happens occasionally (but often enough to trigger every few hours with a few hundreds of servers), multiple infrastructure teams involved and we cannot run and store network traces at that large scale.

We haven't been able to finally trace down what happens, but after weeks/months of trouble, enabling the http2 timeouts brought relief. In fact, by enforcing the timeouts over our entire code base (we "hacked" the vendored x/net library) with a timeout and a debug statement in case the timeout triggered, we were able to actually trace back a whole bunch of random, weird issues to those lost connections al over our own codebase, not only Kubernetes (and actually, we first observed the issue after upgrading Prometheus after they started upgrading connections to http2).

In fact, we assume two root causes in our setup:

• VMware VIO load balancers closing connections without sending a RST package during reconfiguration in specific releases. I think they acknowledged this as a bug by now, maybe it is even resolved. I didn't personally track down the issue further after we applied our own http2 timeout patches. Still this didn't break Kubernetes often enough for us to actually reproduce and debug the issue.
• Some weird, very rare networking issues in a specific cross-datacenter connect. It happens that rarely, that we cannot really blame anybody for this (a network _will_ fail, we'll have to be able to deal with that).

Both of these causes have not been "frequent" at all (for frequent being a matter of scale) -- but if they occurred, they resulted in 15-30 minute outages including pages. Prometheus authors in fact stated a "normal" developer shouldn't even have to think about http2 timeouts, and Golang should have reasonable defaults. I somehow tend to agree with them.

[hpdvanwyk]

@liuyuan10 TCP will retry sending the packet and eventually shutdown the connection. From https://linux.die.net/man/7/tcp

tcp_retries2 (integer; default: 15; since Linux 2.2)
The maximum number of times a TCP packet is retransmitted in established state before giving up. The default value is 15, which corresponds to a duration of approximately between 13 to 30 minutes, depending on the retransmission timeout. The RFC 1122 specified minimum limit of 100 seconds is typically deemed too short.

This is why it takes 10+ minutes for the connection to be shut down if there is something in the transmit buffer instead of the 2 to 3 minutes you would expect if it was just TCP keepalives.

[JensErat]

@hpdvanwyk this pretty much exactly matches our observations, and sounds pretty reasonable. :+1: We had a 10 minutes delay in getting paged (so 10+ minutes of broken connection), but when not in office getting towards your laptop and connecting to the clusters was most often too slow to observe the issue -- already gone again. When we started measuring outage times, we never observed more than ~30 minutes until everything recovered on its own.

I'm really glad we finally have some kind of explanation for the observed recovery times. Networking is _way_ more complicated and involved than one would expect.

[yousong]

In case it may be of interest to anyone, calico accessing apiserver over ipvs also suffers [1] from this issue.

[1] https://github.com/projectcalico/libcalico-go/issues/1267

[liuyuan10]

@hpdvanwyk That's very convincing explanation. Thanks a lot. This basically makes tcp keepalive useless in our case....

[JensErat]

@yousong we had the issues all over our own code base (lots of automation code written in golang accessing the API server). Enforcing ReadIdleTimeout just magically made our entire test base and e2e tests much more robust, lots of the "we need to track that down" flakiness was gone.