diff --git a/python/ray/serve/_private/constants.py b/python/ray/serve/_private/constants.py index 1889d31c2aeb..2e1b5d02c9a5 100644 --- a/python/ray/serve/_private/constants.py +++ b/python/ray/serve/_private/constants.py @@ -850,12 +850,16 @@ else None ) -RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S = ( - # Guarded by the truthiness check below; the two get() calls can't be - # narrowed by mypy. - int(os.environ.get("RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S")) # type: ignore[arg-type] - if os.environ.get("RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S") - else None +# Connection timeout to a replica, in seconds. Replicas 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 redispatch` reach another replica +# while the request still has budget. +# +# Set to 0 to disable. HAProxy stores an unset timeout as 0, so `timeout +# connect 0s` is indistinguishable from omitting the directive: an infinite +# connect timeout, plus HAProxy's "missing timeouts" startup warning. +RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S = get_env_int_non_negative( + "RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S", 5 ) # When enabled, adds 'option http-no-delay' to the HAProxy config defaults, @@ -908,7 +912,7 @@ # redispatch + the `backup` fallback take over. Health checks revive a false # positive in ~0.5s. Backup/fallback servers are never observed. RAY_SERVE_HAPROXY_OBSERVE_MARK_DOWN_ENABLED = get_env_bool( - "RAY_SERVE_HAPROXY_OBSERVE_MARK_DOWN_ENABLED", "0" + "RAY_SERVE_HAPROXY_OBSERVE_MARK_DOWN_ENABLED", "1" ) # Consecutive observed layer4 errors before a server is marked DOWN. Only diff --git a/python/ray/serve/tests/test_haproxy_api.py b/python/ray/serve/tests/test_haproxy_api.py index bbbb8caf736d..cfbc41c8b196 100644 --- a/python/ray/serve/tests/test_haproxy_api.py +++ b/python/ray/serve/tests/test_haproxy_api.py @@ -397,8 +397,8 @@ def test_generate_config_file_internal(haproxy_api_cleanup): http-check expect status 200 default-server fastinter 250ms downinter 250ms fall 2 rise 3 inter 5s check # Servers in this backend - server api_server1 127.0.0.1:8001 check - server api_server2 127.0.0.1:8002 check + server api_server1 127.0.0.1:8001 check observe layer4 error-limit 3 on-error mark-down + server api_server2 127.0.0.1:8002 check observe layer4 error-limit 3 on-error mark-down # Fallback to head node's Serve proxy when no ingress replicas are available server api_fallback_server 127.0.0.1:8500 check backup backend web_backend @@ -417,7 +417,7 @@ def test_generate_config_file_internal(haproxy_api_cleanup): http-check expect status 200 default-server fastinter 250ms downinter 250ms fall 3 rise 2 inter 2s check # Servers in this backend - server web_server1 127.0.0.1:8003 check + server web_server1 127.0.0.1:8003 check observe layer4 error-limit 3 on-error mark-down listen stats bind *:8080 stats enable @@ -941,6 +941,66 @@ def render(overrides): assert "\n retries 2\n" in overridden +# Overriding any of these is supported, and makes the shipped-default +# assertions below meaningless rather than false, so the test opts out instead. +_DEFAULT_TUNING_ENV_VARS = ( + "RAY_SERVE_HAPROXY_OBSERVE_MARK_DOWN_ENABLED", + "RAY_SERVE_HAPROXY_OBSERVE_ERROR_LIMIT", + "RAY_SERVE_HAPROXY_TIMEOUT_CONNECT_S", + "RAY_SERVE_HAPROXY_TIMEOUT_SERVER_S", +) + + +@pytest.mark.skipif( + any(v in os.environ for v in _DEFAULT_TUNING_ENV_VARS), + reason=f"environment overrides one of {_DEFAULT_TUNING_ENV_VARS}", +) +def test_default_data_plane_tuning_renders(haproxy_api_cleanup): + """The shipped defaults bound connect time, mark dead replicas down from + live traffic, and leave `timeout server` unset. + + Every other test in this file passes explicit overrides, so nothing else + would catch a silent change to these; each one changes how every Serve + cluster routes.""" + with tempfile.TemporaryDirectory() as temp_dir: + config_file_path = os.path.join(temp_dir, "haproxy.cfg") + api = HAProxyApi( + cfg=HAProxyConfig(socket_path=os.path.join(temp_dir, "admin.sock")), + backend_configs={ + "api": BackendConfig( + name="api", + path_prefix="/api", + app_name="api", + servers=[ + ServerConfig( + name="s1", host="127.0.0.1", port=8001, replica_id="rid_1" + ) + ], + ) + }, + config_file_path=config_file_path, + ) + api._generate_config_file_internal() + with open(config_file_path) as f: + cfg = f.read() + + # Replicas are in-cluster: a connect that takes seconds means the node is + # gone, and failing fast lets redispatch reach another replica in time. + assert "\n timeout connect 5s\n" in cfg + + # No `timeout server`. It is a server-side inactivity limit applied to every + # connection on the live process, so any value caps request duration -- and + # Serve's own request_timeout_s defaults to no timeout. hard-stop-after + # bounds draining workers on a separate clock. + assert "\n timeout server " not in cfg + + # Live traffic marks a dead replica DOWN without waiting for a health check. + assert ( + "server s1 127.0.0.1:8001 check observe layer4 " + "error-limit 3 on-error mark-down" + ) in cfg + + @pytest.mark.parametrize("forward_body", [True, False]) def test_ingress_request_router_forward_body_gate_renders( haproxy_api_cleanup, monkeypatch, forward_body