envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.71k stars 4.75k forks source link

proposal: add connection-level L4 idle timeout support to clusters #9231

Open goaway opened 4 years ago

goaway commented 4 years ago

Title: Add connection-level L4 idle timeout support to clusters

Description: The logic would effectively be what's currently present in the TCP proxy connection timeout: https://www.envoyproxy.io/docs/envoy/latest/api-v2/config/filter/network/tcp_proxy/v2/tcp_proxy.proto#envoy-api-field-config-filter-network-tcp-proxy-v2-tcpproxy-idle-timeout

This differs from the existing connection idle timeouts currently supported. Current timeouts consider the number of active requests on a connection and the timeout tracks the interval when no requests are present. The new proposed timeout would track the interval between bytes being sent or received.

We'd like to use this on Envoy Mobile to get earlier indications of defunct connections, independent of request timeouts. Request idle timeouts don't quite serve our purpose since we'd like requests to be able to be relatively idle as long as we have reason to believe the connection is healthy. We also generally have a high volume of requests, leaving us without timely insight into the health of an underlying connection. Obviously if there are no active requests, this essentially replicates the behavior of the existing idle timeouts, but it differs in a few important ways when there are active requests:

  1. It's a signal for the health of the connection overall, not just the requests in flight, and can be used to trigger the establishment of new connections in the event that its hit.
  2. It allows us to have relatively idle requests (e.g. for streaming), but still get a timely signal of an unhealthy connection.

In theory we could implement a poorer version of this ourselves without an upstream Envoy change that would instead rely on any L7 transmissions per session. We propose an upstream Envoy change because we think this mechanism could be generally useful, because we'd get a better quality signal at L4, and because we could support more sophisticated behavior (e.g. h2 pings could be leveraged to heartbeat the connection and keep it alive in the absence of requests).

goaway commented 4 years ago

cc @junr03 @mattklein123

mattklein123 commented 4 years ago

Thanks for opening this issue @goaway. The reason that I continue to waffle on this, as we discussed IRL, is that it seems a bit strange to be tracking connection health with a bytes transferred metric when individual request idle timeouts do not work. I.e., what if there are many active requests that all have no data? It seems like a feature that is very specific to a particular request pattern in which you know that there should be some request that is transferring data.

What I'm I'm wondering is if there is some way of handling this via outlier detection, with the outlier detection action being connection closure? This is discussed here: https://github.com/envoyproxy/envoy/issues/9112#issuecomment-557979406 (that entire issue is likely relevant to that conversation).

@snowp @alyssawilk any thoughts on this?

goaway commented 4 years ago

I do see where you're coming from @mattklein123. In support of this approach though, it is precisely because we'd like (at least some) request idle timeouts to be generally long, that this mechanism is appealing to us. Some, but not all, of our requests we expect to be relatively idle (specifically, streaming "push"). Sure we can tune other "busy" request timeouts to be shorter, but if those requests start timing out because the connection has failed, it'd probably make sense to re-establish those streaming requests as well. Moreover, at this point we probably want to ensure we're not retrying these requests on the same socket/connection.

The outlier detection idea is interesting, but it only works if we have those busy requests with short timeouts alongside our fairly-idle requests to provide an indication the connection is unhealthy. (It's been a while since I've looked though - can active healthchecking trigger outlier detection in the absence of "real" requests? Can h2 pings be used for active healthchecking?)

On the other hand, the above proposal could trigger re-establishment of connections even in the absence of such requests - which might not be necessary, but seems more defensive, and probably(?) doesn't carry a lot of downside (especially if we are sending periodic heartbeats).

The fact that this would be configurable to me sort of feels like there's not much downside. Setups where this doesn't make sense simply wouldn't need to configure a timeout.

mattklein123 commented 4 years ago

A few thoughts: 1) This only really applies to HTTP/2, right? In the sense that for HTTP/1.1 a per-request idle timeout would be functionally (though not logically) equivalent. 2) What if we had an option in the HTTP/2 connection pool to allow periodic PING and then a disconnect if the response is not received in a certain amount of time? This seems like a more direct approach to achieving what we want? I actually think this would be very useful not just for mobile but also for HTTP/2 backhaul from POPs, etc.

cc @bobby-stripe @snowp any thoughts on ^?

goaway commented 4 years ago
  1. Essentially, yes.
  2. I think this would be super useful and entirely serve our purposes. It seems like it would address this older gRPC issue https://github.com/envoyproxy/envoy/issues/6464 as well.

If folks like the idea of (2), it's something that I would be happy to work on.

snowp commented 4 years ago

If this is limited to H/2 as you said then yeah I think periodic PINGs are a great solution.

mattklein123 commented 4 years ago

@goaway one other thought. What about TCP keep alives? Are we concerned that might for example work only to the first NAT hop and not reach all the way to the server edge?

goaway commented 4 years ago

The fact that h2 ping is pretty much guaranteed (at least over TLS) to make it through through end-to-end unmolested makes it feel preferable to me. But TCP keep-alive could be a serviceable alternative.

If we send h2 pings, it becomes a stat we can start emitting as well, which is nice.

mattklein123 commented 4 years ago

TCP keep alive will work out of the box today and might be good enough for your immediate need. I like the idea of adding PING support to the HTTP/2 connection pool, so we could do that separately, or just wait and do it in the context of https://github.com/envoyproxy/envoy/issues/7403 which will unify the HTTP/1 and HTTP/2 connection pools to a large degree.

bobby-stripe commented 4 years ago

What if we had an option in the HTTP/2 connection pool to allow periodic PING and then a disconnect if the response is not received in a certain amount of time.

This feels like a good approach to me!