Repository navigation
Roll a worker without dropping traffic - #176
Merged
Merged
Conversation
weilei0120
requested review from
JohnQinAMD,
jiejingzhangamd,
limou102 and
xiaobochen-amd
as code owners
September 23, 2026 11:59
…m decode. A worker rolls surge-free by default, so a single-replica prefill or decode has a gap with nothing serving while its replacement starts. ServiceSpec gains rolloutSurge, which switches that Deployment to maxSurge=1 / maxUnavailable=0: the replacement starts first and the old pod is retired only once the new one is Ready. It stays opt-in because the surge pod needs a spare GPU, and with none free the rollout stalls instead of completing. Rolling one leg at a time also needs the reverse of the prefill barrier. A starting prefill already moves a real KV block to a registered decode before advertising itself; a decode replaced on its own performed no such check, so it could join the fleet with its path to the surviving prefill unexercised. A decode now probes every registered prefill before registering. That probe never waits. A prefill registers only after it finds a decode, so a decode that blocked on finding a prefill would deadlock the first deployment of a pair; finding none skips the probe. Discovery failures skip it too, since decode starts first and must not depend on the backend being reachable. A failed probe is fatal, which is the point. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: weilei <leiwei12@amd.com>
A surge rollout retires the old pod once the replacement reports Ready, so it is only as good as that signal. With skipReadinessProbe a pod counts as Ready the moment it is Running, which for a worker is minutes before its weights are loaded, and the rollout removes the pod that is still serving in favour of one that cannot. That is a worse outage than the surge-free default it replaced, and a silent one. Either setting alone stays supported: every deployment predating rolloutSurge skips the probe, and refusing that would break all of them. The refusal path the external-etcd check already used is now shared, since this is its second caller. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: weilei <leiwei12@amd.com>
A worker rolled old-pod-first, so a single-replica prefill or decode had a gap with nothing serving while its replacement loaded weights. Surge-first rolling removes that gap, but only if readiness means the replacement can serve. It did not. The probe targeted the engine's /health, which answers as soon as sglang has loaded its weights -- before the PD barrier has moved a real KV block and before the worker has registered. A rollout keyed on it retires the pod that is serving in favour of one the router cannot reach yet. The worker now opens a readiness port after registering and closes it when shutdown begins, and the probe targets that, so Ready means what the router means by a live worker. The port simply is not open earlier, so the probe fails closed. With readiness meaningful, maxSurge=1/maxUnavailable=0 is unconditional and the rolloutSurge field goes away. On a saturated cluster the replacement stays Pending and the roll does not finish -- a stall, with the old pod serving throughout, rather than the guaranteed gap the old default produced. skipReadinessProbe stays for idle-deployed workers, which never register and would otherwise sit NotReady forever. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: weilei <leiwei12@amd.com>
asyncio.timeout is 3.11+, and the engine image ships 3.10, so every probe handler raised AttributeError before it could reply. The port accepted the connection and then answered nothing, leaving the pod permanently NotReady -- which stalled the rollout rather than breaking it, since maxUnavailable=0 kept the old pod serving. wait_for is available throughout, and asyncio.TimeoutError likewise: only from 3.11 is it an alias of the builtin, and in 3.10 the builtin is an OSError that would not have matched. Tests run on 3.12 here, so they passed against the version the image does not have; the image itself is now probed after every build. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: weilei <leiwei12@amd.com>
ruff format and gofmt, no behaviour change. Two of the touched files predate this branch; the lint job runs on every file, so they have to be clean here. Signed-off-by: weilei <leiwei12@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
weilei0120
force-pushed
the
feat/rollout-surge
branch
from
September 23, 2026 12:05
af8c2b6 to
598f795
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical readiness and rollout compatibility issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (6)
What changed in this PR
Adds registration-aware readiness, surge-first worker rollouts, and SGLang prefill/decode verification.
Changes:
- Adds readiness listener lifecycle and Kubernetes
/readyprobes. - Adds prefill discovery, verification, and parsing tests.
- Configures worker Deployments with
maxSurge=1,maxUnavailable=0. - Updates operator behavior and CRD documentation.
| File | Description |
|---|---|
tests/unit/engine/test_readiness.py |
Readiness listener tests |
tests/unit/engine/test_decode_barrier.py |
Prefill barrier and parsing tests |
infera/engine/sglang/__main__.py |
Startup barriers and readiness lifecycle |
infera/engine/readiness.py |
Readiness server implementation |
infera/engine/decode_barrier.py |
Prefill peer validation helpers |
deploy/operator/internal/controller/inferadeployment_controller.go |
Refactored refusal handling |
deploy/operator/internal/controller/builders.go |
Readiness probes and rollout strategy |
deploy/operator/internal/controller/builders_test.go |
Operator rollout and readiness tests |
deploy/operator/helm/infera-operator/crds/infera.amd.com_inferadeployments.yaml |
Generated Helm CRD documentation |
deploy/operator/config/crd/bases/infera.amd.com_inferadeployments.yaml |
Base CRD documentation |
deploy/operator/api/v1alpha1/inferadeployment_types.go |
Readiness field documentation |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+441
to
+445
| if addReadiness && c.ReadinessProbe == nil { | ||
| // SGLang's /health runs a tiny prefill self-check that often takes | ||
| // >1s, so a 1s probe timeout (the k8s default) flaps the pod between | ||
| // Ready/NotReady. Use a generous timeout + higher failure threshold so | ||
| // a healthy-but-busy engine is not marked NotReady. | ||
| // Probed on the readiness port, not the engine's /health: only the | ||
| // former means "registered, and the router can reach me". The port | ||
| // simply is not open before then, so the probe fails closed, which is | ||
| // what holds a surge rollout back until the replacement can serve. |
Comment on lines
+442
to
446
| // Probed on the readiness port, not the engine's /health: only the | ||
| // former means "registered, and the router can reach me". The port | ||
| // simply is not open before then, so the probe fails closed, which is | ||
| // what holds a surge rollout back until the replacement can serve. | ||
| c.ReadinessProbe = &corev1.Probe{ |
Comment on lines
+399
to
+404
| func readinessPortFor(c *corev1.Container) int32 { | ||
| for _, e := range c.Env { | ||
| if e.Name != readinessPortEnvVar { | ||
| continue | ||
| } | ||
| if n, err := strconv.Atoi(e.Value); err == nil && n > 0 && n < 65536 { |
Comment on lines
+649
to
+651
| if svc.ComponentType == inferav1alpha1.ComponentTypeWorker { | ||
| maxSurge := intstr.FromInt32(0) | ||
| maxUnavailable := intstr.FromInt32(1) | ||
| maxSurge := intstr.FromInt32(1) | ||
| maxUnavailable := intstr.FromInt32(0) |
Comment on lines
+370
to
+373
| try: | ||
| return host, int(port) | ||
| except ValueError: | ||
| return None |
Comment on lines
+312
to
+316
| async def _run_startup_barrier_until_stop( | ||
| args: SglangWorkerArgs, config, stop: asyncio.Event | ||
| ) -> bool: | ||
| """Run the PD barrier unless shutdown or engine death wins the race.""" | ||
| wait_task = asyncio.create_task(_startup_barrier(args, config)) |
Making surge unconditional broke two classes of deployment. Every shipped example that sets skipReadinessProbe on a serving worker (pd-1p1d-mooncake, pd-kvd, the glm5.2 and kimi-k3 recipes) reported Ready seconds after Running, so the rollout retired the pod that was serving in favour of one still loading weights -- worse than the bounded gap it replaced. And a whole-node pinned worker (replicas=1, nodeSelector, every GPU on the host) could never schedule a surge pod, so its rollout would never complete where surge-free rolling always did. The strategy now follows whether the pod is probed on the readiness port, read off the rendered template so every reason it is not lands in one place: skipReadinessProbe, or an extraPodSpec supplying its own probe. A hand-written /health probe is deliberately treated as no signal, since it answers while the engine is still starting. The probe was also injected for every worker while only the sglang entrypoint opened the port, leaving vLLM workers NotReady forever. vLLM opens and closes it on the same registration boundary now. A valueFrom port is resolved by the kubelet, so the operator cannot know it; injecting a probe on the default would poll a port the worker may not bind, which with maxUnavailable=0 never recovers. Such a pod gets no probe, and so rolls surge-free. Port parsing also matches the worker's int(): no surrounding whitespace, no underscores. Three more, all on the same theme of a signal that was not what it claimed: A failed bind killed the worker after register() and the heartbeat had started, leaving a record the router kept dispatching to. The bind now clears the registration before propagating. Readiness was answered by the supervisor's event loop, so a hung-but-running engine kept the pod Ready and a roll could retire a healthy pod for a wedged one. Each probe now consults the engine's own /health. The decode-side prefill probe caught every exception, so an unresolvable label selector silently skipped the verification it exists for, and a TypeError in that path looked the same as a network blip. Discovery reachability is checked up front, the catch is narrowed to transport and protocol errors, one verified peer is enough, and a spent budget is reported rather than turned into wait_for(0.0). Signed-off-by: weilei <leiwei12@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Treat a probe as the readiness probe only on a known numeric port. - Skip the decode prefill probe on a transient pod label lookup failure. - Try the next registered prefill when one peer probe fails. - Check the decode discovery config before the weights load. - Open the readiness port from the ATOM worker too. - Install SIGTERM handlers before registering in the vLLM and ATOM workers. - Keep serving when the readiness port cannot be bound. Signed-off-by: weilei <leiwei12@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Abort the probe room on both engines when a PD probe is cancelled. - Dial the engine's bind address for the readiness check. - Allow the engine health check 8s, inside the 10s probe timeout. - Report registered prefills that carry no url or bootstrap address. - Describe readinessPortFrom as stricter than the worker's int(). Signed-off-by: weilei <leiwei12@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Treat only a /ready probe on the readiness port as the registration signal. - Listen on IPv4 and IPv6, falling back to IPv4 when the node has no IPv6. Signed-off-by: weilei <leiwei12@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A worker rolled old-pod-first (maxSurge=0, maxUnavailable=1), so a
single-replica prefill or decode had a gap with nothing serving while its
replacement loaded weights. Workers now roll surge-first: the replacement is
created before the old pod is retired, and the old pod goes only once the new
one is Ready.
That only holds if readiness means the replacement can serve. The probe
targeted the engine's /health, which answers as soon as sglang has loaded its
weights — before the PD barrier has moved a real KV block and before the
worker has registered. The worker now opens a readiness port after registering
and closes it when shutdown begins, and the probe targets that port, so Ready
means what the router means by a live worker. The port is simply not open
earlier, so the probe fails closed.
On a GPU-saturated cluster the replacement stays Pending and the roll does not
finish. That is a stall with the old pod serving throughout, rather than the
guaranteed gap the previous strategy produced.
skipReadinessProbe remains for idle-deployed workers, which never register and
would otherwise sit NotReady forever.
Also adds the reverse of the prefill barrier: a decode probes every registered
prefill before registering, so replacing one leg on its own still exercises
the transfer path to the surviving leg. The probe never waits — a prefill
registers only after finding a decode, so a decode blocking on a prefill would
deadlock a first deployment — and discovery failures skip it, since decode
starts first. A failed probe is fatal.
Verified on a live 1P1D deployment: readiness sampled every 10s across the
whole rollout, 91 samples, never fewer than one Ready pod per role.