From b00e97ffe623cbd502e9982053eb7954d86cf657 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 10 Aug 2026 09:53:48 +0000 Subject: [PATCH] fix(xceadmin): probe_config uses Request envelope + follow_redirects (bug-1506, bug-1507) - probe_config() now wraps probe in Request envelope matching push_config() - Controller OPCode_probeConfigTag handler replies through xce_reply() with redirectHost/redirectPort populated - follow_redirects works identically for both push and probe flows --- src/xceadmin/src/xceadmin/push.py | 63 +++++++++++++------ src/xceadmin/tests/test_cli.py | 10 +-- src/xceadmin/tests/test_config_push_mock.py | 67 ++++++++++++--------- 3 files changed, 86 insertions(+), 54 deletions(-) diff --git a/src/xceadmin/src/xceadmin/push.py b/src/xceadmin/src/xceadmin/push.py index 7690c39f..3a95d78f 100644 --- a/src/xceadmin/src/xceadmin/push.py +++ b/src/xceadmin/src/xceadmin/push.py @@ -30,14 +30,13 @@ nothing else to say. Real SHA/version-compatibility validation (Requirement 6.1's aspirational description) is future controller-side work, not something this client can check today. -``probe_config()`` connects directly and does NOT go through -``xceadmin.discovery.follow_redirects``: ``ProbeConfigTagReply`` is a -direct struct with only a ``configTag`` field -- no ``redirectHost``/ -``redirectPort`` exist on it at all, so there is no redirect signal for -``follow_redirects`` (which decodes every reply as ``RequestReply``) to -read. Feeding ``ProbeConfigTagReply`` bytes through it would misinterpret -them as a different struct shape. Only ``push_config()``'s -``Request``/``RequestReply``-wrapped exchange supports redirects. +``probe_config()`` now follows the same ``Request``-envelope + +``follow_redirects`` + ``decode_reply`` path as ``push_config()`` +(bug-1507). The controller gained a real ``OPCode_probeConfigTag`` +handler (bug-1506) that replies through the standard ``xce_reply()`` +→ ``Request{opcode: reply, body.replymsg: RequestReply}`` envelope +with ``redirectHost``/``redirectPort`` populated, so ``follow_redirects`` +works identically for both flows. """ from __future__ import annotations @@ -131,28 +130,52 @@ def push_config(cluster_bin_path: str, controller: str) -> PushResult: def probe_config(cluster_bin_path: str, controller: str) -> DryRunResult: """Probe the controller's current config generation without pushing - anything (6.1). ``cluster_bin_path`` is read only to confirm it - exists and is readable before spending a network round-trip -- its - content plays no role in ``ProbeConfigTagRequest``, which carries no - config data (see module docstring). + anything (6.1, bug-1507). ``cluster_bin_path`` is read only to + confirm it exists and is readable before spending a network + round-trip — its content plays no role in ``ProbeConfigTagRequest``, + which carries no config data. + + Now goes through the standard ``Request`` envelope → + ``follow_redirects`` → ``decode_reply`` path (matching + ``push_config``), using the controller's real ``OPCode_probeConfigTag`` + handler (bug-1506). Before this fix the request was sent as a bare + ``ProbeConfigTagRequest`` direct struct with no controller-side + handler at all, so ``accepted=True`` was a client-side fabrication. Postconditions: exit 0 and "OK: controller will accept this config - (generation N->M)" iff ``accepted`` (6.2); otherwise the caller exits - non-zero with ``reason`` displayed (6.3) -- this function only - reports the result, it does not exit. + (generation N->M)" iff the controller returns a successful reply + (6.2); otherwise the caller exits non-zero with ``reason`` displayed + (6.3) — this function only reports the result, it does not exit. """ Path(cluster_bin_path).read_bytes() mod = load_schema() - request = mod.ProbeConfigTagRequest.new_message() - request.name = "" # matches config_repl.c's xce_probe_config_tag exactly + request = mod.Request.new_message() + request.opcode = "probeConfigTag" + request.seq = 1 + probe = request.body.init("probeconfigtagrequestmsg") + probe.name = "" request_bytes = request.to_bytes() host, port = _split_controller(controller) - reply_bytes = _send_via(request_bytes)(host, port) + _, _, reply_bytes = follow_redirects(host, port, _send_via(request_bytes)) - with mod.ProbeConfigTagReply.from_bytes(reply_bytes) as reply: - current_generation = reply.configTag + with decode_reply(reply_bytes) as reply: + if reply.status != 0: + return DryRunResult( + current_generation=0, + would_be_generation=0, + accepted=False, + reason=f"controller returned status {reply.status}", + ) + if reply.data.which() != "probeconfigtagreplymsg": + reason = ( + f"unexpected reply variant {reply.data.which()!r}, " + f"expected probeconfigtagreplymsg" + ) + raise ReplyDecodeError(f"invalid reply: {reason}", reason) + probe_reply = reply.data.probeconfigtagreplymsg + current_generation = probe_reply.configTag return DryRunResult( current_generation=current_generation, diff --git a/src/xceadmin/tests/test_cli.py b/src/xceadmin/tests/test_cli.py index 6c411803..d89d0c45 100644 --- a/src/xceadmin/tests/test_cli.py +++ b/src/xceadmin/tests/test_cli.py @@ -271,16 +271,18 @@ def test_config_push_connection_refused_exits_nonzero_not_raw_traceback(cluster_ assert result.output.strip() != "" -def test_config_push_dry_run_reports_generations(mock_controller, cluster_bin): +def test_config_push_dry_run_reports_generations(mock_controller, cluster_bin, wrap_reply): from xcecommon import wire mod = load_schema() def _handler(conn): wire.recv_frame(conn) - reply = mod.ProbeConfigTagReply.new_message() - reply.configTag = 4 - wire.send_frame(conn, reply.to_bytes()) + inner = mod.RequestReply.new_message() + inner.status = 0 + probe_reply = inner.data.init("probeconfigtagreplymsg") + probe_reply.configTag = 4 + wire.send_frame(conn, wrap_reply(inner)) srv = mock_controller(_handler) diff --git a/src/xceadmin/tests/test_config_push_mock.py b/src/xceadmin/tests/test_config_push_mock.py index b365a996..66874937 100644 --- a/src/xceadmin/tests/test_config_push_mock.py +++ b/src/xceadmin/tests/test_config_push_mock.py @@ -244,19 +244,29 @@ def test_push_config_explicit_port_parsed(): # --------------------------------------------------------------------------- -def test_probe_config_reports_current_and_would_be_generation(mock_controller, cluster_bin): +def test_probe_config_reports_current_and_would_be_generation(mock_controller, cluster_bin, wrap_reply): + """bug-1507: probe_config now sends a Request envelope and decodes + through follow_redirects → decode_reply, matching push_config's path. + The mock server must unwrap the Request envelope on receive and wrap + its reply the same way a real controller does. """ mod = load_schema() received = {} def _handler(conn): request_bytes = wire.recv_frame(conn) received["request_bytes"] = request_bytes - with mod.ProbeConfigTagRequest.from_bytes(request_bytes) as req: - received["name"] = req.name + # Request{opcode: probeConfigTag, body.probeconfigtagrequestmsg: ...} + with mod.Request.from_bytes(request_bytes) as req: + assert req.opcode == "probeConfigTag" + assert req.body.which() == "probeconfigtagrequestmsg" + received["name"] = req.body.probeconfigtagrequestmsg.name - reply = mod.ProbeConfigTagReply.new_message() - reply.configTag = 41 - wire.send_frame(conn, reply.to_bytes()) + # Build reply: RequestReply{status:0, data.probeconfigtagreplymsg: {configTag:41}} + inner = mod.RequestReply.new_message() + inner.status = 0 + probe_reply = inner.data.init("probeconfigtagreplymsg") + probe_reply.configTag = 41 + wire.send_frame(conn, wrap_reply(inner)) srv = mock_controller(_handler) @@ -266,21 +276,22 @@ def test_probe_config_reports_current_and_would_be_generation(mock_controller, c assert result.would_be_generation == 42 assert result.accepted is True assert result.reason == "" - assert received["name"] == "" # matches config_repl.c's xce_probe_config_tag exactly + assert received["name"] == "" -def test_probe_config_zero_generation_still_accepted(mock_controller, cluster_bin): +def test_probe_config_zero_generation_still_accepted(mock_controller, cluster_bin, wrap_reply): """A fresh cluster with no config committed yet (configTag == 0) is - still a successful probe -- there is no rejection signal on this - wire shape (module docstring); the caller decides significance. - """ + still a successful probe -- configTag == 0 is a valid, real response + from a real controller handler now (bug-1506). """ mod = load_schema() def _handler(conn): wire.recv_frame(conn) - reply = mod.ProbeConfigTagReply.new_message() - reply.configTag = 0 - wire.send_frame(conn, reply.to_bytes()) + inner = mod.RequestReply.new_message() + inner.status = 0 + probe_reply = inner.data.init("probeconfigtagreplymsg") + probe_reply.configTag = 0 + wire.send_frame(conn, wrap_reply(inner)) srv = mock_controller(_handler) @@ -293,27 +304,23 @@ def test_probe_config_zero_generation_still_accepted(mock_controller, cluster_bi @pytest.mark.parametrize("tag_value", [0, 1, 999999]) def test_probe_config_has_no_wire_level_rejection_signal_by_design( - mock_controller, cluster_bin, tag_value + mock_controller, cluster_bin, tag_value, wrap_reply ): - """5.6 asks for both an accepted and a rejected dry-run outcome, but - ``ProbeConfigTagReply`` (module docstring, ``push.py`` module - docstring) has only a bare ``configTag`` -- no field exists on the - real wire contract to signal rejection, matching the real C - reference client (``config_repl.c``'s ``xce_probe_config_tag``, which - also returns only a bare tag). This test locks in that limitation - explicitly rather than silently having no coverage for it: every - successful round-trip, regardless of the returned ``configTag`` - value, yields ``accepted=True`` -- there is no reachable code path - that produces ``accepted=False`` today. A real "would this SHA be - accepted" check is future controller-side work (Requirement 6.1). - """ + """ProbeConfigTagReply (controller.capnp:909) has only a bare + ``configTag`` field -- no separate accept/reject flag exists. A + real controller handler (bug-1506) always returns status==0 here; + every successful round-trip, regardless of the returned configTag + value, yields ``accepted=True``. A real "would this SHA be accepted" + check is future work (Requirement 6.1). """ mod = load_schema() def _handler(conn): wire.recv_frame(conn) - reply = mod.ProbeConfigTagReply.new_message() - reply.configTag = tag_value - wire.send_frame(conn, reply.to_bytes()) + inner = mod.RequestReply.new_message() + inner.status = 0 + probe_reply = inner.data.init("probeconfigtagreplymsg") + probe_reply.configTag = tag_value + wire.send_frame(conn, wrap_reply(inner)) srv = mock_controller(_handler)