Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a retry mechanism with exponential backoff and jitter for transient failures, aligning the router's behavior with the sglang model gateway. The changes include a new RetryConfig dataclass, CLI arguments for configuration, updated documentation, and logic in the request service to handle retryable HTTP status codes (408, 429, 500, 502, 503, 504). Feedback identifies a logic error where retries are effectively disabled by default due to the max_attempts calculation, the inclusion of an unused last_response variable, and a concern that blacklisting URLs for transient errors prevents retrying the same backend in single-node environments.
a5bac11 to
1c7980e
Compare
|
@ruizhang0101 could you please review the MR? Thanks! |
|
@aeon-x Could you take a look at this? |
|
Hey @ikaadil, i think retrying should be an optional, as most users would need fast fail over. Can you make sure that this retry mechanism is turned off unless it is explictly turned on by a flag? |
Done |
waelrabah11
left a comment
There was a problem hiding this comment.
Overall looks good to me. You can consider my comments as nitpicks but I just disagree with using the retry prefix. It's a bit redundant and could be avoided with documentation
…transient failures Signed-off-by: Ifta khairul Alam Adil <ikaadil007@gmail.com>
3d6eb6f to
f046579
Compare
Signed-off-by: Ifta khairul Alam Adil <ikaadil007@gmail.com>
Signed-off-by: Ifta khairul Alam Adil <ikaadil007@gmail.com>
Signed-off-by: Ifta khairul Alam Adil <ikaadil007@gmail.com>
Overview
Adds opt-in automatic retry for transient backend failures using exponential backoff with jitter, and folds the existing instance-failover reroute into the same mechanism.
Motivation: Mitigating the thundering herd problem
A forwarded request fails in one of two ways, which call for opposite responses:
408429500502503504Both draw on one budget:
--max-retries. Retrying is disabled by default, so a request is attempted exactly once unless--enable-retriesis passed.CLI
--enable-retries--max-retries5--initial-backoff-ms50--max-backoff-ms30000--backoff-multiplier1.5--jitter-factor0.2Without
--enable-retriesthe other flags are ignored, so a shared config template carrying them cannot change behaviour or block startup.vllm-router --port 8000 \ --service-discovery static \ --static-backends "http://localhost:9001,http://localhost:9002" \ --static-models "facebook/opt-125m,facebook/opt-125m" \ --routing-logic roundrobin \ --enable-retries \ --max-retries 5 \ --initial-backoff-ms 100 \ --max-backoff-ms 60000 \ --backoff-multiplier 2.0 \ --jitter-factor 0.1Backoff
With the defaults, retries land at
50ms,75ms,112.5ms,168.8ms, each spread across a +/-20% window. Without the jitter, routers that backed off from the same incident retry in lockstep and re-create the overload they backed off from.Changes
services/request_service/retry.pyRetryConfig(frozen, self-validating),RetryState(attempt sequencing),is_retryable_statusservices/request_service/request.pyroute_general_requestdrivesRetryState.attempts(); shared router dispatch extracted into_select_backend()parsers/parser.pyRetryConfigapp.pyapp.state.retry_config = RetryConfig.from_args(args)routers/routing_logic.pyValidation lives in
RetryConfig.__post_init__, so an invalid config cannot be constructed.Behaviour
Breaking change
--max-instance-failover-reroute-attemptsis removed. It rerouted a failed request to another engine and is now subsumed by--max-retries, which covers the same case and adds backoff.Its default was
0(a single attempt), which is also the default here, so deployments that never set it are unaffected. Deployments that did set it should migrate:Note for reviewers
process_requestreports a backend error status by yielding it (yield backend_response.headers, backend_response.status), not by raising, and there is noraise_for_statuson that call. A retry implementation that only catchesHTTPExceptionnever fires for an engine 5xx. This PR inspects the status after the firstanext, andaclose()s a discarded response so the upstream connection is released.Tests
56 new tests, 281 passing suite-wide.
test_retry_config.py— status classification, backoff growth, cap, jitter bounds, validation,from_argsmapping.test_request_retry.py— end to end throughroute_general_request: default single attempt, transport reroute, no-backoff-on-failover, single-engine retry, budget exhaustion passthrough, non-retryable passthrough, generator close on discard, exact sleep sequence.Every backend-status stand-in yields the status, matching
process_request; one that raisesHTTPExceptionwould pass whether or not retrying works.