From 723fdb637ba35255210075133eda845148d2bb27 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 17 Sep 2026 03:53:35 +0000 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC --- .wolf/buglog.json | 57 ++++++ src/xceadmin/src/xceadmin/cli.py | 24 ++- src/xceadmin/src/xceadmin/discovery.py | 84 +++++++++ src/xceadmin/src/xceadmin/push.py | 31 ++-- src/xceadmin/src/xceadmin/scheduler.py | 126 ++++++++----- src/xceadmin/tests/test_config_push_mock.py | 10 +- src/xceadmin/tests/test_discovery.py | 190 ++++++++++++++++++++ src/xceadmin/tests/test_scheduler.py | 81 ++++++++- 8 files changed, 521 insertions(+), 82 deletions(-) diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 060b651e..9e3b0d8f 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -22317,6 +22317,63 @@ ], "occurrences": 1, "last_seen": "2026-09-17T02:45:00Z" + }, + { + "id": "bug-1821", + "timestamp": "2026-09-17T04:10:00Z", + "error_message": "User-reported: xceadmin's --controller option (config push and all 3 scheduler record start/stop/status commands) accepted only a SINGLE host:port -- passing a comma-separated list of all configured controllers (the natural way to query an HA cluster without knowing in advance which node is alive/primary, e.g. `--controller \"localhost:9901,localhost:9902,localhost:9903\"` with 9901 dead) failed outright: `Error: cannot connect to localhost:9901,localhost:9902,localhost:9903: [Errno -2] Name or service not known` -- the whole comma-joined string was parsed as one bogus hostname.", + "file": "src/xceadmin/src/xceadmin/discovery.py (new resolve_controllers/follow_redirects_any), src/xceadmin/src/xceadmin/push.py, src/xceadmin/src/xceadmin/scheduler.py (both migrated off their own duplicated _split_controller), src/xceadmin/src/xceadmin/cli.py (--controller help text)", + "root_cause": "push.py and scheduler.py each had their own private _split_controller(controller: str) -> (host, port) that did a single controller.rsplit(':', 1) -- designed only ever to see one host:port. A comma-separated value was never split on ',' at all; it fell through as one literal (unparseable) hostname straight into socket.create_connection via xcecommon.wire.connect, which propagates the resulting socket.gaierror (a plain OSError subclass) unwrapped by design.", + "fix": "FIXED (2026-09-17, TDD RED->GREEN). Added xceadmin.discovery.resolve_controllers(controller) -> list[(host,port)] (comma-split, whitespace-stripped, per-entry bare-host default port) and xceadmin.discovery.follow_redirects_any(candidates, send_fn) -> (host,port,reply_bytes), which tries each candidate in order via the existing follow_redirects(), catching per-candidate OSError (connection refused, DNS failure) and NoPrimaryError (discovery-level timeout) and moving to the next candidate -- but NOT catching ReplyDecodeError/TooManyRedirectsError, since a genuinely-reachable candidate's protocol-level failure isn't fixed by trying a different node. push.py/scheduler.py's own _split_controller removed; both now call resolve_controllers + follow_redirects_any. cli.py's --controller help text updated on all 4 commands to document the comma-list form. 12 new unit tests in test_discovery.py (single/list/whitespace parsing; first-reachable-wins; skip-refused/skip-DNS-failure/skip-timeout-then-succeed; all-fail raises NoPrimaryError naming every candidate; redirect-following composes correctly with failover; a reachable candidate's decode error does NOT fall through to the next). 2 existing tests per file (test_config_push_mock.py, test_scheduler.py) updated to call the new shared function instead of the removed private one. Full suite 304/304. Live-verified: deployed a 3-node cluster, killed the first-listed controller, confirmed `--controller \"dead,live1,live2\"` correctly skips the dead one and reaches a live one (both healthy-cluster and killed-primary scenarios).", + "tags": [ + "xceadmin", + "discovery.py", + "push.py", + "scheduler.py", + "cli.py", + "--controller", + "failover", + "master-rotation", + "user-reported", + "fixed", + "tdd", + "live-verified" + ], + "related_bugs": [ + "bug-1566", + "bug-1822" + ], + "occurrences": 1, + "last_seen": "2026-09-17T04:10:00Z" + }, + { + "id": "bug-1822", + "timestamp": "2026-09-17T04:10:00Z", + "error_message": "Found live while verifying bug-1821's --controller failover-list fix: querying `xceadmin scheduler record status --controller \"host1,host2,host3\"` immediately after killing the primary hard-failed with `Error: no primary available: none of the configured controllers responded (...)` instead of riding out the cluster's own re-election, even though _send_admin_request() already has a bounded retry loop (bug-1713/1715) built for exactly this 'cluster still settling' condition.", + "file": "src/xceadmin/src/xceadmin/scheduler.py (_send_admin_request)", + "root_cause": "Right after a primary is killed, a standby does not even START a new election until its own heartbeat_timeout_ms elapses (ha.h: HA_DEFAULT_HEARTBEAT_TIMEOUT_MS = 10000, i.e. 10s) -- until then it keeps redirecting every schedAdmin request straight back to the now-dead old primary. follow_redirects_any() (bug-1821) correctly detects that dead redirect target is unreachable and tries the next configured candidate, but if EVERY candidate is in this same stale-redirect state, the whole candidate list is exhausted and follow_redirects_any raises NoPrimaryError -- an exception _send_admin_request()'s existing retry loop never caught. That loop only retried on a REPLY that arrived and said not-ready (RequestReply.status in _TRANSIENT_STATUSES, or SchedAdminReply.ok=false with a known-transient reason) -- a total connection failure (no reply at all) propagated immediately with zero retries, even though it is the identical 'cluster still settling' condition the existing retry logic was built to ride out, just signaled differently.", + "fix": "FIXED (2026-09-17, TDD RED->GREEN). _send_admin_request()'s retry loop restructured from a single bounded for-loop sharing one attempt counter into a `while True` loop with TWO independent counters/budgets: the pre-existing status_attempt (_MAX_TRANSIENT_ATTEMPTS=4, _TRANSIENT_RETRY_DELAY_S=1.5s, unchanged, still shared by the reply.status and SchedAdminReply.ok=false branches exactly as before) and a new no_primary_attempt (_MAX_NO_PRIMARY_ATTEMPTS=8, _NO_PRIMARY_RETRY_DELAY_S=2.0s -- deliberately larger, since this failure mode's worst case is the ~10s heartbeat-timeout detection window plus real election time, categorically longer than the RPC-level 'already electing, replies not-ready' scenario the original budget was tuned for) that catches NoPrimaryError from follow_redirects_any and retries the whole candidate list fresh. 2 new unit tests (monkeypatching follow_redirects_any directly: retries-then-succeeds, and exhaustion-still-propagates-NoPrimaryError after exactly _MAX_NO_PRIMARY_ATTEMPTS attempts). Full suite 304/304. Live-verified: killed a settled cluster's primary, immediately queried the failover-list, confirmed (in one repro) the retry loop visibly cycled the candidate list 5+ times over ~15s while standbys still redirected to the dead primary, then successfully reached the genuinely newly-elected primary once the standbys' own election completed -- the query result correctly transitioned from NoPrimaryError to the new primary's real (if scheduler-daemon-absent-in-this-test-env) reply, proving the retry rode out the real HA re-election, not just a mocked delay.", + "tags": [ + "xceadmin", + "scheduler.py", + "_send_admin_request", + "NoPrimaryError", + "retry", + "heartbeat_timeout_ms", + "election", + "live-verified", + "fixed", + "tdd", + "bug-1713", + "bug-1715" + ], + "related_bugs": [ + "bug-1713", + "bug-1715", + "bug-1821" + ], + "occurrences": 1, + "last_seen": "2026-09-17T04:10:00Z" } ] } diff --git a/src/xceadmin/src/xceadmin/cli.py b/src/xceadmin/src/xceadmin/cli.py index 5a9c3997..45962c2d 100644 --- a/src/xceadmin/src/xceadmin/cli.py +++ b/src/xceadmin/src/xceadmin/cli.py @@ -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.""" diff --git a/src/xceadmin/src/xceadmin/discovery.py b/src/xceadmin/src/xceadmin/discovery.py index 144b6ccf..efa7d99e 100644 --- a/src/xceadmin/src/xceadmin/discovery.py +++ b/src/xceadmin/src/xceadmin/discovery.py @@ -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): diff --git a/src/xceadmin/src/xceadmin/push.py b/src/xceadmin/src/xceadmin/push.py index 3a95d78f..94f1e016 100644 --- a/src/xceadmin/src/xceadmin/push.py +++ b/src/xceadmin/src/xceadmin/push.py @@ -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: diff --git a/src/xceadmin/src/xceadmin/scheduler.py b/src/xceadmin/src/xceadmin/scheduler.py index 279a47d3..c413a910 100644 --- a/src/xceadmin/src/xceadmin/scheduler.py +++ b/src/xceadmin/src/xceadmin/scheduler.py @@ -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 diff --git a/src/xceadmin/tests/test_config_push_mock.py b/src/xceadmin/tests/test_config_push_mock.py index 66874937..aa46cacf 100644 --- a/src/xceadmin/tests/test_config_push_mock.py +++ b/src/xceadmin/tests/test_config_push_mock.py @@ -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 diff --git a/src/xceadmin/tests/test_discovery.py b/src/xceadmin/tests/test_discovery.py index aa189f4c..9d2660dc 100644 --- a/src/xceadmin/tests/test_discovery.py +++ b/src/xceadmin/tests/test_discovery.py @@ -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)] diff --git a/src/xceadmin/tests/test_scheduler.py b/src/xceadmin/tests/test_scheduler.py index 5bfc9976..5e193700 100644 --- a/src/xceadmin/tests/test_scheduler.py +++ b/src/xceadmin/tests/test_scheduler.py @@ -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