Repository navigation
feat(sglang,vllm): run the sglang PD router on smg; move router state to /tmp - #100
Conversation
… to /tmp sglang 0.9.30 defaults the PD router to lightseekorg/smg:1.14.0 and execs `smg launch`, set the same way vllm sets its router image: a default in values plus an inline fallback, so an emptied override can never pair smg with the engine image. Until now smg could only be expressed as a kubectl edit on the live Deployment, because the exec line and state dir are template literals. The smg image runs as non-root (USER smg, uid 65532) and cannot create /run/pd-router, so the boot script and the drift liveness probe move to /tmp/pd-router. vllm 0.9.2 takes the same path so the charts stay aligned and a non-root router image will not hit the same failure. vllm's router image is unchanged and already runs as root, so this is not a vllm bug fix. Tests tie the boot script's path to the probe's, because a mismatch is silent: the probe treats a missing snapshot as "still booting" and passes. Breaking for sglang PD releases: - A pd.router.imageOverride naming an sglang engine build now crashloops (no smg binary). No catalog variant sets one. - swiss's Registry.Mirror and modelsphere's image rewrite do not reach the router image, the same gap vllm's router already has. A site without Docker Hub access must set pd.router.imageOverride itself. - The router now reads the model hostPath as uid 65532. Roll back by pinning the sglang chart below 0.9.30. Verified: helm lint --strict on both charts (CI and PD values), helm unittest (sglang 31/31, vllm 35/35), new tests fail when the change is reverted. Render vs main changes only the router Deployment in pd-ds, and only the router container in sglang colocate; vllm colocate is unchanged. Not yet run on a cluster from the chart. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zhanghaohit
left a comment
There was a problem hiding this comment.
A few minor, non-blocking notes from a read of the diff (not tested on a cluster).
Router container still inherits the engine env and volumeMounts (templates/pd-ds.yaml:428, templates/pd-colocate.yaml:148, outside the diff so noting it here)
The router still gets sglang.mergeEnv over .Values.env plus the user volumeMounts. That was harmless while the router ran on the engine image, but it now runs smg (its own Python venv, uid 65532). Engine-oriented env such as PYTHONPATH, LD_LIBRARY_PATH, HOME=/root or HF_HOME=/root/.cache could make the router import the wrong packages or hit EACCES on a root-owned dir and crashloop while the engines look healthy. In colocate the router shares the engine pod, so a router-only mismatch keeps the whole pod un-Ready. Consider giving the router only the env it needs (or a separate pd.router.env).
| imageOverride: | ||
| repository: "" | ||
| tag: "" | ||
| repository: "lightseekorg/smg" |
There was a problem hiding this comment.
Minor: with non-empty defaults on both fields, a values file that overrides only one of them gets mixed with the smg default for the other. An existing site that pinned the router with only imageOverride.tag: v0.5.x now renders lightseekorg/smg:v0.5.x (no such tag); one that set only repository to its engine mirror keeps tag 1.14.0 in a repo that has no smg. A short note here, or a render-time fail when only one of the two is set, would make this obvious.
There was a problem hiding this comment.
Thanks, good catch! I've added a short note in values.yaml in d9a36c1 saying to set both fields, because one left out keeps its default. I went with the note rather than a fail: Helm merges the defaults in before the templates run, so the chart can't tell a lone tag from a full override. vllm got the same note, since it behaves the same way.
| - name: router | ||
| {{- $img := $pd.router.imageOverride | default dict }} | ||
| image: "{{ $img.repository | default $root.Values.image.repository }}:{{ $img.tag | default $root.Values.image.tag }}" | ||
| image: "{{ $img.repository | default "lightseekorg/smg" }}:{{ $img.tag | default "1.14.0" }}" |
There was a problem hiding this comment.
Minor: the smg repository/tag now lives in four places (values.yaml plus the literals here and at pd-ds.yaml lines 185 and 205). A bump that misses one leaves an emptied override on a different version than the default, and the init container and router could disagree. A single helper in _helpers.tpl (e.g. sglang.pdRouterImage) called from all three sites, or required instead of the literal fallbacks, would keep it to one place.
There was a problem hiding this comment.
Thanks, agreed, that's worth tidying up. d9a36c1 adds sglang.pdRouterImage and calls it from all three sites, so the init container and router can't end up on different versions. The rendered output is byte-identical. The default still has to live in both values.yaml and the helper, but changing either one alone now fails tests/pd_router_smg_test.yaml. I made the same change in vllm (vllm.pdRouterImage) so the two charts stay alike; vllm has no matching test yet.
There was a problem hiding this comment.
Quick update to my note above: vllm now has a matching test as well. 8089d18 adds charts/vllm/tests/pd_router_test.yaml, which mirrors the sglang suite, so a change to only one of its defaults fails there too.
zhanghaohit
left a comment
There was a problem hiding this comment.
LGTM. The notes above are minor and non-blocking.
The fallback image was written at all three template sites, so a bump that missed one could leave the init container and the router on different versions. sglang.pdRouterImage and vllm.pdRouterImage now hold it. Also note in values that repository and tag should be set together, because a field left out keeps its default. No render change: helm template output is byte-identical for both charts in the pd-ds and colocate shapes. In sglang, changing the helper's fallback or the values default alone now fails tests/pd_router_smg_test.yaml. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review, @zhanghaohit! On the router inheriting the engine env and |
Mirror charts/sglang/tests/pd_router_smg_test.yaml: default image on the router and init container, exec line, override, the emptied-override fallback, and the state dir in both shapes. This replaces the narrower pd_router_state_dir_test.yaml, and means a change to only one of the helper's fallback and the values default now fails in vllm too. helm unittest charts/vllm: 40 passed. Seven deliberate breakages each failed the new suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What type of PR is this?
What this PR does and why
sglang 0.9.30: the PD router now runs on smg. The router image defaults to
lightseekorg/smg:1.14.0and both router containersexec smg launch, with arguments unchanged.Until now smg could only be run through a
kubectl editon the live Deployment, because the exec line and state dir are template literals andpd.router.imageOverridechanges only the image. Image and exec have to change together:smg launchdoesn't exist in the sglang image, andsglang_routerdoesn't exist in the smg image. A hand-edited Deployment also conflicts with every later swiss or helm apply.Router state dir moves from
/run/pd-routerto/tmp/pd-router. The smg image runs asUSER smg(uid 65532), so it can't create a directory under root-owned/run, and the router crashloops with a misleading error:vllm 0.9.2 makes the same state dir move. This isn't a bug fix: vllm's router image sets no
USER, so it runs as root and/runworks today. It keeps the two charts aligned and means a non-root vllm router image won't hit the same failure. vllm's router image and exec line are unchanged.The router image is set the way vllm sets it: a default in
values.yamlplus an inline fallback in the templates. An emptied override falls back to smg, so it can't pairsmg launchwith the engine image.The new tests tie the boot script's path to the liveness probe's, because a mismatch produces no error: the probe treats a missing snapshot as "still booting" and passes, so drift detection would silently stop.
Rollout impact
Merging publishes both charts, and floating variants pick them up on their next apply:
>=0.9.21,>=0.9.26and>=0.9.27). Each should get a heads-up and one end-to-end check at apply time.Rollback: pin the sglang chart below
0.9.30, or vllm below0.9.2, in the variant'schart.version.Known gaps
Breaking for sglang PD releases, all three also in the commit message:
Registry.Mirrorand modelsphere'simages.yaml.gotmplonly rewrite the engine and sidecar images, and the offline bundle doesn't carry smg. vllm's router has the same gap. modelsphere pins sglang at 0.8.2, so its installs aren't affected until that pin moves, and it needs a paired change before then.pd.router.imageOverridenaming an sglang engine build now crashloops, with no render-time failure. No catalog variant sets one, but a site's own values could.Raised in review and not addressed here:
--model-pathmust hold a tokenizer layout, and the chart has no routerextraArgsto work around that. A model could pass on the engines and still fail on the router. This comes from help text only.worker_startup_timeout_secsdefaults to 1800s, while the chart budgets 90 minutes for engine load. Colocate execs without waiting for the engines, unlike pd-ds. The outcome is unverified.hack/check-chart-images.shcan't see the router image, because it renders defaults, where PD is off. vllm's pin has the same blind spot, so a future bad tag would pass CI. This deserves its own fix.envandvolumeMounts. Left as is on purpose: catalog variants only set engine tuning env (NCCL_*,MOONCAKE_*,PYTORCH_*), and the live test ran that shape under smg. A site that setsPYTHONPATH,LD_LIBRARY_PATH,HOMEorHF_HOMEglobally could still break the router. The model mount and--model-pathare required by the router and are unaffected.imageOverride.repositoryand.tagkeeps the default for the other. Helm merges the defaults in first, so the chart can't detect this at render time;values.yamlnow says to set both./tmpcan be shadowed by uservolumeMounts. Neither chart's defaults mount anything there.vllm-routergets no--model-pathalthough it mounts the model, and vllm's boot script doesn't have the dead-IP filtering sglang gained in sglang: router boot drops IPs that hard-refuse sidecar /engine #80. Both are existing behaviour, not changed here.Which issue(s) this PR fixes
No issue filed. This implements the change request "sglang chart — default the PD router to
smg, fix its boot command for the smg image" (2026-11-04).The request targeted 0.9.29, but #99 took that version, so this ships as 0.9.30.
How it was tested
Run for this PR, on both charts:
main, PD values:bash -npasses on the rendered router boot and probe scripts, checked before a final comment-only edit.lightseekorg/smg:1.14.0resolves on Docker Hub to digestsha256:a764d127…, and its image config setsUser: smg.hack/check-chart-images.shwas not run locally becausecranewasn't available; CI runs it, with the blind spot noted above.Reported in the change request, not re-run for this PR: the rendered argv was smoke-tested against the image with docker, and the same spec was applied to a live glm5.3 PD release by
kubectl patch: router 1/1, drift liveness green,/v1/chat/completionsround trip through the PD engines. That covers the pd-ds shape on sglang only.Still to do, needs swiss and a cluster:
plan+diffon that release: only the router Deployment should change, with no engine roll./v1/chat/completionsend to end./tmp/pd-router/*-ipswritten and read by the probe.Checks
values.yamlcomments; neither chart has a README)Release note
🤖 Generated with Claude Code