Skip to content

Commit 11f325d

Browse files
committed
fix review comments
Signed-off-by: roytman <roytman@il.ibm.com>
1 parent 34a239d commit 11f325d

3 files changed

Lines changed: 45 additions & 5 deletions

File tree

pkg/epp/framework/plugins/scheduling/filter/bylabel/selector_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,32 @@ func TestLabelSelectorFilterEdgeCases(t *testing.T) {
349349
})
350350
}
351351

352+
// TestLabelSelectorFilterHasNoAllowsNoLabelEscapeHatch documents and verifies the
353+
// property a role-scoped deployment relies on when it shares one InferencePool
354+
// across roles (rather than one InferencePool per role): unlike the decode-filter
355+
// shorthand (NewDecodeRole, allowsNoLabel=true), label-selector-filter's
356+
// matchExpressions has no "endpoint without the label passes anyway" exception. A
357+
// role-scoped InferencePool selector makes decode-filter's allowsNoLabel safe,
358+
// since an unlabeled pod is never a candidate in the first place; a shared
359+
// InferencePool selector removes that guarantee, so the decode role must be
360+
// scoped with label-selector-filter instead to reject unlabeled/mislabeled pods.
361+
func TestLabelSelectorFilterHasNoAllowsNoLabelEscapeHatch(t *testing.T) {
362+
rawParams := json.RawMessage(`{"matchExpressions": [{"key": "llm-d.ai/role", "operator": "In", "values": ["decode", "prefill-decode"]}]}`)
363+
plugin, err := bylabel.SelectorFactory("decode-filter", fwkplugin.StrictDecoder(rawParams), nil)
364+
require.NoError(t, err)
365+
blf, ok := plugin.(*bylabel.Selector)
366+
require.True(t, ok)
367+
368+
ctx := utils.NewTestContext(t)
369+
decodeEndpoint := createEndpoint(k8stypes.NamespacedName{Name: "decode-0"}, "10.0.0.1", map[string]string{"llm-d.ai/role": "decode"})
370+
encodeEndpoint := createEndpoint(k8stypes.NamespacedName{Name: "encode-0"}, "10.0.0.2", map[string]string{"llm-d.ai/role": "encode"})
371+
unlabeledEndpoint := createEndpoint(k8stypes.NamespacedName{Name: "mystery-0"}, "10.0.0.3", map[string]string{})
372+
373+
result := blf.Filter(ctx, nil, []scheduling.Endpoint{decodeEndpoint, encodeEndpoint, unlabeledEndpoint})
374+
assert.Equal(t, []scheduling.Endpoint{decodeEndpoint}, result,
375+
"only the decode-labeled endpoint should pass; the mislabeled and unlabeled endpoints must not")
376+
}
377+
352378
// Example for setting Prefill/Decode roles using a LabelSelector filter.
353379
// Definition of labels is based on https://github.com/llm-d/llm-d-router/issues/220.
354380
func ExamplePrefillDecodeRolesInLWS() {

pkg/epp/framework/plugins/scheduling/profilehandler/headerphase/README.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,10 @@ Reads the configured header from the incoming request and looks up the
1414

1515
- If a matching profile hasn't run yet, it runs that profile alone.
1616
- If the header is missing or names a profile that isn't configured, no profile runs. The
17-
request fails with the scheduler's own generic "no profile ran" error (a 429 to the
18-
client, since the scheduler doesn't distinguish a malformed request from exhausted
19-
capacity); the EPP logs the specific reason (missing header vs. unconfigured value) for
20-
operators.
17+
scheduler reports that no profile could be run at all, without a reason, which the EPP
18+
maps to a 429 response to the client -- misleading, since the scheduler doesn't
19+
distinguish a malformed request from exhausted capacity. The EPP logs the specific
20+
reason (missing header vs. unconfigured value) for operators.
2121

2222
This differs from the [disagg profile handler](../disagg/README.md), which decides which
2323
profiles to run via decider plugins rather than a header.

test/coordinator/e2e/coordinator/configs_test.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,27 @@ pipeline:
6060
// EPP-Phase header value (see header-phase-profile-handler); the role filters
6161
// narrow the combined encode+prefill+decode pod pool down to the profile's own
6262
// role, since the InferencePool now selects across all three roles at once.
63+
//
64+
// The decode profile uses label-selector-filter instead of decode-filter: the
65+
// decode-filter shorthand hardcodes allowsNoLabel=true (bylabel.NewDecodeRole),
66+
// which is safe only when decode has its own role-scoped InferencePool, so a
67+
// pod without the role label was never a candidate in the first place. Here
68+
// the InferencePool selects across all three roles, so an unlabeled or
69+
// mislabeled encode/prefill pod would otherwise be accepted as a decode
70+
// candidate; label-selector-filter's matchExpressions has no such exception.
6371
const eppConfig = `apiVersion: llm-d.ai/v1alpha1
6472
kind: EndpointPickerConfig
6573
plugins:
6674
- type: openai-parser
6775
- type: encode-filter
6876
- type: prefill-filter
69-
- type: decode-filter
77+
- type: label-selector-filter
78+
name: decode-filter
79+
parameters:
80+
matchExpressions:
81+
- key: llm-d.ai/role
82+
operator: In
83+
values: ["decode", "prefill-decode", "both", "encode-prefill-decode"]
7084
- type: queue-scorer
7185
- type: max-score-picker
7286
- type: header-phase-profile-handler

0 commit comments

Comments
 (0)