[12/N][HAProxy stability] - Close direct-ingress listeners once the deregistration window ends - #65698
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a two-phase draining mechanism for Ray Serve replicas running behind HAProxy. When HAProxy is enabled, the replica stays reachable during the deregistration window and then stops accepting new direct ingress HTTP connections before waiting for in-flight requests to complete. This prevents client-visible errors from late-arriving requests on a shutting-down replica. If HAProxy is not enabled, the replica continues to serve requests normally during the draining period. Unit tests have been added to verify these behaviors. I have no additional feedback to provide.
… ends Behind HAProxy, a draining replica today keeps accepting new requests for as long as they arrive. An HAProxy reload leaves the old process running with a frozen backend list (it cannot be updated, only awaited), so a stale worker can keep sending fresh requests to the draining replica for minutes. When the node then goes away, those late requests are severed mid-flight and the client sees an unretryable error. Fix: behind HAProxy, drain in two phases -- serve the deregistration window in full, then close the direct-ingress HTTP listener and wait for in-flight requests. Late arrivals are refused at connect time, which HAProxy retries on another replica (retry-on conn-failure + option redispatch). Without HAProxy nothing changes: there is no retrying party in front, so a refusal would reach the client. Signed-off-by: harshit-anyscale <harshit@anyscale.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
73ed903 to
4f969b7
Compare
harshit-anyscale
changed the title
…roperty> Signed-off-by: harshit-anyscale <harshit@anyscale.com>
eicherseiji
left a comment
eicherseiji
left a comment
There was a problem hiding this comment.
This addresses the immediate spot-preemption case, but the underlying complexity comes from Ray manually managing HAProxy worker generations. A cleaner follow-up could be:
- Run HAProxy in master-worker mode and reload through the master CLI.
- Enumerate current and old workers and mark removed replicas
maintin each before shutdown. - Optionally use Runtime API membership updates later.
This would let HAProxy manage worker lifecycles, remove Ray’s manual PID/process handling, prevent stale-worker routing at the source, and return replicas to the normal drain path. It can be implemented incrementally while this PR remains a targeted mitigation.
|
|
||
| @pytest.mark.asyncio | ||
| async def test_without_haproxy_keeps_serving_until_drained(self): | ||
| """Without a retrying party in front, the replica keeps accepting for |
There was a problem hiding this comment.
Could we do a manual rewrite on these comments
There was a problem hiding this comment.
have re-written them, ptal again, TIA :)
| self._stop_accepting_direct_ingress() | ||
|
|
||
| # Phase 3: wait for in-flight requests (window already served). | ||
| await self._drain_ongoing_requests() |
There was a problem hiding this comment.
_drain_ongoing_requests() begins by sleeping for graceful_shutdown_wait_loop_s before checking the request count. That delay is redundant here because this path has already waited for deregistration and closed HTTP ingress.
With graceful_shutdown_wait_loop_s=10:
- t=0–30: Wait for deregistration.
- t=30: Close HTTP ingress and call this method.
- t=30–40: Sleep before checking the request count.
- t=35: The controller force-kills the replica before cleanup.
Could we pass check_immediately=True here so the request count is checked before entering the polling loop?
There was a problem hiding this comment.
thanks for pointing this out @eicherseiji , pushed the changes.
Review follow-up. `_drain_ongoing_requests` sleeps `graceful_shutdown_wait_loop_s` before it first counts ongoing requests. That is fine when it also owns the draining period, but in the two-phase drain the window is already spent, so an idle replica waits one more loop for nothing. With `graceful_shutdown_wait_loop_s=10` that overruns the shutdown budget: the deregistration window ends at t=30, the drain then sleeps until t=40, and the controller force-kills at t=35 (the ingress floor of `RAY_SERVE_DIRECT_INGRESS_MIN_DRAINING_PERIOD_S` + `RAY_SERVE_DIRECT_INGRESS_SHUTDOWN_BUFFER_S`), so the quiesce and the graceful server shutdown never run. Add `check_immediately` to count before the first sleep, and pass it from the two-phase drain. The default path is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: harshit-anyscale <harshit@anyscale.com>
2 participants
Description
What happens today when a direct-ingress replica behind HAProxy (
RAY_SERVE_ENABLE_HA_PROXY=1) shuts down, in sequence:hard-stop-after, minutes) — and it still routes with the old backend list. It cannot be corrected: config reloads and runtime commands only reach the new process.RAY_SERVE_DIRECT_INGRESS_MIN_DRAINING_PERIOD_S, default 30s).We measured ~40 such failures per spot-interruption event in a staging fleet.
The fix — behind HAProxy, drain in two phases:
A late request from a stale worker is now refused at connect time instead of admitted. HAProxy retries refused connections on another replica (
retry-on conn-failure+option redispatch), so the client sees no error.Without HAProxy nothing changes: there is no retrying proxy in front, a refusal would reach the client, so that path keeps the existing drain.
Related issues
N/A (follow-up to the HAProxy stability series, most recently #63996).
Additional information
Replica._is_direct_ingressproperty (mirrors the one onReplicaMetricsManager), used to pick the drain path.RAY_SERVE_ENABLE_HA_PROXYbecause the early refusal is only safe with a retrying proxy in front.Not a duplicate
Checked open PRs before and after opening this one (
gh pr list --search "haproxy",--search "drain replica graceful shutdown"): no open PR or issue touches replica drain or direct-ingress listener shutdown. The nearest neighbours are unrelated (#65502 draining-node accessor on the controller, #65542 lazy jinja2 import).Tests run
12 passed — the 7 pre-existing tests plus 5 new ones covering: drain-path selection with and without HAProxy, phase ordering, the deregistration window being served in full before the listener closes, the minimum period not being applied twice, and the no-HTTP-server case.
All hooks passed, including
mypy (ray serve)andpyrefly (ray serve).AI assistance
This change was developed with AI assistance (Claude Code). A human author reviewed every changed line and ran the tests above locally before requesting review.