[serve] Production defaults for HAProxy connect timeout and dead-replica detection - #65729
[serve] Production defaults for HAProxy connect timeout and dead-replica detection#65729harshit-anyscale wants to merge 7 commits into
Conversation
ea2c444 to
df64244
Compare
d75b6ca to
a5b695c
Compare
Four HAProxy defaults are safe on a single-node dev cluster but wrong once a cluster runs one HAProxy per node with a shared, fleet-wide backend list. - balance: leastconn -> random(2). Each node's HAProxy sees the same backend list but only its own connection counts, so a global-minimum rule makes N proxies converge on the same replica. Power-of-two-choices keeps the local signal useful without synchronizing the proxies. - broadcast coalesce: 0.1s -> 1.0s. Each reload leaves the outgoing worker soft-stopping with a frozen backend list until hard-stop-after, so the population of stale-routing workers scales with the reload rate. - timeout connect: unset -> 5s. Replicas are in-cluster, so a slow connect means the node is gone; bounding it lets retry-on conn-failure and redispatch reach another replica while the request still has budget. This bounds connection establishment only, not request duration. - observe layer4 mark-down: off -> on. Live traffic marks a dead replica DOWN without waiting for a health checker; a false positive revives in ~0.5s. timeout server is deliberately left unset. It is a server-side inactivity limit applied to every connection on the live process, not only on workers draining after a reload, and hard-stop-after starts counting only on soft-stop; the two are unrelated clocks. Serve's own request_timeout_s defaults to None, so any value here would silently cap request duration. Every one of these remains overridable by its existing environment variable. The new defaults test is skipif-guarded on the variables it asserts about, so overriding them skips rather than fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: harshit-anyscale <harshit@anyscale.com>
a5b695c to
5ce6006
Compare
There was a problem hiding this comment.
Code Review
This pull request updates several HAProxy default configurations in Ray Serve to optimize data plane tuning, including increasing the broadcast coalesce time, setting a default connection timeout of 5 seconds, enabling observe-mark-down by default, and switching the load balancing algorithm from leastconn to random(2). It also adds a new test to verify these default configurations. Feedback suggests simplifying the new test by removing a redundant mock.patch block, as the configuration file path is already explicitly passed to the HAProxyApi constructor.
The comment claimed N proxies read near-identical state and converge on one replica. That is wrong in steady state: each HAProxy counts only its own connections, so the per-proxy views are independent, each proxy spreads its own share evenly, and the sum is even too. The real failure mode is the transition. A replica that appears or returns from DOWN reads as zero connections on every proxy at the same moment, and leastconn is deterministic on that observation, so they all prefer it until local counts catch up. That is the herd HAProxy documents slowstart for, and Serve sets no slowstart; autoscaling makes the window recur rather than being a one-off at startup. random(2) ranks by the same counts but only across two sampled servers, so identical observations stop producing identical choices. No behavior change, comment only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: harshit-anyscale <harshit@anyscale.com>
The test patched RAY_SERVE_HAPROXY_CONFIG_FILE_LOC while also passing config_file_path to the HAProxyApi constructor. The patch could not have had any effect either way: haproxy.py binds the name into its own module namespace at import, so patching it on the constants module does not rebind it, and its only use there is as a parameter default evaluated at def time. Config generation reads self.config_file_path throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: harshit-anyscale <harshit@anyscale.com>
CI (premerge 72475, serve HAProxy tests) failed test_responses_actually_streamed[use_multiple_replicas=True] on all four attempts: two concurrent requests to a two-replica deployment landed on the same replica, which leastconn makes impossible. HAProxy's get_server_rnd (2.8 src/backend.c) draws each candidate from an independent statistical_prng(), so with `random(<draws>)` the draws are taken with replacement -- the `prev != curr` guard in the comparison exists exactly because a draw can repeat. On a repeat the load comparison is skipped and the single drawn server is used regardless of load. The chance of that is 1/n, so at two replicas half of all selections carry no load signal at all and the busier replica is picked about a quarter of the time. leastconn picks the less-loaded server every time. Power-of-two-choices earns its reputation against *random* selection, and against leastconn only when balancers are numerous or their load view is stale -- the regime where leastconn's determinism synchronizes them. A default has to hold for the two-replica deployment too, and there the source shows it strictly loses. Reverting until there is an A/B to weigh the large-fleet gain against this measured small-fleet cost. The remaining defaults in this PR are unaffected; the defaults test drops its balance assertion accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: harshit-anyscale <harshit@anyscale.com>
CI (premerge 72546) failed test_e2e_preserve_prev_replicas_rest_api across all four autoscaling_policy targets, on both shards, surviving per-test retry. Reproduced locally against a real HAProxy with only this env var toggled and everything else held constant: RAY_SERVE_HAPROXY_BROADCAST_COALESCE_S=1.0 -> failed (x3) RAY_SERVE_HAPROXY_BROADCAST_COALESCE_S=0.1 -> passed (x3) and every probed value at or above 0.15 fails, so no smaller value is viable either. The failure is a scale-from-zero request returning 404. An application is reported RUNNING by the controller without waiting for the proxies to apply the corresponding HAProxy config, so for roughly a second after the deploy completes there is no backend for the route at all -- not an empty backend that would fall through to the fallback server, but no backend, which means default_backend and a 404. A request arriving in that window registers no queued demand, so the deployment never scales up: [1.20s] status=RUNNING [1.20s] GET / -> 404 "Path '/' not found." [2.26s] GET / -> 200 That gap exists at 0.1 too; raising the coalesce window simply moves the reload past the point where RUNNING is reported. Fixing it belongs with the readiness reporting rather than here, so this reverts to the previous default and leaves the window alone until then. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: harshit-anyscale <harshit@anyscale.com>
akyang-anyscale
left a comment
There was a problem hiding this comment.
one comment on timeout connect. other change lgtm
| RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S = get_env_int_non_negative( | ||
| "RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S", 5 | ||
| ) |
There was a problem hiding this comment.
How does one disable this?
There was a problem hiding this comment.
Set it to 0. Documented in the comment as of fe769b1.
Details
get_env_int_non_negative accepts 0 (it validates >= 0), and the template gates on is not none, so RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S=0 renders timeout connect 0s. HAProxy stores an unset timeout as 0 internally, so that is indistinguishable from omitting the directive — in 2.8:
proxy_parse_timeoutaccepts a literal0(only sub-millisecond non-zero values are rejected, asPARSE_TIME_UNDER: "minimum non-null value is 1 ms"), then assigns*tv = MS_TO_TICKS(0)==0.- The missing-timeouts warning tests
!curproxy->timeout.connect(cfgparse.c), so0and unset take the same branch. - The tarpit/queue inheritance is
if (!timeout.queue) timeout.queue = timeout.connect, which copies the same0either way — so no divergent side effect from the fact that Serve leavestimeout queueunset.
Confirmed against a running HAProxy with a blackholed backend, config identical apart from this one line:
timeout connect 2s -> HTTP/1.1 503 Service Unavailable after 2.00s
timeout connect 0s -> no response after 12.00s (connect still pending)
So =0 restores the previous behaviour exactly: infinite connect timeout, and the same missing timeouts for backend ... startup warning that omitting it produced.
Caveat on the runtime check: that was HAProxy 3.4.4 locally, since that is what I can run on macOS. The source references above are from 2.8, which is what ray-haproxy ships.
🤖 Investigated and drafted with Claude Code
Review question on the new default: with the constant no longer Optional, how does one turn the timeout off? Setting it to 0 does that. HAProxy stores an unset timeout as 0 internally, so `timeout connect 0s` is indistinguishable from omitting the directive: proxy_parse_timeout accepts a literal 0 (only sub-millisecond non-zero values are rejected as PARSE_TIME_UNDER) and assigns MS_TO_TICKS(0) == 0; the missing-timeouts warning tests `!curproxy->timeout.connect`, so 0 and unset take the same branch; and the queue/tarpit inheritance copies the same 0 either way. Verified against a running HAProxy: with `timeout connect 2s` a blackholed backend returns 503 after 2.00s, while with `0s` the connect is still pending after 12s. Comment only, no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: harshit-anyscale <harshit@anyscale.com>
Why this change
Two HAProxy defaults that are reasonable for a single-node dev cluster but poor once replicas come and go under autoscaling. Each is a default change only; both stay overridable through their existing environment variables.
RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S5RAY_SERVE_HAPROXY_OBSERVE_MARK_DOWN_ENABLED01timeout connect: unset →5sReplicas are in-cluster, so a connect that takes seconds means the node is gone rather than busy. Bounding it lets
retry-on conn-failure+option redispatchreach another replica while the request still has budget. This is connection-establishment time only — it does not bound request duration.timeout serveris deliberately not set here; see Deliberately not included.observe layer4 ... on-error mark-down: off → onLive traffic marks a dead replica DOWN without waiting for a health checker, and
redispatchplus thebackupfallback take over. A false positive revives in ~0.5s via the existing health check. Backup/fallback servers are never observed.Not a duplicate
Checked before opening, no overlapping work found:
The only open PR touching this area is #65698 (mine, direct-ingress listener close), which does not change any of these constants.
Tests
Two environments. (A) nightly
ray-3.0.0.dev0wheel withpython/ray/serveandpython/ray/testssymlinked in,pytest==7.4.4perpython/requirements/test-requirements.txt— used for the rendering and unit runs. (B) the same, plus a real HAProxy binary (brew3.4.4) viaRAY_SERVE_HAPROXY_BINARY_PATH, running the CI job's env (RAY_SERVE_ENABLE_HA_PROXY=1,RAY_SERVE_DIRECT_INGRESS_MIN_DRAINING_PERIOD_S=0.01,SERVE_SOCKET_REUSE_PORT_ENABLED=1) — used for the integration runs.End-to-end with a real HAProxy (B) — the tests premerge flagged on earlier revisions of this PR, re-run at this revision's defaults with no env overrides:
Both were failing on earlier revisions and pass here. Caveats on this environment: it is HAProxy 3.4.4, while CI ships 2.8 via
ray-haproxy; and macOS has nosplice(2), sooption splice-request/option splice-responsehad to be dropped from the rendered config for HAProxy to start locally. Those options affect data forwarding, not routing or timing. CI remains the authority.HAProxy config rendering (A):
New test
test_default_data_plane_tuning_renderspins the shipped values. Every other test in that file passes explicit overrides, so nothing previously caught a silent change to these defaults. It isskipif-guarded on the env vars it asserts about, so overriding any of them skips rather than fails:The skip does not neuter it — changing a shipped default in source still fails the test.
Serve unit suite (A) —
pytest python/ray/serve/tests/unit:Identical, so failure-neutral. The 195 are an artifact of running repo Serve code against a slightly older nightly core wheel on macOS; they are in
test_deployment_state.py/test_handle_options.pyand touch nothing this PR changes. Measured on the six-default revision of this PR; the diff has only shrunk since, so this is a ceiling rather than a current reading.Lint at the versions pinned in
.pre-commit-config.yaml, re-run at this revision:mypy 1.7.0andpyrefly 1.1.1(both of which haveconstants.pyon their allowlists) were clean on an earlier revision that already contained both remaining changes; they were not re-run at this exact commit, since everything removed since was a revert to master's own text.One shape change worth calling out:
RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_Smoves from the hand-rolledint(os.environ.get(...)) if os.environ.get(...) else ...conditional to the file's existingget_env_int_non_negativehelper, which drops a# type: ignore[arg-type]. Its default is changing anyway, so the line is touched regardless, and the helper handles=0correctly — the truthiness form would silently turn an explicit0into the default. It does mean an empty-string value now raises instead of being ignored, matching every otherget_env_*constant in the file. Say the word if you would rather keep the original shape and just swap theelsebranch.AI assistance
This change was drafted with AI assistance (Claude), including the code, the tests, and this description.
Opened as a draft on purpose: per
AGENTS.mda human submitter has to review every changed line and run the relevant tests locally before review is requested. I have not finished that pass yet. Marking ready for review is the signal that it is done — please don't spend review time on it before then.The binary-dependent HAProxy tests (26 deselected above) also still need a run on a machine that has
haproxyinstalled.