Repository navigation
Conversation
… filter Fixes InferencePool `failureMode: FailOpen` being treated the same as `FailClose`. `buildHTTPFilterForInferencePool` (internal/extensionserver/inferencepool.go) previously hardcoded `FailureModeAllow: false` on the injected `envoy.extensions.filters.http.ext_proc.v3.ExternalProcessor` HTTP filter. It now sets `FailureModeAllow` to `pool.Spec.EndpointPickerRef.FailureMode == gwaiev1.EndpointPickerFailOpen`. The InferencePool consumed at that point is reconstructed by `getInferencePoolByMetadata` from a `/`-delimited metadata string, which previously encoded only 6 fields (`namespace/name/serviceName/port/bodyMode/allowModeOverride`) and dropped `failureMode`. The encoding is extended to a 7th field: - `clusterRefInferencePool` (internal/extensionserver/post_cluster_modify.go) takes a new `failureMode` parameter and appends it to the formatted string. - `buildEPPMetadata` (internal/extensionserver/inferencepool.go) appends `string(inferencePool.Spec.EndpointPickerRef.FailureMode)` when building the metadata value, and its doc comment is updated to reflect the new format. - `getInferencePoolByMetadata` now requires 7 `/`-delimited fields (was 6), reads the 7th as `failureMode`, and sets it on the reconstructed pool's `EndpointPickerRef.FailureMode`. Tests updated accordingly: - `extensionserver_test.go`: existing `per_route_rule_inference_pool` metadata fixtures in `TestMaybeModifyClusterExtended`, `TestMaybeModifyListenerAndRoutes`, `TestPatchVirtualHostWithInferencePool`, and `TestBuildClustersForInferencePoolEndpointPickers` are updated to include the trailing `/FailClose` field. - `inferencepool_test.go`: - `TestBuildAndParseMetadata_RoundTrip` sets `FailureMode: gwaiev1.EndpointPickerFailOpen` on the input pool, asserts the encoded string ends in `/FailOpen`, and asserts the parsed pool's `FailureMode` matches. - `TestGetInferencePoolByMetadata_Malformed` fixture string is updated to the 7-field format. - `TestBuildHTTPFilterForInferencePool_Defaults` gains an assertion that `FailureModeAllow` is `false` when `failureMode` is unset. - New `TestBuildHTTPFilterForInferencePool_FailureMode` table test covers `FailOpen`, `FailClose`, and unset `failureMode` against `buildHTTPFilterForInferencePool`. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
dosubot
Bot
added
the
size:S
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2517 +/- ##
==========================================
- Coverage 85.58% 85.57% -0.02%
==========================================
Files 165 165
Lines 23318 23321 +3
==========================================
Hits 19956 19956
- Misses 2188 2190 +2
- Partials 1174 1175 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| result := strings.Split(metadata, "/") | ||
| if len(result) != 6 { | ||
| if len(result) != 7 { |
Contributor
There was a problem hiding this comment.
This strict len(result) == 7 check introduces a mixed-version upgrade risk.
During rolling upgrades, some extension callbacks can still produce the old 6-field metadata (namespace/name/serviceName/port/bodyMode/allowModeOverride), and this parser will return nil for those entries. That can make PostTranslateModify miss InferencePool routes/clusters and skip EPP patching.
Could we make this backward-compatible by accepting both 6 and 7 fields, defaulting failureMode to FailClose when absent?
Suggested shape:
6 fields => parse old format + failureMode = FailClose
7 fields => parse new format with explicit failureMode
kanurag94
reviewed
Aug 13, 2026
kanurag94
left a comment
kanurag94
left a comment
Contributor
There was a problem hiding this comment.
Thanks for this!
When FailOpen is used, once ext_proc is bypassed the client's own x-gateway-destination-endpoint reaches the ORIGINAL_DST cluster, which is configured with use_http_header: true on exactly that header. That means when EPP is down, it lets a client choose it's upstream. We probably should strip this client header.
This may not fix #2502 completely as #2502 also proposes an acceptable degraded behavior when EPP fails. I think this'd just 503. Please CMIIW
|
Hi @pujitha24, are you still working on this? |
missBerg
added
bug
Contributor
Author
|
Closing, and I'm not continuing this: as @kanurag94 noted and @YeiSimon confirmed on #2502, |
missBerg
pushed a commit
that referenced
this pull request
Oct 8, 2026
…tch (#2595) **Description** `patchListenerWithInferencePoolFilters` appends one endpoint-picker `ext_proc` HTTP filter per entry in the `inferencePools` slice it is given. That slice is built in `maybeModifyListenerAndRoutes` from `routeNameToVHRouteNameToInferencePool`, which is keyed by 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 local `poolFilters` slice and are not spliced into `httpConManager.HttpFilters` until after the loop, so the guard never sees what the loop itself just added. Because `httpFilterNameForInferencePool` is derived from the pool's namespace and name, every duplicate shares one filter name. Per-route `typed_per_filter_config` is 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: - 32 `adding inference pool ext proc filter` log lines per translation pass (16 per pool) - 32 identically-named `ext_proc` filters in the listener filter chain - 16 gRPC streams to the EPP, i.e. 16 full scheduling passes, for one request The 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-scorer` rotated its LRU choice on every one of the 16 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`, which generates 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 added test fails there without this patch. **Repro** 1. Revert this patch, or apply only the added test. 2. Run `go test ./internal/extensionserver -run 'TestPatchListenerWithInferencePoolFilters/repeated_inference_pool_is_added_once' -count=1`. 3. The test fails: four entries for one pool produce five filters (four identical `envoy.filters.http.ext_proc/endpointpicker/pool1_test-ns_ext_proc` plus the router) where two are expected. **Fix** Two changes, either of which resolves the reported behavior; together they also guard the other call path: 1. Deduplicate by `namespace/name` when building `listenerToInferencePools`, so the slice handed to `patchListenerWithInferencePoolFilters` holds each pool once per listener. 2. Track filter names added within the loop in `patchListenerWithInferencePoolFilters`, so the duplicate 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 precommit` passes, with no generated-file churn. - `make test` passes. - The added test fails on `main` without the fix (five filters for one pool where two are expected) 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: | Measurement | v1.0.0 | this patch | |---|---|---| | `adding inference pool ext proc filter` per translation pass | 32 | 2 | | EPP gRPC streams per client request | 16 | 1 | | Distinct replicas chosen for 5 distinct cold prompts | 1 of 2 | 3/2 split across both | Controller logs showed no errors across the rollout and the translation pass. The 44 per-route `disabling inference pool filter` lines are unchanged and remain correct: two HTTPRoutes with 16 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.go` and `post_cluster_modify.go`, so the two should rebase cleanly 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/name` rather than the filter name, since `httpFilterNameForInferencePool` already 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. Signed-off-by: Gerrit van Doorn <gerritd@dropbox.com> Co-authored-by: Ignasi Barrera <ignasi@tetrate.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This commit fixes InferencePool
failureMode: FailOpenbeing treated the same asFailClose.buildHTTPFilterForInferencePool(internal/extensionserver/inferencepool.go) previously hardcodedFailureModeAllow: falseon the injectedenvoy.extensions.filters.http.ext_proc.v3.ExternalProcessorHTTP filter. It now setsFailureModeAllowtopool.Spec.EndpointPickerRef.FailureMode == gwaiev1.EndpointPickerFailOpen.The InferencePool consumed at that point is reconstructed by
getInferencePoolByMetadatafrom a/-delimited metadata string, which previously encoded only 6 fields (namespace/name/serviceName/port/bodyMode/allowModeOverride) and droppedfailureMode. The encoding is extended to a 7th field:clusterRefInferencePool(internal/extensionserver/post_cluster_modify.go) takes a newfailureModeparameter and appends it to the formatted string.buildEPPMetadata(internal/extensionserver/inferencepool.go) appendsstring(inferencePool.Spec.EndpointPickerRef.FailureMode)when building the metadata value, and its doc comment is updated to reflect the new format.getInferencePoolByMetadatanow requires 7/-delimited fields (was 6), reads the 7th asfailureMode, and sets it on the reconstructed pool'sEndpointPickerRef.FailureMode.Tests updated accordingly:
extensionserver_test.go: existingper_route_rule_inference_poolmetadata fixtures inTestMaybeModifyClusterExtended,TestMaybeModifyListenerAndRoutes,TestPatchVirtualHostWithInferencePool, andTestBuildClustersForInferencePoolEndpointPickersare updated to include the trailing/FailClosefield.inferencepool_test.go:TestBuildAndParseMetadata_RoundTripsetsFailureMode: gwaiev1.EndpointPickerFailOpenon the input pool, asserts the encoded string ends in/FailOpen, and asserts the parsed pool'sFailureModematches.TestGetInferencePoolByMetadata_Malformedfixture string is updated to the 7-field format.TestBuildHTTPFilterForInferencePool_Defaultsgains an assertion thatFailureModeAllowisfalsewhenfailureModeis unset.TestBuildHTTPFilterForInferencePool_FailureModetable test coversFailOpen,FailClose, and unsetfailureModeagainstbuildHTTPFilterForInferencePool.Related Issues/PRs (if applicable)
Fixes InferencePool
failureMode: FailOpenbeing treated the same asFailClose.Special notes for reviewers (if applicable)
None.
AI assistance: this change was drafted with Claude Code.
Fixes #2502