Repository navigation
fix: add one InferencePool ext proc filter per pool, not per route match - #2595
Conversation
Signed-off-by: Gerrit van Doorn <gerritd@dropbox.com>
|
Ping — this has been open since Aug 28 with no CI runs. As a first-time Why it may be worth a look: when an InferencePool is referenced by N route The change is ~17 lines plus a regression test that fails on Happy to adjust the approach, split it, or add cases — just let me know. |
|
Independent confirmation from a separate cluster, with a single InferencePool. Environment
Observed A single Against this patch A test driving With a single replica the routing outcome is unaffected, but EPP load and its metrics are inflated 16x. With more replicas this becomes the scheduler-state problem described in the PR. |
✅ Deploy Preview for theagentrouter canceled.
|
nacx
left a comment
There was a problem hiding this comment.
Thanks for the patience and apologies for the late review.
Overall LGTM. @kanurag94 mind taking a look?
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Observed on 1.1.0 with Envoyproxy 1.39.x as well. Would be great to get this one on the next release. |
|
/retest |
|
@missBerg Is this a flake and should I run another retest? |
|
/retest |
Description
patchListenerWithInferencePoolFiltersappends one endpoint-pickerext_procHTTP filterper entry in the
inferencePoolsslice it is given. That slice is built inmaybeModifyListenerAndRoutesfromrouteNameToVHRouteNameToInferencePool, which is keyedby xDS route name — so a pool referenced by N route matches appears N times, and the
listener ends up with N identically-named copies of the same filter.
There is already a guard against duplicates, but it only searches
httpConManager.HttpFilters. New filters accumulate in the localpoolFiltersslice and arenot spliced into
httpConManager.HttpFiltersuntil after the loop, so the guard never seeswhat the loop itself just added.
Because
httpFilterNameForInferencePoolis derived from the pool's namespace and name, everyduplicate shares one filter name. Per-route
typed_per_filter_configis keyed by filter name,so the existing per-route disable logic cannot express "enable one of these and disable the
rest" — it can only enable or disable the whole set. On the pool's own routes the set is
enabled, so every copy runs and opens its own gRPC stream to the EPP.
Impact for a single client request, on a Gateway with two InferencePools whose HTTPRoutes each
have 16 InferencePool-backed matches:
adding inference pool ext proc filterlog lines per translation pass (16 per pool)ext_procfilters in the listener filter chainThe last point is the damaging one. It is not only wasted latency and 16x EPP load: each
redundant scheduling pass independently mutates EPP scheduler state, which makes stateful
plugins unusable. In our case
no-hit-lru-scorerrotated its LRU choice on every one of the16 passes, so cold requests never actually spread across replicas, and prefix-cache affinity
accounting was meaningless.
Reducing the HTTPRoute from 16 InferencePool-backed matches to 12 reduced the EPP calls from
16 to 12, confirming the count tracks match count exactly.
Observed on Envoy AI Gateway v1.0.0 with Envoy Gateway v1.8.1 (the pinned pairing from the
v1.0.0 dependency table), Envoy Proxy v1.38.x, using KServe 0.20
LLMInferenceService, whichgenerates an HTTPRoute with 8 InferencePool-backed rules — one of them carrying 8 path
matches — for a total of 16. Both affected code paths are unchanged on
main, and the addedtest fails there without this patch.
Repro
go test ./internal/extensionserver -run 'TestPatchListenerWithInferencePoolFilters/repeated_inference_pool_is_added_once' -count=1.envoy.filters.http.ext_proc/endpointpicker/pool1_test-ns_ext_procplus the router)where two are expected.
Fix
Two changes, either of which resolves the reported behavior; together they also guard the
other call path:
namespace/namewhen buildinglistenerToInferencePools, so the slicehanded to
patchListenerWithInferencePoolFiltersholds each pool once per listener.patchListenerWithInferencePoolFilters, so theduplicate guard accounts for filters not yet spliced into the chain.
No behavior change for the single-match case, and none for distinct pools on a shared
listener: both existing subtests (
listener with new inference pool filter,listener with multiple inference pools) pass unmodified.Verification
make precommitpasses, with no generated-file churn.make testpasses.mainwithout the fix (five filters for one pool where two areexpected) and passes with it.
Beyond the unit tests, the patched controller was deployed to a staging cluster running the
two-InferencePool KServe 0.20 setup described above, replacing the v1.0.0 controller image.
The patch applied there is identical to this one. Measured before and after, same cluster,
same HTTPRoutes:
adding inference pool ext proc filterper translation passController logs showed no errors across the rollout and the translation pass. The 44 per-route
disabling inference pool filterlines are unchanged and remain correct: two HTTPRoutes with16 InferencePool-backed matches each disable the other pool's filter, and 3 Service-backed
catch-all matches each disable both.
Cache affinity is preserved rather than traded away for spreading: re-sending an already-seen
prompt three times pinned to the replica that first served it, scoring 2 points higher there
from the prefix-cache scorer, while cold prompts continued to alternate.
Related Issues/PRs (if applicable)
envoyproxy/gateway#5002 fixed the same class of defect in Envoy Gateway, where extension
filters accumulated one copy per reference and produced multiple matches while processing
routes.
Touches the same InferencePool extension-server path as #2142 (preserve InferencePool metadata
on extension hooks). Overlaps #2517 only in
internal/extensionserver/extensionserver_test.go;that PR changes
inferencepool.goandpost_cluster_modify.go, so the two should rebasecleanly past each other.
For context, istio/istio#58392 describes the inverse failure in a different implementation:
multiple InferencePools on one Gateway losing the ext_proc config for all but the first.
Special notes for reviewers (if applicable)
The dedupe key is
namespace/namerather than the filter name, sincehttpFilterNameForInferencePoolalready derives from both; using the object identity keeps the intent clear at the call site.
AI-assisted patch prep; I reviewed the code and own the change.