test(admin-config-tool): update standby-redirect E2E for bug-1700/1701
test_live_config_push_against_standby_returns_standby_rejection_not_redirect locked in the OLD controller behavior (a standby flatly rejects OP_upgradeConfig with no redirect fields). That gap is now closed: myreq_handler's gate-rejection path calls fill_redirect_reply() on every -XCE_ERR_STANDBY rejection (bug-1700), and handle_ctrlhello() now actually populates primary_host/primary_port from the static --peers list (bug-1701) -- a standby pushing config now redirects to the primary and the push succeeds. Renamed to test_live_config_push_against_standby_redirects_and_succeeds and rewritten to assert the new behavior. _assert_primary_committed() changed from a single-shot log read to a poll loop (up to 5s) for the "config activated from committed marker..." line -- accounts for the documented async gap between the majority-ack-gated client reply and the primary's own local WAL-apply- pump activation, a real race the old single-shot read could hit. meson-build.sh: wire src/tools and each build profile's controller subdir into the isolated repo-mirror the E2E tests run against, and export XCEADMIN_BIN explicitly, so these live tests actually find real binaries instead of always skipping. Verified: xceadmin suite 288/288 (incl. both tests in this file); controller suite 1220/1222 (only the pre-existing bug-1561 flake). Updated admin-config-tool design.md and roadmap.md to mark the dispatch-gate and redirect-wiring gaps RESOLVED, matching this test's new assertions and the live evidence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA
This commit is contained in:
parent
14f95b06fe
commit
6193fa7ea1
4 changed files with 168 additions and 100 deletions
|
|
@ -30,8 +30,8 @@
|
|||
- Test fixtures and golden files under `src/xceadmin/tests/`.
|
||||
|
||||
### Out of Boundary
|
||||
- **Controller-side `OP_upgradeConfig` dispatch** — `ha_dispatch_gate()` excludes `OPCode_upgradeConfig` from the standby allow-list (`ha.c:3926-3951`), and `cluster.c:667-679` calls `handle_upgrade_config()` only when `ha_is_primary()` is **false**. Combined, no controller role reaches `do_upgrade_config_with_bytes()` via the network today — it is exercised only by unit tests calling it directly. This is a controller defect, not an xceadmin defect; xceadmin implements the client side of the documented contract (`config_repl.h:347-349` — "handle an OP_upgradeConfig request from an admin tool") and cannot itself fix the gate. Flagged as a blocking risk below.
|
||||
- **Redirect wiring for `OP_upgradeConfig`** — `fill_redirect_reply()` (`join.c:27-31`) is only invoked from the `OPCode_join` path (`join.c:277`). `RequestReply.redirectHost`/`redirectPort` are never populated for an `upgradeConfig` reply today. xceadmin implements generic, opcode-agnostic redirect-following per the `RequestReply` contract (forward-compatible), but it will not fire for `config push` until the controller is extended.
|
||||
- ~~**Controller-side `OP_upgradeConfig` dispatch**~~ — **RESOLVED 2026-07-25 (bug-1468).** `cluster.c`'s `myreq_handler` `OPCode_upgradeConfig` case had its primary/standby guard polarity inverted by an unrelated commit (`fd93997f`, 2026-05-31: `if (ha_is_primary(...)) reject` instead of the correct `if (!ha_is_primary(...)) reject`), which rejected the one role — primary — that `ha_dispatch_gate()` ever lets reach this switch case. Restored to the correct polarity; the primary now reaches `do_upgrade_config_with_bytes()` via the network as designed. Live-verified: `test_e2e_live_push.py::test_live_config_push_against_real_single_node_controller`, `test_e2e_live_ha_push.py::test_live_config_push_against_primary_reaches_success_path`.
|
||||
- ~~**Redirect wiring for `OP_upgradeConfig`**~~ — **RESOLVED 2026-08-26 (bug-1700/1701).** `cluster.c`'s generic `ha_dispatch_gate()` rejection path now calls `fill_redirect_reply()` whenever the gate rejects with `-XCE_ERR_STANDBY` (`cluster.c:411-437`) — a fix at the gate itself, covering every opcode that hits that rejection, not an `OPCode_upgradeConfig`-specific patch. `RequestReply.redirectHost`/`redirectPort` are now populated when a standby rejects `config push`, and xceadmin's already-generic redirect-following (Allowed Dependencies) transparently retries against the primary. Live-verified: `test_e2e_live_ha_push.py::test_live_config_push_against_standby_redirects_and_succeeds`.
|
||||
- **`ProbeConfigTag` server-side handling** — `xce_probe_config_tag()` (`config_repl.c:1829`) has no server-side dispatch case anywhere in `cluster.c` and no production caller; it is dormant plumbing. xceadmin's dry-run client implements the wire contract but has nothing live to call.
|
||||
- Entity-bearing snapshots, config rollback, version history, config activation status (owned by `config-upgrade-snapshot`, blocked upstream on `entity-bearing-snapshots`).
|
||||
- TLS mutual authentication / hostname verification — none exists anywhere in the codebase (`SSL_VERIFY_NONE` is set explicitly for the controller's own outbound client context, `myapp.c:109`; no `X509_check_host` call exists). xceadmin matches this existing posture; it neither introduces nor removes an authentication guarantee.
|
||||
|
|
@ -45,8 +45,8 @@
|
|||
|
||||
### Revalidation Triggers
|
||||
- `controller.capnp` `ClusterConfig` field renumbering/addition/removal (already happened once this session via `clusterconfig-schema-split`) — invalidates the field-mapping table in Data Models.
|
||||
- A fix to the `cluster.c`/`ha.c` dispatch-gate contradiction — flips `config push` E2E testing from "mock controller only" to "real cluster required," per Testing Strategy.
|
||||
- `fill_redirect_reply()` extended to `OPCode_upgradeConfig` — unlocks live auto-primary-discovery testing for config push (currently only testable against `OPCode_join`-shaped mocks).
|
||||
- ~~A fix to the `cluster.c`/`ha.c` dispatch-gate contradiction~~ — **TRIGGERED 2026-07-25 (bug-1468).** `config push` E2E now runs against a real cluster (`test_e2e_live_push.py`, `test_e2e_live_ha_push.py`), superseding the "mock controller only" framing in Testing Strategy.
|
||||
- ~~`fill_redirect_reply()` extended to `OPCode_upgradeConfig`~~ — **TRIGGERED 2026-08-26 (bug-1700/1701).** Closed via a broader, gate-level fix (every opcode hitting the `-XCE_ERR_STANDBY` rejection gets a redirect, not an `OPCode_upgradeConfig`-specific patch) — see Boundary Commitments. Live auto-primary-discovery testing for config push now exists: `test_e2e_live_ha_push.py::test_live_config_push_against_standby_redirects_and_succeeds`.
|
||||
- A server-side handler added for `OPCode_probeConfigTag` — unlocks `config push --dry-run` E2E testing.
|
||||
|
||||
## Architecture
|
||||
|
|
@ -187,7 +187,7 @@ sequenceDiagram
|
|||
X->>Op: versionTag ackedCount error, exit code
|
||||
```
|
||||
|
||||
At most 3 redirect hops (3.2); each hop is a fresh TLS connection with the same 30s timeout (3.4, 7.6). Because `fill_redirect_reply()` does not currently fire for `OPCode_upgradeConfig` (see Boundary Commitments), the `alt` branch above is exercised only in mock-controller tests until the controller gap is closed.
|
||||
At most 3 redirect hops (3.2); each hop is a fresh TLS connection with the same 30s timeout (3.4, 7.6). `fill_redirect_reply()` now fires for a standby's `OP_upgradeConfig` rejection too (bug-1700/1701, see Boundary Commitments) — the `alt` branch above is live-verified against a real 3-node cluster (`test_e2e_live_ha_push.py::test_live_config_push_against_standby_redirects_and_succeeds`), not just mock-controller tests.
|
||||
|
||||
## Requirements Traceability
|
||||
|
||||
|
|
@ -457,7 +457,7 @@ def probe_config(cluster_bin_path: str, controller: str, cert_dir: str) -> DryRu
|
|||
- Inspects any `Reply` for non-empty `redirectHost`/`redirectPort`; reconnects and resends up to 3 hops (3.1, 3.2), then errors.
|
||||
- Displays the redirect target when a standby redirects (3.3).
|
||||
- 30s "no primary elected" timeout is a property of each connection attempt, not the overall discovery loop (3.4, delegated to `xcecommon.wire.connect`).
|
||||
- Implemented generically against the `RequestReply` schema shape (not `upgradeConfig`-specific) so it is correct today for `OPCode_join`-style redirects and automatically correct for `upgradeConfig` once the controller wires `fill_redirect_reply()` for that opcode too (Simplification — no opcode-specific branching to remove later).
|
||||
- Implemented generically against the `RequestReply` schema shape (not `upgradeConfig`-specific) so it was correct for `OPCode_join`-style redirects from the start and required zero client-side changes once the controller closed the `upgradeConfig` gap (bug-1700/1701, 2026-08-26) — validated in practice, not just in principle (Simplification — no opcode-specific branching was ever needed).
|
||||
|
||||
**Dependencies**
|
||||
- Outbound: `xcecommon.wire` (P0).
|
||||
|
|
@ -532,8 +532,8 @@ Out of scope — `xceadmin` is a one-shot CLI with stderr/exit-code as its only
|
|||
- **Unit — wire framing**: `send_frame`/`recv_frame` round-trip against a fixed byte sequence, and against a loopback `socket.socketpair()` — verifies native-order packing matches `struct.pack("=Q", ...)` semantics without needing a real controller.
|
||||
- **Integration — config build (golden-file)**: `minimal`/`full`/`invalid` fixture directories (File Structure Plan) — `full` now covers all 7 YAML files including the newly-added `xce.yaml`/`scaler.yaml`; byte-compares `build_cluster_config()` output against a committed `expected.bin` and its SHA-256 (1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 1.10, 1.11, 1.12, 9.1, 9.2, 9.3). Plain `assert produced == expected_bytes` fixture comparison — `pytest-regressions` was evaluated (see `research.md`) but not adopted, since this project's nix-pinned dependency list would need independent verification and the byte-equality need here is simple enough not to warrant it (Simplification).
|
||||
- **Integration — validation**: one test per `invalid/` fixture case — schema violation, reserved name, duplicate name (including a limit/queue name collision, proving 4.3's intentionally-narrow scope), undefined reference (warn, not fail).
|
||||
- **Integration — config push (mock controller)**: a Python TLS server speaking just enough of the wire protocol (frame in, decode `Request` or direct-struct `ProbeConfigTagRequest`, encode a canned `Reply`/`ProbeConfigTagReply`) to exercise `push_config()` (2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 3.1, 3.2, 3.3 redirect, 7.1, 7.2, 7.3) and `probe_config()` (6.1, 6.2, 6.3 — accepted and rejected probe replies) against the same mock, since both share `xcecommon.wire`'s framing/send-receive primitives. **This is the only realistic way to test config push today** — not merely "preferred for the dev loop" as `brief.md` originally framed it, but load-bearing: the controller-side dispatch-gate defect found during discovery (Boundary Commitments) means a real 3-node cluster currently rejects `OP_upgradeConfig` from every role, so live-cluster E2E (`brief.md`'s "Option B") is **blocked**, not merely deferred, until that controller defect is fixed (Revalidation Trigger).
|
||||
- **E2E (deferred)**: real-cluster `config push` round-trip via `xce-test-ha` — tracked in `brief.md`, explicitly gated on both the controller dispatch fix and `config-upgrade-snapshot` landing.
|
||||
- **Integration — config push (mock controller)**: a Python TLS server speaking just enough of the wire protocol (frame in, decode `Request` or direct-struct `ProbeConfigTagRequest`, encode a canned `Reply`/`ProbeConfigTagReply`) to exercise `push_config()` (2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 3.1, 3.2, 3.3 redirect, 7.1, 7.2, 7.3) and `probe_config()` (6.1, 6.2, 6.3 — accepted and rejected probe replies) against the same mock, since both share `xcecommon.wire`'s framing/send-receive primitives. This remains valuable for dev-loop speed and `probe_config()` coverage (`ProbeConfigTag` still has no server-side handler — Out of Boundary), but it is no longer the only way to test `push_config()`: the controller-side dispatch-gate defect that once blocked live-cluster testing was fixed (bug-1468, 2026-07-25), and the standby-redirect gap that followed it was fixed too (bug-1700/1701, 2026-08-26) — see Boundary Commitments and E2E below.
|
||||
- **E2E**: real-cluster `config push` round-trip, live against genuine `xce_controller` processes — `test_e2e_live_push.py` (single-node, task 6.2) and `test_e2e_live_ha_push.py` (3-node HA, task 6.3): primary success path (positive `ackedCount`, real standby ACKs), and the standby-redirect success path (`test_live_config_push_against_standby_redirects_and_succeeds`). No longer gated on the controller dispatch fix or the redirect fix — both landed; `brief.md`'s "Option B" is realized.
|
||||
|
||||
## Security Considerations
|
||||
|
||||
|
|
|
|||
|
|
@ -87,7 +87,7 @@ Smaller items that likely don't warrant a full spec; pick up opportunistically o
|
|||
- [ ] validate_host() real authorization — currently a stub returning success for all peers (`join.c:13-16`, `return 1;` unconditionally). **`CONF_ENABLE_UPGRADE_CONFIG` is already `1` in the standard build** (`config_repl.h:27-28`, no build-system override found) — the precondition this item was originally framed to guard against has already been crossed; `OP_upgradeConfig` (and `OPCode_schedAdmin`, added 2026-08-02, `scheduler-core`) are live and unauthenticated TODAY, not a future risk to prevent. Minimum fix: verify peer X.509 CN against a configured allowlist (`--admin-cn` flag or `admin_cn_list` config), reject non-allowlisted `OP_upgradeConfig` and `OPCode_schedAdmin` with clear error, WARN-log rejected attempts with CN+IP. Future: IP allowlist, role-based access, admin-specific CA. _(FUTURE_WORK.md §6; security-critical.)_ **Part of the broader `peer-authentication-hardening` item above.**
|
||||
- [ ] OP_replConfig access control hardening — currently serves the full config blob to any CA-signed SSL peer (trusted-LAN model). Consider: verify requester is a known `--peers` controller, rate-limit per peer, `minimum_role` check (controllers only). _(FUTURE_WORK.md §7.)_ **Part of the broader `peer-authentication-hardening` item above.**
|
||||
- [ ] lost-and-found drain command — `xcecli lost-and-found drain`: reassign or cancel sidelined jobs, reset `lost_and_found_job_count`. Automates operator cleanup after config upgrade. _(FUTURE_WORK.md §4.)_
|
||||
- [x] ~~OP_upgradeConfig standby redirect verification~~ — **RESOLVED 2026-08-19 (finding: item's own premise doesn't match production behavior, but coverage is already complete).** A standby does NOT return the primary's address for `OPCode_upgradeConfig` — `ha_dispatch_gate` rejects it before the handler's own dead defensive check ever runs, replying flat `status=-XCE_ERR_STANDBY, data_which=nodata` with no redirect fields; `fill_redirect_reply()` (`join.c:27`) is never called for this opcode. This is a documented, deliberate feature gap (see `admin-config-tool` design.md's "Revalidation Triggers"), not a missing test. Coverage is already complete from both directions: the client-side redirect-following mechanism is unit-tested against mocks (`src/xceadmin/tests/test_discovery.py`, `test_config_push_mock.py::test_push_config_follows_one_redirect_to_primary`), and the real server-side current behavior (no redirect, flat rejection) is live-verified against a real 3-node cluster and regression-locked (`src/xceadmin/tests/test_e2e_live_ha_push.py::test_live_config_push_against_standby_returns_standby_rejection_not_redirect`). The remaining real work — wiring `fill_redirect_reply()` for `OPCode_upgradeConfig` — is a feature task already tracked under `admin-config-tool`'s Revalidation Triggers, not this item.
|
||||
- [x] ~~OP_upgradeConfig standby redirect verification~~ — **RESOLVED 2026-08-19, then the underlying gap itself CLOSED 2026-08-26 (bug-1700/1701).** As of 2026-08-19, a standby did NOT return the primary's address for `OPCode_upgradeConfig` — `ha_dispatch_gate` rejected it before the handler's own dead defensive check ever ran, replying flat `status=-XCE_ERR_STANDBY, data_which=nodata` with no redirect fields; `fill_redirect_reply()` (`join.c:27`) was never called for this opcode, a documented, deliberate feature gap (`admin-config-tool` design.md's "Revalidation Triggers"). That gap is now closed: `cluster.c`'s generic dispatch-gate rejection path calls `fill_redirect_reply()` on every `-XCE_ERR_STANDBY` rejection (bug-1700/1701, gate-level fix, not opcode-specific), and a standby pushing `config push` now redirects to the primary and succeeds. Coverage: client-side redirect-following remains unit-tested against mocks (`test_discovery.py`, `test_config_push_mock.py::test_push_config_follows_one_redirect_to_primary`); the real server-side behavior is live-verified against a real 3-node cluster (`src/xceadmin/tests/test_e2e_live_ha_push.py::test_live_config_push_against_standby_redirects_and_succeeds`, renamed from `..._returns_standby_rejection_not_redirect` to match the now-correct outcome).
|
||||
- [x] steering sync — `ha-async-io.md` synchronized to the three-state model + own-term gate (transitional-state row/role removed; heartbeat re-arm PRIMARY-only; PRIMARY no-quorum fact allowlist noted). Done as part of ha-single-leader-role task 6.1.
|
||||
- [ ] xce_test WAL/snapshot inspection tooling — extend the offline inspector family in `xce_test` (sibling of the shipped `waldump`, `src/controller/testsuite/waldump.{c,h}` + `cmd_waldump`). Two remaining commands: **(a) `snapdump <snap_dir|snap_file>`** — decode the `XCESNAP\0` header (8-byte LSN; `persist.c:1266` `load_snapshot`) and report whether the snapshot carries job/queue entities or is an LSN-only stub. This directly settles Open Fork #3 of the failover job-loss investigation (`.kiro/specs/ha-commit-durability/INVESTIGATION-failover-job-loss.md`): if snapshots are entity-less, any empty-rejoin/WAL-prune below committed is unrecoverable BY DESIGN. **(b) `walcompare <dirA> <dirB>`** — diff two nodes' WALs by `lsn → crc/jobid` to flag divergence/prefix-gaps between members instantly. Reuse the `waldump.c` reader/decoder; same zero-alloc capnp jobid decode + `c_capnproto_dep`/`zlib_dep` wiring already added to `xce_test_deps`. Nonzero exit on entity-less snapshot / divergence so both double as E2E assertions. _(recorded 2026-06-20; `waldump` already done.)_
|
||||
- [x] ~~xce_test shell command support~~ — **IMPLEMENTED** (task 2.1, spec `xce-test-e2e-primitives`, closed 2026-06-19; stale entry corrected 2026-08-19). `preprocess_and_run()` (`src/controller/testsuite/test.c:950`) detects a leading `!` and runs the remainder via `system()`; the shell command's exit status becomes `g_last_status` (the `$?` auto-variable) and a non-zero status sets `g_session_failed`, aborting the script the same way any other failing command does — `system()`'s child inherits the parent's stdout/stderr fds, so output lands in whatever the invoking `xce_test` session's own output is already redirected to. Works in every REPL context (no `leave_ctx()`/disconnect), including mid-session inside `worker`/`client` contexts — used this session to fix bug-1579 (resource-value-verification's nested controller-check pattern). No parser/harness-layer change remains to do.
|
||||
|
|
|
|||
|
|
@ -63,6 +63,29 @@ mkdir -p "$MIRROR_DIR/src"
|
|||
ln -s "$REPO_ROOT/ha" "$MIRROR_DIR/ha"
|
||||
ln -s "$SRC_DIR/configs" "$MIRROR_DIR/src/configs"
|
||||
ln -s "$SRC_DIR/schema" "$MIRROR_DIR/src/schema"
|
||||
# tests/_live_e2e_common.py's DEPLOY_HA_SH points at a sibling tools/
|
||||
# dir -- same fixed-relative-path assumption as configs/schema above.
|
||||
ln -s "$SRC_DIR/tools" "$MIRROR_DIR/src/tools"
|
||||
|
||||
# tests/_live_e2e_common.py's CONTROLLER_CANDIDATES look for a real
|
||||
# xce_controller binary under a sibling xce.build/build.release/build.dbg
|
||||
# dir (xce-build.sh's own profile-dir naming, src/tools/xce-build.sh) next
|
||||
# to this file's own SRC_DIR -- same fixed-relative-path assumption as
|
||||
# ha/configs/schema above, so it needs the same out-of-tree symlink
|
||||
# treatment or the live E2E tests can never find a controller binary that
|
||||
# was actually built, no matter what's really on disk. Only the `controller`
|
||||
# subdir is linked (not the whole profile-root xce.build/etc.) because
|
||||
# `controller` and `xceadmin` are always siblings under a profile root --
|
||||
# unlike the profile root itself, this is never an ancestor of $MIRROR_DIR
|
||||
# (which nests only under <profile>/xceadmin), so it can't form a
|
||||
# self-referential symlink loop the way linking the whole root would.
|
||||
for _live_e2e_build_dir in xce.build build.release build.dbg; do
|
||||
_live_e2e_controller_dir="$SRC_DIR/$_live_e2e_build_dir/controller"
|
||||
if [ -d "$_live_e2e_controller_dir" ]; then
|
||||
mkdir -p "$MIRROR_DIR/src/$_live_e2e_build_dir"
|
||||
ln -s "$_live_e2e_controller_dir" "$MIRROR_DIR/src/$_live_e2e_build_dir/controller"
|
||||
fi
|
||||
done
|
||||
|
||||
cp -r "$SOURCE_DIR" "$WORK_DIR"
|
||||
# Never trust a stray dev-loop artifact from the live checkout (xce-build.sh's
|
||||
|
|
@ -115,7 +138,18 @@ rm -rf "$VENV_DIR"
|
|||
"$VENV_DIR/bin/pip" install "$WORK_DIR" --quiet
|
||||
|
||||
echo "=== xceadmin (meson): running tests ==="
|
||||
"$VENV_DIR/bin/python" -m pytest "$WORK_DIR/tests" -v
|
||||
# tests/_live_e2e_common.py's XCEADMIN_CANDIDATES look for a sibling
|
||||
# xce.build/build.release/build.dbg/xceadmin/.venv/bin/xceadmin next to this
|
||||
# file's own SRC_DIR -- but for THIS profile that path is $BUILD_DIR itself
|
||||
# (the xceadmin subdir currently being built), which is an ancestor of
|
||||
# $MIRROR_DIR and so can't be symlinked into the mirror without a
|
||||
# self-referential loop (unlike the sibling `controller` dir linked above).
|
||||
# Route around it the same way a caller with a custom/explicit build dir
|
||||
# already would: XCE_CONTROLLER_BIN/XCEADMIN_BIN
|
||||
# (resolve_binary_candidates in _live_e2e_common.py) override discovery
|
||||
# entirely, and this build's own freshly pip-installed binary is exactly
|
||||
# what the live E2E tests should exercise.
|
||||
XCEADMIN_BIN="$VENV_DIR/bin/xceadmin" "$VENV_DIR/bin/python" -m pytest "$WORK_DIR/tests" -v
|
||||
|
||||
# Build an installable wheel from the SAME schema-synced $WORK_DIR the pinned
|
||||
# install above just tested -- not a second, separately-assembled tree --
|
||||
|
|
|
|||
|
|
@ -12,48 +12,59 @@ pattern -- see ``live_ha_cluster`` below):
|
|||
``test_live_config_push_against_primary_reaches_success_path`` genuinely
|
||||
exits 0 with a positive ``ackedCount`` from real standby ACKs.
|
||||
|
||||
REAL FINDING, not a test bug -- the redirect scenario does NOT close the
|
||||
way the task text originally anticipated: pushing `config push` to a
|
||||
*standby* does NOT redirect to the primary against a real controller
|
||||
today, and this is not something a test-only change can fix. This is an
|
||||
ALREADY-DOCUMENTED, pre-existing gap in design.md's own "Boundary
|
||||
Commitments" section (`.kiro/specs/admin-config-tool/design.md`, "Out of
|
||||
Boundary" -- "Redirect wiring for `OP_upgradeConfig`") and "Revalidation
|
||||
Triggers" (`fill_redirect_reply()` extended to `OPCode_upgradeConfig` is
|
||||
listed there as a FUTURE, not-yet-done trigger): `fill_redirect_reply()`
|
||||
(`join.c:27-31`) is only ever invoked from the `OPCode_join` path
|
||||
(`join.c:277`); nothing calls it for `OPCode_upgradeConfig`.
|
||||
`RequestReply.redirectHost`/`redirectPort` are therefore never populated
|
||||
UPDATE (2026-08-28): the redirect gap described below WAS closed, exactly
|
||||
as this docstring's own original prediction anticipated -- see
|
||||
``test_live_config_push_against_standby_redirects_and_succeeds`` (renamed
|
||||
from ``..._returns_standby_rejection_not_redirect``), which now live-
|
||||
verifies the success-via-redirect path instead. The investigation history
|
||||
below is kept for context (why the task's original text and the
|
||||
mid-investigation finding diverged, and exactly what changed to close the
|
||||
gap); treat every "does NOT redirect" / "no redirect line, no success"
|
||||
statement below as describing REAL, live-verified controller behavior AS
|
||||
OF 2026-08-26 (the day before bug-1700/1701 landed), not current behavior.
|
||||
|
||||
ORIGINAL FINDING (now superseded) -- the redirect scenario did NOT close
|
||||
the way the task text originally anticipated: pushing `config push` to a
|
||||
*standby* did NOT redirect to the primary against a real controller as of
|
||||
this file's original writing, and this was not something a test-only
|
||||
change could fix. This was an ALREADY-DOCUMENTED, pre-existing gap in
|
||||
design.md's own "Boundary Commitments" section
|
||||
(`.kiro/specs/admin-config-tool/design.md`, "Out of Boundary" -- "Redirect
|
||||
wiring for `OP_upgradeConfig`") and "Revalidation Triggers"
|
||||
(`fill_redirect_reply()` extended to `OPCode_upgradeConfig` was listed
|
||||
there as a FUTURE, not-yet-done trigger): `fill_redirect_reply()`
|
||||
(`join.c:27-31`) was only ever invoked from the `OPCode_join` path
|
||||
(`join.c:277`); nothing called it for `OPCode_upgradeConfig`.
|
||||
`RequestReply.redirectHost`/`redirectPort` were therefore never populated
|
||||
for an `upgradeConfig` reply. Confirmed directly in `cluster.c`'s
|
||||
`myreq_handler`: a standby's `OP_upgradeConfig` request is rejected by the
|
||||
`myreq_handler`: a standby's `OP_upgradeConfig` request was rejected by the
|
||||
generic `ha_dispatch_gate()` role check (`ha.c`, opcode absent from the
|
||||
`CTRL_STANDBY` allow-list) BEFORE the opcode-specific `case
|
||||
OPCode_upgradeConfig:` handler ever runs -- the gate-rejection reply path
|
||||
(`cluster.c` ~line 273-280) sends `reply.status = -XCE_ERR_STANDBY`
|
||||
OPCode_upgradeConfig:` handler ever ran -- the gate-rejection reply path
|
||||
sent `reply.status = -XCE_ERR_STANDBY`
|
||||
(`errnox.h`: `E_XCE_STANDBY == XCE_ERRNO_BASE + 13 == 10014`) with
|
||||
`data_which = RequestReply_data_nodata` and never touches
|
||||
`data_which = RequestReply_data_nodata` and never touched
|
||||
`redirectHost`/`redirectPort` at all. Live-verified against a real 3-node
|
||||
cluster: pushing to a standby returns `versionTag=0 ackedCount=0`, exit 1,
|
||||
stderr `"Error: controller returned status -10014"` -- no redirect line,
|
||||
no success. `xceadmin.discovery.follow_redirects`/`decode_reply` behave
|
||||
exactly per Requirement 3.1's contract given this reply (both fields
|
||||
absent/empty => no redirect => the bare rejection is returned as-is); the
|
||||
gap is entirely server-side and out of this task's test-only boundary to
|
||||
fix (design.md: "This is a controller defect, not an xceadmin defect;
|
||||
xceadmin implements the client side of the documented contract... and
|
||||
cannot itself fix the gate"). Task 5.6 already covers the SUCCESSFUL
|
||||
redirect-follow path against mocks (`test_config_push_mock.py`), which is
|
||||
still the only place that path is exercised.
|
||||
``test_live_config_push_against_standby_returns_standby_rejection_not_redirect``
|
||||
below live-verifies and regression-locks this REAL current behavior
|
||||
instead of the hypothetical "redirect then succeed" outcome -- the same
|
||||
resolution task 6.2 used for its own real-vs-literal-task-text mismatch
|
||||
(single-node `ackedCount == 0`/exit 1). The moment a future controller-side
|
||||
task wires `fill_redirect_reply()` for `OPCode_upgradeConfig` (already
|
||||
tracked as a Revalidation Trigger in design.md), THIS test will start
|
||||
failing on the "no redirect line" assertion -- that failure is the signal
|
||||
to replace it with a real success-path assertion, matching what this
|
||||
docstring originally described before investigation.
|
||||
cluster at the time: pushing to a standby returned `versionTag=0
|
||||
ackedCount=0`, exit 1, stderr `"Error: controller returned status
|
||||
-10014"` -- no redirect line, no success.
|
||||
|
||||
WHAT CLOSED IT (bug-1700/1701, fixed 2026-08-26): `cluster.c`'s generic
|
||||
`myreq_handler` dispatch-gate rejection path (~`cluster.c:411-437`) now
|
||||
calls `fill_redirect_reply(&reply, &c->ha)` whenever `ha_dispatch_gate()`
|
||||
rejects with `-XCE_ERR_STANDBY` -- a fix at the GATE itself, covering
|
||||
every opcode that hits that rejection, not an `OPCode_upgradeConfig`-
|
||||
specific patch (broader than what the original Revalidation Trigger
|
||||
anticipated). A second, independently-discovered blocker (bug-1701:
|
||||
`ha_state_t.primary_host`/`primary_port` were never actually written by
|
||||
any production code path on a standby, so `fill_redirect_reply()` had
|
||||
nothing to fill) was closed the same day via a local lookup in
|
||||
`handle_ctrlhello()` against the statically-known `--peers` list. Together
|
||||
these make a standby's `OP_upgradeConfig` rejection carry a real, populated
|
||||
redirect hint, and `xceadmin.discovery.follow_redirects()` -- already
|
||||
generic and already tested against mocks (`test_config_push_mock.py::
|
||||
test_push_config_follows_one_redirect_to_primary`) -- transparently
|
||||
retries against the primary with zero client-side changes required.
|
||||
|
||||
Shares binary discovery / port / stdout-parsing helpers with
|
||||
``test_e2e_live_push.py`` via ``_live_e2e_common`` (extracted there as part
|
||||
|
|
@ -68,10 +79,16 @@ that merging them would cost more readability than it would save.
|
|||
Skip convention: identical to task 6.2 -- an imperative ``pytest.skip``
|
||||
inside the fixture (no controller binary / no ``openssl``), plus an
|
||||
in-test skip for a missing ``xceadmin`` binary, both matching this suite's
|
||||
existing cert-dependent skip precedent (task 5.3). Skipping is the NORMAL
|
||||
outcome under the fast ``xce-build.sh xceadmin`` / meson path, which runs
|
||||
pytest from an isolated ``repo-mirror`` with no sibling
|
||||
``xce.build``/``build.dbg``/``build.release`` controller binaries.
|
||||
existing cert-dependent skip precedent (task 5.3). UPDATE (2026-08-28):
|
||||
skipping is no longer the normal outcome under the ``xce-build.sh
|
||||
xceadmin`` / meson path -- ``meson-build.sh`` now symlinks each profile's
|
||||
``controller`` subdirectory (and ``src/tools/``, for ``deploy-ha.sh``) into
|
||||
the isolated ``repo-mirror`` it runs pytest from, and sets ``XCEADMIN_BIN``
|
||||
to that build's own freshly-installed binary, so these tests find and
|
||||
exercise a real controller/xceadmin whenever one exists anywhere in the
|
||||
repo's ``xce.build``/``build.dbg``/``build.release`` dirs. Skipping now
|
||||
correctly means what it always should have: no such binary was ever built
|
||||
in this checkout.
|
||||
|
||||
Process-leak fix (task 6.2's own finding, reused here for all 3 nodes):
|
||||
each controller is launched as the real ``xce_controller`` binary
|
||||
|
|
@ -413,11 +430,36 @@ def _assert_no_decode_crash(push_result: subprocess.CompletedProcess) -> None:
|
|||
assert "Traceback" not in push_result.stderr, push_result.stderr
|
||||
|
||||
|
||||
def _assert_primary_committed(log_text: str, version_tag_hex: str) -> None:
|
||||
# config_activate_committed() (config_repl.c) runs off the WAL apply-pump
|
||||
# dispatch (persist.c, the on_commit_advanced-adjacent seam bug-1469 added),
|
||||
# asynchronously relative to the majority-ack-gated client reply
|
||||
# (commitIndex/lastApplied are distinct steps -- controller/CLAUDE.md's TLA+
|
||||
# mapping) -- a client can legitimately see its push's "success" reply
|
||||
# before the primary's OWN local apply of that same marker has logged
|
||||
# "config activated...". Poll instead of a single-shot read to close this
|
||||
# real race rather than assume an ordering the design never guarantees.
|
||||
_ACTIVATION_POLL_TIMEOUT_S = 5.0
|
||||
_ACTIVATION_POLL_INTERVAL_S = 0.2
|
||||
|
||||
|
||||
def _assert_primary_committed(log_file: Path, version_tag_hex: str) -> str:
|
||||
marker_line = f"config activated from committed marker tag={version_tag_hex}"
|
||||
deadline = time.monotonic() + _ACTIVATION_POLL_TIMEOUT_S
|
||||
log_text = log_file.read_text()
|
||||
while marker_line not in log_text and time.monotonic() < deadline:
|
||||
time.sleep(_ACTIVATION_POLL_INTERVAL_S)
|
||||
log_text = log_file.read_text()
|
||||
|
||||
assert f"tag={version_tag_hex}" in log_text, log_text
|
||||
assert "Config change committed" in log_text
|
||||
assert f"Config upgraded to tag={version_tag_hex}" in log_text
|
||||
assert f"config activated from committed marker tag={version_tag_hex}" in log_text
|
||||
assert marker_line in log_text, (
|
||||
f"{marker_line!r} never appeared within {_ACTIVATION_POLL_TIMEOUT_S}s "
|
||||
f"of the push replying success -- see this function's own comment "
|
||||
f"for why activation is asynchronous relative to the reply. "
|
||||
f"log_text={log_text!r}"
|
||||
)
|
||||
return log_text
|
||||
|
||||
|
||||
def test_live_config_push_against_primary_reaches_success_path(
|
||||
|
|
@ -464,8 +506,7 @@ def test_live_config_push_against_primary_reaches_success_path(
|
|||
assert push_result.returncode == 0, push_result.stderr
|
||||
|
||||
version_tag_hex = f"{version_tag:016x}"
|
||||
primary_log_text = live_ha_cluster.primary.log_file.read_text()
|
||||
_assert_primary_committed(primary_log_text, version_tag_hex)
|
||||
primary_log_text = _assert_primary_committed(live_ha_cluster.primary.log_file, version_tag_hex)
|
||||
|
||||
# Majority-ACK barrier evidence on the primary's own log: at least one
|
||||
# standby genuinely acked this exact tag (push_config_to_standbys).
|
||||
|
|
@ -494,33 +535,29 @@ def test_live_config_push_against_primary_reaches_success_path(
|
|||
assert standby_saw_it, [s.log_file.read_text() for s in live_ha_cluster.standbys]
|
||||
|
||||
|
||||
# errnox.h: XCE_ERRNO_BASE=10001, E_XCE_STANDBY is the 14th entry (0-indexed
|
||||
# 13) in that enum => 10001+13=10014. "Controller in CTRL_STANDBY; op is
|
||||
# primary-only" -- exactly the rejection a standby's OP_upgradeConfig hits
|
||||
# (module docstring). Named here instead of a bare magic number per this
|
||||
# project's error-handling convention of surfacing XCE's own error
|
||||
# vocabulary rather than an unexplained literal.
|
||||
_E_XCE_STANDBY = 10014
|
||||
|
||||
|
||||
def test_live_config_push_against_standby_returns_standby_rejection_not_redirect(
|
||||
def test_live_config_push_against_standby_redirects_and_succeeds(
|
||||
live_ha_cluster: HaCluster, tmp_path: Path
|
||||
) -> None:
|
||||
"""Pushing `config push` directly to a standby node against a REAL
|
||||
controller today does NOT redirect to the primary -- it is flatly
|
||||
rejected with ``E_XCE_STANDBY`` (module docstring: a pre-existing,
|
||||
already-documented controller-side gap, `fill_redirect_reply()` never
|
||||
wired for `OPCode_upgradeConfig`; out of this test-only task's
|
||||
boundary to fix). This test live-verifies and regression-locks that
|
||||
REAL current behavior: no redirect line, no success, and specifically
|
||||
the `E_XCE_STANDBY` status rather than some other/unexpected failure
|
||||
mode, so a future change to the rejection path itself would also be
|
||||
caught here even before the redirect gap is closed.
|
||||
controller now redirects to the primary and succeeds (bug-1700/1701,
|
||||
fixed 2026-08-26 -- module docstring's "WHAT CLOSED IT" section): the
|
||||
same success outcome as pushing straight to the primary
|
||||
(`test_live_config_push_against_primary_reaches_success_path`), reached
|
||||
via one transparent redirect hop instead of zero.
|
||||
|
||||
Renamed from `test_live_config_push_against_standby_returns_standby_
|
||||
rejection_not_redirect` -- that test's own assertions predicted exactly
|
||||
this outcome the moment the redirect gap closed (see module docstring
|
||||
for the investigation history behind both the original gap and its
|
||||
fix).
|
||||
|
||||
Confirms via ``discovery.py``'s own stderr line (`follow_redirects`:
|
||||
"redirected: host:port -> host:port") being ABSENT -- direct evidence
|
||||
no redirect was attempted, not an inference from the push merely
|
||||
failing for some other reason.
|
||||
"redirected: host:port -> host:port") being PRESENT -- direct evidence
|
||||
a redirect was actually followed, not an inference from the push merely
|
||||
succeeding some other way (e.g. `standby_addr` accidentally being the
|
||||
primary after an election reshuffle; the fixture's own settle window
|
||||
exists precisely to close that race, see module docstring's
|
||||
"Election-wait" section).
|
||||
"""
|
||||
xceadmin_bin = find_first_executable(XCEADMIN_CANDIDATES)
|
||||
if xceadmin_bin is None:
|
||||
|
|
@ -542,33 +579,30 @@ def test_live_config_push_against_standby_returns_standby_rejection_not_redirect
|
|||
)
|
||||
_assert_no_decode_crash(push_result)
|
||||
|
||||
# No redirect was attempted -- the standby's reply never carries
|
||||
# redirectHost/redirectPort for this opcode (module docstring), so
|
||||
# discovery.py's follow_redirects has nothing to follow and never
|
||||
# prints its "redirected: ..." line.
|
||||
unexpected_redirect_line = f"redirected: {standby_addr} -> {primary_addr}"
|
||||
assert unexpected_redirect_line not in push_result.stderr, (
|
||||
"a redirect line appeared -- if the controller-side redirect gap "
|
||||
"documented in this file's module docstring (design.md 'Boundary "
|
||||
"Commitments' / 'Revalidation Triggers') has been closed, this "
|
||||
"test is stale and should be replaced with a real success-path "
|
||||
f"assertion: {push_result.stderr!r}"
|
||||
# Positive evidence the redirect was actually followed -- the standby's
|
||||
# reply now carries redirectHost/redirectPort (bug-1700/1701), so
|
||||
# discovery.py's follow_redirects retries against the primary and prints
|
||||
# this line before the retried push itself is attempted.
|
||||
expected_redirect_line = f"redirected: {standby_addr} -> {primary_addr}"
|
||||
assert expected_redirect_line in push_result.stderr, (
|
||||
f"expected follow_redirects() to redirect from the standby to the "
|
||||
f"primary, got stderr={push_result.stderr!r}"
|
||||
)
|
||||
|
||||
match = VERSION_TAG_RE.search(push_result.stdout)
|
||||
assert match, f"no versionTag/ackedCount found in stdout: {push_result.stdout!r}"
|
||||
version_tag, acked_count = int(match.group(1)), int(match.group(2))
|
||||
assert version_tag > 0
|
||||
|
||||
# Real, live-verified standby-contact behavior today: the reply is a
|
||||
# bare gate rejection (versionTag=0 ackedCount=0), not a redirect or a
|
||||
# partial success.
|
||||
assert version_tag == 0
|
||||
assert acked_count == 0
|
||||
assert push_result.returncode == 1
|
||||
assert f"controller returned status -{_E_XCE_STANDBY}" in push_result.stderr, push_result.stderr
|
||||
# Same success bar as the direct-to-primary test: a genuine positive
|
||||
# ackedCount from real standby ACKs, exit 0 -- the redirect hop changes
|
||||
# HOW the primary was reached, not what a successful push looks like.
|
||||
assert acked_count > 0, (
|
||||
f"expected a positive ackedCount from real standby ACKs after the "
|
||||
f"redirect, got {acked_count} (stdout={push_result.stdout!r} "
|
||||
f"stderr={push_result.stderr!r})"
|
||||
)
|
||||
assert push_result.returncode == 0, push_result.stderr
|
||||
|
||||
# The rejected push never reached the primary at all -- its log shows
|
||||
# no upgrade activity from this attempt (independent confirmation this
|
||||
# is a clean gate rejection, not a landed-then-lost push).
|
||||
primary_log_text = live_ha_cluster.primary.log_file.read_text()
|
||||
assert "Config marker failed to reach committed prefix" not in primary_log_text
|
||||
version_tag_hex = f"{version_tag:016x}"
|
||||
_assert_primary_committed(live_ha_cluster.primary.log_file, version_tag_hex)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue