Skip to content

fix(model-deployment): route External DP members and add walkthroughs - #752

Merged
thxCode merged 4 commits into
mainfrom
docs-rtx6000-scaling-walkthrough
Oct 8, 2026
Merged

thxCode merged 4 commits into
mainfrom
docs-rtx6000-scaling-walkthrough

Conversation

@thxCode

@thxCode thxCode commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

What type of PR is this?

/kind bug
/kind documentation
/area worker

What this PR does / why we need it:

A managed Router registered only member 0 of an External DP Server group, even though every rank served HTTP. Remove that leader restriction for qualified External Server endpoints while preserving endpoint eligibility. Internal and Prefill/Decode groups keep their leader restriction, including mixed-role deployments. Peer discovery remains unchanged.

Recognize vLLM's normalized LB mode and reject conflicting modes or MultiPort in managed roles. Keep one shared HTTP serving port, including an explicitly configured custom port. Validation applies on both create and update; compatibility with previously stored unsupported LB configurations is outside this change.

Group complete examples under docs/walkthroughs/<module>/. Separate Elastic EP collective growth from fixed External DP serving, share model-prefetch preparation, and preserve captured CR output beside the checks. Move the existing device and KV-cache walkthroughs and update guides, navigation, exports, and skill routes.

Which issue(s) this PR links to:

Relates #741
Relates #754
Relates #601

Special notes for your reviewer:

  • The final full race-enabled test suite passed. Runtime tests used operator binary 20d674b; the subsequent code change only clarifies errors for unreadable start-rank node counts. Generated-source consistency is checked separately. Focused tests cover real Service/Router selectors, Internal and P/D protection, mixed roles, normalized modes, and admission.
  • Official gpustack/runner:cuda13.0-vllm0.29.0 ran unchanged. Fixed External TP2/DP2/EP4 passed ordinary and streaming requests directly on both ranks. Four Router requests reached both registered endpoints, with each rank's counter increasing by two.
  • Elastic Internal TP2/DP2→4/EP4→8 passed with and without a Router. The master Pod and HTTP process remained unchanged. These are functional checks, not a sustained-load benchmark.
  • Hybrid runtime and External DP within P/D remain unverified. Native Elastic EP with External LB is not supported by the tested vLLM version. Scale-down remains separate work.
  • Final application deletion released all GPUs, but Router/Service dependents remained during a 131-second observation. UID-bound explicit cleanup succeeded; the cause is unconfirmed. The cloud cluster and its owned resources were subsequently deleted and independently verified absent.

Does this PR introduce a user-facing change?

Managed routing now includes all qualified HTTP members of External DP Server groups while preserving Internal and Prefill/Decode leader routing. Managed roles reject conflicting LB modes and MultiPort configurations. Walkthroughs are grouped by module and include measured Elastic EP and External DP examples using the official GPUStack runner.

@thxCode
thxCode marked this pull request as draft October 8, 2026 09:33
@gpustack-code-review

gpustack-code-review Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

🔍 OpenCodeReview found 6 issue(s) in this PR.

  • ✅ Successfully posted inline: 0 comment(s)
  • 📋 Routed to summary by policy: 5 comment(s)
  • ⏭️ Skipped (overlap with history): 1 comment(s)

documentation · low

📄 docs/modules/model-deployment/routing.md (L18-L20)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category documentation)

This page adds a 'Which Pods receive traffic' section to the module page, but the module-map rule says shared routing/skill associations belong in docs/README.md#module-map. If these routing semantics are shared with walkthroughs/other modules, ensure the module map is updated accordingly (or that this page is confirmed as the single owning source).


documentation · low

📄 docs/modules/model-deployment/_index.md (L24-L24)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category documentation)

All-pages table registration: new/renamed walkthrough pages (docs/walkthroughs/...) must be registered in docs/README.md's All-pages table with labels matching their H1s — please confirm the docs/README.md (+19/-7) change covers all four walkthrough pages and this elastic-ep.md cross-link target exists at the stated path.


documentation · low

📄 docs/modules/model-deployment/routing.md (L28-L28)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category documentation)

Contents list check: the new section heading 'Which Pods receive traffic' was added to Contents — good. However this section introduces the 'Unknown mode or unverified group' table row whose rule ('Keep the existing leader restriction; do not expand discovery') is behavior stated only here; consider confirming this matches the admission code in pkg/worker/webhooks/worker/model_deployment.go so the doc and code change together.


documentation · low

📄 docs/walkthroughs/model-deployment/external-dp.md (L308-L309)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category documentation)

Contradiction within two adjacent sentences: the instruction says to verify "all four GPU claims are released" (the External DP group consumes four GPUs per this page's own shape description), but the recorded result then states "all eight cards returned to their free state". Eight free cards is only possible if the Elastic EP application (which held the other four) was already deleted, which this walkthrough does not state. Clarify the recorded preconditions (e.g. "after both walkthrough applications were deleted" or correct to four).


documentation · low

📄 docs/modules/model-deployment/routing.md (L39-L39)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category documentation)

The MultiPort refusal is scoped in code to managed roles: validateModelDeploymentRoleLoadBalance returns early for a role that replaced its command line (role.Command take-over), so a take-over role passing --data-parallel-multi-port-external-lb is not rejected at admission. The sentence reads as an unconditional rule; consider qualifying it with "managed roles" to match the surrounding paragraph's scope ("Managed roles expose one fixed HTTP port per Pod").


ℹ️ All inline comments overlapped with existing reviews; nothing new was posted.

@thxCode

thxCode commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the one summary-routed documentation finding: the shared-memory paragraph now records measured mount capacity and links to the configuration guide for the default rule. There were no top-level reviews or inline threads to resolve.

The walkthrough now uses the official GPUStack runner throughout. Its unchanged vLLM 0.29.0 / Ray 2.54.0 installation passed elastic width 2→4 with TP=2, native DP 2→4 and EP world 4→8. Ordinary/SSE requests succeeded before and after expansion; the master Pod and HTTP process were retained. The image contains only the CUDA 13 CuPy package. Captured CR projections match the official run. The obsolete field discussion has been removed.

Verification: documentation lint, server dry-run for all YAML examples, current-generation observation checks, and code generation with a clean generated tree. The existing full race-enabled test run passed. This round uses a fixup commit; squash merge will absorb it.

@thxCode
thxCode marked this pull request as ready for review October 8, 2026 09:53
@thxCode thxCode changed the title docs(model-deployment): add measured scaling walkthrough docs(model-deployment): add scaling and external DP walkthroughs Oct 8, 2026
Comment thread docs/modules/model-deployment/external-dp-walkthrough.md Outdated
@thxCode
thxCode force-pushed the docs-rtx6000-scaling-walkthrough branch from 952d26b to faf20fe Compare October 8, 2026 11:04
Comment thread docs/modules/model-deployment/routing.md Outdated
Comment thread pkg/worker/controllers/worker/model_deployment_parallelism.go
Comment thread pkg/worker/controllers/worker/model_deployment_parallelism.go
Comment thread pkg/worker/webhooks/worker/model_deployment.go
Signed-off-by: thxCode <thxcode0824@gmail.com>
Signed-off-by: thxCode <thxcode0824@gmail.com>
@thxCode

thxCode commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Reviewed all feedback buckets: 3 top-level reviews (marker-only bodies), 5 inline findings, and 2 issue comments. The current summary contains 2 additional findings; the earlier summary-only shared-memory wording finding was already addressed.

Finding Result
External walkthrough missing Next footer Fixed. The moved walkthrough links to Model Deployment Status.
P/D boundary repeated across three pages Fixed. Prefill and Decode owns the limitation and issue link; other guides link to it.
--nnodes lacks takesValue False positive. Both engine tables already set takesValue: true; the vLLM entry also includes -n. Spaced-form parsing and routing tests passed.
Start-rank plus External should remain valid Kept the rejection. vLLM 0.29.0's CLI counts start-rank as Hybrid and rejects combining it with an External flag or explicit DP rank. The comment about an assigned rank refers to --data-parallel-rank, which agrees with External.
Environment-derived DP bypasses LB admission False positive. After a successful scan, ReadModelDeploymentRoleParallelism reports Source.Kind=ExtraArgs, including the VLLM_DP_SIZE fallback. There is no environment source kind on this path. Conflicting wiring still reaches admission.
Normalize Hybrid local=global=1 to Internal first Kept native ordering. vLLM 0.29.0 first switches local=1 to External, then clears Hybrid for an all-local group without clearing External. Reversing the checks would disagree with the engine.
Restore a fixed allowlist for future mode values Kept the requested policy: recognized non-Internal modes use External endpoint selection. Unknown remains excluded and mode values originate from the bounded parser. Hybrid GPU validation is explicitly deferred.

Primary engine evidence: CLI mode validation and LB normalization.

The two documentation findings are addressed. The three disputed inline findings receive source-based replies and remain open for reviewer judgment. Full product tests, code generation, and the runtime tests passed. The final documentation pass checks the reorganized site and its exports. The branch's existing fixup commit is folded as part of final history cleanup.

@thxCode thxCode changed the title docs(model-deployment): add scaling and external DP walkthroughs fix(model-deployment): route External DP members and add walkthroughs Oct 8, 2026
@thxCode
thxCode force-pushed the docs-rtx6000-scaling-walkthrough branch from 20d674b to c0bca4f Compare October 8, 2026 12:22
Comment thread pkg/worker/controllers/worker/model_deployment.go
Comment thread pkg/worker/webhooks/worker/model_deployment.go
Comment thread pkg/worker/webhooks/worker/model_deployment.go
Signed-off-by: thxCode <thxcode0824@gmail.com>
Signed-off-by: thxCode <thxcode0824@gmail.com>
@thxCode

thxCode commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

This review emitted 7 findings: 3 inline, 3 summary-routed, and 1 skipped as an overlap with earlier feedback. All current inline and summary findings were read.

Finding Disposition
Legacy MultiPort selectors change after upgrade Intentionally not changed. The maintainer explicitly excluded backward compatibility from this work. Managed MultiPort remains unsupported and rejected; this PR does not add migration or legacy routing guarantees.
Existing MultiPort objects cannot update under the new rule Intentionally not changed for the same scope decision. No grandfathering was added. New validation applies on both create and update.
Regression tests and generated webhook output reminder Verified. model_deployment_balance_test.go covers admission, and routing tests cover generated Service/Router selectors and P/D protection. make generate produced no drift; its CI check passed.
nnodes parse failure has a misleading local-size message Fixed. Unreadable or nonpositive node counts now identify the node-count problem. A positive multi-node value still requests explicit local DP size. Four cases cover both diagnostic paths; the new unreadable cases failed before the change and pass after it. Routing classification is unchanged.
Long prose line in docs index Wrapped to match the surrounding prose.
README link prefix style Restored the adjacent ./docs/ style for both moved links.
Skipped overlap: External plus start-rank should be accepted Retained the earlier source-based rejection: vLLM 0.29.0 CLI rejects that combination as multiple LB modes.

The compatibility decisions are scope choices, not claims that the legacy concerns are impossible. The skipped overlap was also read from the review log and matched to the earlier response.

The GPU runtime record remains the tested operator binary 20d674b7 with the unchanged official runner. The subsequent code change only distinguishes errors for unsupported or unreadable start-rank node counts; it does not change the measured External or Elastic route. No cloud resources were recreated.

@thxCode
thxCode force-pushed the docs-rtx6000-scaling-walkthrough branch from c0bca4f to a76328a Compare October 8, 2026 12:38
@thxCode
thxCode merged commit 5ca7e36 into main Oct 8, 2026
10 checks passed
@thxCode
thxCode deleted the docs-rtx6000-scaling-walkthrough branch October 8, 2026 12:41
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