diff --git a/.kiro/specs/controller-peers-env-fallback/tasks.md b/.kiro/specs/controller-peers-env-fallback/tasks.md index 3e557597..023b6277 100644 --- a/.kiro/specs/controller-peers-env-fallback/tasks.md +++ b/.kiro/specs/controller-peers-env-fallback/tasks.md @@ -79,7 +79,7 @@ - _Requirements: 2.2, 2.8_ - _Boundary: xceadmin discovery and errors_ -- [ ] 4.2 (P) Read the new variable ahead of the legacy one in every `--controller` option +- [x] 4.2 (P) Read the new variable ahead of the legacy one in every `--controller` option - Write the failing CLI tests first: the variable alone; both variables set (new one wins); explicit `--controller` overriding both; neither set (built-in default); each option's `--help` naming both variables - Add `monkeypatch.delenv` for the new variable to every existing test that asserts legacy-variable or no-variable behavior, so ambient state cannot affect them - Make the shared env-var setting an ordered list (new variable first, legacy second) and rewrite each option's help string and the precedence comment, since the help strings interpolate the setting today diff --git a/src/xceadmin/src/xceadmin/cli.py b/src/xceadmin/src/xceadmin/cli.py index 45962c2d..e9dd2c44 100644 --- a/src/xceadmin/src/xceadmin/cli.py +++ b/src/xceadmin/src/xceadmin/cli.py @@ -99,14 +99,25 @@ def _compose_version() -> str: _DEFAULT_OUTPUT = "cluster.bin" _DEFAULT_CONTROLLER = "localhost:9991" -# Every --controller option below also reads this env var (click precedence: -# explicit --controller flag > $XCEADMIN_CONTROLLER > _DEFAULT_CONTROLLER). +# Every --controller option below also reads these env vars, in order. click +# returns the first one set to a non-empty value (an empty one counts as +# unset), so the precedence is: explicit --controller flag > +# $XCE_CONTROLLER_PEERS (shared with the controller/scheduler/worker/harness; +# may carry name=host:port entries, which resolve_controllers() strips) > +# $XCEADMIN_CONTROLLER (legacy, still honored) > _DEFAULT_CONTROLLER. # _DEFAULT_CONTROLLER has no relationship to any REAL deployment's actual # ports -- it's a bare fallback for a totally bare invocation. A deployment # that knows its own real controller ports (e.g. /xce-deploy-ha's generated -# scripts/xceadmin wrapper) should export this instead of requiring every +# scripts/xceadmin wrapper) should export these instead of requiring every # command to be typed with an explicit --controller. -_CONTROLLER_ENV_VAR = "XCEADMIN_CONTROLLER" +_CONTROLLER_ENV_VAR = ["XCE_CONTROLLER_PEERS", "XCEADMIN_CONTROLLER"] +_CONTROLLER_HELP = ( + "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 " + "$XCE_CONTROLLER_PEERS if set (preferred; accepts name=host:port entries), " + "then the legacy $XCEADMIN_CONTROLLER if set, then the bare default." +) _STATUS_LABEL_WIDTH = 13 # len("started at:") + 2 -- requirements.md's "Status output" example @@ -140,10 +151,7 @@ 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), 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.", + help=_CONTROLLER_HELP, ) @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: @@ -273,10 +281,7 @@ 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), 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.", + help=_CONTROLLER_HELP, ) def scheduler_record_start(file: str | None, controller: str) -> None: """Start scheduler event recording, optionally to FILE (auto-generated when omitted).""" @@ -289,10 +294,7 @@ 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), 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.", + help=_CONTROLLER_HELP, ) def scheduler_record_stop(controller: str) -> None: """Stop scheduler event recording.""" @@ -305,10 +307,7 @@ 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), 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.", + help=_CONTROLLER_HELP, ) def scheduler_record_status(controller: str) -> None: """Report current scheduler event-recording state.""" diff --git a/src/xceadmin/tests/test_cli.py b/src/xceadmin/tests/test_cli.py index 6b89430c..bdfd2b01 100644 --- a/src/xceadmin/tests/test_cli.py +++ b/src/xceadmin/tests/test_cli.py @@ -12,6 +12,7 @@ import ssl import socket import threading from pathlib import Path +from types import SimpleNamespace import pytest import yaml @@ -597,6 +598,7 @@ def test_format_sched_admin_error_plain_when_not_recording() -> None: def test_scheduler_record_start_success_prints_formatted_result(monkeypatch) -> None: + monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False) captured = {} def _fake_record_start(controller, filename): @@ -650,6 +652,7 @@ def test_scheduler_record_start_passes_filename_and_controller_arguments(monkeyp def test_controller_option_falls_back_to_xceadmin_controller_env_var(monkeypatch) -> None: + monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False) captured = {} def _fake_record_status(controller): @@ -668,6 +671,7 @@ def test_controller_option_falls_back_to_xceadmin_controller_env_var(monkeypatch def test_controller_flag_overrides_xceadmin_controller_env_var(monkeypatch) -> None: + monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False) captured = {} def _fake_record_status(controller): @@ -687,6 +691,126 @@ def test_controller_flag_overrides_xceadmin_controller_env_var(monkeypatch) -> N assert captured["controller"] == "127.0.0.1:9999" +# --------------------------------------------------------------------------- +# --controller precedence with $XCE_CONTROLLER_PEERS (controller-peers-env- +# fallback Requirement 2.1, 2.4-2.7, 4.3): explicit --controller > +# $XCE_CONTROLLER_PEERS > $XCEADMIN_CONTROLLER > _DEFAULT_CONTROLLER, for +# every command that takes --controller. Each case sets or deletes BOTH +# variables itself, so ambient environment state cannot leak in. +# --------------------------------------------------------------------------- + +_RECORD_RESULT = RecordResult(recording=False, file="", event_count=0, started_at=0) + + +def _patch_controller_command(monkeypatch, command: str, tmp_path: Path) -> tuple[list[str], dict]: + """Patch the backend function behind ``command`` so it records the + controller value it was given; return the CLI args that invoke it.""" + captured: dict = {} + + def _record(controller, *_rest): + captured["controller"] = controller + return _RECORD_RESULT + + if command == "config push": + + def _fake_push_config(cluster_bin_path, controller): + captured["controller"] = controller + return SimpleNamespace(version_tag=1, acked_count=1, error="") + + monkeypatch.setattr(cli_module, "push_config", _fake_push_config) + bin_path = tmp_path / "cluster.bin" + bin_path.write_bytes(b"unused -- push_config() is monkeypatched here") + return ["config", "push", str(bin_path)], captured + + name = command.split()[-1] + monkeypatch.setattr(cli_module, f"record_{name}", _record) + return ["scheduler", "record", name], captured + + +_CONTROLLER_COMMANDS = [ + "config push", + "scheduler record start", + "scheduler record stop", + "scheduler record status", +] + + +@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS) +def test_controller_option_uses_xce_controller_peers_alone(monkeypatch, tmp_path, command) -> None: + monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False) + monkeypatch.setenv("XCE_CONTROLLER_PEERS", "ctrl-1=localhost:9921,ctrl-2=localhost:9922") + args, captured = _patch_controller_command(monkeypatch, command, tmp_path) + + result = CliRunner().invoke(cli, args) + + assert result.exit_code == 0, result.output + assert captured["controller"] == "ctrl-1=localhost:9921,ctrl-2=localhost:9922" + + +@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS) +def test_controller_option_prefers_xce_controller_peers_over_legacy(monkeypatch, tmp_path, command) -> None: + monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921") + monkeypatch.setenv("XCEADMIN_CONTROLLER", "localhost:9901") + args, captured = _patch_controller_command(monkeypatch, command, tmp_path) + + result = CliRunner().invoke(cli, args) + + assert result.exit_code == 0, result.output + assert captured["controller"] == "localhost:9921" + + +@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS) +def test_controller_option_empty_xce_controller_peers_falls_through_to_legacy( + monkeypatch, tmp_path, command +) -> None: + monkeypatch.setenv("XCE_CONTROLLER_PEERS", "") + monkeypatch.setenv("XCEADMIN_CONTROLLER", "localhost:9901") + args, captured = _patch_controller_command(monkeypatch, command, tmp_path) + + result = CliRunner().invoke(cli, args) + + assert result.exit_code == 0, result.output + assert captured["controller"] == "localhost:9901" + + +@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS) +def test_controller_flag_overrides_both_env_vars(monkeypatch, tmp_path, command) -> None: + monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921") + monkeypatch.setenv("XCEADMIN_CONTROLLER", "localhost:9901") + args, captured = _patch_controller_command(monkeypatch, command, tmp_path) + + result = CliRunner().invoke(cli, [*args, "--controller", "127.0.0.1:9999"]) + + assert result.exit_code == 0, result.output + assert captured["controller"] == "127.0.0.1:9999" + + +@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS) +def test_controller_option_uses_default_when_neither_env_var_set(monkeypatch, tmp_path, command) -> None: + monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False) + monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False) + args, captured = _patch_controller_command(monkeypatch, command, tmp_path) + + result = CliRunner().invoke(cli, args) + + assert result.exit_code == 0, result.output + assert captured["controller"] == cli_module._DEFAULT_CONTROLLER + + +@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS) +def test_controller_option_help_names_both_env_vars_in_order(command) -> None: + result = CliRunner().invoke(cli, [*command.split(), "--help"]) + + assert result.exit_code == 0, result.output + help_text = " ".join(result.output.split()) + assert "$XCE_CONTROLLER_PEERS" in help_text + assert "$XCEADMIN_CONTROLLER" in help_text + assert help_text.index("$XCE_CONTROLLER_PEERS") < help_text.index("$XCEADMIN_CONTROLLER") + assert "name=host:port" in help_text + assert "['XCE" not in result.output + assert "$[" not in result.output + + def test_scheduler_record_stop_success_prints_formatted_result(monkeypatch) -> None: def _fake_record_stop(controller): return RecordResult(recording=False, file="final.replay", event_count=14273, started_at=1000) @@ -778,6 +902,7 @@ def test_scheduler_record_status_connection_timeout_exits_nonzero_not_raw_traceb def test_scheduler_record_start_connection_refused_exits_nonzero_not_raw_traceback(monkeypatch) -> None: + monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False) def _fake_record_start(controller, filename): raise ConnectionRefusedError("refused")