fix(flowcontrol): apply effective defaults for DefaultRequestTTL and per-band MaxRequests#2123
Conversation
…per-band MaxRequests Neither field had an effective default despite documentation claiming otherwise: an unset defaultRequestTTL applied no TTL at all, and the per-band MaxRequests doc referenced a defaultPriorityBandMaxRequests constant that did not exist. A default configuration could therefore queue requests with no time or count bound until the per-band byte limit (1 GB). - Default DefaultRequestTTL to 60s in controller.NewConfig. The TTL is a queue-wait budget: a request that cannot dispatch within it is shed with a retryable backpressure error rather than served with severely degraded time-to-first-token. It is the only bound on queue wait when neither the client nor the gateway enforces a request deadline (the well-lit guides configure no gateway request timeout); deadlines that fire sooner evict via context cancellation instead. An explicit 0s disables the TTL (disconnect-only), distinguishable from unset via the pointer-typed API field. - Default per-band MaxRequests to 5000 in applyDefaults, mirroring the MaxBytes handling: band-level 0 is treated as unset and receives the default. The pair is self-consistent: under a dispatch halt, occupancy self-limits at arrival rate x TTL, so the count cap only binds above ~83 req/s per band. - Global limits remain unlimited by default (omitted or explicit 0); band count is bounded by the distinct priorities defined in InferenceObjectives. - Fix stale docs: FlowGCTimeout default (5 minutes, not 1 hour), the nonexistent maxBytes-0-drops-immediately claim on DefaultNegativePriorityBand (band-level 0 has never meant zero capacity), and the InitialEffectiveTTL adapter docstring. Fixes llm-d#2095 Part of llm-d#1187 Signed-off-by: Luke Van Drie <lukevandrie@google.com>
|
There is one consequence of the 60s TLL default worth deciding. The queue also serves as the scale-from-zero waiting room (with an empty pool, requests queue instead of failing). The default TTL cuts that room to 60s. Cold starts may take O(minutes), and the chart's route timeout would have carried them, so a scale-from-zero deployment on pure defaults now sheds everything during cold start (after 60s). Options:
[3] is probably the right end state, but it's new mechanism. This is the biggest user-facing decision for enablement by default, so we want to make sure we get it right. |
|
cc: @ahg-g, @RishabhSaini |
| // time-to-first-token. It is also the only bound on queue wait when neither the client nor the | ||
| // gateway enforces a request deadline (the well-lit guides configure no gateway request timeout); | ||
| // where such deadlines exist and fire sooner, context cancellation evicts the request first. | ||
| defaultRequestTTL = 60 * time.Second |
There was a problem hiding this comment.
Requests can come with a timeout in their context, right? if so, what happens if this is lower than the timeout set by the client?
There was a problem hiding this comment.
The two deadlines compose with minimum-wins semantics.
We derive the queue context via context.WithDeadlineCause(ctx, enqueueTime.Add(TTL), ErrTTLExpired), where ctx is the inbound request context.
- On client deadline sooner than the TTL, the parent's deadline wins and fires first. Crucially, the ErrTTLExpired cause only attaches if the TTL's own deadline is the one that expires; when the parent fires, the cause is the parent's cancellation. So it flows down the QueueOutcomeEvictedContextCancelled path and is reported as client disconnect/timeout, not as a TTL shed. Attribution stays honest in metrics and status mapping.
- On client deadline later (or absent), the 60s TTL fires first with the ErrTTLExpired cause → EvictedTTL → the TTLExpired dropped-reason response. The client's larger budget is deliberately cut at 60s of queue wait. The remaining budget is preserved for generation. I.e., TTL is "queue-wait budget" not "client-deadline proxy".
In short, if the client sets a higher timeout, we still enforce maximum of 60s queue-wait budget by default.
There was a problem hiding this comment.
Long-term I would like to be able to express TTL at the per-priority (InfObj) and request level, but that is not in scope for this change as it would be a new transport mechanism.
What type of PR is this?
/kind bug
What this PR does / why we need it:
DefaultRequestTTLand per-bandMaxRequestshad no effective defaults:validate()only rejects negative TTLs, and the bandMaxRequestsdoc comment references adefaultPriorityBandMaxRequestsconstant that does not exist. A default configuration could queue requests with no time or count bound until the 1 GB per-band byte limit.Changes:
defaultRequestTTLdefaults to 60s when unset. The API field is a*metav1.Duration, so unset and explicit zero are distinguishable: an explicit"0s"keeps disconnect-only behavior and is documented as such. The 60s value is a queue-wait budget. The well-lit guides configure no gateway request timeout, so this is the only bound on queue wait in those deployments. Where a shorter gateway or client deadline exists, context cancellation evicts the request first. This repo's chart defaults the route timeout to 300s, so with default config the TTL fires first; a request queued for 60s has already lost most of its time-to-first-token budget, and operators who prefer longer waits can raisedefaultRequestTTL.maxRequestsdefaults to 5000 when unset, applied inapplyDefaultsalongside the existingMaxByteshandling. Band-level 0 is treated as unset, matching howmaxBytes: 0is handled today. The two defaults are consistent: under a dispatch halt, occupancy self-limits at arrival rate times TTL, so the count cap only binds above roughly 83 req/s per band.FlowGCTimeoutdefault (5 minutes; the comment claimed 1 hour), theDefaultNegativePriorityBandcomment claimingmaxBytes: "0"drops traffic immediately (band-level 0 has never meant zero capacity), explicit zero-semantics notes on all four limit fields, and theInitialEffectiveTTLadapter docstring with a TODO for per-request and per-band TTL scopes ([Flow Control] Allow setting different default maxBytes configs for priorities >=0 and for < 0 #1090).Tests: defaults asserted at the controller, registry (including the auto-provisioned priority-0 band and both dynamic templates), and loader layers, with nil vs explicit-zero round-trips for both fields. The flowcontrol tree passes under
-race.Design discussion and the recorded 60s rationale are in the plan comment on #2095.
Which issue(s) this PR fixes:
Fixes #2095
Part of #1187.
Release note: