diff --git a/nodescraper/plugins/inband/amdsmi/amdsmi_collector.py b/nodescraper/plugins/inband/amdsmi/amdsmi_collector.py index 27cec594..13f993a1 100644 --- a/nodescraper/plugins/inband/amdsmi/amdsmi_collector.py +++ b/nodescraper/plugins/inband/amdsmi/amdsmi_collector.py @@ -77,6 +77,42 @@ from nodescraper.plugins.inband.amdsmi.collector_args import AmdSmiCollectorArgs from nodescraper.utils import get_exception_traceback +_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.""" @@ -87,13 +123,13 @@ class AmdSmiCollector(InBandDataCollector[AmdSmiDataModel, AmdSmiCollectorArgs]) DATA_MODEL = AmdSmiDataModel - CMD_VERSION = "version --json" - CMD_LIST = "list --json" - CMD_PROCESS = "process --json" - CMD_PARTITION = "partition --json" - CMD_FIRMWARE = "firmware --json" - CMD_STATIC = "static -g all --json" - CMD_STATIC_GPU = "static -g {gpu_id} --json" + CMD_VERSION = "version" + CMD_LIST = "list" + CMD_PROCESS = "process" + CMD_PARTITION = "partition" + CMD_FIRMWARE = "firmware" + CMD_STATIC = "static -g all" + CMD_STATIC_GPU = "static -g {gpu_id}" CMD_TOPOLOGY = "topology" CMD_METRIC = "metric -g all" CMD_BAD_PAGES = "bad-pages" @@ -144,6 +180,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, @@ -157,6 +194,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, @@ -202,7 +240,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: @@ -979,7 +1018,13 @@ def get_static(self) -> Optional[list[AmdSmiStatic]]: ) # NUMA - numa_node = int(numa.get("node", 0) or 0) + 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" @@ -1496,24 +1541,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 9e3a7950..ff0d75fc 100644 --- a/nodescraper/plugins/inband/amdsmi/amdsmidata.py +++ b/nodescraper/plugins/inband/amdsmi/amdsmidata.py @@ -41,6 +41,58 @@ from nodescraper.utils import find_annotation_in_container _NUM_UNIT_RE = re.compile(r"^\s*([-+]?\d+(?:\.\d+)?)(?:\s*([A-Za-z%/][A-Za-z0-9%/._-]*))?\s*$") +_METRIC_CLOCK_FIELDS = {"clk", "min_clk", "max_clk", "clk_locked", "deep_sleep"} + + +def _as_metric_clock(value: object) -> object: + """Return one clock as MetricClockData input. + + Args: + value: A clock object, a value with a unit, or an absent marker. + + Returns: + The original clock object, or a clock object whose clk is value. + """ + if isinstance(value, dict) and _METRIC_CLOCK_FIELDS.intersection(value): + return value + return { + "clk": value, + "min_clk": None, + "max_clk": None, + "clk_locked": None, + "deep_sleep": None, + } + + +def normalize_amdsmi_metric_dict(data: dict) -> dict: + """Accept older metric JSON and per-block clock and PCIe count fields. + + Args: + data: One amd-smi metric object. + + Returns: + A copy prepared for AmdSmiMetric validation. + """ + out = dict(data) + pcie = out.get("pcie") + if isinstance(pcie, dict): + pcie = dict(pcie) + if "lc_perf_other_end_recovery" not in pcie and "lc_perf_other_end_recovery_count" in pcie: + pcie["lc_perf_other_end_recovery"] = pcie["lc_perf_other_end_recovery_count"] + out["pcie"] = pcie + clock = out.get("clock") + if isinstance(clock, dict): + normalized_clock = {} + for name, value in clock.items(): + if isinstance(value, dict) and value and not _METRIC_CLOCK_FIELDS.intersection(value): + normalized_clock[name] = { + block_name: _as_metric_clock(block_value) + for block_name, block_value in value.items() + } + else: + normalized_clock[name] = value + out["clock"] = normalized_clock + return out def na_to_none(values: Union[int, str]): @@ -393,7 +445,7 @@ class StaticXgmiPlpd(BaseModel): class StaticNuma(BaseModel): - node: int + node: Union[int, str] # can be N/A affinity: Union[int, str] # can be N/A @@ -777,7 +829,7 @@ class AmdSmiMetric(BaseModel): gpu: int usage: MetricUsage power: MetricPower - clock: dict[str, MetricClockData] + clock: dict[str, Union[MetricClockData, dict[str, MetricClockData]]] temperature: MetricTemperature pcie: MetricPcie ecc: MetricEccTotals @@ -792,6 +844,21 @@ class AmdSmiMetric(BaseModel): na_validator = field_validator("xgmi_err", "perf_level", mode="before")(na_to_none) + @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 0ab137a7..e3189622 100644 --- a/test/unit/plugin/test_amdsmi_analyzer.py +++ b/test/unit/plugin/test_amdsmi_analyzer.py @@ -39,6 +39,7 @@ EccState, Fw, FwListItem, + MetricClockData, MetricEccTotals, MetricPcie, Partition, @@ -906,6 +907,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") + 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"].clk.value == 1900 + assert metric.clock["socclks_mid"]["mid_1"].clk is None + 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_check_amdsmi_metric_pcie_width_fail(mock_analyzer): """PCIe width not x16 generates error.""" analyzer = mock_analyzer diff --git a/test/unit/plugin/test_amdsmi_collector.py b/test/unit/plugin/test_amdsmi_collector.py index 16d3e8af..7b975929 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, AmdSmiTstData, @@ -493,6 +498,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""" @@ -522,7 +567,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, @@ -536,6 +581,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): @@ -596,6 +646,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""" @@ -639,6 +716,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):