Skip to content

Commit 60f4c2b

Browse files
committed
refactor to sync request-thread confinement
Concludes the EPP multithreading model by removing background goroutines, channels, and select statements during plugin execution. All data producers now run synchronously and sequentially on the main request-handling goroutine. Introduces three-phase request orchestration in the Director relative to the admission boundary. Request attributes are wrapped in an immutable, read-only snapshot during wait-queue blocking to allow lockless concurrent reads by queue policies, and restored to a writable local map post-admission. Implements individual plugin panic recovery via deferred recovery stubs to isolate plugin crashes and protect router process stability. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
1 parent b6bc98b commit 60f4c2b

6 files changed

Lines changed: 280 additions & 50 deletions

File tree

cmd/epp/runner/runner.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -690,15 +690,16 @@ func (r *Runner) parseConfigurationPhaseTwo(ctx context.Context, rawConfig *conf
690690
}
691691
}
692692

693-
// Sort data plugins in DAG order (topological sort). Also check DAG for cycles.
693+
// Sort and partition data plugins into Pre-Admission and Post-Admission in DAG order.
694694
// This must run after auto-created producers are added so they are included in the ordering.
695-
dag, err := datalayer.ValidateAndOrderDataDependencies(handle.GetAllPlugins())
695+
preAdmission, postAdmission, err := datalayer.CompilePipeline(handle.GetAllPlugins())
696696
if err != nil {
697697
return nil, fmt.Errorf("failed to load the configuration - %w", err)
698698
}
699699

700-
// The plugins will be executed in topologically sorted order to ensure that data is produced before it is consumed.
701-
r.requestControlConfig.OrderDataProducerPlugins(dag)
700+
// Map and register the topologically sorted pre-admission and post-admission data producers.
701+
r.requestControlConfig.OrderPreAdmissionDataProducers(preAdmission)
702+
r.requestControlConfig.OrderPostAdmissionDataProducers(postAdmission)
702703

703704
r.parser = handlers.NewParser(cfg.ParserConfig)
704705
logger.Info("loaded configuration from file/text successfully")

pkg/epp/requestcontrol/director.go

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -213,25 +213,42 @@ func (d *Director) HandleRequest(ctx context.Context, reqCtx *handlers.RequestCo
213213
FairnessID: fairnessID,
214214
Objectives: requestObjectives,
215215
RequestSizeBytes: reqCtx.RequestSize,
216-
Attributes: fwkdl.NewAttributes(),
216+
Attributes: fwkdl.NewLocalAttributes(), // Lockless map confined to the main request-handling goroutine
217217
}
218218

219219
logger = logger.WithValues("objectiveKey", reqCtx.ObjectiveKey, "incomingModelName", reqCtx.IncomingModelName, "targetModelName", reqCtx.TargetModelName, "priority", infObjective.Spec.Priority)
220220
ctx = log.IntoContext(ctx, logger)
221221
logger.V(logutil.DEBUG).Info("LLM request assembled")
222222

223+
// --- Pre-Admission Data Production (Pre-Queue) ---
224+
if err := d.runPreAdmissionDataProducers(ctx, reqCtx.SchedulingRequest); err != nil {
225+
return reqCtx, err
226+
}
227+
223228
if err := d.runPreAdmissionPlugins(ctx, reqCtx.SchedulingRequest); err != nil {
224229
return reqCtx, err
225230
}
231+
226232
if reqCtx.SchedulingRequest.FairnessID == "" {
227233
reqCtx.SchedulingRequest.FairnessID = metadata.DefaultFairnessID
228234
}
229235

236+
// --- Admission Control Queuing (The Queue Boundary) ---
237+
// Capture a reference to the original writable attributes map.
238+
localAttributesSnapshot := reqCtx.SchedulingRequest.Attributes
239+
240+
// Wrap the attributes into a thread-safe read-only snapshot during queue blocking.
241+
reqCtx.SchedulingRequest.Attributes = fwkdl.NewReadOnlyAttributes(localAttributesSnapshot)
242+
230243
// Admit may block until flow control admits the request.
231244
if err := d.admissionController.Admit(ctx, reqCtx, priority); err != nil {
232245
return reqCtx, err
233246
}
234247

248+
// --- Post-Admission Routing & Scheduling (Post-Queue) ---
249+
// Restore the writable local attributes map populated with Pre-Admission traits.
250+
reqCtx.SchedulingRequest.Attributes = localAttributesSnapshot
251+
235252
endpointCandidates := d.endpointCandidates.Locate(ctx, reqCtx.Request.Metadata)
236253
if len(endpointCandidates) == 0 {
237254
return reqCtx, errcommon.Error{
@@ -241,11 +258,12 @@ func (d *Director) HandleRequest(ctx context.Context, reqCtx *handlers.RequestCo
241258
}
242259

243260
snapshotOfCandidatePods := d.toSchedulerEndpoints(endpointCandidates)
244-
// Prepare per request data by running DataProducer plugins.
245-
err = d.runDataProducerPlugins(ctx, reqCtx.SchedulingRequest, snapshotOfCandidatePods)
261+
// Prepare per request data by running Post-Admission DataProducer plugins sequentially on the
262+
// main request-handling goroutine
263+
err = d.runPostAdmissionDataProducers(ctx, reqCtx.SchedulingRequest, snapshotOfCandidatePods)
246264
if err != nil {
247-
// Don't fail the request if DataProducer plugins fail.
248-
logger.Error(err, "failed to prepare per request data")
265+
// Don't fail the request if Post-Admission DataProducer plugins fail.
266+
logger.Error(err, "failed to prepare post-admission per request data")
249267
}
250268

251269
// Run admit request plugins
@@ -523,12 +541,19 @@ func (d *Director) runPreAdmissionPlugins(ctx context.Context, request *fwksched
523541
return nil
524542
}
525543

526-
func (d *Director) runDataProducerPlugins(ctx context.Context,
527-
request *fwksched.InferenceRequest, endpoints []fwksched.Endpoint) error {
528-
if len(d.requestControlPlugins.dataProducerPlugins) == 0 {
544+
func (d *Director) runPreAdmissionDataProducers(ctx context.Context, request *fwksched.InferenceRequest) error {
545+
if len(d.requestControlPlugins.preAdmissionDataProducers) == 0 {
546+
return nil
547+
}
548+
emptyEndpoints := []fwksched.Endpoint{}
549+
return dataProducerPluginsWithTimeout(ctx, dataProducerTimeout, d.requestControlPlugins.preAdmissionDataProducers, request, emptyEndpoints)
550+
}
551+
552+
func (d *Director) runPostAdmissionDataProducers(ctx context.Context, request *fwksched.InferenceRequest, endpoints []fwksched.Endpoint) error {
553+
if len(d.requestControlPlugins.postAdmissionDataProducers) == 0 {
529554
return nil
530555
}
531-
return dataProducerPluginsWithTimeout(ctx, dataProducerTimeout, d.requestControlPlugins.dataProducerPlugins, request, endpoints)
556+
return dataProducerPluginsWithTimeout(ctx, dataProducerTimeout, d.requestControlPlugins.postAdmissionDataProducers, request, endpoints)
532557
}
533558

534559
func (d *Director) runAdmissionPlugins(ctx context.Context,

pkg/epp/requestcontrol/director_test.go

Lines changed: 120 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,14 @@ var (
6565
// --- Mocks ---
6666

6767
type mockAdmissionController struct {
68-
admitErr error
68+
admitErr error
69+
admitCallback func(request *fwksched.InferenceRequest)
6970
}
7071

71-
func (m *mockAdmissionController) Admit(context.Context, *handlers.RequestContext, int) error {
72+
func (m *mockAdmissionController) Admit(ctx context.Context, reqCtx *handlers.RequestContext, priority int) error {
73+
if m.admitCallback != nil {
74+
m.admitCallback(reqCtx.SchedulingRequest)
75+
}
7276
return m.admitErr
7377
}
7478

@@ -141,6 +145,27 @@ func newMockDataProducerPlugin(name string) *mockDataProducerPlugin {
141145
}
142146
}
143147

148+
type mockDataProducer struct {
149+
name string
150+
produces map[fwkplugin.DataKey]any
151+
produceCallback func(ctx context.Context, request *fwksched.InferenceRequest, endpoints []fwksched.Endpoint) error
152+
}
153+
154+
func (m *mockDataProducer) TypedName() fwkplugin.TypedName {
155+
return fwkplugin.TypedName{Type: "mock-data-producer", Name: m.name}
156+
}
157+
158+
func (m *mockDataProducer) Produces() map[fwkplugin.DataKey]any {
159+
return m.produces
160+
}
161+
162+
func (m *mockDataProducer) Produce(ctx context.Context, request *fwksched.InferenceRequest, endpoints []fwksched.Endpoint) error {
163+
if m.produceCallback != nil {
164+
return m.produceCallback(ctx, request, endpoints)
165+
}
166+
return nil
167+
}
168+
144169
type mockAdmissionPlugin struct {
145170
typedName fwkplugin.TypedName
146171
denialError error
@@ -328,7 +353,10 @@ func TestDirector_HandleRequest(t *testing.T) {
328353
preRequestPlugin *mockPreRequestPlugin
329354
wantMutatedBody map[string]any
330355
fairnessIDHeader string // If non-empty, set as metadata.FlowFairnessIDKey on the incoming request.
356+
preAdmissionProducers []fwkrc.DataProducer
357+
postAdmissionProducers []fwkrc.DataProducer
331358
wantFairnessID string // If non-empty, asserted against returnedReqCtx.SchedulingRequest.FairnessID.
359+
checkFn func(t *testing.T, reqCtx *handlers.RequestContext)
332360
}{
333361
{
334362
name: "successful completions request",
@@ -734,6 +762,86 @@ func TestDirector_HandleRequest(t *testing.T) {
734762
wantErrCode: errcommon.Internal,
735763
inferenceObjectiveName: objectiveName,
736764
},
765+
{
766+
name: "pre-admission data producer failure aborts request",
767+
reqBodyMap: map[string]any{
768+
"model": model,
769+
"prompt": "prompt with failed pre-producer",
770+
},
771+
mockAdmissionController: &mockAdmissionController{admitErr: nil},
772+
inferenceObjectiveName: objectiveName,
773+
preAdmissionProducers: []fwkrc.DataProducer{
774+
&mockDataProducer{
775+
name: "failed-pre-producer",
776+
produceCallback: func(ctx context.Context, request *fwksched.InferenceRequest, endpoints []fwksched.Endpoint) error {
777+
return errcommon.Error{
778+
Code: errcommon.Internal,
779+
Msg: "simulated pre-admission data producer error",
780+
}
781+
},
782+
},
783+
},
784+
wantErrCode: errcommon.Internal,
785+
},
786+
{
787+
name: "attributes wrapped in read-only container during queue blocking",
788+
reqBodyMap: map[string]any{
789+
"model": model,
790+
"prompt": "prompt for queue blocking safety",
791+
},
792+
mockAdmissionController: &mockAdmissionController{
793+
admitErr: nil,
794+
admitCallback: func(req *fwksched.InferenceRequest) {
795+
assert.PanicsWithValue(t, "cannot write to ReadOnlyAttributes", func() {
796+
req.Attributes.Put("illegal_key", "illegal_value")
797+
})
798+
val, ok := req.Attributes.Get("pre_key")
799+
assert.True(t, ok)
800+
assert.Equal(t, "pre_val", val)
801+
},
802+
},
803+
schedulerMockSetup: func(m *mockScheduler) {
804+
m.scheduleResults = defaultSuccessfulScheduleResults
805+
},
806+
inferenceObjectiveName: objectiveName,
807+
preAdmissionProducers: []fwkrc.DataProducer{
808+
&mockDataProducer{
809+
name: "pre-producer",
810+
produces: map[fwkplugin.DataKey]any{fwkplugin.NewDataKey("pre_key", "pre-producer"): nil},
811+
produceCallback: func(ctx context.Context, request *fwksched.InferenceRequest, endpoints []fwksched.Endpoint) error {
812+
request.Attributes.Put("pre_key", "pre_val")
813+
return nil
814+
},
815+
},
816+
},
817+
postAdmissionProducers: []fwkrc.DataProducer{
818+
&mockDataProducer{
819+
name: "post-producer",
820+
produces: map[fwkplugin.DataKey]any{fwkplugin.NewDataKey("post_key", "post-producer"): nil},
821+
produceCallback: func(ctx context.Context, request *fwksched.InferenceRequest, endpoints []fwksched.Endpoint) error {
822+
request.Attributes.Put("post_key", "post_val")
823+
return nil
824+
},
825+
},
826+
},
827+
checkFn: func(t *testing.T, returnedReqCtx *handlers.RequestContext) {
828+
require.NotNil(t, returnedReqCtx.SchedulingRequest)
829+
assert.NotPanics(t, func() {
830+
returnedReqCtx.SchedulingRequest.Attributes.Put("writable_now", "success")
831+
})
832+
v1, ok := returnedReqCtx.SchedulingRequest.Attributes.Get("pre_key")
833+
assert.True(t, ok)
834+
assert.Equal(t, "pre_val", v1)
835+
836+
v2, ok := returnedReqCtx.SchedulingRequest.Attributes.Get("post_key")
837+
assert.True(t, ok)
838+
assert.Equal(t, "post_val", v2)
839+
840+
v3, ok := returnedReqCtx.SchedulingRequest.Attributes.Get("writable_now")
841+
assert.True(t, ok)
842+
assert.Equal(t, "success", v3)
843+
},
844+
},
737845
}
738846

739847
period := time.Second
@@ -781,6 +889,12 @@ func TestDirector_HandleRequest(t *testing.T) {
781889
test.schedulerMockSetup(mockSched)
782890
}
783891
config := NewConfig()
892+
if len(test.preAdmissionProducers) > 0 {
893+
config = config.WithPreAdmissionDataProducers(test.preAdmissionProducers...)
894+
}
895+
if len(test.postAdmissionProducers) > 0 {
896+
config = config.WithPostAdmissionDataProducers(test.postAdmissionProducers...)
897+
}
784898
if test.dataProducerPlugin != nil {
785899
config = config.WithDataProducerPlugins(test.dataProducerPlugin)
786900
}
@@ -870,6 +984,9 @@ func TestDirector_HandleRequest(t *testing.T) {
870984
t.Errorf("reqCtx.Request.RawBody mismatch (-want +got):\n%s", diff)
871985
}
872986
}
987+
if test.checkFn != nil {
988+
test.checkFn(t, returnedReqCtx)
989+
}
873990
assert.Equal(t, len(reqCtx.Request.RawBody), reqCtx.RequestSize)
874991
})
875992
}
@@ -1448,7 +1565,7 @@ type testResponseStreaming struct {
14481565
respsOnStreaming []*fwkrc.Response
14491566
targetPodsOnStreaming []string
14501567

1451-
// Legacy fields for existing tests if any, but better to update them
1568+
// Legacy fields maintained for backward compatibility.
14521569
lastRespOnStreaming *fwkrc.Response
14531570
lastTargetPodOnStreaming string
14541571
}

pkg/epp/requestcontrol/plugin_executor.go

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -22,42 +22,49 @@ import (
2222
"fmt"
2323
"time"
2424

25+
"sigs.k8s.io/controller-runtime/pkg/log"
26+
2527
fwkrc "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/requestcontrol"
2628
fwksched "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/scheduling"
2729
)
2830

29-
// executePluginsAsDAG executes DataProducer plugins as a DAG based on their dependencies asynchronously.
30-
// So, a plugin is executed only after all its dependencies have been executed.
31-
// If there is a cycle or any plugin fails with error, it returns an error.
31+
// executePluginsAsDAG executes DataProducer plugins sequentially based on their dependencies.
32+
// If any plugin fails with error or panics, it returns an error.
3233
func executePluginsAsDAG(ctx context.Context, plugins []fwkrc.DataProducer, request *fwksched.InferenceRequest, endpoints []fwksched.Endpoint) error {
3334
for _, plugin := range plugins {
34-
if err := plugin.Produce(ctx, request, endpoints); err != nil {
35+
if err := ctx.Err(); err != nil {
36+
return err
37+
}
38+
39+
err := func() (prodErr error) {
40+
defer func() {
41+
if r := recover(); r != nil {
42+
prodErr = fmt.Errorf("panic in DataProducer %q: %v", plugin.TypedName().String(), r)
43+
log.FromContext(ctx).Error(prodErr, "panic caught during plugin execution", "plugin", plugin.TypedName().String())
44+
}
45+
}()
46+
return plugin.Produce(ctx, request, endpoints)
47+
}()
48+
if err != nil {
3549
return fmt.Errorf("DataProducer %q failed: %w", plugin.TypedName().String(), err)
3650
}
3751
}
3852
return nil
3953
}
4054

41-
// dataProducerPluginsWithTimeout executes DataProducer plugins with a timeout.
42-
// The child context is cancelled when the timeout fires so plugins can observe cancellation
43-
// (e.g. abort outbound HTTP calls) and avoid committing state after the director has moved on.
55+
// dataProducerPluginsWithTimeout executes DataProducer plugins sequentially with a timeout.
56+
// The child context is cancelled when the timeout fires so plugins can observe cancellation.
4457
func dataProducerPluginsWithTimeout(ctx context.Context, timeout time.Duration, plugins []fwkrc.DataProducer,
4558
request *fwksched.InferenceRequest, endpoints []fwksched.Endpoint) error {
4659
ctx, cancel := context.WithTimeout(ctx, timeout)
4760
defer cancel()
4861

49-
errCh := make(chan error, 1)
50-
go func() {
51-
errCh <- executePluginsAsDAG(ctx, plugins, request, endpoints)
52-
}()
53-
54-
select {
55-
case err := <-errCh:
56-
return err
57-
case <-ctx.Done():
62+
err := executePluginsAsDAG(ctx, plugins, request, endpoints)
63+
if err != nil {
5864
if errors.Is(ctx.Err(), context.DeadlineExceeded) {
5965
return fmt.Errorf("DataProducer execution timed out: %w", ctx.Err())
6066
}
61-
return ctx.Err()
67+
return err
6268
}
69+
return nil
6370
}

pkg/epp/requestcontrol/plugin_executor_test.go

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,7 @@ func (p *ctxObservingPlugin) Produces() map[fwkplugin.DataKey]any { return nil }
9494
// child context passed to plugins is cancelled with DeadlineExceeded when the
9595
// timeout fires. Without this cancellation, a slow plugin would continue
9696
// executing past the director's deadline and potentially commit state after
97-
// downstream hooks have already observed an "empty" state — the root cause of
98-
// the orphan-decrement drift we're fixing in the predicted-latency producer.
97+
// downstream hooks have already observed an "empty" state, preventing state drift.
9998
func TestDataProducerPluginsWithTimeout_CancelsPluginContext(t *testing.T) {
10099
plugin := &ctxObservingPlugin{name: "slow", block: time.Second}
101100
plugin.wg.Add(1)
@@ -349,3 +348,38 @@ func TestExecutePluginsAsDAG(t *testing.T) {
349348
})
350349
}
351350
}
351+
352+
type panickingDataProducerPlugin struct {
353+
name string
354+
executed bool
355+
}
356+
357+
func (m *panickingDataProducerPlugin) TypedName() fwkplugin.TypedName {
358+
return fwkplugin.TypedName{Type: "mock", Name: m.name}
359+
}
360+
361+
func (m *panickingDataProducerPlugin) Produce(context.Context, *fwksched.InferenceRequest, []fwksched.Endpoint) error {
362+
m.executed = true
363+
panic("simulate database connection failure")
364+
}
365+
366+
func (m *panickingDataProducerPlugin) Produces() map[fwkplugin.DataKey]any {
367+
return nil
368+
}
369+
370+
func TestExecutePluginsAsDAG_PanicRecovery(t *testing.T) {
371+
panicker := &panickingDataProducerPlugin{name: "panicker"}
372+
nextPlugin := &executorMockDataProducerPlugin{name: "next"}
373+
374+
err := executePluginsAsDAG(
375+
context.Background(),
376+
[]fwkrc.DataProducer{panicker, nextPlugin},
377+
&fwksched.InferenceRequest{},
378+
nil,
379+
)
380+
381+
assert.Error(t, err)
382+
assert.Contains(t, err.Error(), "panic in DataProducer \"panicker/mock\": simulate database connection failure")
383+
assert.True(t, panicker.executed)
384+
assert.False(t, nextPlugin.executed, "Subsequent plugin should not be executed after a panic")
385+
}

0 commit comments

Comments
 (0)