Skip to content

feat(gateway): new routing algorithms designed or from papers - #2571

Draft
CarolWinddd wants to merge 1 commit into
vllm-project:mainfrom
CarolWinddd:route-dev
Draft

feat(gateway): new routing algorithms designed or from papers#2571
CarolWinddd wants to merge 1 commit into
vllm-project:mainfrom
CarolWinddd:route-dev

Conversation

@CarolWinddd

Copy link
Copy Markdown
Contributor

Pull Request Description

This PR introduces additional routing algorithms to the gateway plugin for experimentation and evaluation. The new implementations are registered alongside the existing routing strategies and can be selected via the standard routing-strategy configuration.

These algorithms are either newly designed or adapted from recent papers and still require further experimental verification. The current results are not yet satisfactory, these algorithms are under active development and may be iterated on in follow-up changes. Thus, the PR is temporarily marked as a draft.

Related Issues

Resolves: None

Important: Before submitting, please complete the description above and review the checklist below.


Contribution Guidelines (Expand for Details)

We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:

Pull Request Title Format

Your PR title should start with one of these prefixes to indicate the nature of the change:

  • [Bug]: Corrections to existing functionality
  • [CI]: Changes to build process or CI pipeline
  • [Docs]: Updates or additions to documentation
  • [API]: Modifications to aibrix's API or interface
  • [CLI]: Changes or additions to the Command Line Interface
  • [Misc]: For changes not covered above (use sparingly)

Note: For changes spanning multiple categories, use multiple prefixes in order of importance.

Submission Checklist

  • PR title includes appropriate prefix(es)
  • Changes are clearly explained in the PR description
  • New and existing tests pass successfully
  • Code adheres to project style and best practices
  • Documentation updated to reflect changes (if applicable)
  • Thorough testing completed, no regressions introduced

By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.

Signed-off-by: TengYin <1282479656@qq.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces three new routing algorithms: DualMapRouter, lMetricLocalRouter, and proposedRouter. The feedback identifies several critical concurrency issues in DualMapRouter, including data races in GlobalRequestQueue, hash ring initialization, and state updates. Additionally, there is a deterministic random seed issue in the tie-breaker logic, a metric double-counting bug in lMetricLocalRouter, potential nil pointer dereferences in the post-route updates, and excessive log spam in the proposed router.

Comment on lines +180 to +185
type GlobalRequestQueue struct {
podName string
queue PriorityQueue
globalActualWaitingTokens int64
globalInputWaitingTokens int64
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The GlobalRequestQueue struct and its methods (such as Push, Pop, DelReq, etc.) are accessed concurrently by multiple goroutines during routing and request completion, but they lack any synchronization. This will lead to data races on the underlying queue slice and other fields, causing runtime panics or silent data corruption.

Please add a sync.Mutex to GlobalRequestQueue and protect all accesses to its fields.

Suggested change
type GlobalRequestQueue struct {
podName string
queue PriorityQueue
globalActualWaitingTokens int64
globalInputWaitingTokens int64
}
type GlobalRequestQueue struct {
mu sync.Mutex
podName string
queue PriorityQueue
globalActualWaitingTokens int64
globalInputWaitingTokens int64
}

Comment on lines +505 to +511
type DualMapRouter struct {
cache cache.Cache
tokenizer tokenizer.Tokenizer
prefixCacheIndexer *prefixcacheindexer.PrefixHashTable

hashRing1 *HashRing
hashRing2 *HashRing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The initHashRingsIfNeeded method is called concurrently by multiple routing goroutines (via addRequestToBestGlobalQueue and ScoreAll), but it modifies and rebuilds r.hashRing1 and r.hashRing2 without any synchronization. This causes a severe data race.

Please add a hashRingMu sync.Mutex to DualMapRouter and use it to synchronize hash ring initialization and updates.

Suggested change
type DualMapRouter struct {
cache cache.Cache
tokenizer tokenizer.Tokenizer
prefixCacheIndexer *prefixcacheindexer.PrefixHashTable
hashRing1 *HashRing
hashRing2 *HashRing
type DualMapRouter struct {
cache cache.Cache
tokenizer tokenizer.Tokenizer
prefixCacheIndexer *prefixcacheindexer.PrefixHashTable
hashRingMu sync.Mutex
hashRing1 *HashRing
hashRing2 *HashRing

Comment on lines +1550 to +1551
now := time.Now()
r.podLastPrefillCompletedAt[targetPod.Name] = now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In PostRouteUpdate, r.podLastPrefillCompletedAt[targetPod.Name] is written to without holding r.podTimestampsMu. This causes a data race with getLoadStates which reads from this map under r.podTimestampsMu.Lock().

Please protect this write with r.podTimestampsMu.

Suggested change
now := time.Now()
r.podLastPrefillCompletedAt[targetPod.Name] = now
now := time.Now()
r.podTimestampsMu.Lock()
r.podLastPrefillCompletedAt[targetPod.Name] = now
r.podTimestampsMu.Unlock()

Comment on lines +277 to +309
r.mu.Lock()
info, ok := r.requests[requestID]
if !ok {
r.mu.Unlock()
newRequestTokens := r.calcNewRequestTokens(ctx, targetPod)
r.mu.Lock()
info, ok = r.requests[requestID]
if !ok {
info = &localRequestInfo{
podName: targetPod.Name,
tokens: newRequestTokens,
}
r.requests[requestID] = info
}
}

stats := r.getOrCreatePodStatsLocked(info.podName)
stats.pendingTokens += info.tokens
stats.pendingRequests++

podName := info.podName
tokens := info.tokens
pendingTokens := stats.pendingTokens
pendingRequests := stats.pendingRequests
r.mu.Unlock()

klog.V(4).InfoS("l_metric_local_add_request",
"request_id", requestID,
"pod_name", podName,
"tokens", tokens,
"pending_tokens", pendingTokens,
"pending_requests", pendingRequests)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The AddRequestCount method can be called multiple times for the same request to support real-time statistics updates. However, the pending counters (stats.pendingTokens and stats.pendingRequests) are incremented outside the if !ok block. This means every call to AddRequestCount will repeatedly increment the counters, leading to double-counting and permanently inflated metrics that never return to zero.

Please move the counter increments inside the inner if !ok block so they are only executed when the request is newly tracked.

	r.mu.Lock()
	info, ok := r.requests[requestID]
	if !ok {
		r.mu.Unlock()
		newRequestTokens := r.calcNewRequestTokens(ctx, targetPod)
		r.mu.Lock()
		info, ok = r.requests[requestID]
		if !ok {
			info = &localRequestInfo{
				podName: targetPod.Name,
				tokens:  newRequestTokens,
			}
			r.requests[requestID] = info

			stats := r.getOrCreatePodStatsLocked(info.podName)
			stats.pendingTokens += info.tokens
			stats.pendingRequests++

			klog.V(4).InfoS("l_metric_local_add_request",
				"request_id", requestID,
				"pod_name", info.podName,
				"tokens", info.tokens,
				"pending_tokens", stats.pendingTokens,
				"pending_requests", stats.pendingRequests)
		}
	}
	r.mu.Unlock()

Comment on lines +467 to +469
func (c *lazyExpansionController) process(prefixHashes []uint64) int {
c.cnt++
depth := c.table.lookup(prefixHashes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The process method of lazyExpansionController is called concurrently by multiple routing goroutines, but c.cnt++ is not thread-safe and causes a data race.

Please use atomic.AddInt64 (after changing cnt to int64) or protect it with a mutex.

seed int64,
primaryIsMax bool,
) (string, string) {
rng := rand.New(rand.NewSource(seed))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a constant seed 42 for rand.NewSource(seed) inside selectReplicasBasedOnMetrics makes the random number generator completely deterministic. Every time the function is called, rng.Intn(2) will return the exact same value, defeating the purpose of a random tie-breaker and causing load imbalance.

Please use a non-deterministic seed (e.g., time.Now().UnixNano()) or a shared thread-safe random source.

Suggested change
rng := rand.New(rand.NewSource(seed))
rng := rand.New(rand.NewSource(time.Now().UnixNano()))

Comment on lines +226 to +227
func (r *lMetricLocalRouter) PostRouteUpdate(ctx *types.RoutingContext, readyPodList types.PodList, targetPod *v1.Pod) error {
tokenizerToUse := r.getTokenizerForRequest(ctx, readyPodList)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If targetPod is nil, accessing targetPod.Name will cause a nil pointer dereference panic. Please add a defensive nil check at the beginning of PostRouteUpdate.

func (r *lMetricLocalRouter) PostRouteUpdate(ctx *types.RoutingContext, readyPodList types.PodList, targetPod *v1.Pod) error {
	if targetPod == nil {
		return nil
	}

Comment on lines +327 to +328
func (r *proposedRouter) PostRouteUpdate(ctx *types.RoutingContext, readyPodList types.PodList, targetPod *v1.Pod) error {
if r.kvSyncRouter != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If targetPod is nil, accessing its fields or passing it to kvSyncRouter will cause a nil pointer dereference panic. Please add a defensive nil check at the beginning of PostRouteUpdate.

Suggested change
func (r *proposedRouter) PostRouteUpdate(ctx *types.RoutingContext, readyPodList types.PodList, targetPod *v1.Pod) error {
if r.kvSyncRouter != nil {
func (r *proposedRouter) PostRouteUpdate(ctx *types.RoutingContext, readyPodList types.PodList, targetPod *v1.Pod) error {
if targetPod == nil {
return nil
}

Comment on lines +315 to +321
klog.Infof("proposed: P(prompt_tokens)=%d lambda=%.2f alpha=%.4f maxSeqLoad=%.2f avgC=%.4f",
P, lambda, alpha, denomSeq, avgC)

// Print each pod's score breakdown
for i := 0; i < n; i++ {
klog.Infof("proposed pod=%s M=%.4f seq_raw=%.2f seq_norm=%.4f kv=%.4f C=%.4f miPow=%.6f loadTerm=%.6f score=%.6f",
pods[i].Name, M[i], loadSeq[i], loadSeq[i]/denomSeq, loadKv[i], C[i], miPows[i], loadTerms[i], scores[i])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using klog.Infof at level 0 for per-request and per-pod scoring details will generate an excessive amount of log spam in production under high traffic.

Please change these to klog.V(4).Infof or klog.V(5).Infof to keep the default logs clean.

Suggested change
klog.Infof("proposed: P(prompt_tokens)=%d lambda=%.2f alpha=%.4f maxSeqLoad=%.2f avgC=%.4f",
P, lambda, alpha, denomSeq, avgC)
// Print each pod's score breakdown
for i := 0; i < n; i++ {
klog.Infof("proposed pod=%s M=%.4f seq_raw=%.2f seq_norm=%.4f kv=%.4f C=%.4f miPow=%.6f loadTerm=%.6f score=%.6f",
pods[i].Name, M[i], loadSeq[i], loadSeq[i]/denomSeq, loadKv[i], C[i], miPows[i], loadTerms[i], scores[i])
// Print basic parameters
klog.V(4).Infof("proposed: P(prompt_tokens)=%d lambda=%.2f alpha=%.4f maxSeqLoad=%.2f avgC=%.4f",
P, lambda, alpha, denomSeq, avgC)
// Print each pod's score breakdown
for i := 0; i < n; i++ {
klog.V(4).Infof("proposed pod=%s M=%.4f seq_raw=%.2f seq_norm=%.4f kv=%.4f C=%.4f miPow=%.6f loadTerm=%.6f score=%.6f",
pods[i].Name, M[i], loadSeq[i], loadSeq[i]/denomSeq, loadKv[i], C[i], miPows[i], loadTerms[i], scores[i])
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant