feat: circuit breaker, unary failover, graceful scale-down, and a /scale surface - #83
Open
jiejingzhangamd wants to merge 20 commits into
Open
feat: circuit breaker, unary failover, graceful scale-down, and a /scale surface#83jiejingzhangamd wants to merge 20 commits into
jiejingzhangamd wants to merge 20 commits into
Conversation
jiejingzhangamd
requested review from
JohnQinAMD,
limou102 and
xiaobochen-amd
as code owners
August 4, 2026 00:25
jiejingzhangamd
changed the title
Closes #82. Failover already retries a failed dispatch on another worker, but its memory is a per-request `tried` set that is discarded when the request returns. A worker that is broken for inference yet healthy to the platform -- it accepts the connection, answers /health, stays ACTIVE in discovery -- is therefore re-picked by the very next request, and every request after that pays the failover cost again. Kubernetes cannot see this worker and neither can discovery; the router is the only component that knows, and until now it forgot immediately. Adds a three-state breaker (closed / open / half-open) to both routers, with identical semantics and identical flags: after N consecutive faults a worker leaves the candidate list for a cooldown, then exactly one probe is admitted; success restores it, failure doubles the cooldown up to a cap. Backing off matters because the usual cause -- a worker wedged on a bad KV handoff -- does not clear on the first retry, and a fixed cooldown becomes a probe every N seconds forever. Scoped deliberately: * Only pre-first-byte failures count. Once bytes have streamed the failure is already non-retryable, and counting it would open the breaker on ordinary client disconnects. * 4xx never counts. A malformed request returns 400 from every worker it reaches, so feeding it in would trip the entire healthy fleet on one bad client. 429 is excluded too: it means "full right now", which the policy's load accounting already routes around. * If every candidate is open the router dispatches anyway rather than returning 503 -- a request served by a probably-bad worker beats turning a partial outage into a total one. * PD filters its two pools independently: a wedged prefill and a wedged decode are different events, and one must not evict the other role's healthy workers. * WorkerStatus is never written. That field belongs to discovery; this is the router's private opinion, exported instead as infera_router_worker_breaker_state / _trips_total. A worker tripping repeatedly while discovery still calls it ACTIVE is the alert worth having. --breaker-failure-threshold=0 disables the whole thing. Tests assert the cost, not the status code: failover made every request in these cases succeed both before and after the fix, so a status-only test proves nothing. Against a dead worker and a healthy one, the dead worker was dispatched to 10 times out of 10 before and 3 times after (Python), and 5 of 10 before and 3 after under round-robin (Rust). Both suites were re-run against the pre-fix behaviour to confirm they fail. The PD tests exist because lint, not the suite, caught two of the five record sites referring to a local name that does not exist in those scopes -- a NameError that would have fired only while handling a decode outage. Nothing had ever executed those generators. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
…gine Discovery, routing, failover, the circuit breaker, drain and any future autoscaling loop are all testable without a model. What blocked that was that there was no way to *be* a worker without loading one: the e2e harness starts real engines in containers, so the cheapest fleet anyone could assemble cost a GPU and a multi-minute weight load per member. `infera-fake-worker` registers through the real registration clients with a real EngineConfig, so the payload is built by build_worker_payload and parsed back by worker_info_from_json -- the same functions every engine and every discovery backend use. It cannot drift from the contract without failing its own tests. Only what happens after a request arrives is faked. Three flags exist specifically to make expensive problems cheap: * --startup-delay-s simulates the 5-15 minute weight load, holding /health at 503. That delay is the main reason naive autoscaling overshoots, since an unready replica still counts in the fleet while consuming 0% of the metric. * --max-concurrency gives requests somewhere to queue, which is what makes num_requests_waiting non-zero. Without a queue, the metric the whole industry scales on is identically zero and a scaling test would pass without testing anything. * --fail-first drives the circuit breaker through open -> half-open -> closed without killing a process. /metrics uses engine-native names selected by --engine, so a scaling rule written against fakes transfers to a real fleet unchanged. The SGLang names are second-hand and flagged as unverified in the README. Deliberately not simulated: KV transfer. A PD fake takes part in pool membership and routing and nothing else, which the README says plainly so nobody concludes Mooncake works because a fake fleet answered. Verified end to end, not just unit tested: three fakes registered into a real etcd, a real infera server discovered all three, and unary, SSE and round-robin distribution (4/4 across two workers) all behaved. Building that exposed a bug in an earlier draft -- register() writes the record but the caller must run heartbeat_loop(), so without it the 30s lease expired and workers silently vanished from the pool. Fixed, and written up in the README since it presents as a discovery bug and is not one. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Follows the GPU-free fake worker. Pool membership was already there; what
was missing was everything that makes PD and DP *routing* assertable.
The fake now advertises the same disagg_meta a real worker does --
{"protocol": ..., "params": {"bootstrap_addr": ...}}, prefill only --
with --pd-protocol constrained to the router's own protocol registry, so
a typo dies at argparse instead of surfacing as a protocol error on the
first request. The earlier draft had both wrong (underscore instead of
hyphen, no params at all) and PD simply did not dispatch.
GET /debug/routing reports what the *router* decided: per-rank request
counts keyed on the X-Data-Parallel-Rank header it sent, and the handoff
fields it injected. None of that is observable with a real engine -- a
malformed handoff does not raise, it hangs on KVPoll until a ~300s
timeout, and the failure surfaces nowhere near the router that caused it.
Verified end to end against a real router and real etcd, no GPU:
* PD dual-dispatch: both legs receive the same bootstrap_room, with
bootstrap_host/port pointing at the prefill worker's advertised
endpoint.
* DP attention, rank-multiplexed: 8 requests over a dp_size=4 prefill
fanned out 2/2/2/2 across ranks via the header, decode received
disagg_prefill_dp_rank, and room % dp_size == dp_rank held on 12 of 12
requests -- the invariant SGLang's follow_bootstrap_room balancer
enforces with a KVTransferError.
Two things found by running it. A bind failure still registered the
worker: uvicorn logs "address already in use" and gives up, but the
process keeps running, so a port collision produced a worker that was in
the pool and served nothing. Registration now waits for the socket and
exits 3 on collision. And DP_RANK_HEADER is extracted from dp_routing so
the header name has one definition rather than a literal the fake could
silently stop matching after a rename; Rust already had this.
Also documents the DP shape distinction that makes the path look dead:
is_rank_multiplexed() is dp_size > 1 AND dp_rank is None, so a worker
that registers its own rank is an endpoint and correctly opts out of
router-side DP routing.
Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Scale-down cut live requests. Three separate gaps, each independently sufficient to lose a generation mid-stream. **A condemned Pod stayed a routing candidate.** discovery_k8s removed a worker on DELETE, on a cleared annotation, or on phase != Running -- never on deletionTimestamp. A terminating Pod keeps phase Running until its containers exit, and the operator injects a preStop sleep before SIGTERM, so for that entire delay the worker was condemned, healthy in the router's eyes, and still being assigned new work. The sleep meant to make shutdown graceful was instead buying more requests guaranteed to be cut. Reading deletionTimestamp turns that window into drain time. **HTTP transport never drained.** On NATS infera owns the request path and stop(drain=True) waits for in-flight work. On HTTP the router talks straight to the engine, so infera never sees the request and cannot count it -- shutdown went deregister() then engine.stop(), severing everything active. It now asks the engine, which does know: poll its /metrics for running+queued until zero or --drain-timeout. Queued requests count, because a request the engine accepted but has not started is work a client is waiting on. An unreadable metric returns immediately with a loud warning rather than waiting out the timeout: a rolling update that stalls on a parse failure is worse than one that cuts a request, and a silent full-timeout wait is indistinguishable from a genuinely busy worker. Engine metric names live in common/engine_metrics so drain, the fake worker and any future autoscaler share one mapping; ATOM is deliberately absent rather than guessed, since a wrong name reads as an idle engine and an idle engine is what makes a drain cut requests. **WorkerStatus.DRAINING was never set.** It has been in the enum and filtered from list_active since the beginning with nothing writing it. Both registration clients gain announce_draining(), and shutdown is reordered to announce -> drain -> deregister. Note this adds little on the Kubernetes path now that deletionTimestamp is honoured; the value is on etcd, where nothing else observes that the process is going away, and in observability -- a worker that vanishes looks like one that crashed, while /v1/workers showing "active, draining" says a rollout is progressing. The status field is omitted entirely when ACTIVE so the record stays identical to what older workers wrote. Verified end to end against a real router and real etcd, no GPU, using the fake worker: SIGTERM mid-generation, 30 requests through the drain window, 0 failures, 0 reached the draining worker, /v1/workers reported one active and one draining throughout, and the in-flight 250-chunk stream completed with [DONE]. Router removal latency measured at 15 ms. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Validated graceful drain against real SGLang 0.5.15 serving Qwen3-8B on
MI355X, three instances, one GPU each, registering through the real
worker entrypoint. Six concurrent 4000-token generations were in flight
(num_running_reqs = 6.0) when the container got SIGTERM; all six returned
200 with full-length output, 22s later. The log shows the intended
sequence: announced DRAINING -> "engine idle for 6s, 6 request(s)
completed" -> deregistered -> engine stopping.
Getting there exposed four things unit tests could not.
**sglang serves /metrics only with --enable-metrics; otherwise it 404s.**
Drain reads the in-flight count from there, so on a default deployment it
would have found nothing and shut down without waiting — correct
behaviour on my side, feature silently absent in practice. The worker now
injects the flag, since infera depends on that endpoint for a correct
shutdown.
**The gauges are labelled per rank.** Real output is
sglang:num_running_reqs{engine_type=...,tp_rank="0",...}, one series per
rank, and the parser took only the first match — so with tp_size > 1 an
idle rank could hide a busy one and the drain would exit immediately. It
now sums every label set, which is safe here because the sum is zero
exactly when every rank is zero.
**The gauges lag.** num_running_reqs stayed at 12 for 5-15s after the
last HTTP response completed; SGLang refreshes them on its own schedule.
Stale-high is harmless, but stale-low at the start is not: a request
accepted moments before SIGTERM may not be counted yet, so a single zero
reading cannot distinguish idle from not-yet-counted. Drain now requires
zero to persist for a settle window (6s, chosen from the measured lag)
before believing it.
**The metric names were right.** sglang:num_running_reqs,
sglang:num_queue_reqs and sglang:token_usage all exist as guessed — they
had been carrying a "second-hand, unverified" caveat since they were
added. That caveat is now discharged for SGLang. Reading the real
exposition also surfaced the PD handoff queues
(num_prefill_bootstrap/inflight_queue_reqs,
num_decode_prealloc/transfer_queue_reqs), which are now counted as
in-flight: a prefill worker can show no running and no queued requests
while KV transfers are still outstanding, and killing it there strands
the decode workers waiting on that KV.
Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Adds manual/features/scaling.md. Every figure on the page came from a run on this hardware; nothing is projected. The page is organised around the asymmetry that actually shapes the problem: scale-up is bounded by model load (measured 140 s for an 8B on one MI355X) while scale-down is bounded by the longest in-flight generation, with the router dropping the worker in 15 ms. That gap is why "react to load by starting a worker" does not work for bursts shorter than a cold start, and the page says so rather than leaving it implied. Also corrects a real bug found while validating vLLM. The mapping had vllm:gpu_cache_usage_perc; the running engine exposes vllm:kv_cache_usage_perc. Reading the wrong name returns "no KV in use" silently, so both spellings are now listed and callers take whichever is present. This is exactly the metric-name drift the SGLang entry was already flagged for. Both engines' names are now verified against running engines rather than documentation, and the caveats say so. Measurements on the page: SGLang 0.5.15 and vLLM 0.1.dev19253, Qwen3-8B, one MI355X per instance, HTTP transport, etcd discovery, real router. Drain under load -- six concurrent 4000-token generations in flight at SIGTERM, 6/6 completed with 200 and full-length output on both engines. Scale up then down -- two instances under continuous traffic, a third added and one removed, 260 requests and 0 failures including the windows around each transition. What was not measured is called out in a warning block: multi-node, TP > 1, PD under scaling, and scale-down during an active KV transfer. The PD handoff queues are counted in the drain but that path has not been exercised on hardware. The autoscaling section states the position plainly -- infera ships no autoscaler, and an external one cannot drive an InferaDeployment today because the operator reconciles replicas from the CR on every pass. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
A worker 5xx before any byte reached the client is precisely what failover exists for, and the non-streaming paths returned it verbatim instead. Both of them: direct HTTP, and NATS unary. Only transport errors and non-JSON bodies raised _Retry. Two consequences, the second worse than the first. A unary request never failed over, so a single wedged worker turned 1/N of all traffic into errors even with healthy workers idle. And because the circuit breaker records failures from the same _Retry path, it never saw those failures either -- the breaker shipped in the previous commit was inert on the most common configuration there is, non-streaming over HTTP. The streaming path and the Rust router both already retried this; this brings the remaining two into line. 4xx still passes straight through: the request is bad, not the worker, so every worker would answer the same and retrying only triples the latency of an error the client needs. Measured against a real SGLang Qwen3-8B worker plus one worker that always 503s, 20 unary requests: 20/20 succeeded and the bad worker was tried 3 times before the breaker opened it. The same setup before this change lost 6 of 20 and left the breaker with no samples at all. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Adds an "Across machines" section. The mechanism does not change when workers live on different hosts, because discovery is already the coordination point; what changes is two pieces of configuration, and both fail quietly enough to be worth naming. --advertise-host must be the node's routable address. Leaving it at the single-node default registers a URL that resolves to the wrong machine everywhere else, and the symptom is a router that lists the worker and cannot reach it -- which reads as a broken worker rather than a misconfiguration. Discovery has the mirror-image problem: an etcd advertising a loopback client URL works perfectly on its own node and is invisible from the others. Both are one curl to check, so the section gives the curls. Measured on chi2800 / chi2866, one MI355X each, workers advertising their own IPs, etcd and router on the first node: both registered with distinct addresses, 12 requests split 7/7 across the machines, and a SIGTERM to the remote worker drained cleanly -- three in-flight 3000-token generations all completed (13.7-14.4k characters), it left the fleet after 30s, and 100 requests flowing through the router across the whole transition saw 0 failures. Scoped honestly in a warning block: this is workers on separate machines, not one worker spanning machines (numberOfNodes > 1 / LeaderWorkerSet) and not PD over RDMA between nodes. Neither was exercised. Also notes that rdma/hca is not an allocatable resource on this cluster, so a PD deployment here would need host networking rather than a device plugin. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
The scale-down figure conflated two timings that differ by two orders of magnitude, and the smaller one is the operationally important one. A worker stops being routed to within a second of SIGTERM -- that is the DRAINING announcement plus the router's watch, and it answers the question that matters: is traffic still being sent to something about to die. How long the process then lives is set by the longest generation it was already serving, and is unrelated. Measured with a fake worker holding a 40-second generation: under a second to stop receiving new requests, 38 s until the record disappeared, with the generation completing in between (400/400 chunks). The earlier "30 s" came from watching /v1/workers, which lists draining workers too -- deliberately, so a rollout is visible while it happens. Counting rows there measures process lifetime, not routing. The page now says so and points at the status field instead, and the multi-node paragraph carries the same clarification. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
The grace period was a fixed 120s carrying a comment that it "must exceed preStop + the worker --drain-timeout". Nothing parsed that flag -- it lives in free-form args -- so the invariant was documented and unenforced, and raising --drain-timeout for long generations, which is the only reason anyone raises it, pushed shutdown past the grace and turned the drain back into a kill. Measured on the live cluster before this change: a service declaring --drain-timeout 300 was rendered with terminationGracePeriodSeconds 120. The worst-case budget needs 365s; the kubelet would SIGKILL at 120, roughly 105s into the drain. The grace is now preStop + --drain-timeout + 50s of teardown headroom, floored at the previous 120s and still only ever raised, never lowered. The headroom is not padding: engine.stop() alone waits up to 30s for the engine's process group before escalating to SIGKILL, and deregistration plus KV-plane teardown follow the drain. The flag is read from ServiceSpec.Args and from the container's own command/args, because extraPodSpec templates are passed through verbatim and a deployment that tuned the drain is likely to have written it there. Adds the first Go tests in deploy/operator, covering the arithmetic, both flag spellings, fractional and malformed values, the floor, and the extraPodSpec case. Verified with go build, go vet, gofmt and go test in a golang:1.25 container, since Go is not installed here. Also documents the Kubernetes shutdown sequence and the per-stage worst-case budget in manual/features/scaling.md -- the recipes deploy with kubernetes discovery and HTTP transport, which is a different path from the etcd deployments the drain was first measured on. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
The Kubernetes path was the one with the least verification -- the deletionTimestamp check had unit tests and nothing else, while the recipes deploy on exactly that path (kubernetes discovery, HTTP transport). Exercised end to end on the live k3s cluster with no GPU: fake workers registering by Pod annotation, the real infera server watching them with --discovery-backend kubernetes, and a Pod deleted while holding an in-flight generation. kubectl delete pod took the worker out of routing in 93 ms, against the 15 000 ms preStop delay the operator injects. That gap is the entire reason for reading deletionTimestamp: the alternatives -- the DELETE event, or phase leaving Running -- only fire once the container has exited, so without it the router would have kept assigning work for the whole preStop window and then had it killed. The 300-chunk generation already in flight completed in full, and the replacement Pod had registered before the drain finished. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
The Kubernetes path had been measured only with fake workers. This runs it as the recipes actually deploy: an InferaDeployment reconciled by the operator, real SGLang serving Qwen3-8B on two MI355X, Kubernetes discovery by Pod annotation, HTTP transport, with the repo mounted over the image's installed infera so the drain under test is the one running. kubectl delete pod took the worker out of routing in 87 ms against the 15 000 ms preStop delay, and the four concurrent 2500-token generations it was serving all completed with 200 and full-length output (6.5-13.3 kB). The replacement Pod had registered before the drain finished. Also documents something that cost an hour to find and will cost the next person the same: spec.services.<name>.resources is silently ignored when extraPodSpec is set, because the template is passed through verbatim. A worker that relies on it schedules and starts and then dies with "No accelerator available", which reads as a driver or device-plugin problem rather than a manifest one. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Not from this branch. examples/sglang_1p1d_glm5.2/README.md arrived on main in 7609bb1 (#84) with a trailing-whitespace line, which the pre-commit hook rejects -- so the lint job fails on the merge result for every open PR, including this one, while each branch passes on its own. Fixed here only because it blocks this PR's CI. One line, whitespace only, no content change. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
jiejingzhangamd
force-pushed
the
fix/router-circuit-breaker
branch
from
August 4, 2026 18:48
f8a921d to
5cac722
Compare
Scaling either PD pool to zero fails closed, which is right. The diagnostic was not: AutoRouter falls through to the mixed router when only one pool is populated, and that router answers "no active mixed worker for model=..." -- pointing the reader at something they never deployed while the surviving decode (or prefill) pool sits right there. Found by testing PD scale-down, which the docs had listed as unverified. Now, when exactly one PD pool has workers and there are no mixed workers to absorb the traffic, the 503 says which pool is empty and how many workers the other one has, and the same goes to the log. A mixed worker alongside half a PD fleet still routes normally -- that is the rolling upgrade case and must not trip this. Also records the PD scaling measurement in the docs: a 1P1D fake fleet grown to 2P2D and shrunk back under continuous traffic, 200 requests and 0 failures, both pools scaling independently. The warning block now distinguishes what was measured with fakes from what still needs a real engine, since no KV moved in that run. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
The fake could only be reached over direct HTTP, so the NATS transport -- the path with the better drain semantics, and the one a colleague correctly flagged as important -- was untestable without a real engine. --request-transport nats starts the real NatsRequestServer, which proxies to this process's own HTTP surface exactly as it proxies to a real engine's. The transport under test is the production one; only what answers at the far end is fake. Shutdown likewise goes through the real stop(drain=True), so the drain being exercised is the shipped one. Measured against a real NATS 2.10 broker and a real router: two workers registered with request_transport=nats, unary and SSE both flowed through the broker (12 in / 10 out messages, 3 connections), and SIGTERM during a 300-chunk generation drained cleanly -- the log reads "draining 1 in-flight NATS request(s)", the generation completed 300/300, and the worker deregistered 21.3s later, which is just the remaining generation time with no polling overhead. The README says plainly that the fake's *HTTP* drain is not representative: this process serves its own requests, so it knows its in-flight count exactly, while a real worker on HTTP has to poll the engine's lagging /metrics behind a settle window. Comparing the fake's two transports therefore measures nothing -- both are exact. That difference only appears with a real engine, and is already measured there. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Adds a section measuring what NATS buys over HTTP for scale-down, after a review comment made the point that dropping NATS would be a loss. The measurements support that, and for a more specific reason than implementation quality: the two transports differ in *where the information lives*. On NATS infera owns the request path and holds the in-flight set, so the drain is exact. On HTTP the router dials the engine directly and never sees the request, so infera has to infer the count from the engine's gauges -- which lag, hence the settle window. Measured with the same fake worker and the same generation: * NATS, one in-flight generation: "draining 1 in-flight NATS request(s)" -- it knows the count -- 300/300 chunks completed, deregistered 21.3s later, which is just the remaining generation time. * NATS, nothing in flight: announce to deregister in 3 ms. * HTTP with a real engine, nothing in flight: at least the 6 s settle window, because one zero reading cannot be told from a stale gauge. Also notes what this costs and what else it buys -- a broker, and request cancellation the HTTP path lacks -- and that the Rust router does not implement the NATS transport, so the Rust data plane and the NATS drain are currently an either/or. That tradeoff is worth being explicit about rather than discovered later. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Under the ZMQ event transport the router rebuilds each worker's cache view by pulling its /v1/kv-snapshot, on a 30 s SnapshotReconciler loop. register_target only added the target to the dict; the loop was sitting in wait_for(kick, timeout=interval_s), so a worker that joined while the reconciler was running went unpulled for up to a full interval. The one code path that does kick, trigger_gap_recovery, has no caller in production -- only a test. So nothing shortened that wait. For a genuinely new worker this is harmless: its cache is empty, so an empty routing view is accurate. It is not harmless on a router restart or a rolling upgrade, which is where it actually bites. Every existing worker arrives through this same path with a cache that is warm, and until its snapshot lands kv-aware routing scores all of them as holding nothing -- so the first 30 s after a router comes back routes as if the fleet had no cache at all. The NATS event transport does not have this gap: NatsKvEventClient bootstraps from a JetStream KV bucket watchall, which pushes all initial values on subscribe. Found while measuring what NATS buys over ZMQ, after a review comment argued the NATS path matters. Two tests: a target registered while the loop is running is pulled within 0.3 s (fails on the old code, waiting out the interval), and re-registering a known target does not pull again -- registration is re-asserted routinely, since the Kubernetes backend rewrites its Pod annotation every 30 s and etcd redelivers on relist, so kicking on each would turn a self-heal into a stampede proportional to fleet size. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Documents what INFERA_NATS_REQ_MAX_PENDING actually does, after verifying it end to end against a real NATS 2.10 broker with JetStream. It matters for scaling because it covers the window scaling cannot: a burst shorter than a 140 s cold start cannot be answered by adding workers, so the only choice is between queueing behind a saturated worker and steering away from one. The section leads with how to observe it, because the obvious signal is the wrong one and cost me two failed measurements. A refusal raises the same retryable failure as any other pre-first-byte error, so the request fails over to a freer worker and the client sees 200 -- a 429 only reaches the client when every worker is over the limit and retries are exhausted. Looking for 429s finds nothing and reads as "the throttle does not work". The distribution is the real signal. With one deliberately saturated worker (concurrency 1, 300 ms/token) and one fast one at limit 3, twenty requests sent under backlog went +0 / +20 where round-robin would have been +10 / +10. The saturated consumer read num_ack_pending = 10 at the time; the ack is deliberately after the request is fully proxied so the gauge reflects in-flight work rather than delivery. Also notes the limit of the mechanism: the check is per dispatch, so a simultaneous burst is admitted in full -- every admission check runs before any of them has built backlog. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
5 tasks
The README described discovery ("workers register into etcd and
heartbeat") but said nothing about what happens when the fleet changes
size -- which is the question anyone evaluating this for Kubernetes asks
first.
Adds a Scaling paragraph to the Kubernetes section and a key-features
bullet. The design is worth stating plainly because it is unusual: there
is no scaling controller. Workers self-register when ready and deregister
when they drain, and the router routes to whatever is registered at that
instant, so scaling is just starting and stopping workers.
Leads with the asymmetry rather than burying it, because it constrains
everything built on top: scale up is 140 s (weight loading), scale down
stops receiving in under a second, and adding plus removing instances
under continuous traffic cost 0 failed requests out of 260. The
consequence -- a burst shorter than a cold start cannot be answered by
adding workers -- is the thing a reader most needs to know before
designing around this, so it is said outright.
Also states that infera ships no autoscaler, rather than leaving it to be
inferred, and links the manual page for what is and is not in place for
one. Links point at the published docs site, not repo-relative paths,
which 404 there.
Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Nothing could drive scaling from outside. An InferaDeployment exposes only status; the child Deployment and LeaderWorkerSet expose /scale natively, but the reconciler assigns their whole .Spec every pass and Owns() them, so an external write is reverted almost immediately -- measured, a kubectl scale to 3 went back to 1 in under 3 seconds, not on the 15s resync. Every HPA, KEDA ScaledObject and custom planner loses that race. An InferaDeployment also cannot carry /scale itself, and that is structural rather than unfinished: spec.services is a map with user-chosen keys, while the scale subresource requires specReplicasPath to be a static dot-notation JSONPath. There is no way to name the replicas of an arbitrary map entry. Dynamo hit the identical wall with spec.components[] and solved it the same way. Adds InferaScalingAdapter -- one object per scalable service, carrying specpath=.spec.replicas, statuspath=.status.replicas, selectorpath=.status.selector. kubectl scale, HPA, KEDA and a planner all work through it with no per-tool support in the operator. Single writer by construction: while an adapter has spec.replicas set, the InferaDeployment reconciler reads it instead of the CR's own count. The adapter controller never touches the workload -- it owns only the half /scale reads back. An adapter without spec.replicas is deliberately inert, so an autoscaler can be attached and observed before it is trusted. Also fixes status.replicas on both the adapter and ServiceStatus to report the observed count rather than echoing the desired one. An autoscaler computes desired = ceil(current * metric/target); with current equal to what it just asked for it cannot tell a scale-up has not landed, and keeps multiplying through a 140s model load. Verified on the live k3s cluster: the API now serves inferascalingadapters/scale, kubectl scale works, GET /scale returns a proper autoscaling/v1 Scale, and an HPA targeting it reports AbleToScale=True / SucceededGetScale. With status.selector empty the HPA says InvalidSelector and refuses -- populating it clears that and the only remaining error is a missing metrics source, i.e. the scale interface itself is fully accepted. Go build, vet, gofmt and tests clean in a golang:1.25 container. The four pre-existing gofmt offenders on main are untouched. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
jiejingzhangamd
changed the title
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
1 participant
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.
Closes #82.
Started as the circuit breaker from #82. Verifying it on hardware turned up
further defects on the same path, each of which made the layer above it
ineffective, so they ship together — a breaker that never sees failures, a drain
the kubelet kills halfway, or a
/scalewrite reverted in 3 seconds is notworth reviewing in isolation.
The through-line: a worker can now join and leave a live fleet without
dropping a request, and something outside the operator can decide when.
1. Circuit breaker (the original issue)
Failover retries a failed dispatch elsewhere, but its memory is a per-request
triedset. A worker that is broken for inference yet healthy to the platform —it accepts connections, answers
/health, staysACTIVEin discovery — wasre-picked by the very next request, forever.
Adds a three-state breaker (closed / open / half-open) to both data planes,
infera/router/breaker.pyandrust/router/src/breaker.rs, with identicalsemantics and flags. After N consecutive faults a worker leaves the candidate
list for a cooldown, then one probe is admitted; success restores it, failure
doubles the cooldown up to a cap.
Scoped deliberately: only pre-first-byte failures count; 4xx never counts
(a malformed request 400s on every worker, so counting it would trip the whole
fleet); 429 never counts (that is backpressure, which load accounting already
routes around); if every candidate is open the router dispatches anyway rather
than turning a partial outage into a total one; PD filters its two pools
independently;
WorkerStatusis never written — that belongs to discovery.--breaker-failure-thresholdINFERA_BREAKER_FAILURE_THRESHOLD3(0disables)--breaker-cooldown-sINFERA_BREAKER_COOLDOWN_S5--breaker-max-cooldown-sINFERA_BREAKER_MAX_COOLDOWN_S602. Unary 5xx never failed over — so the breaker was inert
Both non-streaming paths (direct HTTP and NATS unary) returned a worker's 5xx
verbatim. Only transport errors and non-JSON bodies raised
_Retry.Two consequences, the second worse: a unary request never failed over, so one
wedged worker turned 1/N of traffic into errors with healthy workers idle. And
because the breaker records failures from that same
_Retrypath, it neversaw those failures — the breaker in §1 was inert on the most common
configuration there is, non-streaming over HTTP. The streaming path and the Rust
router already retried this.
Measured against a real SGLang worker plus one that always 503s, 20 unary
requests: 20/20 succeeded, bad worker tried 3 times before the breaker opened
it. Before: 14/20, and the breaker had no samples at all.
3. Graceful scale-down
Scale-down cut live generations. Three independent gaps:
discovery_k8sremoved aworker on DELETE, cleared annotation, or
phase != Running— never ondeletionTimestamp. A terminating Pod keepsphase: Running, and theoperator injects a
preStop sleepbefore SIGTERM, so for that whole delay theworker was condemned, healthy in the router's eyes, and still being assigned
work that was about to be killed.
HTTP the router talks straight to the engine, so infera cannot count in-flight
work — shutdown went
deregister()thenengine.stop(). It now asks theengine, polling its
/metricsuntil running + queued + PD-handoff queuesreach zero, bounded by
--drain-timeout.WorkerStatus.DRAININGwas never set. In the enum and filtered fromlist_activesince the beginning, with nothing writing it. Shutdown is nowannounce → drain → deregister, so a worker is visibly draining rather than
simply gone — a worker that vanishes looks exactly like one that crashed.
An unreadable metric returns immediately with a loud warning rather than waiting
out the timeout: a rolling update that stalls on a parse failure is worse than
one that cuts a request, and a silent full-timeout wait is indistinguishable
from a genuinely busy worker.
4. The grace period was unenforced
terminationGracePeriodSecondswas a fixed 120 s carrying a comment that it"must exceed preStop + the worker
--drain-timeout". Nothing parsed that flag —it lives in free-form args — so raising
--drain-timeoutfor longgenerations, the only reason anyone raises it, pushed shutdown past the grace
and turned the drain back into a kill.
Confirmed on the live cluster: a service declaring
--drain-timeout 300wasrendered with
terminationGracePeriodSeconds: 120. Worst case needs 365 s; thekubelet would SIGKILL ~105 s into the drain.
Now derived as
preStop + --drain-timeout + 50 steardown, floored at 120 s,only ever raised. Read from
ServiceSpec.Argsand the container's owncommand/args, since
extraPodSpecis passed through verbatim.5. A GPU-free fake worker
infera-fake-workerregisters through the real registration clients with areal
EngineConfig, so the payload is built bybuild_worker_payloadandparsed by
worker_info_from_json— the same functions every engine and everydiscovery backend use. It cannot drift from the contract without failing its own
tests. Only what happens after a request arrives is faked.
Three flags make expensive problems cheap:
--startup-delay-s(simulate the5–15 min weight load),
--max-concurrency(give requests somewhere to queue, sonum_requests_waitingis non-zero),--fail-first(drive the breaker throughopen → half-open → closed).
GET /debug/routingreports what the routerdecided — per-rank counts and the PD handoff fields it injected — which is not
observable with a real engine, where a malformed handoff hangs on KVPoll for
~300 s and surfaces nowhere near the router that caused it.
This is what made most of the measurements below affordable.
How the tests are written
Assert the cost, not the status code. Failover made every request in the
breaker cases succeed both before and after the fix, so a status-only test
proves nothing. The assertions are on how many times the bad worker was
dispatched to.
Every test was run against the pre-fix code to confirm it fails. Not a
figure of speech — each fix was reverted in place and the suite re-run:
bad worker offered 10 times, expected 3dead worker offered 5 times, expected 3deletionTimestampNameErrorlint caught, tests had not)Never let "unknown" read as "idle". The drain tests are mostly about
measurement failure, because that is where the damage is: a missing metric that
defaulted to
0would make the drain pass instantly and cut live generations,silently.
parse_metricreturnsNone, not0.0, and there is a test for it.Inject the clock; never sleep. The breaker's cooldown, backoff, cap and
half-open window are tested at full speed with a fake clock (
now=in Python,private
*_at()hooks in Rust). The whole Python breaker suite runs in 0.06 s.Test the concurrency that actually exists. The Rust breaker is shared across
tokio threads, so one test races 16 threads at a half-open window and asserts
exactly one probe is admitted.
Counts: ~80 Python, 21 Rust (19 unit + 2 functional), 4 Go — the
first tests in
deploy/operator, which had none.8. A standard Kubernetes
/scalesurfaceNothing could drive scaling from outside. The child
DeploymentandLeaderWorkerSetexpose/scalenatively, but the reconciler assigns theirwhole
.Specevery pass andOwns()them — measured, akubectl scaleto 3was reverted to 1 in under 3 seconds, not on the 15 s resync. Every HPA, KEDA
ScaledObject and custom planner loses that race.
An
InferaDeploymentalso cannot carry/scaleitself, and that is structuralrather than unfinished:
spec.servicesis a map with user-chosen keys, whilethe scale subresource requires
specReplicasPathto be a static dot-notationJSONPath. Dynamo hit the identical wall with
spec.components[]and solved itthe same way.
New
InferaScalingAdapter, one per scalable service:kubectl scale, HPA, KEDA and a custom planner all work through it with noper-tool support in the operator.
Single writer by construction. While an adapter has
spec.replicasset theInferaDeployment reconciler reads it instead of the CR's own count; the adapter
controller never touches the workload, owning only the half
/scalereads back.An adapter without
spec.replicasis deliberately inert, so an autoscaler canbe attached and observed before it is trusted.
Verified on the live k3s cluster: the API serves
inferascalingadapters/scale,kubectl scaleworks,GET /scalereturns aproper
autoscaling/v1Scale, and an HPA targeting it reportsAbleToScale=True / SucceededGetScale. Withstatus.selectorempty the HPAsays
InvalidSelectorand refuses — populating it clears that and the onlyremaining error is a missing metrics source, i.e. the scale interface itself
is fully accepted.
Also fixes
status.replicason both the adapter andServiceStatusto reportthe observed count rather than echoing the desired one — with
currentequalto what it just asked for, an autoscaler cannot tell a scale-up has not landed
and keeps multiplying through a 140 s model load.
What the transport buys, measured
The fake worker now speaks the NATS request transport too, via the real
NatsRequestServer— so the transport under test is the production one.draining 1 in-flight NATS request(s), 300/300 chunks, deregistered at 21.3 s = remaining generation time/metricsbehind a settle windowinfera.cancel.<worker>tears down the engine connectionJetStream admission control (
INFERA_NATS_REQ_MAX_PENDING) verified against areal broker. Look at the distribution, not status codes: a refusal raises the
same retryable failure as any pre-first-byte error, so the request fails over and
the client sees
200. With one saturated worker (concurrency 1) and one fast oneat limit 3, twenty requests under backlog went +0 / +20 where round-robin
would have been +10 / +10.
This is scoped as measurement and documentation — no NATS behaviour was
changed. The Rust router still has no NATS transport, which is now filed as
#88 rather than left to be discovered.
Verification
Unit/functional: Python 1257 passed; Rust 91 passed,
cargo fmt --checkandclippy --all-targets -D warningsclean; Go build/vet/gofmt/test clean in agolang:1.25container.ruffclean. Manual builds with no warnings.(The four pre-existing
gofmtoffenders onmainare left untouched — CI'slint does not check
gofmt, so they predate this branch.)The 5
tests/unit/kv/test_sglang_wiring.pyfailures and thegaiecollectionerror reproduce identically on a clean tree — missing optional deps, unrelated.
On hardware — SGLang 0.5.15 and vLLM 0.1.dev19253, Qwen3-8B, MI355X:
docker run→ in/v1/workers)InferaDeploymentvia operator, real SGLang, Kubernetes discovery:kubectl delete pod→ out of routing in 87 ms vs 15 000 ms preStop; 4 in-flight 2500-token generations 4/4 completed6. A newly-joined worker was invisible to kv-aware routing for 30 s
Under the ZMQ event transport the router rebuilds each worker's cache view by
pulling its
/v1/kv-snapshoton a 30 sSnapshotReconcilerloop.register_targetonly added the target to a dict; the loop sits inwait_for(kick, timeout=interval_s), and the one path that does kick —trigger_gap_recovery— has no caller in production, only a test. So aworker joining while the reconciler was running went unpulled for up to a full
interval.
For a genuinely new worker that is harmless: its cache is empty, so an empty
view is accurate. It bites on router restart and rolling upgrade, where every
existing worker arrives through the same path with a warm cache — the first 30 s
after a router comes back routes as if the fleet had no cache at all.
The NATS event transport has no such gap:
NatsKvEventClientbootstraps from aJetStream KV bucket
watchall(), which pushes all initial values on subscribe.Found while measuring what NATS buys over ZMQ, after a review comment argued the
NATS path matters — it does, and this is why.
Two tests: a target registered while the loop runs is pulled within 0.3 s (fails
on the old code), and re-registering a known target does not pull again —
registration is re-asserted routinely (Kubernetes rewrites its Pod annotation
every 30 s, etcd redelivers on relist), so kicking on each would turn a self-heal
into a stampede proportional to fleet size.
7. Half a PD deployment reported the wrong thing
Found while testing PD scale-down. Taking either PD pool to zero fails closed,
which is right — but
AutoRouterfalls through to the mixed router, whichanswers
no active mixed worker for model=..., pointing the reader at somethingthey never deployed while the surviving pool sits right there. The 503 now names
the empty pool and how many workers the other one has. A mixed worker alongside
half a PD fleet still routes normally; that is the rolling-upgrade case.
Not verified: multi-node workers (
numberOfNodes > 1/ LWS), PD scalingwith a real engine (the run above used fake workers, so no KV moved),
scale-down during an active KV transfer. The PD handoff queues are
counted in the drain, but that path has not been exercised on hardware. Called
out in a warning block in the docs.
Docs
New
manual/features/scaling.md(in the toc, builds clean) — every figure onthe page is measured, none projected. Also documents two things that cost real
time here:
spec.services.<name>.resourcesis silently ignored whenextraPodSpecis set, and sglang serves/metricsonly with--enable-metrics(now injected by the worker, since the drain depends on it).
Two metric-name corrections came out of running real engines:
vllm:gpu_cache_usage_percdoes not exist on current vLLM (it is
kv_cache_usage_perc— both spellings arenow accepted), and the SGLang names, previously carrying an "unverified" caveat,
are confirmed.