feat(flowcontrol): support scoped request queue TTLs - #2649
Conversation
Allow operators to select queue-wait bounds per priority band and callers to shorten that bound through the inference TTL header. Signed-off-by: Nikhil Thomas <nikhil.thomas@mistral.ai>
Account for enqueue-channel residence in the active queue-wait budget so expired requests cannot dispatch. Signed-off-by: Nikhil Thomas <nikhil.thomas@mistral.ai>
|
/assign @LukeAVanDrie |
| if isExpired(item, p.clock.Now(), regime, p.noEndpointRequestTTL) { | ||
| p.finalizeAndRecordDrop(item, expiryError(regime.empty)) | ||
| return | ||
| } |
There was a problem hiding this comment.
sorry, you mention this already in the issue description.
There was a problem hiding this comment.
yes, I understand the code it's a latent bug that probably only matters under very tight TTLs
There was a problem hiding this comment.
+1; thanks for closing this!
| // DefaultRequestTTL returns the queue-wait bound configured for the leased priority band and whether it was set. | ||
| func (c *connection) DefaultRequestTTL() (time.Duration, bool) { | ||
| c.registry.mu.RLock() | ||
| defer c.registry.mu.RUnlock() |
There was a problem hiding this comment.
accessing the lock of another entity here is error prone, I think we should put the part that requires locking in a function implemented by the registry, and we call the func here.
Addresses a concern that accessing a lock for another entity would be error prone. Signed-off-by: Nikhil Thomas <nikhil.thomas@mistral.ai>
LukeAVanDrie
left a comment
There was a problem hiding this comment.
Thanks for driving this!
Both new scopes only narrow the saturation-regime budget. isExpired switches to the global noEndpointRequestTTL whenever the pool is empty, and the context backstop is max(saturationTTL, noEndpointRequestTTL), so a request carrying x-llm-d-inference-ttl: 2s during a scale-from-zero still waits up to noEndpointRequestTTL.
That is consistent for the band value, since it overrides defaultRequestTTL which has the same scope. For the request header it is less obvious: the header expresses caller intent, which does not depend on the regime. I do not think this PR needs to change behavior, but the apix field doc and README should say what the scope is.
Also, the release notes only mentions the band-level defaultRequestTTL. The x-llm-d-inference-ttl header is user-facing too, so let's include it there.
| MaxRequests *resource.Quantity `json:"maxRequests,omitempty"` | ||
|
|
||
| // +optional | ||
| // DefaultRequestTTL bounds how long a request may wait in this priority band before it is evicted. |
There was a problem hiding this comment.
This reads as the whole queue-wait bound, but it only replaces DefaultRequestTTL, which applies while the pool has endpoints. NoEndpointRequestTTL stays global and still governs the empty-pool regime for every band, and "0s" here does not make waiting unbounded while the pool is empty. Suggest mirroring the scope language the global field uses:
| // DefaultRequestTTL bounds how long a request may wait in this priority band before it is evicted. | |
| // DefaultRequestTTL replaces the global DefaultRequestTTL for this priority band: the queue-wait bound | |
| // while the candidate pool has endpoints. NoEndpointRequestTTL is not band-scoped and still governs | |
| // queue wait while the pool is empty. If omitted, the global DefaultRequestTTL is used; "0s" disables | |
| // eviction in this band while the pool has endpoints. |
| request is shed. Keep it under the client or gateway deadline, and size it to the time-to-first-token | ||
| budget you are willing to spend waiting on a saturated pool. | ||
| budget you are willing to spend waiting on a saturated pool. Priority-band entries and templates | ||
| may replace the global value, including with `0s` for unbounded queue wait. Clients may shorten the |
There was a problem hiding this comment.
Same scope point for the operator-facing doc. The header shortens the saturation budget only; a client asking for 2s during a scale-from-zero still waits up to noEndpointRequestTTL. Worth a sentence so nobody reads the header as a hard queue deadline.
E.g.,
Clients may shorten the selected operator bound with
x-llm-d-inference-ttlusing Go duration syntax. Both scopes narrow the saturation budget only;noEndpointRequestTTLstays global and is not shortened by the header.
| ) error { | ||
| reqID := item.OriginalRequest().ID() | ||
| select { | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
nit (non-blocking): The reason this early exit exists is not obvious from the code: Submit is non-blocking and would otherwise hand an already-expired item to the processor.
// Submit does not observe ctx, so an item whose budget expired during lease acquisition must be
// rejected here rather than handed to the processor.
select {
case <-ctx.Done():
...
| initialEffectiveTTL := time.Duration(0) | ||
| if rawTTL, ok := metadata.GetLowerCaseHeaderValue(reqCtx.Request.Headers, metadata.InferenceTTLHeaderKey); ok { | ||
| parsedTTL, err := time.ParseDuration(strings.TrimSpace(rawTTL)) | ||
| if err == nil && parsedTTL > 0 { |
There was a problem hiding this comment.
nit (non-blocking): A malformed or non-positive value is dropped with no signal, so an operator debugging why their TTL header did nothing has nowhere to look. The predicted-latency plugin logs its unparseable SLO headers at DEBUG and continues; same pattern would fit here:
if err == nil && parsedTTL > 0 {
initialEffectiveTTL = parsedTTL
} else {
logger.V(logutil.DEBUG).Info("Ignoring invalid request TTL header",
"requestID", reqCtx.SchedulingRequest.RequestID, "value", rawTTL, "err", err)
}
LukeAVanDrie
left a comment
There was a problem hiding this comment.
Approved with some small doc / logging nits; thanks!
/kind feature
What this PR does / why we need it:
Allow operators to select queue-wait bounds per priority band and callers to shorten that bound through the inference TTL header.
My problem: I want to be able to buffer latency-insensitive work longer than latency-sensitive work, so I'd like to use higher priority bands with lower TTLs to serve interactive traffic, with low priority / higher TTL options for more async workflows.
Side fix: commit
e0db7a7addresses what looks like a potential regression in #2284, where I THINK a request can pass controller submission, get buffered, expire, but the processor doesn't double check expiry time. If that's by design, I can back out my change.Which issue(s) this PR fixes:
Fixes #2316
Release note (write
NONEif no user-facing change):