diff --git a/.kiro/specs/controller-peers-env-fallback/tasks.md b/.kiro/specs/controller-peers-env-fallback/tasks.md index 023b6277..f4b8b6bc 100644 --- a/.kiro/specs/controller-peers-env-fallback/tasks.md +++ b/.kiro/specs/controller-peers-env-fallback/tasks.md @@ -87,7 +87,7 @@ - _Requirements: 2.1, 2.4, 2.5, 2.6, 2.7, 4.3_ - _Boundary: xceadmin CLI_ -- [ ] 4.3 Name the environment source in xceadmin's controller errors +- [x] 4.3 Name the environment source in xceadmin's controller errors - Write the failing CLI tests first: in-order failover over an env-derived named list (patching the per-candidate request so the first fails and the second answers); unreachable values from each variable and from an explicit `--controller` (source note only for the two variables); a missing cluster-bin file with the variable set (no source note); a malformed entry from an explicit `--controller` and from the variable (entry quoted, note only for the variable). Use `config push` with a real cluster-bin file, or patch out the record commands' retry pause - Add a helper that returns "(from $NAME)" when the value came from the environment, using the same first-non-empty rule click applies - Append it to the "cannot connect" messages only for network errors (not local file errors), and to the controller-reachability and controller-value errors in the existing handlers; leave other errors unchanged diff --git a/src/xceadmin/src/xceadmin/cli.py b/src/xceadmin/src/xceadmin/cli.py index e9dd2c44..471feabb 100644 --- a/src/xceadmin/src/xceadmin/cli.py +++ b/src/xceadmin/src/xceadmin/cli.py @@ -27,14 +27,23 @@ traceback. from __future__ import annotations +import os from collections.abc import Callable from datetime import datetime, timezone from importlib.metadata import PackageNotFoundError from importlib.metadata import version as _package_version import click +from click.core import ParameterSource -from xcecommon.errors import SchedAdminError, XceAdminError +from xcecommon.errors import ( + ConnectionTimeoutError, + ControllerValueError, + NoPrimaryError, + SchedAdminError, + TooManyRedirectsError, + XceAdminError, +) from xceadmin.build import build_cluster_config, dump_cluster_config, write_output from xceadmin.push import probe_config, push_config from xceadmin.scheduler import RecordResult, dump, record_start, record_status, record_stop @@ -118,9 +127,46 @@ _CONTROLLER_HELP = ( "$XCE_CONTROLLER_PEERS if set (preferred; accepts name=host:port entries), " "then the legacy $XCEADMIN_CONTROLLER if set, then the bare default." ) +# Errors about the --controller value itself (malformed, or nothing reachable +# at it): when that value came from the environment, their message names the +# variable (controller-peers-env-fallback 2.8). Every other XceAdminError +# (e.g. a bad cluster-bin file, a SchedAdminError reply) keeps its text. +_CONTROLLER_ERRORS = (ControllerValueError, ConnectionTimeoutError, TooManyRedirectsError, NoPrimaryError) _STATUS_LABEL_WIDTH = 13 # len("started at:") + 2 -- requirements.md's "Status output" example +def _controller_source_note() -> str: + """``" (from $NAME)"`` when the current command's ``--controller`` value + came from the environment, ``""`` otherwise (explicit flag or default). + NAME is the first variable in ``_CONTROLLER_ENV_VAR`` set to a non-empty + value -- the same rule click applies when it reads the envvar list. + """ + ctx = click.get_current_context(silent=True) + if ctx is None or ctx.get_parameter_source("controller") is not ParameterSource.ENVIRONMENT: + return "" + for name in _CONTROLLER_ENV_VAR: + if os.environ.get(name): + return f" (from ${name})" + return "" + + +def _controller_error_message(e: XceAdminError) -> str: + """``e.message``, plus the env source note for a controller-value error.""" + if isinstance(e, _CONTROLLER_ERRORS): + return e.message + _controller_source_note() + return e.message + + +def _cannot_connect_message(controller: str, e: OSError) -> str: + """The "cannot connect" text for an ``OSError``; the env source note is + added only for a network error -- a local file error (missing or + unreadable cluster-bin file) carries ``filename`` and is not the + controller value's fault. + """ + note = _controller_source_note() if e.filename is None else "" + return f"cannot connect to {controller}: {e}{note}" + + @click.group() @click.version_option(version=_compose_version(), prog_name="xceadmin") def cli() -> None: @@ -172,12 +218,12 @@ def config_push(cluster_bin_path: str, controller: str, dry_run: bool) -> None: raise click.ClickException(result.error or "push rejected (ackedCount == 0)") # 2.7 # 2.6: positive ackedCount and no error -- fall through to exit 0. except XceAdminError as e: - raise click.ClickException(e.message) from e + raise click.ClickException(_controller_error_message(e)) from e except OSError as e: # ConnectionRefusedError and other bare OS-level failures propagate # unwrapped out of xcecommon.wire by design (see wire.py's module # docstring) -- this is the layer that formats them (7.1). - raise click.ClickException(f"cannot connect to {controller}: {e}") from e + raise click.ClickException(_cannot_connect_message(controller, e)) from e except KeyboardInterrupt: raise click.ClickException("cancelled") from None @@ -265,12 +311,12 @@ def _run_scheduler_admin_command(controller: str, call: Callable[[], RecordResul except SchedAdminError as e: raise click.ClickException(_format_sched_admin_error(e)) from e except XceAdminError as e: - raise click.ClickException(e.message) from e + raise click.ClickException(_controller_error_message(e)) from e except OSError as e: # Same rationale as config_push's own OSError branch: bare OS-level # failures (ConnectionRefusedError etc.) propagate unwrapped out of # xcecommon.wire by design -- this is the layer that formats them. - raise click.ClickException(f"cannot connect to {controller}: {e}") from e + raise click.ClickException(_cannot_connect_message(controller, e)) from e except KeyboardInterrupt: raise click.ClickException("cancelled") from None diff --git a/src/xceadmin/tests/test_cli.py b/src/xceadmin/tests/test_cli.py index bdfd2b01..bc5f0225 100644 --- a/src/xceadmin/tests/test_cli.py +++ b/src/xceadmin/tests/test_cli.py @@ -967,3 +967,221 @@ def test_scheduler_dump_missing_file_exits_nonzero_with_clean_message(monkeypatc assert result.exit_code != 0 assert not isinstance(result.exception, KeyError | AttributeError | TypeError | FileNotFoundError) assert "does-not-exist.replay" in result.output + + +# --------------------------------------------------------------------------- +# Controller errors name an environment source (controller-peers-env-fallback +# Requirements 2.3, 2.8): a controller value taken from $XCE_CONTROLLER_PEERS +# or $XCEADMIN_CONTROLLER gets " (from $NAME)" appended to its unreachable / +# malformed-entry error; an explicit --controller value and errors unrelated +# to the controller value (a missing cluster-bin file) get no note. +# --------------------------------------------------------------------------- + + +def _closed_port() -> int: + listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + listener.bind(("127.0.0.1", 0)) + port = listener.getsockname()[1] + listener.close() + return port + + +def _error_line(result) -> str: + """The ``Error: ...`` line click prints for a ClickException (stderr + lines from the failover loop may precede it in ``result.output``).""" + lines = [line for line in result.output.splitlines() if line.startswith("Error: ")] + assert len(lines) == 1, result.output + return lines[0] + + +def _assert_clean_failure(result) -> None: + assert result.exit_code == 1, result.output + assert isinstance(result.exception, SystemExit), repr(result.exception) + assert "Traceback" not in result.output + + +def test_config_push_fails_over_env_candidates_in_order(monkeypatch, cluster_bin, wrap_reply) -> None: + """2.3: an env-derived named list is tried in env order with name= + stripped; an unreachable first candidate fails over to the second, and + the third is never needed.""" + from xceadmin import discovery + + monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False) + monkeypatch.setenv("XCE_CONTROLLER_PEERS", "a=127.0.0.1:1,b=127.0.0.1:2,127.0.0.1:3") + + mod = load_schema() + reply = mod.RequestReply.new_message() + reply.status = 0 + upgrade_reply = reply.data.init("upgradeconfigreplymsg") + upgrade_reply.versionTag = 7 + upgrade_reply.ackedCount = 2 + upgrade_reply.error = "" + reply_bytes = wrap_reply(reply) + + tried: list[tuple[str, int]] = [] + + def _fake_follow_redirects(host, port, send_fn): + tried.append((host, port)) + if (host, port) == ("127.0.0.1", 1): + raise ConnectionRefusedError("refused") + return host, port, reply_bytes + + monkeypatch.setattr(discovery, "follow_redirects", _fake_follow_redirects) + + result = CliRunner().invoke(cli, ["config", "push", cluster_bin]) + + assert result.exit_code == 0, result.output + assert tried == [("127.0.0.1", 1), ("127.0.0.1", 2)] + assert "versionTag=7 ackedCount=2" in result.output + + +@pytest.mark.parametrize("dry_run", [False, True], ids=["push", "dry-run"]) +@pytest.mark.parametrize( + ("env_name", "note"), + [ + ("XCE_CONTROLLER_PEERS", " (from $XCE_CONTROLLER_PEERS)"), + ("XCEADMIN_CONTROLLER", " (from $XCEADMIN_CONTROLLER)"), + (None, ""), + ], + ids=["xce-controller-peers", "xceadmin-controller", "explicit"], +) +def test_config_push_unreachable_controller_names_env_source( + monkeypatch, cluster_bin, env_name, note, dry_run +) -> None: + """2.8: an unreachable controller value is a clean error ending with + the variable it came from; an explicit --controller gets no note.""" + monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False) + monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False) + value = f"127.0.0.1:{_closed_port()}" + args = ["config", "push", cluster_bin] + (["--dry-run"] if dry_run else []) + if env_name is None: + args += ["--controller", value] + else: + monkeypatch.setenv(env_name, value) + + result = CliRunner().invoke(cli, args) + + _assert_clean_failure(result) + line = _error_line(result) + assert value in line + assert "no primary available" in line + if note: + assert line.endswith(note), line + else: + assert "(from $" not in line, line + + +@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS) +@pytest.mark.parametrize( + "error", + [ + ConnectionRefusedError("refused"), + ConnectionTimeoutError("timed out connecting to 127.0.0.1:9", "127.0.0.1", 9), + ], + ids=["os-error", "connection-timeout"], +) +def test_controller_errors_name_env_source_in_every_command( + monkeypatch, tmp_path, command, error +) -> None: + """2.8: every --controller command appends the env source note to a + network OSError ("cannot connect to ...") and to a controller + reachability error raised by its backend call.""" + monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False) + monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921") + args, _captured = _patch_controller_command(monkeypatch, command, tmp_path) + + def _raise(*_args, **_kwargs): + raise error + + target = "push_config" if command == "config push" else f"record_{command.split()[-1]}" + monkeypatch.setattr(cli_module, target, _raise) + + result = CliRunner().invoke(cli, args) + + _assert_clean_failure(result) + line = _error_line(result) + assert line.endswith(" (from $XCE_CONTROLLER_PEERS)"), line + if isinstance(error, OSError): + assert line.startswith("Error: cannot connect to localhost:9921: refused"), line + + +def test_scheduler_record_sched_admin_error_gets_no_env_source_note(monkeypatch) -> None: + """A SchedAdminError is the scheduler's own reply, not a controller + value problem: its text stays unchanged even with the variable set.""" + monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921") + + def _fake_record_stop(controller): + raise SchedAdminError( + "scheduler admin request failed: not recording", + error="not recording", + recording=False, + file="", + event_count=0, + started_at=0, + ) + + monkeypatch.setattr(cli_module, "record_stop", _fake_record_stop) + + result = CliRunner().invoke(cli, ["scheduler", "record", "stop"]) + + _assert_clean_failure(result) + assert _error_line(result) == "Error: not recording" + + +def test_config_push_missing_cluster_bin_gets_no_env_source_note(monkeypatch, tmp_path) -> None: + """2.8: a local file error (FileNotFoundError carries a filename) is + not blamed on the variable even when the controller value came from + it.""" + monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False) + monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921") + missing = tmp_path / "does-not-exist.bin" + + result = CliRunner().invoke(cli, ["config", "push", str(missing)]) + + _assert_clean_failure(result) + line = _error_line(result) + assert str(missing) in line + assert "(from $" not in line, line + + +def test_config_push_malformed_explicit_controller_entry_is_clean_error_without_note( + monkeypatch, cluster_bin +) -> None: + """2.8: a malformed entry from an explicit --controller is quoted in a + clean error with no source note, even with both variables set.""" + monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921") + monkeypatch.setenv("XCEADMIN_CONTROLLER", "localhost:9901") + + result = CliRunner().invoke(cli, ["config", "push", cluster_bin, "--controller", "a=h:x"]) + + _assert_clean_failure(result) + line = _error_line(result) + assert "'a=h:x'" in line + assert "(from $" not in line, line + + +@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS) +@pytest.mark.parametrize( + "env_name", ["XCE_CONTROLLER_PEERS", "XCEADMIN_CONTROLLER"] +) +def test_malformed_env_controller_entry_is_clean_error_naming_variable( + monkeypatch, cluster_bin, command, env_name +) -> None: + """2.8: a malformed entry from either variable is quoted in a clean + error (no traceback) ending with the variable's name, in every + --controller command (backends not patched: the real + resolve_controllers() raises before any network I/O).""" + monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False) + monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False) + monkeypatch.setenv(env_name, "a=h:x") + if command == "config push": + args = ["config", "push", cluster_bin] + else: + args = command.split() + + result = CliRunner().invoke(cli, args) + + _assert_clean_failure(result) + line = _error_line(result) + assert "'a=h:x'" in line + assert line.endswith(f" (from ${env_name})"), line