Repository navigation
policy-controller: raise gRPC h2 connection window to avoid GOAWAYs - #15668
Conversation
|
Thanks for the PR. This looks like a fine solution but I would like to get the teams feedback if this is the best path forward. Are there any negative reprecutions to raising the initial window. Should we raise the limit even more? Should this be configurable? Curious why did you choose 8 MiB and if you can share how many watches that scales up to. It would also be good to have test to verify this behavior. |
|
On repercussions: mainly the per-connection flow-control buffer, which was previously unconfigured and defaulted to h2/hyper's much smaller built-in window. Raising it means the server can hold more buffered data per connection, so it's a memory/window tradeoff, not a free lunch. On the value: a 16 MiB connection window maps to an 8 MiB DATA-frame budget because h2's Auto scaling is On the test: none of the existing tests in policy-controller open a real h2/gRPC connection today - they all call the service/index logic in-process. A test that actually exercises this would mean standing up the real tonic server on a loopback socket and driving many streams with a raw h2 client (h2 would need to become a dev-dependency), which is more than a small addition and the first test of its kind in this crate. I'd rather build that once the window value/configurability question is settled so it isn't thrown away - or I can fold in the standalone harness I used for validation as a starting point if that's an acceptable scope. Let me know which you'd prefer. |
|
Raising the limit makes a lot of sense and setting a sane default also seems fine. I would like to see two things:
Better defaultSince a larger initial_window can result in higher memory usage I would like to keep that safety check. What it the minimum budget your cluster topology needs? The original issue mentioned 100-1000s watches but 35K is much larger than you need. The budget is refunded once a request completes so I would imagine we need some multiple of what you are operating. Maybe 5x so up to 5K concurrent watches? TestingWe should add test for this. I see two options. h2: Could you share you harness that you used for testing. Was it within kube or was it using a standalone h2 server? Most of this Within kube: |
|
Thanks, pushed 6c7c3aa. Default. The window is now 2.5 MiB, down from 16 MiB. h2 sizes the budget at half the window, so that gives a 1.25 MiB budget. Reading h2 0.4.19 more closely, I need to correct two things I said earlier:
At 2.5 MiB, the worst case (1-byte bodies) is just over 5,000 unread watches on one connection, the 5x you suggested. Real Watch() bodies are larger than 1 byte, so each costs less. I don't run a cluster of my own; the issue reports hundreds to low thousands of watches per proxy. I left the window as a constant rather than a flag, since a sane default seemed fine to you. Test. I went the h2 route.
Harness. The harness was a standalone h2 client and server, not running in kube. The tests use the same approach: many Watch()-framed streams on one connection, with a server that never reads them. They run over an in-memory duplex instead of a socket. I've updated the PR description to match. |
|
Pushed the smaller default and the h2 test last week; let me know if anything else would make this easier to review. |
There was a problem hiding this comment.
Looks good overall, just a few changes.
|
This change looks good to me. @pujitha24 Thank you for the patch! You might need to rebase your changes on main. |
Problem
The h2 upgrade from 0.4.15 to 0.4.17 added a per-connection budget that
charges for small, non-final HTTP/2 DATA frames, to guard against
frame-fragmentation memory amplification. In h2 0.4.17 this budget is a
hardcoded 25,600 bytes with no way to configure it.
Each policy Watch() request from a proxy sends its (small) request body
as a non-final DATA frame followed by a separate empty end-of-stream DATA
frame. A proxy with many outbound destinations opens many such watches on
one connection, and can exhaust the 25,600-byte budget after only a few
hundred watches, causing the policy controller to close the connection
with a GOAWAY(ENHANCE_YOUR_CALM). The proxy then has to reconnect and
re-establish every watch, repeating the problem.
Solution
h2 0.4.18/0.4.19 make the default ("Auto") budget scale with the
connection's HTTP/2 flow-control window instead of being a fixed 25,600:
budget = max(connection_window / 2, 25600). Neither tonic 0.14 nor hyper
1.x expose h2's new explicit data_frame_budget() builder method, but
tonic's Server::builder() does expose initial_connection_window_size,
which is threaded down to h2 and controls the Auto budget's scaling.
This change:
- Bumps the vendored h2 dependency from 0.4.17 to 0.4.19 (Cargo.lock
only; tonic already declares h2 = "0.4", no Cargo.toml change needed).
- Sets initial_connection_window_size to 16 MiB on the policy
controller's gRPC server, so the Auto data-frame budget scales up to
roughly 8 MiB instead of ~32 KiB, giving it enough headroom for a large
number of concurrent policy watches on a single connection.
This raises the number of small-frame watches a single connection can
sustain by roughly two orders of magnitude; it does not remove the limit
entirely, a sufficiently large connection could still exhaust it. It also
modestly increases the flow-control buffering the server may need to
track per connection, which was previously unconfigured (defaulting to
h2/hyper's much smaller built-in window).
Validation
Confirmed the mechanism by reading the vendored h2 0.4.17 and 0.4.19
sources (proto/streams/counts.rs, proto/connection.rs), and reproduced
the bug and the fix with a standalone h2 client/server harness (not part
of this diff) that opens many streams on one connection, each sending a
small non-final DATA frame followed by an empty end-of-stream DATA frame,
mirroring how a Watch() request is framed on the wire:
- h2 0.4.17 (previously pinned), default config: connection closed by the
server after ~9-110 accepted streams, matching the issue's "a few
hundred watches" report.
- h2 0.4.19, default window (no initial_connection_window_size
override): still closed after ~109-220 streams, bumping h2 alone is
not sufficient.
- h2 0.4.19 with initial_connection_window_size set to 16 MiB (this
change): survived roughly 35,000 accepted streams before the budget was
exhausted.
Ran, from the repository root:
- cargo fmt --all -- --check: clean.
- cargo build --frozen --workspace --exclude linkerd-policy-test
--all-features: succeeds.
- cargo clippy --frozen -p linkerd-policy-controller-runtime -p
linkerd-policy-controller-grpc --all-targets --no-deps --all-features:
no new warnings (one pre-existing, unrelated warning in
admission.rs). Note: this ran on a locally installed Rust 1.97.1
toolchain rather than this repo's pinned 1.90.0 (rust-toolchain.toml),
since rustup wasn't available in this environment; a full-workspace
--all-targets clippy run also surfaces 4 pre-existing lint errors in
policy-controller/k8s/index, confirmed via git stash to already exist
on main and unrelated to this change, most likely due to that same
toolchain drift.
- cargo test --frozen -p linkerd-policy-controller-runtime -p
linkerd-policy-controller-grpc --all-features: all 26 tests pass.
- Did not run cargo deny check (not installed in this environment) or
the policy-test integration suite (requires a live cluster).
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
Lower the gRPC server's initial connection window from 16 MiB to 2.5 MiB. h2 sizes its small-DATA-frame budget at half the connection window, and a non-final DATA frame shorter than 256 bytes costs 256 - len bytes until the server reads it. A 1.25 MiB budget therefore leaves room for more than 5,000 unread Watch() requests on one connection, even with 1-byte bodies, while keeping the flow-control window (and the data a connection can buffer) much smaller than before. Add h2-level tests that open 5,000 Watch()-shaped requests (a 1-byte non-final DATA frame, then an empty END_STREAM frame) on one connection against an h2 server that never reads the bodies: the connection survives with GRPC_INITIAL_CONNECTION_WINDOW_SIZE and is closed with hyper's default 1 MiB window. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-opus-5-5 (via Claude Code)
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
6e6b76c to
5ef39fb
Compare
|
Rebased onto current main (no conflicts), so this needs a force-push. The branch is otherwise unchanged from what you reviewed. After the rebase the two |
policy-controller: raise gRPC h2 connection window to avoid GOAWAYs
Problem
The h2 upgrade from 0.4.15 to 0.4.17 added a per-connection budget that
charges for small, non-final HTTP/2 DATA frames, to guard against
frame-fragmentation memory amplification. In h2 0.4.17 this budget is a
hardcoded 25,600 bytes with no way to configure it.
Each policy
Watch()request from a proxy sends its (small) request bodyas a non-final DATA frame followed by a separate empty end-of-stream DATA
frame. A proxy with many outbound destinations opens many such watches on
one connection, and can exhaust the 25,600-byte budget after only a few
hundred watches, causing the policy controller to close the connection
with a GOAWAY(ENHANCE_YOUR_CALM). The proxy then has to reconnect and
re-establish every watch, repeating the problem.
Solution
h2 0.4.18/0.4.19 make the default ("Auto") budget scale with the
connection's HTTP/2 flow-control window instead of being a fixed 25,600:
budget = max(connection_window / 2, 25600). A non-final DATA frame shorter
than 256 bytes costs
256 - lenbytes of that budget until the serverreads it, so the budget bounds how many small Watch() bodies on one
connection the controller can have left unread. Neither tonic 0.14 nor
hyper 1.x expose h2's explicit
data_frame_budget()builder method, buttonic's
Server::builder()exposesinitial_connection_window_size, whichis passed down to h2 and sets the Auto budget.
This change:
h2from 0.4.17 to 0.4.19 (Cargo.lock; tonic already declaresh2 = "0.4").initial_connection_window_sizeto 2.5 MiB on the policycontroller's gRPC server, for a 1.25 MiB budget: room for more than
5,000 unread Watch() requests on one connection, even with 1-byte
bodies. With h2 0.4.19 and hyper's default 1 MiB window (what tonic uses
when the window isn't set), the budget is 512 KiB, about 2,000.
h2andhttpas dev-dependencies of the runtime crate for thetests below.
This does not remove the limit. It sizes it for about 5x the hundreds to
low thousands of watches per proxy reported in the issue. The larger
window also lets one connection buffer up to 2.5 MiB of unread request
data, up from 1 MiB.
Validation
Confirmed the mechanism by reading the h2 0.4.17 and 0.4.19 sources
(
proto/streams/counts.rs,proto/streams/streams.rs,proto/connection.rs).args::tests(h2-level, no cluster needed) opens 5,000 Watch()-shapedrequests on one connection (a 1-byte non-final DATA frame, then an empty
END_STREAM frame) against an h2 server that accepts them and never reads
the bodies, then pings to confirm every frame was processed:
grpc_connection_window_allows_concurrent_watches: withGRPC_INITIAL_CONNECTION_WINDOW_SIZE, the connection survives.default_connection_window_does_not_allow_concurrent_watches: withhyper's default 1 MiB window, the connection is closed.
Ran, from the repository root:
cargo fmt --all -- --check-- clean.cargo clippy --frozen -p linkerd-policy-controller-runtime --all-targets --no-deps-- no new warnings (one pre-existing, unrelated warning inadmission.rs). This ran on a locally installed Rust 1.97.1 toolchain
rather than the pinned 1.90.0 (
rust-toolchain.toml), sincerustupwasn't available in this environment.
cargo test --frozen -p linkerd-policy-controller-runtime -p linkerd-policy-controller-grpc-- all 28 tests pass.cargo deny checkor thepolicy-testintegration suite(requires a live cluster).
Report: #15665
Fixes #15665