diff --git a/.kiro/specs/admin-config-tool/tasks.md b/.kiro/specs/admin-config-tool/tasks.md index 7789769..8abde43 100644 --- a/.kiro/specs/admin-config-tool/tasks.md +++ b/.kiro/specs/admin-config-tool/tasks.md @@ -57,7 +57,7 @@ - _Depends: 2.2_ - [ ] 3. Core: xceadmin CLI orchestration -- [ ] 3.1 (P) Compile a configs directory into a deployable cluster binary +- [x] 3.1 (P) Compile a configs directory into a deployable cluster binary - Running the build command against a directory of YAML files produces a binary output file with a printed checksum - A missing individual YAML file is treated as an empty section, not an error - Unrecognized-but-valid YAML fields produce a warning, not a failure @@ -150,3 +150,4 @@ - **2.2**: The real `controller.capnp` names its reply struct `RequestReply`, not `Reply` as design.md's postcondition states — an inexact design pseudo-type, corrected empirically (verified directly against `src/schema/controller.capnp`, not assumed). pycapnp 2.0.0/capnproto 1.2.0 abort the whole process (uncaught C++ exception, not a Python exception) on a missing cross-file import or malformed `.capnp` content passed to `capnp.load()` — a bare `try/except` cannot catch this. `xcecommon.schema.load_schema()` works around it by pre-validating in an isolated subprocess before ever calling `capnp.load()` in the caller's own process, converting any abort into a clean `SchemaNotFoundError`. Any future code that calls `capnp.load()` directly (bypassing this loader) reintroduces the crash risk — always go through `xcecommon.schema.load_schema()`. - **2.3**: `map_all_sections()` (a `SECTION_MAP`-driven orchestrator, not explicitly named in design.md's Service Interface but consistent with its Contracts) is the single call `xceadmin.build` (task 3.1) needs — it treats a missing YAML file identically to a present-but-empty section (byte-for-byte), satisfying Requirement 1.5 structurally. `target.init(field, 0)` and never touching an empty/absent list field produce *different* serialized bytes for the same logical "nothing here" value (verified empirically) — the mapper always picks "never touch" for byte-determinism (9.1). Calling `.to_bytes()` twice on the same capnp builder triggers a spurious "message already written" `UserWarning` even when no field was mutated between calls — capture the bytes once and reuse them rather than calling `.to_bytes()` again. - **2.4**: `errors.py`'s exception subclasses (task 1.2) all take `message` as an explicit first constructor argument in addition to their structured fields (e.g. `ConnectionTimeoutError(message, host, port)`, `ReplyDecodeError(message, reason)`) — design.md's abbreviated `Service Interface` type stubs (`class ConnectionTimeoutError(XceAdminError): host: str; port: int`) read as if only the structured fields were needed; always check the real constructor in `errors.py`, not the design doc's shorthand. `xcecommon.wire` is deliberately opcode-agnostic — `send_request`/`send_frame`/`recv_frame` move raw `bytes` only and never import a capnp type; callers own `.to_bytes()`/`.from_bytes()`. Reused `ha/data/xce.crt`/`xce.key` (the repo's existing dev cert pair, same one `deploy-ha.sh` already copies for controller TLS) as the test fixture for both the TLS test server and the client `cert_dir` — no synthetic cert generation needed. +- **3.1**: Requirement 1.7's "unrecognized-but-schema-valid field" warning is real and fires on essentially every real build using `src/configs/resources.yaml` — `resources.yaml`'s `type` key is *required* by `resources.schema.json` (`required: [name, type]`) but has no `ClusterConfig` destination (per `xcecommon.mapping`'s documented decision not to guess the `valtype`/`ordtype`/`flags` integer encoding), so every valid resource entry triggers a "field 'type' ... ignored" warning unconditionally, not just for unusual input. This is a disclosed spec tradeoff, not a bug — flagged here so a future pass over `xceadmin.build`'s warning UX (e.g. only warning once per field-name per run instead of once per item) knows the noise is expected, not a regression. `xceadmin.build`'s unmapped-field table (`_UNMAPPED_BUT_VALID_ITEM_KEYS`) was derived by cross-referencing `xcecommon.mapping`'s per-item builder functions against the real `src/configs/schema/*.schema.json` property lists directly (not from prose alone) to avoid drift. diff --git a/src/xceadmin/src/xceadmin/build.py b/src/xceadmin/src/xceadmin/build.py new file mode 100644 index 0000000..d26dd5b --- /dev/null +++ b/src/xceadmin/src/xceadmin/build.py @@ -0,0 +1,135 @@ +"""``config build`` orchestration for xceadmin (design.md, ``xceadmin.build``; +Requirements 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.9, 8.3, 8.4). + +Pipeline: scan a configs directory for the 7 known YAML files -> validate +each present file against its bundled schema -> run the cross-file +business-rule checks -> map validated data to ``ClusterConfig`` -> return +the serialized bytes. Unrecognized-but-schema-valid fields and undefined +cross-references are printed to stderr as warnings, never raised (1.7, +4.4) -- only schema violations, reserved names, and duplicates are hard +failures (``ValidationError``/``ReservedNameError``/``DuplicateEntryError``, +all subclassing ``XceAdminError``). + +No ``click``/CLI dependency here -- ``xceadmin.cli`` (task 4.1) is a thin +wrapper around :func:`build_cluster_config`/:func:`write_output` that owns +argument parsing and the actual "print SHA-256 to stderr" step (1.4); this +module's job is the pipeline and its warnings only. +""" + +from __future__ import annotations + +import hashlib +import sys +from pathlib import Path + +from xcecommon import mapping, validation + +#: Schema-valid YAML fields that intentionally have no ClusterConfig +#: destination (design.md's ``xcecommon.mapping`` module docstring, +#: "Deliberately Unmapped Fields") -- cross-checked field-by-field against +#: the real bundled JSON schemas (``src/configs/schema/*.schema.json``) so +#: this table can't silently drift from what schema validation actually +#: allows. Keyed by SECTION_MAP's ``capnp_field`` (not ``yaml_file``, +#: since ``hosts.yaml`` feeds two fields with two different item shapes). +_UNMAPPED_BUT_VALID_ITEM_KEYS: dict[str, frozenset[str]] = { + "resources": frozenset({"type", "order", "consumable", "release", "resourcemap"}), + "queues": frozenset( + { + "backfill", + "cpulimit", + "description", + "exclusive", + "fairshare", + "gpu_req", + "job_starter", + "memlimit", + "nice", + "preemption", + "rerunable", + "reservation", + "runlimit", + "runtimeest", + } + ), + "hostConfigs": frozenset({"description"}), + # ugroups, hgroups, hproviders, limits: every schema-valid key is + # mapped (mapping.py docstring) -- no entry needed. +} + +#: The 7 known input filenames, derived from SECTION_MAP rather than +#: hardcoded a second time -- scanning is closed-set, not an open-ended +#: directory walk (1.1). +_KNOWN_YAML_FILES: tuple[str, ...] = tuple( + dict.fromkeys(spec["yaml_file"] for spec in mapping.SECTION_MAP) +) + + +def _warn(message: str) -> None: + print(f"warning: {message}", file=sys.stderr) + + +def _warn_unrecognized_fields(sections: dict[str, dict]) -> None: + """Requirement 1.7: a field that passes JSON Schema validation but has + no ``ClusterConfig`` destination is a warning, not a failure. + """ + for spec in mapping.SECTION_MAP: + if spec["kind"] != "list": + continue + unmapped_keys = _UNMAPPED_BUT_VALID_ITEM_KEYS.get(spec["capnp_field"]) + if not unmapped_keys: + continue + schema_name = spec["yaml_file"].removesuffix(".yaml") + items = (sections.get(schema_name) or {}).get(spec["yaml_key"]) or [] + for item in items: + present = unmapped_keys & item.keys() + for key in sorted(present): + item_name = item.get("name") or item.get("hostname") or item.get("groupname") + _warn( + f"{spec['yaml_file']}: field '{key}' on '{item_name}' is schema-valid " + f"but has no ClusterConfig destination -- ignored" + ) + + +def _read_sections(configs_dir: str) -> dict[str, dict]: + """Read and validate every present known YAML file. A missing file + becomes an empty section, not an error (1.5). + """ + configs_path = Path(configs_dir) + sections: dict[str, dict] = {} + for yaml_file in _KNOWN_YAML_FILES: + file_path = configs_path / yaml_file + if not file_path.is_file(): + continue + schema_name = yaml_file.removesuffix(".yaml") + sections[schema_name] = validation.validate_yaml(str(file_path), schema_name) + return sections + + +def build_cluster_config(configs_dir: str) -> bytes: + """Full pipeline: validate all present sections, map to ``ClusterConfig``, + serialize. Raises ``ValidationError``/``ReservedNameError``/ + ``DuplicateEntryError`` on hard failures (all subclass ``XceAdminError``). + """ + sections = _read_sections(configs_dir) + + validation.check_reserved_names(sections) + validation.check_duplicates(sections) + for warning in validation.check_undefined_refs(sections): + _warn(warning) + _warn_unrecognized_fields(sections) + + target = mapping.new_cluster_config_builder() + mapping.map_all_sections(sections, target) + return target.to_bytes() + + +def write_output(data: bytes, output_path: str) -> str: + """Write ``data`` to ``output_path`` (``"-"`` means stdout). Returns + the hex SHA-256 digest of ``data`` -- printing it (1.4) is the CLI + layer's job, not this function's. + """ + if output_path == "-": + sys.stdout.buffer.write(data) + else: + Path(output_path).write_bytes(data) + return hashlib.sha256(data).hexdigest() diff --git a/src/xceadmin/tests/test_config_build.py b/src/xceadmin/tests/test_config_build.py new file mode 100644 index 0000000..93f850a --- /dev/null +++ b/src/xceadmin/tests/test_config_build.py @@ -0,0 +1,261 @@ +"""Golden-file-style build tests for ``xceadmin.build`` (design.md, +``xceadmin.build``; Requirements 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.9, +8.3, 8.4). +""" + +from __future__ import annotations + +import hashlib +from pathlib import Path + +import pytest +import yaml + +from xcecommon.errors import DuplicateEntryError, ReservedNameError, ValidationError +from xcecommon.schema import load_schema +from xceadmin import build + +REPO_CONFIGS_DIR = Path(__file__).resolve().parents[2] / "configs" + + +def _write_yaml(path: Path, data: dict) -> None: + path.write_text(yaml.safe_dump(data)) + + +# --------------------------------------------------------------------------- +# 1.1 / 1.5: closed-set scan, missing file -> empty section +# --------------------------------------------------------------------------- + + +def test_build_missing_all_files_still_succeeds(tmp_path): + """Observable (task 2.1's bullet, reused here for build): a configs + directory with none of the 7 known files present still builds -- + every section is empty, not an error. + """ + data = build.build_cluster_config(str(tmp_path)) + mod = load_schema() + with mod.ClusterConfig.from_bytes(data) as reader: + assert reader.clusterName == "" + assert len(reader.queues) == 0 + + +def test_build_ignores_unrecognized_extra_files(tmp_path): + (tmp_path / "notes.txt").write_text("not a config file") + (tmp_path / "schema").mkdir() # an operator-supplied schema/ dir -- ignored (1.6) + (tmp_path / "schema" / "queues.schema.json").write_text("{}") + + data = build.build_cluster_config(str(tmp_path)) + assert isinstance(data, bytes) + + +def test_build_missing_one_file_matches_present_empty_section(tmp_path): + _write_yaml(tmp_path / "xce.yaml", {"cluster_name": "c1", "heartbeat_interval": 5, + "inactive_heartbeat_count": 2}) + without_resources = build.build_cluster_config(str(tmp_path)) + + _write_yaml(tmp_path / "resources.yaml", {"resources": []}) + with_empty_resources = build.build_cluster_config(str(tmp_path)) + + assert without_resources == with_empty_resources + + +# --------------------------------------------------------------------------- +# 1.9 / determinism: same input twice -> identical bytes +# --------------------------------------------------------------------------- + + +def test_build_same_input_twice_is_byte_identical(tmp_path): + _write_yaml(tmp_path / "xce.yaml", {"cluster_name": "eda1", "heartbeat_interval": 30, + "inactive_heartbeat_count": 3}) + _write_yaml(tmp_path / "queues.yaml", {"queues": [{"name": "normal", "priority": 30}]}) + + first = build.build_cluster_config(str(tmp_path)) + second = build.build_cluster_config(str(tmp_path)) + assert first == second + + +# --------------------------------------------------------------------------- +# Hard failures propagate (schema violation, reserved name, duplicates) +# --------------------------------------------------------------------------- + + +def test_build_raises_validation_error_on_schema_violation(tmp_path): + _write_yaml(tmp_path / "queues.yaml", {"queues": [{"priority": 30}]}) # missing "name" + + with pytest.raises(ValidationError): + build.build_cluster_config(str(tmp_path)) + + +def test_build_raises_reserved_name_error(tmp_path): + _write_yaml(tmp_path / "queues.yaml", {"queues": [{"name": "lost_and_found", "priority": 1}]}) + + with pytest.raises(ReservedNameError): + build.build_cluster_config(str(tmp_path)) + + +def test_build_raises_duplicate_entry_error(tmp_path): + _write_yaml( + tmp_path / "queues.yaml", + {"queues": [{"name": "dup", "priority": 1}, {"name": "dup", "priority": 2}]}, + ) + + with pytest.raises(DuplicateEntryError): + build.build_cluster_config(str(tmp_path)) + + +# --------------------------------------------------------------------------- +# 1.7: unrecognized-but-schema-valid fields warn, never fail +# --------------------------------------------------------------------------- + + +def test_build_warns_not_fails_on_schema_valid_unmapped_resource_fields(tmp_path, capsys): + _write_yaml( + tmp_path / "resources.yaml", + { + "resources": [ + { + "name": "gpus", + "type": "numeric", + "order": "increase", + "consumable": True, + "release": False, + } + ] + }, + ) + + data = build.build_cluster_config(str(tmp_path)) # must not raise + + mod = load_schema() + with mod.ClusterConfig.from_bytes(data) as reader: + assert reader.resources[0].name == "gpus" + + stderr = capsys.readouterr().err + assert "resources.yaml" in stderr + assert "'type'" in stderr + assert "'order'" in stderr + assert "'consumable'" in stderr + assert "'release'" in stderr + assert "gpus" in stderr + + +def test_build_warns_on_schema_valid_unmapped_queue_fields(tmp_path, capsys): + _write_yaml( + tmp_path / "queues.yaml", + {"queues": [{"name": "night", "priority": 30, "nice": 10, "fairshare": [{"user": "u1", "share": 5}]}]}, + ) + + build.build_cluster_config(str(tmp_path)) + + stderr = capsys.readouterr().err + assert "'nice'" in stderr + assert "'fairshare'" in stderr + assert "night" in stderr + + +def test_build_warns_on_schema_valid_unmapped_host_description(tmp_path, capsys): + _write_yaml( + tmp_path / "hosts.yaml", + {"hosts": [{"hostname": "h1", "description": "a worker node"}], "hostgroups": []}, + ) + + build.build_cluster_config(str(tmp_path)) + + stderr = capsys.readouterr().err + assert "'description'" in stderr + assert "h1" in stderr + + +def test_build_no_warnings_when_only_mapped_fields_present(tmp_path, capsys): + _write_yaml(tmp_path / "scaler.yaml", {"scalers": [{"name": "s1", "type": "script", "script_path": "/x.sh"}]}) + + build.build_cluster_config(str(tmp_path)) + + assert capsys.readouterr().err == "" + + +# --------------------------------------------------------------------------- +# 4.4 undefined-ref warnings surface without blocking the build +# --------------------------------------------------------------------------- + + +def test_build_surfaces_undefined_ref_warnings_without_failing(tmp_path, capsys): + _write_yaml( + tmp_path / "queues.yaml", + {"queues": [{"name": "q1", "priority": 1, "hosts": {"members": ["nonexistent_hg"]}}]}, + ) + _write_yaml(tmp_path / "hosts.yaml", {"hosts": [], "hostgroups": []}) + + data = build.build_cluster_config(str(tmp_path)) # must not raise + assert isinstance(data, bytes) + + stderr = capsys.readouterr().err + assert "nonexistent_hg" in stderr + + +# --------------------------------------------------------------------------- +# Real src/configs/*.yaml fixtures +# --------------------------------------------------------------------------- + + +@pytest.mark.skipif(not REPO_CONFIGS_DIR.is_dir(), reason="src/configs/ fixtures not present") +def test_build_real_repo_configs_succeeds_and_warns_on_known_gaps(capsys): + data = build.build_cluster_config(str(REPO_CONFIGS_DIR)) + + mod = load_schema() + with mod.ClusterConfig.from_bytes(data) as reader: + assert reader.clusterName == "eda1" + + # src/configs/hosts.yaml has 2 pre-existing undefined refs (resource + # "mg", host "h3" -- see task 2.1's Implementation Notes) and every + # resources.yaml entry has a schema-required "type" field with no + # ClusterConfig destination -- both are expected warnings, not errors. + stderr = capsys.readouterr().err + assert "warning:" in stderr + + +@pytest.mark.skipif(not REPO_CONFIGS_DIR.is_dir(), reason="src/configs/ fixtures not present") +def test_write_output_real_repo_configs_hash_matches_manual_sha256(tmp_path): + data = build.build_cluster_config(str(REPO_CONFIGS_DIR)) + out_path = tmp_path / "cluster.bin" + + digest = build.write_output(data, str(out_path)) + + assert out_path.read_bytes() == data + assert digest == hashlib.sha256(data).hexdigest() + + +# --------------------------------------------------------------------------- +# write_output(): file path, stdout, and hash correctness +# --------------------------------------------------------------------------- + + +def test_write_output_writes_file_and_returns_correct_hash(tmp_path): + data = b"some cluster.bin bytes" + out_path = tmp_path / "out.bin" + + digest = build.write_output(data, str(out_path)) + + assert out_path.read_bytes() == data + assert digest == hashlib.sha256(data).hexdigest() + + +def test_write_output_dash_writes_to_stdout_buffer(capsys): + data = b"stdout-bound bytes" + + digest = build.write_output(data, "-") + + captured = capsys.readouterr() + assert captured.out.encode() == data + assert digest == hashlib.sha256(data).hexdigest() + + +def test_write_output_does_not_print_the_hash_itself(tmp_path, capsys): + """1.4's "print SHA-256 to stderr" is the CLI layer's job (task 4.1) -- + write_output only returns the digest, it never prints anything. + """ + data = b"quiet" + build.write_output(data, str(tmp_path / "out.bin")) + + captured = capsys.readouterr() + assert captured.err == ""