refactor(flowcontrol): unify on a single priority queue#1831
Conversation
With the dispatch queue single-ended, the FCFS-only ListQueue and the capability-negotiation machinery that selected between queue types no longer earn their complexity. Collapse to one queue. - Delete ListQueue; route all flows through the priority queue. FCFS orders by its existing enqueue-time comparator, yielding exact logical-arrival ordering (the heap honors the policy, unlike ListQueue's physical-insertion approximation). - Remove the QueueCapability system end to end: Capabilities() from the queue contract and implementation, RequiredQueueCapabilities() from OrderingPolicy and all ordering policies, and the capabilityChecker validation in the registry config. - Queue selection collapses to always using the priority queue. The registry no longer validates a band's queue name at config construction; an unknown name now surfaces when the flow is first provisioned. The Queue config field is retained here and removed in a follow-up. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
| // Queue specifies the default name of the SafeQueue implementation for flow queues in this band. | ||
| // Optional: Defaults to defaultQueue ("ListQueue"). | ||
| // Optional: Defaults to defaultQueue ("PriorityQueue"). |
There was a problem hiding this comment.
I thought there is only one queue type. What default?
There was a problem hiding this comment.
I'm planning on removing this config loading code in a follow-up PR as this diff is already large. This whole Queue field will be deleted from the config API subsequently. For now, I just updated the doc. Though, you are correct in that it's somewhat misleading.
There was a problem hiding this comment.
If you want, I can carve this code path out as part of this PR too
There was a problem hiding this comment.
Alright, it can be done in a followup PR
|
@LukeAVanDrie After reviewing again this PR along with PR #1846, I have a question. What does the ordering policy do? I understand that the fairness policy tries to grant a certain level of fairness between flows. But isn't ordering policy within the flow, which should now be simply by priority? Also I want to verify, a priority band, is not a range of priorities, it is a single priority. Correct? |
The topology is: (1) InfObj.Priority --> (2) FairnessID --> (3) Request. (1) defines the priority band, which is a set of fairness IDs keyed to an InfObj.Priority. We serve bands by priority descending. Each member of the band (2) gets its own heap which is sorted by our ordering policy. As there are many priority queues in each band (i.e., FairnessID matched to InfObj.Priority), the fairness policy selects across the set to see which tenant is most deserving to be served next. Then the ordering policy tells us which request (3) from that selected heap is the most urgent. Basically, we have two levels of priority that compose
This is better described in our dev site architecture guide. There is also a clearer diagram for this: https://llm-d.ai/docs/architecture/core/router/epp/flow-control#architecture-overview. |
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
|
/retest |
|
@LukeAVanDrie Please rebase on main. Some CI test fixes have been added lately |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Now that the flow-control dispatch queue is single-ended (#1690), the FCFS-only
ListQueueand the capability-negotiation machinery that chose between queue implementations no longer earn their complexity. With one queue type, the "which queue?" decision selects nothing. This collapses the layer to a single priority queue.ListQueue; route every flow through the priority queue. FCFS orders by its existing enqueue-time comparator, which yields exact logical-arrival ordering, an upgrade fromListQueue's physical-insertion "approximate FCFS" (the heap honors the policy and survives the controller's bounce-and-retry).QueueCapabilitysystem end to end:Capabilities()from the queue contract and implementation,RequiredQueueCapabilities()from theOrderingPolicyplugin interface and all ordering policies (FCFS/EDF/SLO), and thecapabilityCheckerpolicy/queue compatibility validation in the registry config (and the now-unusedErrPolicyQueueIncompatible).container/heapwith index tracking is already the established pattern inflowcontrol/eviction/queue.go, so this aligns the dispatch queue with existing style.Note: the registry no longer validates a band's queue name at config construction; an unknown name now surfaces when the flow is first provisioned. (The per-band
Queueconfig field itself is to be removed in a small follow-up.)Performance:
ListQueueexisted as an O(1) FIFO fast path; the priority queue is O(log n). That difference is immaterial at this layer's scale: per-flow queue depth is bounded by per-band capacity, and each dequeued request drives multi-second model inference, so a handful of comparisons per enqueue/dequeue is lost in the noise. A quick benchmark of both implementations (directional, not rigorous) bears this out:At single-item operations the heap is actually faster (contiguous slice + fewer allocations vs.
container/list's node-per-item).ListQueue's O(1) advantage only appears under depth/contention, and even with 1000 queued items it's ~9 ns/op which is negligible.Which issue(s) this PR fixes:
Follow-up from #1690
Release note: