Skip to content

fix(config): auto-inject saturation detector as scheduling filter#1741

Open
RishabhSaini wants to merge 4 commits into
llm-d:mainfrom
RishabhSaini:fix-auto-inject-saturation-filter
Open

fix(config): auto-inject saturation detector as scheduling filter#1741
RishabhSaini wants to merge 4 commits into
llm-d:mainfrom
RishabhSaini:fix-auto-inject-saturation-filter

Conversation

@RishabhSaini

@RishabhSaini RishabhSaini commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Auto-inject the saturation detector as a scheduling filter into explicit scheduling profiles.

Problem

When a user defines an explicit scheduling profile, the saturation detector's Filter method is never called. Saturated endpoints remain scheduling candidates, causing hotspotting when high-weight scorers (e.g., prefix-cache-scorer at weight 3) consistently pick the overloaded pod. Flow control's HoL gate holds load near the saturation threshold, preventing the natural rebalancing that occurs without flow control.

Reported by @asm582 with prefix-cache-scorer (weight 3) + queue-scorer (weight 2) + kv-cache-utilization-scorer (weight 2) on a static two-replica system. The second pod processed zero requests with flow control enabled.

Thanks to @LukeAVanDrie for diagnosing this issue.

Fix

In ensureSaturationDetector, after the detector is resolved, check if it implements scheduling.Filter. If yes, inject it into all scheduling profiles that don't already reference it. Both utilization-detector and concurrency-detector implement Filter.

For empty profiles, the existing auto-population in ensureSchedulingLayer already includes all filters. This change closes the gap for explicit profiles.

Behavior change

Configurations with flow control enabled and an explicit scheduling profile will now have the saturation detector's filter active. Saturated pods are removed from scheduling candidates when at least one non-saturated alternative exists. When all pods exceed the threshold, the filter returns all of them (existing fallback behavior).

Test plan

  • Existing config loader tests pass
  • Verify with Abhishek's config: two replicas, prefix-cache-scorer weight 3, flow control enabled → both pods receive traffic

@github-actions github-actions Bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 25, 2026
@RishabhSaini
RishabhSaini marked this pull request as ready for review June 25, 2026 03:16
@RishabhSaini
RishabhSaini requested review from a team and shmuelk as code owners June 25, 2026 03:16
@RishabhSaini
RishabhSaini requested review from liu-cong and vMaroon June 25, 2026 03:16
LukeAVanDrie
LukeAVanDrie previously approved these changes Jun 25, 2026

@LukeAVanDrie LukeAVanDrie 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.

Approved with nits; thanks @RishabhSaini!

/lgtm

Comment thread pkg/epp/config/loader/defaults.go

if sd, ok := handle.GetAllPluginsWithNames()[sdConfig.PluginRef]; ok {
if _, isFilter := sd.(fwksched.Filter); isFilter {
injectFilterIntoProfiles(cfg.SchedulingProfiles, sdConfig.PluginRef)

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.

Non-blocking design question: this injects the detector into every profile with no opt-out. For multi-profile setups where a profile intentionally excludes saturation filtering, there's no way to decline now. Consistent with the other ensure* defaults, but unlike ensureDataLayer's InjectDefaults escape hatch.

This seems fine for the single-profile case this fixes; just flagging whether per-profile opt-out should be a follow-up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

filed: #1826

Comment thread pkg/epp/config/loader/defaults.go Outdated
@github-actions github-actions Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 26, 2026
@RishabhSaini
RishabhSaini force-pushed the fix-auto-inject-saturation-filter branch from b3d7b10 to 1cea649 Compare June 26, 2026 17:51
@github-actions github-actions Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 26, 2026
When the saturation detector implements scheduling.Filter, inject it
into all explicit scheduling profiles that don't already reference it.

Without this, explicit profiles only contain user-listed plugins and
the detector's Filter method is never called. Saturated endpoints
remain scheduling candidates, causing hotspotting when high-weight
scorers (e.g., prefix-cache-scorer) consistently pick the overloaded
pod. Flow control's HoL gate holds load near the saturation threshold,
preventing the natural rebalancing that occurs without flow control.

The utilization-detector and concurrency-detector both implement
Filter. For empty profiles, the existing auto-population at
ensureSchedulingLayer already includes all filters. This change
closes the gap for explicit profiles.

Signed-off-by: RishabhSaini <rishabhsaini01@gmail.com>
- Use allPlugins parameter instead of re-fetching from handle
- Add tests for ensureSaturationDetector with Filter-implementing
  detector (injected) vs plain detector (not injected)

Signed-off-by: RishabhSaini <rishabhsaini01@gmail.com>
@RishabhSaini
RishabhSaini force-pushed the fix-auto-inject-saturation-filter branch from 1cea649 to 78a3e2e Compare July 14, 2026 19:38
@ahg-g

ahg-g commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Are we sure this will not impact p/d?

@asm582

asm582 commented Jul 24, 2026

Copy link
Copy Markdown

@RishabhSaini @LukeAVanDrie we see the same issue when running with the optimized-baseline plugins with flow-controller enabled. Will this PR fix the issue for a different set of plugins too?

@LukeAVanDrie

LukeAVanDrie commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@RishabhSaini @LukeAVanDrie we see the same issue when running with the optimized-baseline plugins with flow-controller enabled. Will this PR fix the issue for a different set of plugins too?

@asm582 Can you elaborate a bit more on this? Note: this is for auto-wiring the saturation detector into a secondary role of acting as a scheduling filter (to prevent hotspotting based on per-endpoint saturation). As a workaround, you can explicitly reference the detector as a filter in the scheduling profile. The utilization-detector or concurrency-detector types implement both the SaturationDetector (primary) and Filter (secondary) extension points and are configurable as such.

This PR is to make this config more intuitive as it is non-obvious that the detector must be explicitly added as a filter too for the behavior to take effect.

Rishabh is OOO, but I think this PR is mostly ready. I'll respond to @AHG's feedback here and then provided there is no merge conflicts on rebase, we can try to get this in the next release.

@LukeAVanDrie

Copy link
Copy Markdown
Contributor

Are we sure this will not impact p/d?

Yes. The filter is appended after each profile's existing filters, so it only narrows within the already role-filtered candidate set, and it can't empty a result (when all remaining candidates are saturated it returns them all, failing open). Net
effect: a saturated prefill/decode pod is skipped only when a non-saturated same-role alternative exists. If a deployment ever wants it on one role only, #1826 (per-profile opt-out) is the escape hatch.

The red pd-shared-storage-disagg check isn't this PR: the decode pod's sim (v0.9.2) panics on the new /inference/v1/generate e2e test. Main already fixed this (sim bumped to v0.10.2 in #2030); this branch's July 14 merge of main predates that. Updating the branch should turn it green.

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

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants