Repository navigation
feat(worker): add router qualification and abortable retirement - #711
Conversation
Add the optional status.retirement object (absent means no operation, carrying lastConsumedRetryToken) and the per-role parallelism/endpoints status fields, initialized Unknown/NotConfigured with nil unknown counts by the status controller. Existing ready/quota/parse semantics are unchanged and no production eligibility selector activates. Spec: specs/2026-10-01-s1-router-qualification-and-drain.md
Read each role's declared stream (extra args, or the effective data-parallel environment) into the per-role parallelism status: declared degrees are present only where declared (an explicit 1 and an explicit local 0 are preserved as written), observed modes carry their explicit booleans, and the load-balance shape derives from the wiring flags, with disagreeing flags named in source.unreadableReason. A role that replaced its command line refuses the read (Unknown, complete: false, reason Unmanaged), and a degree beyond the status degree fields refuses the whole reading. ParseModelDeploymentDeclaredParallelism becomes a thin adapter over the shared scan; its numeric and error behavior is unchanged and the transfer fixtures stay byte-identical. The unreadableReason documented meaning is widened to admit a complete reading carrying a reason when a derived fact is undecidable; generated assets regenerated. Spec: specs/2026-10-01-s1-router-qualification-and-drain.md
Maintain one operator-owned runtime label (modeldeployment.gpustack.ai/endpoint-eligible) on exactly the endpoints a Router or ordinary Service may select: leader-only for leader-served roles, every ready rank-carrying member for External-DP-shaped roles. The ordinary Services and both Router selector surfaces gain the same equality term; headless replica Services keep selecting every member with publishNotReadyAddresses untouched. The single level-based reconciler write path runs in the converge pass before the Services sync, so existing healthy endpoints are backfilled before any selector narrows — first enable never drops ordinary-Service selection to zero healthy endpoints. The key is written outside the hash boundary: removing or restoring it changes no Pod generation, UID, Workload or stored hash, and a replaced member is eligible only through its own verification. The Router discovery selector joins the term, so first enable rolls Router pods once; instance pods never roll. Spec: specs/2026-10-01-s1-router-qualification-and-drain.md
Reorganize pack/llm-router/patches into per-router directories and teach the Dockerfile to apply them per stage. The vllm-router stage now copies patches/vllm-router/: the mooncake lazy-bootstrap lookup patch moves there byte-identical, joined by the new discovery-readiness and deselect-removal patch. The sglang-gateway stage gains an apply step that runs git apply from the clone root (the patch paths are repository-root-relative) and greps for a patched symbol, so a path-skipped patch fails the build instead of silently producing an unpatched one. The discovery patches carry the source-level router fixes: discovery readiness gating and deselect-on-removal for both Rust routers, per-URL FIFO lanes in the SGLang gateway so one waiting URL no longer head-of-line blocks the others, and resend of failed Add events under the same pod UID. llm-d needs no patch and its behavior is unchanged. Both Rust builder targets compile green on the authorized packaging host (0 errors, 0 warnings) and the built binaries smoke clean through the real CLI. Runtime acceptance is deliberately split out: the event-matrix verification moves to the per-router observation task, and the two fault-injection cells stay unverified under the root ruling against adding injection hooks to the production patches, to be revisited at the real-cluster phase. Spec: specs/2026-10-01-s1-router-qualification-and-drain.md
T3 is complete: the router builder stages and per-router patch application are committed on branch dp-ep-s1-router-patches @ 8e9a06e. T4's source and build levels ride in that same commit because the Dockerfile copies the two patch directories T4 owns; T4 stays unchecked while its runtime acceptance moves to T6 (five matrix cells), with the failed-Add re-list and failed-Remove retry cells UNVERIFIED under the root ruling against fault-injection hooks in the production patches, revisited at the real-cluster phase. Spec: specs/2026-10-01-s1-router-qualification-and-drain.md
T6 (per-router observation) owns both observer-only patch additions under pack/llm-router and the Go client/aggregation, so the two S1 lines converge here: dp-ep-s1-router-patches @ 8e9a06e (T3+T4) merged into dp-ep-s1-router-drain. The patch lines are disjoint from the Go/spec lines; no conflicts. The patches branch ref stays at 8e9a06e and is retired after this merge.
…dicate Add the group health predicate (model_deployment_health.go) evaluating five named legs with three verdicts: FAILED revokes, UNKNOWN holds, VERIFIED admits. No engine-level group-forward observation exists yet, so that leg is Unsupported for multi-member replica groups and their eligibility stays held with the reason in status; single-member replicas (including one-member DP replicas) qualify as before. Restore requires the predicate on the same membership generation, and a revocation precedes any activation gate so a definite fault withdraws even where the group leg cannot be observed.
T7's gate accepted the committed predicate; multi-member replica groups stay held until T11 lands a verifiable per-engine group-forward observation, so T10's docs sync now depends on it too.
For non-P/D multi-rank groups on the pinned engine versions, admission
and restore of a held replica fire one bounded 1-token completion
request per member (request id prefix gpustack-qualify-) and read
VERIFIED only on the identity-bound echo: vLLM answers id=cmpl-<rid>,
SGLang answers meta_info.id=<rid> with a finish_reason. Any mismatch,
missing field, HTTP error, or the single timeout bound reads Unknown
and holds, never FAILED. The version guard admits vLLM {0.25.1,
0.29.0} and SGLang {0.5.18, 0.5.21}; P/D shapes on both engines stay
Unsupported with the concrete reason (client-coordinated two-request
transfer protocol; fake-bootstrap skip of the TP all-reduce metadata
gate). Activated() now requires Verified or NotApplicable, so held
non-P/D multi-rank groups qualify through the observation while P/D
groups stay held. Single-member behavior is byte-identical. Mutation
suite 15/15 RED.
…eletion Every removal path (count trim, surplus shed, departed-role release) now hands the deletion to a retirement reservation instead of cutting the Pod and its Workload mid-flight: the first removal intent admits a reservation binding role, ordinal, member UIDs, Workload UID and observedGeneration with a 300s overall budget, and the pass that admits deletes nothing; a second intent while one is in flight is refused and holds for a later pass. The FSM walks Admitted through Disqualified/Withdrawing/Draining to a commit delete carrying UID preconditions, aborts with capacity retained on budget exhaustion, and survives a controller restart through the persisted state plus a retry-token annotation whose replay check precedes the Aborted gate. The rollout and the incomplete-replica rule step over a reservation-named ordinal entirely, so the surplus rule's per-member choice is never undone by a per-ordinal one. A target with no UID is refused rather than admitted, because an empty-UID binding resolves to every Pod in the deployment. The drain step reads a transport seam that returns Unsupported until T9 lands, so an admitted reservation holds at Draining and deletes nothing in a real cluster today. Mutation suite 33/33 RED.
…ain collector The retirement protocol's Draining phase now has a production reader behind the seam: one bounded read-only pod/exec into the member's own main container, running a python3 one-shot that GETs the engine's /metrics over loopback on the Pod's prometheus.io/port annotation and prints one name<TAB>value line per expected series, with a 1 MiB body cap, a 2s HTTP timeout and a 5s exec bound. A read is matched to the member by Pod UID and container ID checked pre and post, and a restart is detected statelessly from the live Pod's RestartCount -- a member with any restart reads Unknown and holds, bounded by the frozen drain and overall budgets to Aborted-with-retention. The container is now carried by the target from the renderer's own constant, and the reader verifies it exists in the member's spec. A member whose parallelism scan or SGLang disaggregation-mode argument marks it disaggregated is refused with the concrete reason, which closes the departed-role P/D reachability from the reader side without touching the FSM guard or the wire. The expected-series names are passed in from modelDeploymentInFlightMetrics so the collector and the FSM's completeness re-check cannot disagree. No new RBAC object: the worker is bound to cluster-admin, so pods/exec is already granted and a namespaced Role would assert a boundary that does not exist. Mutation suite 16/16 RED.
Sync the owning docs with the shipped T5-T9/T11 behavior: the five qualification legs and their verdict semantics, the retirement field's states and budgets, the three removals that start an operation, the one-at-a-time refusal, the drain read's bounds and refusal table, the restart hold, the P/D refusal, and the per-Router-Pod serving observation with its freshness bound. Correct a stale claim that a scale-down deletes on the spot. Also record the T10 delivery's finding as spec task T12: the drain loop reads every member, so a leader-served multi-member replica cannot finish a retirement and is never deleted.
The retirement drain read every member of the target and required each to answer idle twice. Non-leader members of a leader-served replica serve no metrics, so such a replica was held until the drain budget expired and was never retired. Which members answer is now one derivation shared with endpoint eligibility: every member of an External-DP replica, the leader member of a leader-served replica, the one member at size one. The drain classifies from the member Pods so a departed role still resolves, and holds -- naming the shape -- when leadership is missing or duplicated, when the role and the rendered command disagree, or when the declared size and the member count disagree. The leader is found by a direct label read, never through the helper that defaults a missing label to the leader index. The selection narrows only what is measured; the reservation's member UIDs stay whole as the delete-precondition binding.
…fecycle Closure-review follow-ups: remove a debug println left on the rollout condemn path, correct the stranded-sweep comment to credit the standing-member rule it actually relies on, narrow the spec's UID precondition sentence to the protocol's own deletes, and add the lifecycle test that drives a 2-to-1 scale-down from no reservation through clearing and the create gate reopening.
The state is set on the same pass that clears the reservation, so the previous comment's 'observed end' promised an observability no reader gets; the generated OpenAPI enum description carried the same claim. Regenerated via scratch tree; only the one Description line moved.
The discovery patches previously reacted only to watch-stream events, so a missed or delayed event could leave a departed replica selectable until the next event happened to arrive. Both Rust router patches now run the watch stream as one arm of a tokio::select! whose other arm is a fifteen-second reconciliation timer: each tick relists and reconciles every tracked pod UID against a fresh list, withdrawing by stored UID anything that lost selector membership, readiness, or a usable address. Runtime acceptance on instrumented local-cluster runs measured by a one-second poller: label-off withdrawal removed the replica from the router pool at about one second and readiness withdrawal at about six seconds, both well inside the thirty-second freeze budget; the earlier over-budget figures were harness log-line observation delay, not router behavior. Tick the spec's T4 and T6 boxes to record that acceptance; the two fault-injection event-matrix cells stay unverified under the root ruling against injection hooks in production patches. Spec: specs/2026-10-01-s1-router-qualification-and-drain.md
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s). Reviewed |
![gpustack-code-review[bot]](https://avatars.githubusercontent.com/in/4788230?s=60&v=4){kind=small}
| // mergePodAllocationsStrict is mergePodAllocations with its errors returned instead of logged. | ||
| // | ||
| // The two differ only in what happens to a failure, which is the whole point: the ordinary rebuild | ||
| // cannot fail, because a pod it cannot read must still leave a ledger kubelet can be answered from, | ||
| // and the cost of that is that an unreadable pod drops out and its cards read free. A caller asking | ||
| // whether capacity was RELEASED cannot be answered by a ledger that may be missing a holder, so it | ||
| // gets the same arithmetic and the error. The arithmetic is not reimplemented here, and | ||
| // BuildDesiredStatus is this function with the errors discarded, so the two cannot drift. | ||
| func mergePodAllocationsStrict( |
There was a problem hiding this comment.
The doc comment claims the strict and ordinary merges "cannot drift", but that is only true today because the two loop bodies were written as manual copies. mergePodAllocationsStrict and mergePodAllocations are ~40-line duplicates whose ordering, filtering (terminating/terminal pods), heldAllocation application, and accumulatePhysicalOccupied/foldPhysicalLedger calls must stay in lockstep forever. Any future edit to the ordinary body (e.g. a new pod filter or an extra fold input) that misses the strict copy would silently give the release predicate a different ledger than the published one — exactly the failure mode this file exists to prevent. Consider extracting one shared reducer parameterized by an error sink (e.g. a func(pod, err) bool continue-or-fail callback, or a strict bool flag) so the invariant is structural rather than documentary.
There was a problem hiding this comment.
The drift risk is real and is already covered by a fixture exercising both the strict and the ordinary merge entry points against the same inputs (TestBuildDesiredStatusStrict_PreservesTheOrdinaryRebuild in pkg/deviceplugin/allocation_validation_test.go); this review keeps that coverage and defers the structural refactor rather than claiming it landed. Extracting the shared reducer is now tracked with source, impact and closure scope in #712.
| if before.RestartCount > 0 { | ||
| return modelDeploymentDrainHold(modelDeploymentDrainUnknown, | ||
| fmt.Sprintf("member %s container %q has restarted %d time(s) since it was created", | ||
| pod.Name, target.Container, before.RestartCount)), nil | ||
| } |
There was a problem hiding this comment.
An unconditional hold on RestartCount > 0 means any member that crashed once at any point in its life — long before the retirement began — can never drain, so every retirement of it burns the full 240s drain budget and aborts with capacity retained, and each retry repeats the cycle. The comment argues the drain budget bounds the hold, but the budget does not distinguish a restart that predates admission from one that happened during the operation. Consider freezing the restart count observed at admission into the reservation (like TargetMemberUIDs) and holding only on an increase, so a replica that recovered before retirement can still retire.
There was a problem hiding this comment.
Intentional, and kept. The gauges this protocol compares are served by the engine that exists after the restart; they cannot speak for the work the protocol set out to account for, so any restart at all — including one that predates admission, as you note — invalidates the before/after comparison this evidence is built on. The cost you describe is real and bounded by design: the hold ends the retirement as Aborted with the member retained, so nothing is ever deleted on unaccountable numbers. Freezing the admission-time count into the reservation is a reasonable strengthening, but it changes what the drain evidence claims, so it needs its own reviewed change rather than a review-round edit.
| if md.Spec.Router == nil { | ||
| return "this deployment declares no router, so the established direct-Service connections " + | ||
| "this target's service may still hold cannot be observed; the operation holds", true | ||
| } |
There was a problem hiding this comment.
Router-less deployments (md.Spec.Router == nil) can still admit a retirement reservation — refuseModelDeploymentRemoval admits any removal path regardless of router presence — but observeModelDeploymentRetirementResidual then holds unconditionally at Withdrawing, so every removal of a router-less deployment is guaranteed to abort after the 30s withdrawal budget with capacity retained. Combined with the abort-wedge below (an Aborted reservation keeps plan.Hold true and refuseModelDeploymentRemoval refuses every removal while plan.InFlight || plan.Hold), a router-less scale-down leaves a retained replica that blocks all further removal intents on that deployment until a manual retry annotation or the spec re-declares the ordinal. Consider gating admission on router presence (or letting router-less deployments pass the residual step via the drain-only evidence) so scale-down of router-less deployments is not a guaranteed abort-and-wedge.
There was a problem hiding this comment.
Intentional, and documented in both places: the residual hold's own comment ("established direct-Service connections this target's service may still hold cannot be observed; the operation holds") and spec AC-4.8 gate a router-less retirement on Service-only withdrawal precisely because nothing readable reports whether the established connections have gone quiet. Gating admission on router presence would turn that safety hold into a refusal at admit time — a behavior change to a documented decision rather than a defect, so it is left as is; the abort-and-wedge consequence you trace is real but is the accepted cost of never deleting on unobservable quiescence.
|
All 24 items of this review batch (effective reviewer batch 1 of 4) are dispositioned: 18 fixed - the CI proto round-trip time equality (test only), I1, I2, I5 (budget documented as one shared whole-replica bound), I7 (cheap legs decide before any probe; a condemned replica reaches the transport zero times), I8 (PodReady=Unknown holds, only False fails), I10 (each view stamped after its own read), I11, S1, S2, S3, S4, S5 (a whole-record JSON null is an invalid record), S7 (the Dockerfile guard greps the observer patch's own file and type), S8 (no second pod list), S10 (the slot table is pinned by test), S11 (the retry-token patch carries the read resourceVersion), and I12: eligibility narrowing is now decided by one internal gate on the front Service, role Services, router-discovery published selector and discovery-config equality - a shape that can never qualify (unverified engine version, prefill/decode role, replaced command) renders legacy routing, a decided pass narrows through real per-pod labels so a first-enable definite fault withdraws every affected member, and a previously narrowed surface survives Unknown passes on all three shipped router profiles (retention reads the live llm-d endpoint-selector key and the argv-configured router Deployment). I3 is verified against its existing both-entry-points fixture and deferred as #712. Kept intentionally with documented rationale: I4 (restart holds invalidate the drain evidence), I9 (a router-less retirement has no readable quiescence and holds, spec AC-4.8), I6 (a take-over role cannot be probed; no port is invented), S9 (a scan error answers leader-only; limitation documented). S6 is a false positive: Kubernetes forbids resources on ephemeral containers, so the claimed Allocate input is unreachable. Fixed threads are resolved; the intentional holds and I3 stay open on purpose. |
|
Batch two of the current-head review, three items, all fixed: (1) high - the eligibility pass-decision helper now mirrors the recorded condition's precedence, so mixed qualified+held and qualified+unsupported passes decide nothing instead of narrowing while the condition holds; (2) low - the superseded ConfigMap-only retention comment was removed, keeping the all-profile explanation; (3) CI - the JSON wire round-trip test asserts instants like the proto one, green under UTC and non-UTC timezones. Affected race suites, make lint, and make lint docs exit 0 on the final bytes, and the admission run additionally passed canonical make generate. |
What type of PR is this?
/kind enhancement
/kind api-change
/area worker
/area devicemanager
/area integrations
What this PR does / why we need it:
Which issue(s) this PR links to:
NONE. This PR tracks the approved S1 specification.
Special notes for your reviewer:
make lint,make lint docs, andGODEBUG=gotypesalias=0 CGO_ENABLED=1 GOMAXPROCS=2 go test -p 2 -count=1 -race ./pkg/worker/controllers/worker/... ./pkg/deviceplugin/....make generatepassed in a canonical scratch checkout containing the exact current source; no generated differences, and all 28 API/webhook generated files matched byte for byte.Does this PR introduce a user-facing change?