diff --git a/nodescraper/plugins/inband/amdsmi/amdsmi_collector.py b/nodescraper/plugins/inband/amdsmi/amdsmi_collector.py index 67bda30b..5dd4b599 100644 --- a/nodescraper/plugins/inband/amdsmi/amdsmi_collector.py +++ b/nodescraper/plugins/inband/amdsmi/amdsmi_collector.py @@ -91,6 +91,42 @@ T = TypeVar("T", bound=BaseModel) +_AFID_ABSENT = {"-", "n/a", "na"} +# First integer in the AFID column. The file name can contain digits. +_AFID_VALUE_RE = re.compile(r"\d+") + + +def _parse_afid_output(output: str) -> Optional[int]: + """Return the AFID from amd-smi output, or None when the value is absent. + + Args: + output: stdout from ras --afid --cper-file. + + Returns: + The AFID integer, or None for a dash or N/A. + """ + lines = [line.strip() for line in output.splitlines() if line.strip()] + if not lines: + raise ValueError("empty AFID output") + header_index = next( + (index for index, line in enumerate(lines) if line.lower().startswith("file_name")), + None, + ) + if header_index is None: + data_line = lines[0] + elif header_index + 1 >= len(lines): + raise ValueError("AFID table has no data row") + else: + data_line = lines[header_index + 1] + tokens = data_line.split() + cell = tokens[0] if len(tokens) == 1 else " ".join(tokens[1:]) + if cell.lower() in _AFID_ABSENT: + return None + match = _AFID_VALUE_RE.search(cell) + if match is None: + raise ValueError(f"no AFID in {cell!r}") + return int(match.group(0)) + class AmdSmiCollector(InBandDataCollector[AmdSmiDataModel, AmdSmiCollectorArgs]): """Class for collection of inband tool amd-smi data.""" @@ -208,6 +244,7 @@ def _run_amd_smi(self, cmd: str) -> Optional[str]: "command": cmd, "exit_code": cmd_ret.exit_code, "stderr": cmd_ret.stderr, + "stdout": cmd_ret.stdout, }, priority=EventPriority.WARNING, console_log=True, @@ -221,6 +258,7 @@ def _run_amd_smi(self, cmd: str) -> Optional[str]: "command": cmd, "exit_code": cmd_ret.exit_code, "stderr": cmd_ret.stderr, + "stdout": cmd_ret.stdout, }, priority=EventPriority.ERROR, console_log=True, @@ -266,7 +304,8 @@ def _run_amd_smi_dict(self, cmd: str) -> Optional[Union[dict, list[dict]]]: Returns: Optional[Union[dict, list[dict]]]: parsed JSON output or None on error """ - cmd += " --json" + if "--json" not in cmd.split(): + cmd += " --json" cmd_ret = self._run_amd_smi(cmd) if cmd_ret: try: @@ -1312,7 +1351,13 @@ def get_static(self) -> Optional[list[AmdSmiStatic]]: ) # NUMA - numa_node = numa.get("node") + numa_node_raw = numa.get("node") + if numa_node_raw in (None, "", "N/A"): + numa_node: Union[int, str] = "N/A" + elif isinstance(numa_node_raw, str): + numa_node = numa_node_raw + else: + numa_node = int(numa_node_raw) if numa_node_raw is not None else "N/A" affinity_raw = numa.get("affinity") if affinity_raw in (None, "", "N/A"): affinity_val: Union[int, str] = "N/A" @@ -1883,24 +1928,33 @@ def _get_cper_afid(self, cper_file_path: str) -> Optional[int]: return None try: - afid = int(result.strip()) + afid = _parse_afid_output(result) + except ValueError: self._log_event( category=EventCategory.APPLICATION, - description=f"Successfully retrieved AFID from CPER file: {cper_file_path}", - data={"afid": afid, "cper_file": cper_file_path}, - priority=EventPriority.INFO, + description=f"Failed to parse AFID value from output: {result}", + data={"output": result, "cper_file": cper_file_path}, + priority=EventPriority.ERROR, console_log=True, ) - return afid - except ValueError: + return None + if afid is None: self._log_event( category=EventCategory.APPLICATION, - description=f"Failed to parse AFID value from output: {result}", + description=f"No AFID reported for CPER file: {cper_file_path}", data={"output": result, "cper_file": cper_file_path}, - priority=EventPriority.ERROR, + priority=EventPriority.INFO, console_log=True, ) return None + self._log_event( + category=EventCategory.APPLICATION, + description=f"Successfully retrieved AFID from CPER file: {cper_file_path}", + data={"afid": afid, "cper_file": cper_file_path}, + priority=EventPriority.INFO, + console_log=True, + ) + return afid def collect_data( self, diff --git a/nodescraper/plugins/inband/amdsmi/amdsmidata.py b/nodescraper/plugins/inband/amdsmi/amdsmidata.py index 41070935..9b43cf00 100644 --- a/nodescraper/plugins/inband/amdsmi/amdsmidata.py +++ b/nodescraper/plugins/inband/amdsmi/amdsmidata.py @@ -523,12 +523,10 @@ class StaticXgmiPlpd(BaseModel): class StaticNuma(BaseModel): - model_config = ConfigDict(extra="allow") - node: Optional[int] + node: Union[int, str] # can be N/A affinity: Union[int, str] # can be N/A - na_validator = field_validator("node", mode="before")(na_to_none) class StaticVram(AmdSmiBaseModel): @@ -1286,6 +1284,21 @@ def clock_rails(self) -> MetricClockRails: """Enumerated view of clock catching a dropped/renamed known rail.""" return MetricClockRails.model_validate(self.clock or {}) + @model_validator(mode="before") + @classmethod + def _normalize_metric_input(cls, value: object) -> object: + """Normalize one metric object before field validation. + + Args: + value: Raw metric input. + + Returns: + The input, with per-block clocks and the PCIe count alias normalized. + """ + if isinstance(value, dict): + return normalize_amdsmi_metric_dict(value) + return value + @field_validator("ecc_blocks", mode="before") @classmethod def validate_ecc_blocks(cls, value: Union[dict[str, EccData], str]) -> dict[str, EccData]: diff --git a/test/unit/plugin/test_amdsmi_analyzer.py b/test/unit/plugin/test_amdsmi_analyzer.py index 3674ea40..25c0a90f 100644 --- a/test/unit/plugin/test_amdsmi_analyzer.py +++ b/test/unit/plugin/test_amdsmi_analyzer.py @@ -45,6 +45,7 @@ Fw, FwListItem, LinkStatusTable, + MetricClockData, MetricEccTotals, MetricPcie, Partition, @@ -1108,6 +1109,44 @@ def _minimal_amdsmi_metric( ) +def test_metric_accepts_legacy_and_per_block_clocks(): + """Older clock objects and per-block clocks validate together.""" + raw = _minimal_amdsmi_metric().model_dump() + raw["clock"] = { + "GFX_0": { + "clk": {"value": 132, "unit": "MHz"}, + "min_clk": {"value": 500, "unit": "MHz"}, + "max_clk": {"value": 2100, "unit": "MHz"}, + "clk_locked": 0, + "deep_sleep": 0, + }, + "uclk_aid": { + "aid_0": {"value": 1900, "unit": "MHz"}, + "aid_1": {"value": 1900, "unit": "MHz"}, + }, + "socclks_mid": { + "mid_0": {"value": 1350, "unit": "MHz"}, + "mid_1": "N/A", + }, + } + raw["pcie"].pop("lc_perf_other_end_recovery", None) + raw["pcie"]["lc_perf_other_end_recovery_count"] = 4 + + metric = AmdSmiMetric.model_validate(raw) + + assert isinstance(metric.clock["GFX_0"], MetricClockData) + assert metric.clock["GFX_0"].min_clk.value == 500 + assert metric.clock["uclk_aid"]["aid_0"]["value"] == 1900 + assert metric.clock["socclks_mid"]["mid_1"] == "N/A" + assert metric.pcie.lc_perf_other_end_recovery == 4 + + legacy = _minimal_amdsmi_metric() + legacy_raw = legacy.model_dump() + legacy_raw["pcie"]["lc_perf_other_end_recovery"] = 2 + legacy_raw["pcie"]["lc_perf_other_end_recovery_count"] = 9 + assert AmdSmiMetric.model_validate(legacy_raw).pcie.lc_perf_other_end_recovery == 2 + + def test_gpu_mismatch_description_helpers(): """Per-GPU mismatch helpers include GPU id, expected, and actual in output.""" desc, details = _gpu_mismatch_description( diff --git a/test/unit/plugin/test_amdsmi_collector.py b/test/unit/plugin/test_amdsmi_collector.py index d47dd65f..9857880a 100644 --- a/test/unit/plugin/test_amdsmi_collector.py +++ b/test/unit/plugin/test_amdsmi_collector.py @@ -31,8 +31,13 @@ import pytest +from nodescraper.enums.eventpriority import EventPriority +from nodescraper.enums.executionstatus import ExecutionStatus from nodescraper.enums.systeminteraction import SystemInteractionLevel -from nodescraper.plugins.inband.amdsmi.amdsmi_collector import AmdSmiCollector +from nodescraper.plugins.inband.amdsmi.amdsmi_collector import ( + AmdSmiCollector, + _parse_afid_output, +) from nodescraper.plugins.inband.amdsmi.amdsmidata import ( AmdSmiDataModel, ) @@ -523,6 +528,46 @@ def mock_run_sut_cmd(cmd: str) -> MagicMock: assert stat[0].vbios is None +def test_static_numa_node_na(conn_mock, system_info, monkeypatch): + """N/A numa node is kept as a string, the same way affinity is.""" + + def mock_run_sut_cmd(cmd: str) -> MagicMock: + if "which amd-smi" in cmd: + return make_cmd_result("/usr/bin/amd-smi") + if "static -g all --json" in cmd: + return make_cmd_result( + make_json_response( + [ + { + "gpu": 0, + "bus": {"bdf": "0000:0b:00.0"}, + "numa": {"node": "N/A", "affinity": "N/A"}, + }, + { + "gpu": 1, + "bus": {"bdf": "0000:0c:00.0"}, + "numa": {"node": 3, "affinity": 0}, + }, + ] + ) + ) + return make_cmd_result("") + + c = AmdSmiCollector( + system_info=system_info, + system_interaction_level=SystemInteractionLevel.PASSIVE, + connection=conn_mock, + ) + monkeypatch.setattr(c, "_run_sut_cmd", mock_run_sut_cmd) + + stat = c.get_static() + assert stat is not None and len(stat) == 2 + assert stat[0].numa.node == "N/A" + assert stat[0].numa.affinity == "N/A" + assert stat[1].numa.node == 3 + assert stat[1].numa.affinity == 0 + + def test_json_parse_error(conn_mock, system_info, monkeypatch): """Test handling of malformed JSON""" @@ -552,7 +597,7 @@ def test_command_error(conn_mock, system_info, monkeypatch): def mock_cmd_error(cmd: str) -> MagicMock: if "which amd-smi" in cmd: return make_cmd_result("/usr/bin/amd-smi") - return make_cmd_result("", "Command failed", 1) + return make_cmd_result("Command output", "Command failed", 1) c = AmdSmiCollector( system_info=system_info, @@ -566,6 +611,11 @@ def mock_cmd_error(cmd: str) -> MagicMock: assert data.version is None assert data.gpu_list == [] assert len(result.events) > 0 + command_error = next( + event for event in result.events if event.description == "Error running amd-smi command" + ) + assert command_error.data["stdout"] == "Command output" + assert command_error.data["stderr"] == "Command failed" def test_multi_json_parsing(conn_mock, system_info, monkeypatch): @@ -626,6 +676,33 @@ def mock_single_json(cmd: str) -> MagicMock: assert result[0]["tool"] == "amdsmi" +def test_run_amd_smi_dict_appends_json_once(conn_mock, system_info, monkeypatch): + """Every JSON command receives one --json from the runner.""" + seen = [] + + def mock_run_sut_cmd(cmd: str) -> MagicMock: + seen.append(cmd) + return make_cmd_result(make_json_response({"ok": True})) + + c = AmdSmiCollector( + system_info=system_info, + system_interaction_level=SystemInteractionLevel.PASSIVE, + connection=conn_mock, + ) + monkeypatch.setattr(c, "_run_sut_cmd", mock_run_sut_cmd) + + assert c._run_amd_smi_dict(c.CMD_LIST) == {"ok": True} + assert c._run_amd_smi_dict(c.CMD_STATIC) == {"ok": True} + assert c._run_amd_smi_dict(c.CMD_TOPOLOGY) == {"ok": True} + assert c._run_amd_smi_dict("list --json") == {"ok": True} + assert seen == [ + "amd-smi list --json", + "amd-smi static -g all --json", + "amd-smi topology --json", + "amd-smi list --json", + ] + + def test_get_cper_afid_success(conn_mock, system_info, monkeypatch): """Test successful AFID retrieval from CPER file""" @@ -669,6 +746,69 @@ def mock_run_sut_cmd(cmd: str) -> MagicMock: afid = c._get_cper_afid("/path/to/test.cper") assert afid is None + assert any(event.priority == EventPriority.ERROR for event in c.result.events) + + +def test_get_cper_afid_table(conn_mock, system_info, monkeypatch): + """Test AFID retrieval when amd-smi prints a file_name and list of afids table""" + + def mock_run_sut_cmd(cmd: str) -> MagicMock: + if "which amd-smi" in cmd: + return make_cmd_result("/usr/bin/amd-smi") + if "ras --afid --cper-file" in cmd: + return make_cmd_result("file_name list of afids\ncorrected-1.cper 29\n") + return make_cmd_result("") + + c = AmdSmiCollector( + system_info=system_info, + system_interaction_level=SystemInteractionLevel.PASSIVE, + connection=conn_mock, + ) + monkeypatch.setattr(c, "_run_sut_cmd", mock_run_sut_cmd) + + afid = c._get_cper_afid("/path/to/corrected-1.cper") + c.result.finalize() + + assert afid == 29 + assert c.result.status == ExecutionStatus.OK + assert not any(event.priority >= EventPriority.ERROR for event in c.result.events) + + +def test_get_cper_afid_absent(conn_mock, system_info, monkeypatch): + """Test a dash or N/A AFID is recorded as none and does not fail the task""" + + def mock_run_sut_cmd(cmd: str) -> MagicMock: + if "which amd-smi" in cmd: + return make_cmd_result("/usr/bin/amd-smi") + if "ras --afid --cper-file" in cmd: + if "boot.cper" in cmd: + return make_cmd_result("file_name list of afids\nboot.cper N/A\n") + return make_cmd_result("-\n") + return make_cmd_result("") + + c = AmdSmiCollector( + system_info=system_info, + system_interaction_level=SystemInteractionLevel.PASSIVE, + connection=conn_mock, + ) + monkeypatch.setattr(c, "_run_sut_cmd", mock_run_sut_cmd) + + assert c._get_cper_afid("/path/to/boot.cper") is None + assert c._get_cper_afid("/path/to/other.cper") is None + c.result.finalize() + + assert c.result.status == ExecutionStatus.OK + assert not any(event.priority >= EventPriority.ERROR for event in c.result.events) + + +def test_parse_afid_output(): + """Parse a bare AFID, an AFID table, and an absent marker.""" + assert _parse_afid_output("12345\n") == 12345 + assert _parse_afid_output("file_name list of afids\ncorrected-1.cper 29\n") == 29 + assert _parse_afid_output("file_name list of afids\nboot.cper N/A\n") is None + assert _parse_afid_output("-\n") is None + with pytest.raises(ValueError): + _parse_afid_output("not_a_number\n") def test_get_cper_afid_command_failure(conn_mock, system_info, monkeypatch):