AmdSmiPlugin updates and fixes

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,

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]:

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