Stop Envoy reusing upstream connections to actors - #549
Stop Envoy reusing upstream connections to actors#549Chuang Wang (chuangw6) wants to merge 1 commit into
Conversation
Envoy pools HTTP/1.1 connections by destination address, which is safe when an address means one stable server. For actors it does not. A worker pod keeps its IP for its whole lifetime, but the actor sandbox behind port 80 is torn down on every Suspend and a different actor takes the slot. A pooled connection therefore belongs to an actor that may already be gone, and reusing it races the far end's close: the request goes into a dead socket and Envoy synthesizes a 503. At 50 actors churning across 50 workers this was ~42% of pings failing, with Envoy reporting 450 upstream_cx_destroy_remote_with_active_rq. Setting max_requests_per_connection to 1 took it to 0% and 0. The handshake this costs is sub-millisecond in-cluster against pings that take ~22ms, and it removes the race rather than recovering from it after the fact. Carrying HttpProtocolOptions on a dynamic_forward_proxy cluster also needs allow_insecure_cluster_options. Envoy guards that because a DFP cluster forwards to whatever host the request names, so a TLS upstream must validate against it; this cluster has no transport socket at all and the authority is an IP literal. Without the flag Envoy NACKs every CDS push and drops the cluster, which looks like "all actor traffic 503s" rather than a config error, so there is a test pinning it.
Max Smythe (maxsmythe)
left a comment
Max Smythe (maxsmythe)
left a comment
There was a problem hiding this comment.
Mostly nits, but the hard-coding of HTTP/1.1 is something I'd like to understand the impact of more.
Bowei Du (@bowei) thoughts?
| // is destroyed on every Suspend and a different actor takes the slot. A | ||
| // pooled connection therefore belongs to an actor that may already be gone, | ||
| // and reusing it races the far end's close: the request goes into a dead | ||
| // socket and Envoy synthesizes a 503. Measured at ~40% of pings with 50 |
There was a problem hiding this comment.
We should avoid citing specific latency/measurement references as those are coupled with infrastructure and may quickly go stale as project progresses. Noting the shape of the concern and that it has been observed should be enough to help with Chesterton's Fence in the future.
| }, | ||
| UpstreamProtocolOptions: &httpv3.HttpProtocolOptions_ExplicitHttpConfig_{ | ||
| ExplicitHttpConfig: &httpv3.HttpProtocolOptions_ExplicitHttpConfig{ | ||
| // HTTP/1.1 upstream, matching what actors serve on port 80. |
There was a problem hiding this comment.
Do we know what actors will be serving on port 80? Currently we are assuming HTTP/1.1 as a simplification. That may not always be true.
Not familiar enough with envoy to know how limiting this configuration will be on future updates. Are we precluded from supporting streaming connections, proto servers, etc. in the future with this choice?
There was a problem hiding this comment.
I think these are all defaults so nothing is locking us into http 1.1
3 participants
Problem
Envoy pools HTTP/1.1 connections by destination address. That is safe when an address means one stable server, and for actors it does not. A worker pod keeps its IP for its whole lifetime, but the actor sandbox behind port 80 is torn down on every Suspend and a different actor takes the slot. A pooled connection therefore belongs to an actor that may already be gone, and reusing it races the far end's close.
Symptom
The request goes into a dead socket and Envoy synthesizes a 503. At 50 actors churning across 50 workers, ~42% of pings failed, with Envoy reporting 450
upstream_cx_destroy_remote_with_active_rq.Solution
Set
max_requests_per_connectionto 1 on the dynamic forward proxy cluster. Failures and the reset counter both went to 0.Notes
The handshake this costs is sub-millisecond in-cluster, against pings that take ~22ms. It removes the race rather than recovering from it after the fact, which is why this is preferred over a route-level retry policy.
Carrying
HttpProtocolOptionson adynamic_forward_proxycluster also requiresallow_insecure_cluster_options. Envoy guards that because a DFP cluster forwards to whatever host the request names, so a TLS upstream must validate against it. This cluster has no transport socket at all and the authority is an IP literal, so there is nothing to validate. Without the flag Envoy NACKs every CDS push and drops the cluster, which presents as "all actor traffic 503s" rather than as a config error, so there is a test pinning it.Independent of the benchmarking stack (#542 / #543 / #545); this only touches
cmd/atenet/internal/router.