Repository navigation
Conversation
✅ Deploy Preview for theagentrouter canceled.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
80b213b to
7d6a1a0
Compare
|
/retest |
1 similar comment
|
/retest |
39bc12c to
b7a5631
Compare
|
Key changes: http_filters:
- name: envoy.filters.http.ext_proc/endpointpicker/primary-inference-pool_inference-conformance-app-backend_ext_proc
typed_config:
'@type': type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExternalProcessor
grpc_service:
envoy_grpc:
authority: primary-endpoint-picker-svc.inference-conformance-app-backend.svc:9002
cluster_name: envoy.clusters.endpointpicker_primary-inference-pool_inference-conformance-app-backend_ext_proc
message_timeout: 300s
processing_mode:
request_body_mode: FULL_DUPLEX_STREAMED
request_header_mode: SEND
request_trailer_mode: SEND
response_body_mode: FULL_DUPLEX_STREAMED
response_header_mode: SEND
response_trailer_mode: SEND
- name: envoy.filters.http.ext_proc/endpointpicker/secondary-inference-pool_inference-conformance-app-backend_ext_proc
typed_config:
'@type': type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExternalProcessor
grpc_service:
envoy_grpc:
authority: secondary-endpoint-picker-svc.inference-conformance-app-backend.svc:9002
cluster_name: envoy.clusters.endpointpicker_secondary-inference-pool_inference-conformance-app-backend_ext_proc
message_timeout: 300s
processing_mode:
request_body_mode: FULL_DUPLEX_STREAMED
request_header_mode: SEND
request_trailer_mode: SEND
response_body_mode: FULL_DUPLEX_STREAMED
response_header_mode: SEND
response_trailer_mode: SEND
- name: envoy.filters.http.header_to_metadata
typed_config:
'@type': type.googleapis.com/envoy.extensions.filters.http.header_to_metadata.v3.Config
request_rules:
- header: agent-session-id
on_header_present:
key: session.id
metadata_namespace: io.envoy.ai_gatewayRDS: weighted_clusters:
clusters:
- name: httproute/inference-conformance-app-backend/httproute-weighted-two-pools/rule/0/inferencepool/primary-inference-pool
typed_per_filter_config:
envoy.filters.http.ext_proc/endpointpicker/secondary-inference-pool_inference-conformance-app-backend_ext_proc:
'@type': type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExtProcPerRoute
disabled: true
weight: 70
- name: httproute/inference-conformance-app-backend/httproute-weighted-two-pools/rule/0/inferencepool/secondary-inference-pool
typed_per_filter_config:
envoy.filters.http.ext_proc/endpointpicker/primary-inference-pool_inference-conformance-app-backend_ext_proc:
'@type': type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExtProcPerRoute
disabled: true
weight: 30
|
|
/retest |
1 similar comment
|
/retest |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@zirain curious why we scoped this to HTTPRoute only and didn’t include AIGatewayRoute as well? A couple of follow-ups on the AIGatewayRoute side:
|
We could do that later, the main goal is for GAIE conformance test. |
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
fad2e92 to
bc3c5e8
Compare
Signed-off-by: zirain <zirain2009@gmail.com>
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
| if !ok { | ||
| return nil, fmt.Errorf("could not determine owning HTTPRoute rule from route name") | ||
| } | ||
| httpRoute, err := s.retrieveAndCacheHTTPRoute(ctx, cache, key) |
There was a problem hiding this comment.
The HTTPRoute (and, at :817, each InferencePool's appProtocol) is still read through s.k8sClient. That is the cached mgr.GetClient() (cmd/controller/main.go:486), and nothing orders it against Envoy Gateway's own HTTPRoute watch. The impact is now larger, because failures here are fatal. A newly created HTTPRoute that Envoy Gateway translates before the controller cache sees it hits HTTPRoute ... not found (:806). A rule just appended can hit rule index ... out of range (:810). Either one fails the whole PostTranslateModify, not just this route. A weight-only edit can still program the old weights silently.
Use an uncached reader (mgr.GetAPIReader()) for these Gets. Or treat not-found and out-of-range as "leave this route merged and narrow it to one pool" rather than a global error. Either way, document the remaining window. I could not observe how large the lag is in practice.
| } | ||
| if len(weighted) == 0 { | ||
| if len(otherClusterWeights) == 0 { | ||
| return nil, fmt.Errorf("route %q has no non-zero-weight InferencePool backendRef and no other backend to keep", route.Name) |
There was a problem hiding this comment.
Setting every InferencePool backendRef to weight: 0 is valid in Gateway API, and it is a normal way to drain a rule. The spec says such requests get a 500. Here it returns an error, and splitWeightedInferencePoolRoutes (:759-760) and PostTranslateModify (:148-150) pass it up. The extension hook then fails for every listener, route and cluster on the gateway, not only this rule. manifests/envoy-gateway-values.yaml doesn't set failOpen, so one tenant's HTTPRoute can block every other xDS update until it is fixed. What Envoy Gateway does on a hook error I could not determine from this repo.
Treat it as config, not an internal error. Rewrite the route to a DirectResponse 500, or to a WeightedClusters with only Envoy Gateway's invalid-backend-cluster. Set the route metadata to an empty pool list, as the drain branch already does. The "all weights zero fails the translation" table case would then become a positive test.
if len(otherClusterWeights) == 0 {
// Every backendRef has weight 0: Gateway API says to return 500 for this rule only.
clone.Action = &routev3.Route_DirectResponse{DirectResponse: &routev3.DirectResponseAction{Status: 500}}
return clone, nil
}| } | ||
| clusterWeights := make([]*routev3.WeightedCluster_ClusterWeight, 0, len(otherClusterWeights)+len(weighted)) | ||
| for _, cw := range otherClusterWeights { | ||
| // A non-InferencePool backend: no endpoint picker may run for it. |
There was a problem hiding this comment.
This keeps the Service's ClusterWeight and turns off every EPP filter on it, so the request goes to the Service's cluster with no x-gateway-destination-endpoint header. Per the comment at :1057-1059 and the test fixture ("EG hands the route's extension resources to every per-backendRef cluster of the rule, so the Service's cluster carries the pools' metadata too"), that cluster also went through PostClusterModify. There, handleInferencePoolCluster set it to ORIGINAL_DST / CLUSTER_PROVIDED with UseHttpHeader, and set EdsClusterConfig = nil (post_cluster_modify.go:73-97). It no longer points at the Service's endpoints. Its traffic share then goes to whatever ORIGINAL_DST falls back to when the header is missing, not to the Service. splitWeightedInferencePoolRoutes never removes or restores that cluster.
The tests hide this. Both mixed-backend cases build svcCluster with createMergedInferencePoolCluster (ORIGINAL_DST) and only check the route side. Options: in PostClusterModify, only apply handleInferencePoolCluster when the cluster is actually an InferencePool backend. Envoy Gateway's per-backendRef cluster name (.../backend/<i>) together with the HTTPRoute tells you which ones are. Alternatively, reject mixed rules for now and document that. Add a test that checks the Service cluster's discovery type and EDS config after the full PostClusterModify + PostTranslateModify sequence.
| for _, vh := range rc.VirtualHosts { | ||
| for i, route := range vh.Routes { | ||
| pools := getInferencePoolsByMetadata(route.Metadata) | ||
| if len(pools) < 2 { |
There was a problem hiding this comment.
The new mixed-backend support only starts when the route carries two or more pools. Take a rule with one InferencePool and a Service. Per the comment at :822-825, Envoy Gateway emits a WeightedClusters with only the Service, so the pool gets no traffic. The route metadata still names the pool, so patchVirtualHostWithInferencePool leaves its EPP on for the Service traffic. That behaviour predates this PR. But the PR now handles two pools plus a Service and not one pool plus a Service, which is surprising. Also enter the split when the route action is WeightedClusters and the metadata has at least one pool, or document the gap. Neither doc page mentions mixing pools with other backend kinds.
| } | ||
| clone.Name = name | ||
| buildEPPMetadataForCluster(clone, []*gwaiev1.InferencePool{pool}) | ||
| setClusterMetadataBackendName(clone, pool.Namespace, pool.Name, httpRouteName, routeRuleIndex, backendRefIndex) |
There was a problem hiding this comment.
Why stamp per_route_rule_backend_name on HTTPRoute clones? In production, only maybeModifyCluster writes it, and only for AIGatewayRoute-owned clusters (:389, :451). For a plain HTTPRoute nothing sets it on the merged cluster. The mixed-backend test sets it on its input clusters by hand, which doesn't happen in production. If a reader needs it, say which one in a comment. Note also that the namespace segment is the pool's namespace, not the route's, unlike the AIGatewayRoute callers that pass aigwRoute.Namespace. If nothing reads it, drop it.
|
wait for #2657, marked this as draft. |
Description
Add proposal for Weighted Routing Across Multiple InferencePools, this's generally for GAIE GatewayWeightedAcrossTwoInferencePools conformance test
Related Issues/PRs (if applicable)
xref: #2638
Special notes for reviewers (if applicable)