feat(xceadmin): accept a comma-separated --controller failover list (bug-1821, bug-1822)
xceadmin's --controller option (config push, and scheduler record start/stop/status) accepted only a single host:port. Passing every configured controller as a comma-separated list -- the natural way to query an HA cluster without knowing which node is alive/primary -- mis-parsed the whole joined string as one bogus hostname and failed with a DNS-resolution error. Adds xceadmin.discovery.resolve_controllers() (comma-split candidate parsing) and follow_redirects_any() (tries each candidate in order, skipping ones that are unreachable at the network level -- connection refused, DNS failure, or a discovery timeout -- mirroring xcecli/ xce_test's own client-side master-rotation failover). A candidate that IS reachable but returns a genuine protocol-level failure is not retried against a different node. push.py and scheduler.py's own duplicated single-host _split_controller helpers are removed in favor of this shared implementation. Also fixes a related gap bug-1821's own live verification surfaced: right after a primary is killed, a standby doesn't start a new election until its own heartbeat timeout elapses (~10s), so it keeps redirecting every candidate to the now-dead old primary -- exhausting the whole candidate list and raising NoPrimaryError, which _send_admin_request()'s existing transient-retry loop (bug-1713/1715) never caught. That loop now retries on NoPrimaryError too, using its own (longer) budget sized for the heartbeat-timeout-plus-election window rather than the RPC-level "already electing" budget the original retry logic was tuned for. TDD RED->GREEN, 304/304 tests passing. Live-verified end to end: a comma-list correctly skips a dead first candidate and reaches a live one on a healthy cluster, and correctly rides out a real primary kill and re-election (retry loop visibly cycling ~15s) to reach the newly- elected primary rather than failing immediately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
parent
baa07d78db
commit
723fdb637b
8 changed files with 521 additions and 82 deletions
|
|
@ -140,8 +140,10 @@ def config_build(configs_dir: str, output: str) -> None:
|
|||
"--controller",
|
||||
default=_DEFAULT_CONTROLLER,
|
||||
envvar=_CONTROLLER_ENV_VAR,
|
||||
help=f"host:port of any controller node (2.2). Falls back to ${_CONTROLLER_ENV_VAR} "
|
||||
"if set, then the bare default.",
|
||||
help=f"host:port of any controller node (2.2), or a comma-separated list of "
|
||||
"host:port candidates to try in order (failing over past unreachable/dead "
|
||||
"ones, e.g. --controller host1:9991,host2:9991,host3:9991). Falls back to "
|
||||
f"${_CONTROLLER_ENV_VAR} if set, then the bare default.",
|
||||
)
|
||||
@click.option("--dry-run", is_flag=True, help="Probe acceptance without committing (Requirement 6).")
|
||||
def config_push(cluster_bin_path: str, controller: str, dry_run: bool) -> None:
|
||||
|
|
@ -271,8 +273,10 @@ def _run_scheduler_admin_command(controller: str, call: Callable[[], RecordResul
|
|||
"--controller",
|
||||
default=_DEFAULT_CONTROLLER,
|
||||
envvar=_CONTROLLER_ENV_VAR,
|
||||
help=f"host:port of any controller node (2.2). Falls back to ${_CONTROLLER_ENV_VAR} "
|
||||
"if set, then the bare default.",
|
||||
help=f"host:port of any controller node (2.2), or a comma-separated list of "
|
||||
"host:port candidates to try in order (failing over past unreachable/dead "
|
||||
"ones, e.g. --controller host1:9991,host2:9991,host3:9991). Falls back to "
|
||||
f"${_CONTROLLER_ENV_VAR} if set, then the bare default.",
|
||||
)
|
||||
def scheduler_record_start(file: str | None, controller: str) -> None:
|
||||
"""Start scheduler event recording, optionally to FILE (auto-generated when omitted)."""
|
||||
|
|
@ -285,8 +289,10 @@ def scheduler_record_start(file: str | None, controller: str) -> None:
|
|||
"--controller",
|
||||
default=_DEFAULT_CONTROLLER,
|
||||
envvar=_CONTROLLER_ENV_VAR,
|
||||
help=f"host:port of any controller node (2.2). Falls back to ${_CONTROLLER_ENV_VAR} "
|
||||
"if set, then the bare default.",
|
||||
help=f"host:port of any controller node (2.2), or a comma-separated list of "
|
||||
"host:port candidates to try in order (failing over past unreachable/dead "
|
||||
"ones, e.g. --controller host1:9991,host2:9991,host3:9991). Falls back to "
|
||||
f"${_CONTROLLER_ENV_VAR} if set, then the bare default.",
|
||||
)
|
||||
def scheduler_record_stop(controller: str) -> None:
|
||||
"""Stop scheduler event recording."""
|
||||
|
|
@ -299,8 +305,10 @@ def scheduler_record_stop(controller: str) -> None:
|
|||
"--controller",
|
||||
default=_DEFAULT_CONTROLLER,
|
||||
envvar=_CONTROLLER_ENV_VAR,
|
||||
help=f"host:port of any controller node (2.2). Falls back to ${_CONTROLLER_ENV_VAR} "
|
||||
"if set, then the bare default.",
|
||||
help=f"host:port of any controller node (2.2), or a comma-separated list of "
|
||||
"host:port candidates to try in order (failing over past unreachable/dead "
|
||||
"ones, e.g. --controller host1:9991,host2:9991,host3:9991). Falls back to "
|
||||
f"${_CONTROLLER_ENV_VAR} if set, then the bare default.",
|
||||
)
|
||||
def scheduler_record_status(controller: str) -> None:
|
||||
"""Report current scheduler event-recording state."""
|
||||
|
|
|
|||
|
|
@ -42,6 +42,90 @@ from xcecommon.schema import load_schema
|
|||
#: to 3 hops are followed; a 4th redirect is the error.
|
||||
_MAX_REDIRECT_HOPS = 3
|
||||
|
||||
#: Default port for a bare host with no ":port" suffix (Requirement 2.2).
|
||||
#: Single source of truth -- ``push.py``/``scheduler.py`` used to each keep
|
||||
#: their own copy of this constant alongside their own ``_split_controller``;
|
||||
#: both now delegate to :func:`resolve_controllers` below instead.
|
||||
_DEFAULT_PORT = 9991
|
||||
|
||||
|
||||
def resolve_controllers(controller: str) -> list[tuple[str, int]]:
|
||||
"""Parse a ``--controller`` value into an ordered list of ``(host,
|
||||
port)`` candidates.
|
||||
|
||||
Accepts either a single ``"host:port"`` (or a bare host, defaulting to
|
||||
port 9991) -- the original, still-supported form -- or a
|
||||
comma-separated list of such entries (``"host1:port1,host2:port2,..."``)
|
||||
for an HA cluster where the caller doesn't know in advance which node
|
||||
is reachable, mirrors the client-side master-rotation failover
|
||||
``xcecli``/``xce_test`` already use against a configured masters list
|
||||
(``.kiro/steering/api-standards.md``: "the XCE client shall attempt the
|
||||
configured controllers ... until one responds or the attempts are
|
||||
exhausted"). Each entry is parsed independently, so a bare host without
|
||||
a port anywhere in the list still gets the 9991 default.
|
||||
|
||||
Whitespace around each comma-separated entry is stripped and empty
|
||||
entries (e.g. a stray trailing comma) are dropped, so ``"a:1, b:2,"``
|
||||
parses the same as ``"a:1,b:2"``.
|
||||
"""
|
||||
candidates: list[tuple[str, int]] = []
|
||||
for part in controller.split(","):
|
||||
part = part.strip()
|
||||
if not part:
|
||||
continue
|
||||
if ":" in part:
|
||||
host, port_str = part.rsplit(":", 1)
|
||||
candidates.append((host, int(port_str)))
|
||||
else:
|
||||
candidates.append((part, _DEFAULT_PORT))
|
||||
return candidates
|
||||
|
||||
|
||||
def follow_redirects_any(
|
||||
candidates: list[tuple[str, int]],
|
||||
send_fn,
|
||||
) -> tuple[str, int, bytes]:
|
||||
"""Try each ``(host, port)`` in ``candidates``, in order, following
|
||||
redirects (via :func:`follow_redirects`) from whichever one answers
|
||||
first.
|
||||
|
||||
A candidate that is unreachable at the network level -- connection
|
||||
refused, DNS resolution failure (both plain :class:`OSError`
|
||||
subclasses that ``xcecommon.wire.connect`` deliberately propagates
|
||||
unwrapped), or a discovery-level timeout (:class:`NoPrimaryError`,
|
||||
already raised by :func:`follow_redirects` itself) -- is skipped in
|
||||
favor of the next candidate, exactly like ``xcecli``'s/``xce_test``'s
|
||||
own client-side failover across a configured masters list. A candidate
|
||||
that IS reachable but returns a genuinely undecodable reply
|
||||
(:class:`ReplyDecodeError`) or a redirect chain that's too long
|
||||
(:class:`TooManyRedirectsError`) is NOT retried against another
|
||||
candidate -- those are real protocol-level failures a different node
|
||||
wouldn't fix, so they propagate immediately.
|
||||
|
||||
Raises :class:`NoPrimaryError`, naming every candidate tried, if none
|
||||
of them answer.
|
||||
"""
|
||||
last_error: Exception | None = None
|
||||
for host, port in candidates:
|
||||
try:
|
||||
return follow_redirects(host, port, send_fn)
|
||||
except (NoPrimaryError, OSError) as exc:
|
||||
print(
|
||||
f"controller {host}:{port} unreachable ({exc}) -- trying next candidate",
|
||||
file=sys.stderr,
|
||||
)
|
||||
last_error = exc
|
||||
continue
|
||||
|
||||
tried = ", ".join(f"{host}:{port}" for host, port in candidates) or "(none)"
|
||||
last_host, last_port = candidates[-1] if candidates else ("", 0)
|
||||
raise NoPrimaryError(
|
||||
f"no primary available: none of the configured controllers responded ({tried})"
|
||||
+ (f"; last error: {last_error}" if last_error is not None else ""),
|
||||
last_host,
|
||||
last_port,
|
||||
) from last_error
|
||||
|
||||
|
||||
@contextlib.contextmanager
|
||||
def decode_reply(reply_bytes: bytes):
|
||||
|
|
|
|||
|
|
@ -10,10 +10,11 @@ struct, no wrapper) -- their replies share no fields, so a shared return
|
|||
type would misrepresent one of the two.
|
||||
|
||||
Both delegate connection + redirect-following to
|
||||
``xceadmin.discovery.follow_redirects`` before decoding a reply, and both
|
||||
build/decode using the real ``controller.capnp`` schema
|
||||
(``xcecommon.schema.load_schema()``) -- never touching the socket
|
||||
directly except through ``xcecommon.wire``.
|
||||
``xceadmin.discovery.follow_redirects_any`` (candidate-list failover across
|
||||
every ``--controller`` address, parsed by ``xceadmin.discovery.
|
||||
resolve_controllers``) before decoding a reply, and both build/decode using
|
||||
the real ``controller.capnp`` schema (``xcecommon.schema.load_schema()``)
|
||||
-- never touching the socket directly except through ``xcecommon.wire``.
|
||||
|
||||
``probe_config()`` has no live controller counterpart today
|
||||
(``research.md``, "ProbeConfigTag liveness": no ``OPCode_probeConfigTag``
|
||||
|
|
@ -48,9 +49,7 @@ from pathlib import Path
|
|||
from xcecommon import wire
|
||||
from xcecommon.errors import ReplyDecodeError
|
||||
from xcecommon.schema import load_schema
|
||||
from xceadmin.discovery import decode_reply, follow_redirects
|
||||
|
||||
_DEFAULT_PORT = 9991
|
||||
from xceadmin.discovery import decode_reply, follow_redirects_any, resolve_controllers
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
|
|
@ -68,16 +67,6 @@ class DryRunResult:
|
|||
reason: str
|
||||
|
||||
|
||||
def _split_controller(controller: str) -> tuple[str, int]:
|
||||
"""``"host:port"`` -> ``(host, port)``; a bare host uses the default
|
||||
port 9991 (Requirement 2.2's default).
|
||||
"""
|
||||
if ":" in controller:
|
||||
host, port_str = controller.rsplit(":", 1)
|
||||
return host, int(port_str)
|
||||
return controller, _DEFAULT_PORT
|
||||
|
||||
|
||||
def _send_via(request_bytes: bytes):
|
||||
def _send(host: str, port: int) -> bytes:
|
||||
sock = wire.connect(host, port)
|
||||
|
|
@ -109,8 +98,8 @@ def push_config(cluster_bin_path: str, controller: str) -> PushResult:
|
|||
upgrade.bytes = data
|
||||
request_bytes = request.to_bytes()
|
||||
|
||||
host, port = _split_controller(controller)
|
||||
_, _, reply_bytes = follow_redirects(host, port, _send_via(request_bytes))
|
||||
candidates = resolve_controllers(controller)
|
||||
_, _, reply_bytes = follow_redirects_any(candidates, _send_via(request_bytes))
|
||||
|
||||
with decode_reply(reply_bytes) as reply:
|
||||
if reply.status != 0:
|
||||
|
|
@ -157,8 +146,8 @@ def probe_config(cluster_bin_path: str, controller: str) -> DryRunResult:
|
|||
probe.name = ""
|
||||
request_bytes = request.to_bytes()
|
||||
|
||||
host, port = _split_controller(controller)
|
||||
_, _, reply_bytes = follow_redirects(host, port, _send_via(request_bytes))
|
||||
candidates = resolve_controllers(controller)
|
||||
_, _, reply_bytes = follow_redirects_any(candidates, _send_via(request_bytes))
|
||||
|
||||
with decode_reply(reply_bytes) as reply:
|
||||
if reply.status != 0:
|
||||
|
|
|
|||
|
|
@ -14,11 +14,13 @@ docstring and design rationale.
|
|||
|
||||
``record_start``/``record_stop``/``record_status`` mirror ``push.py``'s
|
||||
``push_config()`` pipeline exactly: build request bytes with the real
|
||||
``controller.capnp`` schema (``xcecommon.schema.load_schema()``) -> connect
|
||||
+ follow redirects to the primary (``xceadmin.discovery.follow_redirects``,
|
||||
Requirement 3.1-3.4 -- the scheduler is a singleton attached to the
|
||||
primary controller, so these commands must reach it the same way ``config
|
||||
push`` reaches the primary) -> decode the ``Request{opcode: reply,
|
||||
``controller.capnp`` schema (``xcecommon.schema.load_schema()``) -> resolve
|
||||
the ``--controller`` value to a candidate list and connect + follow
|
||||
redirects to the primary (``xceadmin.discovery.resolve_controllers`` +
|
||||
``follow_redirects_any``, Requirement 3.1-3.4 -- the scheduler is a
|
||||
singleton attached to the primary controller, so these commands must reach
|
||||
it the same way ``config push`` reaches the primary) -> decode the
|
||||
``Request{opcode: reply,
|
||||
body.replymsg: RequestReply}`` envelope (``xceadmin.discovery.decode_reply``)
|
||||
-> extract the ``SchedAdminReply`` payload. All three actions share one
|
||||
``SchedAdminRequest``/``SchedAdminReply`` wire shape
|
||||
|
|
@ -67,11 +69,9 @@ import time
|
|||
from dataclasses import dataclass
|
||||
|
||||
from xcecommon import wire
|
||||
from xcecommon.errors import ReplayFileError, ReplyDecodeError, SchedAdminError
|
||||
from xcecommon.errors import NoPrimaryError, ReplayFileError, ReplyDecodeError, SchedAdminError
|
||||
from xcecommon.schema import load_schema
|
||||
from xceadmin.discovery import decode_reply, follow_redirects
|
||||
|
||||
_DEFAULT_PORT = 9991
|
||||
from xceadmin.discovery import decode_reply, follow_redirects_any, resolve_controllers
|
||||
|
||||
# bug-1713: errnox.h's enum (XCE_ERRNO_BASE=10001) numbers E_XCE_NOMASTER at
|
||||
# index 10 (10001+10=10011, "Standby has no known primary -- client should
|
||||
|
|
@ -111,6 +111,26 @@ _TRANSIENT_STATUSES = (_E_XCE_NOMASTER, _E_XCE_NOT_READY)
|
|||
_MAX_TRANSIENT_ATTEMPTS = 4
|
||||
_TRANSIENT_RETRY_DELAY_S = 1.5
|
||||
|
||||
# Bounded retry budget for a total candidate-list exhaustion
|
||||
# (NoPrimaryError from follow_redirects_any) -- a DIFFERENT, longer worst
|
||||
# case than _MAX_TRANSIENT_ATTEMPTS/_TRANSIENT_RETRY_DELAY_S above, so it
|
||||
# gets its own dedicated constants rather than reusing those untouched.
|
||||
# _TRANSIENT_STATUSES' scenario is "the cluster is already electing and
|
||||
# replies not-ready" (RPC-level settling, single-digit seconds). This
|
||||
# scenario is live-verified to be categorically slower: right after a
|
||||
# primary is killed, a standby doesn't even START a new election until its
|
||||
# own heartbeat_timeout_ms elapses (ha.h: HA_DEFAULT_HEARTBEAT_TIMEOUT_MS =
|
||||
# 10000, i.e. 10s) -- it just keeps redirecting every request to the now-
|
||||
# dead old primary until then. 7 attempts * 2.0s = 14s of sleeping, plus
|
||||
# connection overhead, comfortably covers that 10s detection window plus a
|
||||
# real election (observed ~2-3s in this project's own HA soak runs) with
|
||||
# margin, rather than the 4.5s _MAX_TRANSIENT_ATTEMPTS's budget provides
|
||||
# (confirmed too short live: a kill-then-immediately-query repro exhausted
|
||||
# _MAX_TRANSIENT_ATTEMPTS's budget while every candidate was still
|
||||
# redirecting to the dead primary).
|
||||
_MAX_NO_PRIMARY_ATTEMPTS = 8
|
||||
_NO_PRIMARY_RETRY_DELAY_S = 2.0
|
||||
|
||||
# bug-1715: a SECOND, differently-signaled transient-condition family, on
|
||||
# top of bug-1713's numeric _TRANSIENT_STATUSES above. cluster.c's
|
||||
# OPCode_schedAdmin dispatch (cluster.c:880-1090) can send
|
||||
|
|
@ -168,19 +188,6 @@ class RecordResult:
|
|||
started_at: int
|
||||
|
||||
|
||||
def _split_controller(controller: str) -> tuple[str, int]:
|
||||
"""``"host:port"`` -> ``(host, port)``; a bare host uses the default
|
||||
port 9991 -- same convention as ``push.py``'s own ``_split_controller``
|
||||
(each resource module owns this trivial parsing rather than sharing a
|
||||
private helper across modules, matching this package's existing
|
||||
per-module-self-contained precedent).
|
||||
"""
|
||||
if ":" in controller:
|
||||
host, port_str = controller.rsplit(":", 1)
|
||||
return host, int(port_str)
|
||||
return controller, _DEFAULT_PORT
|
||||
|
||||
|
||||
def _send_via(request_bytes: bytes):
|
||||
def _send(host: str, port: int) -> bytes:
|
||||
sock = wire.connect(host, port)
|
||||
|
|
@ -205,16 +212,44 @@ def _build_request(action: str, record_file: str = "") -> bytes:
|
|||
|
||||
def _send_admin_request(controller: str, action: str, record_file: str = "") -> RecordResult:
|
||||
request_bytes = _build_request(action, record_file)
|
||||
host, port = _split_controller(controller)
|
||||
candidates = resolve_controllers(controller)
|
||||
|
||||
for attempt in range(1, _MAX_TRANSIENT_ATTEMPTS + 1):
|
||||
# bug-1713: re-run follow_redirects() from scratch on every attempt,
|
||||
# not just a bare resend to the same host:port -- the whole point of
|
||||
# retrying E_XCE_NOMASTER/E_XCE_NOT_READY is "the cluster is still
|
||||
# settling", so a fresh attempt may discover a different (or newly-
|
||||
# settled) primary once an election concludes, exactly like a
|
||||
# brand-new invocation of this command would.
|
||||
_, _, reply_bytes = follow_redirects(host, port, _send_via(request_bytes))
|
||||
# Two independent retry counters/budgets, not one shared `attempt`:
|
||||
# no_primary_attempt (NoPrimaryError, total candidate-list exhaustion)
|
||||
# and status_attempt (a reply that DID arrive but says not-ready, shared
|
||||
# by the reply.status and admin_reply.ok==False branches below, exactly
|
||||
# as before). See _MAX_NO_PRIMARY_ATTEMPTS's own comment for why these
|
||||
# two failure modes need differently-sized budgets.
|
||||
no_primary_attempt = 0
|
||||
status_attempt = 0
|
||||
while True:
|
||||
# bug-1713: re-run follow_redirects_any() from scratch on every
|
||||
# attempt, not just a bare resend to the same host:port -- the whole
|
||||
# point of retrying E_XCE_NOMASTER/E_XCE_NOT_READY is "the cluster is
|
||||
# still settling", so a fresh attempt may discover a different (or
|
||||
# newly-settled) primary once an election concludes, exactly like a
|
||||
# brand-new invocation of this command would. Re-walking the full
|
||||
# candidate list each time also means a controller that was dead on
|
||||
# attempt 1 but has since come back gets a fresh chance too.
|
||||
try:
|
||||
_, _, reply_bytes = follow_redirects_any(candidates, _send_via(request_bytes))
|
||||
except NoPrimaryError:
|
||||
# Live-verified gap: right after a primary is killed, a standby
|
||||
# hasn't necessarily noticed yet (heartbeat_timeout_ms, ~10s)
|
||||
# and can still redirect every candidate straight back to the
|
||||
# now-dead old primary -- follow_redirects_any then exhausts
|
||||
# the whole candidate list with nothing genuinely reachable,
|
||||
# even though this is the exact same "cluster still settling"
|
||||
# condition _TRANSIENT_STATUSES already retries below, just
|
||||
# signaled by a total connection failure instead of a reply
|
||||
# carrying a not-ready status. Retry the whole candidate list
|
||||
# fresh using its own (longer) budget/delay rather than giving
|
||||
# up after a single pass.
|
||||
no_primary_attempt += 1
|
||||
if no_primary_attempt < _MAX_NO_PRIMARY_ATTEMPTS:
|
||||
time.sleep(_NO_PRIMARY_RETRY_DELAY_S)
|
||||
continue
|
||||
raise
|
||||
|
||||
with decode_reply(reply_bytes) as reply:
|
||||
if reply.status != 0:
|
||||
|
|
@ -223,9 +258,11 @@ def _send_admin_request(controller: str, action: str, record_file: str = "") ->
|
|||
# even be the schedadminreplymsg variant (e.g. a bare
|
||||
# debugmsg), so this is reported before ever inspecting
|
||||
# reply.data.which().
|
||||
if reply.status in _TRANSIENT_STATUSES and attempt < _MAX_TRANSIENT_ATTEMPTS:
|
||||
time.sleep(_TRANSIENT_RETRY_DELAY_S)
|
||||
continue
|
||||
if reply.status in _TRANSIENT_STATUSES:
|
||||
status_attempt += 1
|
||||
if status_attempt < _MAX_TRANSIENT_ATTEMPTS:
|
||||
time.sleep(_TRANSIENT_RETRY_DELAY_S)
|
||||
continue
|
||||
|
||||
reason = f"controller returned status {reply.status}"
|
||||
if reply.status in _TRANSIENT_STATUSES:
|
||||
|
|
@ -235,8 +272,8 @@ def _send_admin_request(controller: str, action: str, record_file: str = "") ->
|
|||
# error string so this outcome is never confused with an
|
||||
# immediate, non-retried hard failure.
|
||||
reason += (
|
||||
f" (still transient after {attempt} attempts over "
|
||||
f"~{(attempt - 1) * _TRANSIENT_RETRY_DELAY_S:.1f}s -- giving up)"
|
||||
f" (still transient after {status_attempt} attempts over "
|
||||
f"~{(status_attempt - 1) * _TRANSIENT_RETRY_DELAY_S:.1f}s -- giving up)"
|
||||
)
|
||||
raise SchedAdminError(
|
||||
f"scheduler admin request failed: {reason}",
|
||||
|
|
@ -261,9 +298,11 @@ def _send_admin_request(controller: str, action: str, record_file: str = "") ->
|
|||
# scratch shape as the reply.status branch above, since a
|
||||
# fresh attempt can equally discover a newly-reconnected
|
||||
# scheduler or a newly-freed admission slot.
|
||||
if admin_reply.error in _TRANSIENT_ADMIN_REASONS and attempt < _MAX_TRANSIENT_ATTEMPTS:
|
||||
time.sleep(_TRANSIENT_RETRY_DELAY_S)
|
||||
continue
|
||||
if admin_reply.error in _TRANSIENT_ADMIN_REASONS:
|
||||
status_attempt += 1
|
||||
if status_attempt < _MAX_TRANSIENT_ATTEMPTS:
|
||||
time.sleep(_TRANSIENT_RETRY_DELAY_S)
|
||||
continue
|
||||
|
||||
reason = admin_reply.error
|
||||
if admin_reply.error in _TRANSIENT_ADMIN_REASONS:
|
||||
|
|
@ -272,8 +311,8 @@ def _send_admin_request(controller: str, action: str, record_file: str = "") ->
|
|||
# outcome is never confused with an immediate,
|
||||
# non-retried hard failure (e.g. "already recording").
|
||||
reason += (
|
||||
f" (still transient after {attempt} attempts over "
|
||||
f"~{(attempt - 1) * _TRANSIENT_RETRY_DELAY_S:.1f}s -- giving up)"
|
||||
f" (still transient after {status_attempt} attempts over "
|
||||
f"~{(status_attempt - 1) * _TRANSIENT_RETRY_DELAY_S:.1f}s -- giving up)"
|
||||
)
|
||||
raise SchedAdminError(
|
||||
f"scheduler admin request failed: {reason}",
|
||||
|
|
@ -291,11 +330,6 @@ def _send_admin_request(controller: str, action: str, record_file: str = "") ->
|
|||
started_at=admin_reply.startedAt,
|
||||
)
|
||||
|
||||
# Unreachable: every branch above either `continue`s (only when
|
||||
# attempt < _MAX_TRANSIENT_ATTEMPTS) or returns/raises, so the loop
|
||||
# always exits through one of those, never by falling off the end.
|
||||
raise AssertionError("unreachable: _send_admin_request retry loop exited without returning or raising")
|
||||
|
||||
|
||||
def record_start(controller: str, filename: str | None = None) -> RecordResult:
|
||||
"""Start scheduler recording (``xceadmin scheduler record start
|
||||
|
|
|
|||
|
|
@ -24,7 +24,7 @@ import pytest
|
|||
from xcecommon import wire
|
||||
from xcecommon.errors import ConnectionTimeoutError, NoPrimaryError, ReplyDecodeError, TooManyRedirectsError
|
||||
from xcecommon.schema import load_schema
|
||||
from xceadmin import push
|
||||
from xceadmin import discovery, push
|
||||
|
||||
|
||||
def _server_context(cert_file: Path, key_file: Path) -> ssl.SSLContext:
|
||||
|
|
@ -228,13 +228,17 @@ def test_push_config_rejects_bare_requestreply_bytes_old_wire_shape_regression(
|
|||
|
||||
|
||||
def test_push_config_default_port_used_when_omitted():
|
||||
host, port = push._split_controller("somehost")
|
||||
# push.py delegates --controller parsing to xceadmin.discovery.
|
||||
# resolve_controllers (shared with scheduler.py) -- see test_discovery.py
|
||||
# for that function's own dedicated coverage; this test just confirms
|
||||
# push_config's pipeline still reaches it for the single-host case.
|
||||
(host, port), = discovery.resolve_controllers("somehost")
|
||||
assert host == "somehost"
|
||||
assert port == 9991
|
||||
|
||||
|
||||
def test_push_config_explicit_port_parsed():
|
||||
host, port = push._split_controller("somehost:1234")
|
||||
(host, port), = discovery.resolve_controllers("somehost:1234")
|
||||
assert host == "somehost"
|
||||
assert port == 1234
|
||||
|
||||
|
|
|
|||
|
|
@ -4,6 +4,8 @@ Requirements 3.1, 3.2, 3.3, 3.4, 7.3).
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import socket
|
||||
|
||||
import pytest
|
||||
|
||||
from xcecommon.errors import ConnectionTimeoutError, NoPrimaryError, ReplyDecodeError, TooManyRedirectsError
|
||||
|
|
@ -218,3 +220,191 @@ def test_follow_redirects_propagates_reply_decode_error_on_corrupt_reply():
|
|||
|
||||
with pytest.raises(ReplyDecodeError):
|
||||
discovery.follow_redirects("node1", 9991, send_fn)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# resolve_controllers(): parses a --controller value (single "host:port",
|
||||
# or a comma-separated list of them) into an ordered candidate list.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_resolve_controllers_single_bare_host_uses_default_port():
|
||||
assert discovery.resolve_controllers("somehost") == [("somehost", 9991)]
|
||||
|
||||
|
||||
def test_resolve_controllers_single_host_with_explicit_port():
|
||||
assert discovery.resolve_controllers("somehost:1234") == [("somehost", 1234)]
|
||||
|
||||
|
||||
def test_resolve_controllers_comma_separated_list_parsed_in_order():
|
||||
assert discovery.resolve_controllers("host1:9991,host2:9992,host3:9993") == [
|
||||
("host1", 9991),
|
||||
("host2", 9992),
|
||||
("host3", 9993),
|
||||
]
|
||||
|
||||
|
||||
def test_resolve_controllers_list_mixes_bare_hosts_and_explicit_ports():
|
||||
assert discovery.resolve_controllers("host1,host2:1234") == [
|
||||
("host1", 9991),
|
||||
("host2", 1234),
|
||||
]
|
||||
|
||||
|
||||
def test_resolve_controllers_strips_whitespace_and_drops_empty_entries():
|
||||
# A stray trailing comma or accidental spaces around each entry (e.g.
|
||||
# copy-pasted from a config file) must not produce a bogus "" candidate
|
||||
# or an unparsed leading/trailing space in a hostname.
|
||||
assert discovery.resolve_controllers(" host1:9991 , host2:9992 ,") == [
|
||||
("host1", 9991),
|
||||
("host2", 9992),
|
||||
]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# follow_redirects_any(): tries each (host, port) candidate in order,
|
||||
# failing over past ones that are unreachable at the network level.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_follow_redirects_any_returns_first_candidate_when_reachable():
|
||||
reply = _reply_bytes()
|
||||
calls = []
|
||||
|
||||
def send_fn(host, port):
|
||||
calls.append((host, port))
|
||||
return reply
|
||||
|
||||
final_host, final_port, reply_bytes = discovery.follow_redirects_any(
|
||||
[("host1", 9991), ("host2", 9992)], send_fn
|
||||
)
|
||||
|
||||
assert (final_host, final_port) == ("host1", 9991)
|
||||
assert reply_bytes == reply
|
||||
# The second candidate must never even be attempted when the first
|
||||
# answers -- failover is a last resort, not a fan-out.
|
||||
assert calls == [("host1", 9991)]
|
||||
|
||||
|
||||
def test_follow_redirects_any_skips_connection_refused_and_tries_next(capsys):
|
||||
"""A dead controller raises a plain OSError (ConnectionRefusedError is
|
||||
a subclass) from wire.connect, propagated unwrapped through
|
||||
follow_redirects by design (xcecommon.wire's own docstring) --
|
||||
follow_redirects_any must catch it at the per-candidate level and move
|
||||
on, not let it escape as a raw OSError.
|
||||
"""
|
||||
reply = _reply_bytes()
|
||||
|
||||
def send_fn(host, port):
|
||||
if host == "dead-host":
|
||||
raise ConnectionRefusedError(111, "Connection refused")
|
||||
return reply
|
||||
|
||||
final_host, final_port, reply_bytes = discovery.follow_redirects_any(
|
||||
[("dead-host", 9901), ("live-host", 9902)], send_fn
|
||||
)
|
||||
|
||||
assert (final_host, final_port) == ("live-host", 9902)
|
||||
assert reply_bytes == reply
|
||||
assert "dead-host:9901" in capsys.readouterr().err
|
||||
|
||||
|
||||
def test_follow_redirects_any_skips_dns_failure_and_tries_next():
|
||||
"""socket.gaierror ("Name or service not known") is also a plain
|
||||
OSError subclass -- the exact failure mode from the original bug
|
||||
report (a comma-separated --controller value mis-parsed as one bogus
|
||||
hostname). Confirms a per-candidate DNS failure is skipped too, not
|
||||
just connection-refused.
|
||||
"""
|
||||
reply = _reply_bytes()
|
||||
|
||||
def send_fn(host, port):
|
||||
if host == "bogus.invalid":
|
||||
raise socket.gaierror(-2, "Name or service not known")
|
||||
return reply
|
||||
|
||||
final_host, final_port, reply_bytes = discovery.follow_redirects_any(
|
||||
[("bogus.invalid", 9901), ("real-host", 9902)], send_fn
|
||||
)
|
||||
|
||||
assert (final_host, final_port) == ("real-host", 9902)
|
||||
assert reply_bytes == reply
|
||||
|
||||
|
||||
def test_follow_redirects_any_skips_no_primary_error_and_tries_next():
|
||||
"""A discovery-level timeout on one candidate (NoPrimaryError, already
|
||||
raised by follow_redirects itself) is also skipped in favor of the
|
||||
next candidate, not just a raw OSError.
|
||||
"""
|
||||
reply = _reply_bytes()
|
||||
|
||||
def send_fn(host, port):
|
||||
if host == "slow-host":
|
||||
raise ConnectionTimeoutError("timeout connecting to slow-host:9901", host, port)
|
||||
return reply
|
||||
|
||||
final_host, final_port, reply_bytes = discovery.follow_redirects_any(
|
||||
[("slow-host", 9901), ("fast-host", 9902)], send_fn
|
||||
)
|
||||
|
||||
assert (final_host, final_port) == ("fast-host", 9902)
|
||||
assert reply_bytes == reply
|
||||
|
||||
|
||||
def test_follow_redirects_any_raises_no_primary_error_when_all_candidates_fail():
|
||||
def send_fn(host, port):
|
||||
raise ConnectionRefusedError(111, "Connection refused")
|
||||
|
||||
with pytest.raises(NoPrimaryError) as exc_info:
|
||||
discovery.follow_redirects_any(
|
||||
[("host1", 9991), ("host2", 9992), ("host3", 9993)], send_fn
|
||||
)
|
||||
|
||||
message = str(exc_info.value)
|
||||
assert "host1:9991" in message
|
||||
assert "host2:9992" in message
|
||||
assert "host3:9993" in message
|
||||
|
||||
|
||||
def test_follow_redirects_any_follows_redirect_from_whichever_candidate_answers():
|
||||
"""A candidate that answers with a redirect still gets the normal
|
||||
follow_redirects() hop-following applied -- failover across candidates
|
||||
and redirect-following within one candidate compose cleanly.
|
||||
"""
|
||||
reply_from_standby = _reply_bytes(redirect_host="primary", redirect_port=9991)
|
||||
reply_from_primary = _reply_bytes()
|
||||
|
||||
def send_fn(host, port):
|
||||
if host == "dead-host":
|
||||
raise ConnectionRefusedError(111, "Connection refused")
|
||||
if (host, port) == ("standby", 9992):
|
||||
return reply_from_standby
|
||||
if (host, port) == ("primary", 9991):
|
||||
return reply_from_primary
|
||||
raise AssertionError(f"unexpected send_fn call: {host}:{port}")
|
||||
|
||||
final_host, final_port, reply_bytes = discovery.follow_redirects_any(
|
||||
[("dead-host", 9901), ("standby", 9992)], send_fn
|
||||
)
|
||||
|
||||
assert (final_host, final_port) == ("primary", 9991)
|
||||
assert reply_bytes == reply_from_primary
|
||||
|
||||
|
||||
def test_follow_redirects_any_does_not_retry_a_reachable_candidates_decode_error():
|
||||
"""A candidate that IS reachable but sends back garbage is a real
|
||||
protocol failure, not a connectivity problem -- must propagate
|
||||
immediately, never fall through to the next candidate (a different
|
||||
node wouldn't fix a decode error from THIS one).
|
||||
"""
|
||||
calls = []
|
||||
|
||||
def send_fn(host, port):
|
||||
calls.append((host, port))
|
||||
return b"\x00" * 4 # too short to be a valid capnp frame
|
||||
|
||||
with pytest.raises(ReplyDecodeError):
|
||||
discovery.follow_redirects_any([("host1", 9991), ("host2", 9992)], send_fn)
|
||||
|
||||
# Only the first candidate was ever attempted.
|
||||
assert calls == [("host1", 9991)]
|
||||
|
|
|
|||
|
|
@ -36,7 +36,7 @@ import pytest
|
|||
from xcecommon import wire
|
||||
from xcecommon.errors import NoPrimaryError, ReplayFileError, ReplyDecodeError, SchedAdminError
|
||||
from xcecommon.schema import load_schema
|
||||
from xceadmin import scheduler
|
||||
from xceadmin import discovery, scheduler
|
||||
|
||||
|
||||
def _server_context(cert_file: Path, key_file: Path) -> ssl.SSLContext:
|
||||
|
|
@ -592,6 +592,76 @@ def test_record_status_retry_exhaustion_raises_distinguishable_error(
|
|||
assert f"{scheduler._MAX_TRANSIENT_ATTEMPTS} attempts" in exc_info.value.error
|
||||
|
||||
|
||||
def test_record_status_retries_on_no_primary_error_then_succeeds(monkeypatch, wrap_reply):
|
||||
"""Right after a primary is killed, every configured candidate can
|
||||
still redirect to the now-dead old primary (a standby hasn't
|
||||
necessarily noticed within the heartbeat timeout yet), so
|
||||
follow_redirects_any exhausts the whole candidate list and raises
|
||||
NoPrimaryError even though the cluster is only mid-election, not
|
||||
genuinely unreachable. This must be retried using NoPrimaryError's own
|
||||
(longer) budget/delay, _MAX_NO_PRIMARY_ATTEMPTS/
|
||||
_NO_PRIMARY_RETRY_DELAY_S -- not treated as an immediate hard failure,
|
||||
and not sharing _TRANSIENT_STATUSES' smaller budget. FAILS against the
|
||||
pre-fix code (which let NoPrimaryError propagate out of the retry loop
|
||||
on the very first attempt)."""
|
||||
mod = load_schema()
|
||||
sleep_calls: list[float] = []
|
||||
monkeypatch.setattr(scheduler.time, "sleep", lambda s: sleep_calls.append(s))
|
||||
|
||||
reply = mod.RequestReply.new_message()
|
||||
reply.status = 0
|
||||
admin_reply = reply.data.init("schedadminreplymsg")
|
||||
admin_reply.ok = True
|
||||
admin_reply.error = ""
|
||||
admin_reply.recording = True
|
||||
admin_reply.file = "settled.replay"
|
||||
admin_reply.eventCount = 0
|
||||
admin_reply.startedAt = 6000
|
||||
success_reply_bytes = wrap_reply(reply)
|
||||
|
||||
calls = {"n": 0}
|
||||
|
||||
def fake_follow_redirects_any(candidates, send_fn):
|
||||
calls["n"] += 1
|
||||
if calls["n"] < 3:
|
||||
raise NoPrimaryError("no primary available: none of the configured controllers responded", "host1", 9991)
|
||||
return "host2", 9992, success_reply_bytes
|
||||
|
||||
monkeypatch.setattr(scheduler, "follow_redirects_any", fake_follow_redirects_any)
|
||||
|
||||
result = scheduler.record_status("host1:9991,host2:9992,host3:9993")
|
||||
|
||||
assert result.file == "settled.replay"
|
||||
assert calls["n"] == 3
|
||||
assert sleep_calls == [scheduler._NO_PRIMARY_RETRY_DELAY_S] * 2
|
||||
|
||||
|
||||
def test_record_status_no_primary_error_retry_exhaustion_propagates(monkeypatch):
|
||||
"""If NoPrimaryError persists across the whole retry budget (the
|
||||
cluster genuinely never settles, or every candidate really is down),
|
||||
it must still propagate as NoPrimaryError -- not be silently
|
||||
swallowed or converted into a less-informative error -- after exactly
|
||||
_MAX_NO_PRIMARY_ATTEMPTS attempts (its own, longer budget -- NOT
|
||||
_MAX_TRANSIENT_ATTEMPTS), matching the reply-status exhaustion test's
|
||||
own proof shape."""
|
||||
sleep_calls: list[float] = []
|
||||
monkeypatch.setattr(scheduler.time, "sleep", lambda s: sleep_calls.append(s))
|
||||
|
||||
calls = {"n": 0}
|
||||
|
||||
def fake_follow_redirects_any(candidates, send_fn):
|
||||
calls["n"] += 1
|
||||
raise NoPrimaryError("no primary available: none of the configured controllers responded", "host1", 9991)
|
||||
|
||||
monkeypatch.setattr(scheduler, "follow_redirects_any", fake_follow_redirects_any)
|
||||
|
||||
with pytest.raises(NoPrimaryError):
|
||||
scheduler.record_status("host1:9991,host2:9992,host3:9993")
|
||||
|
||||
assert calls["n"] == scheduler._MAX_NO_PRIMARY_ATTEMPTS
|
||||
assert len(sleep_calls) == scheduler._MAX_NO_PRIMARY_ATTEMPTS - 1
|
||||
|
||||
|
||||
def test_record_status_standby_without_redirect_not_retried(mock_controller, wrap_reply, monkeypatch):
|
||||
"""A standby rejection with NO populated redirect (the genuine "op is
|
||||
primary-only, no known primary to redirect to" case) is a hard,
|
||||
|
|
@ -822,18 +892,21 @@ def test_record_stop_not_recording_never_retried(sequential_mock_controller, wra
|
|||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _split_controller(): same default-port convention as push.py
|
||||
# scheduler.py delegates --controller parsing to xceadmin.discovery.
|
||||
# resolve_controllers (shared with push.py) -- see test_discovery.py for
|
||||
# that function's own dedicated coverage; these two just confirm
|
||||
# _send_admin_request's pipeline still reaches it for the single-host case.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_record_default_port_used_when_omitted():
|
||||
host, port = scheduler._split_controller("somehost")
|
||||
(host, port), = discovery.resolve_controllers("somehost")
|
||||
assert host == "somehost"
|
||||
assert port == 9991
|
||||
|
||||
|
||||
def test_record_explicit_port_parsed():
|
||||
host, port = scheduler._split_controller("somehost:1234")
|
||||
(host, port), = discovery.resolve_controllers("somehost:1234")
|
||||
assert host == "somehost"
|
||||
assert port == 1234
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue