Merge pull request #142 from amd/alex_amdsmi

AmdSmiPlugin: enhancement

Author: alexandraBara

nodescraper/plugins/inband/amdsmi/amdsmi_analyzer.py

@@ -35,7 +35,6 @@
     AmdSmiDataModel,
     AmdSmiMetric,
     AmdSmiStatic,
-    AmdSmiTstData,
     EccData,
     Fw,
     Partition,
@@ -47,7 +46,7 @@
 
 
 class AmdSmiAnalyzer(CperAnalysisTaskMixin, DataAnalyzer[AmdSmiDataModel, None]):
-    """Check AMD SMI Application data for PCIe, ECC errors, CPER data, and analyze amdsmitst metrics"""
+    """Check AMD SMI Application data for PCIe, ECC errors, and CPER data."""
 
     DATA_MODEL = AmdSmiDataModel
 
@@ -667,6 +666,9 @@ def check_expected_xgmi_link_speed(
             )
             return
 
+        expected_str = ", ".join(str(s) for s in expected_xgmi_speed)
+        mismatches: list[dict] = []
+
         for xgmi_data in xgmi_metric:
             link_metric = xgmi_data.link_metrics
             try:
@@ -700,32 +702,26 @@ def check_expected_xgmi_link_speed(
                 continue
 
             if xgmi_float not in expected_xgmi_speed:
-                self._log_event(
-                    category=EventCategory.IO,
-                    description="XGMI link speed is not as expected",
-                    priority=EventPriority.ERROR,
-                    data={
+                mismatches.append(
+                    {
                         "gpu": xgmi_data.gpu,
-                        "xgmi_bit_rate": xgmi_float,
-                        "expected_xgmi_speed": expected_xgmi_speed,
-                    },
-                    console_log=True,
+                        "actual_gt_s": xgmi_float,
+                        "expected_gt_s": expected_str,
+                    }
                 )
 
-    def check_amdsmitst(self, amdsmitst_data: AmdSmiTstData):
-        """Check AMD SMI test results
-
-        Args:
-            amdsmitst_data (AmdSmiTstData): AMD SMI test data
-        """
-        if amdsmitst_data.failed_test_count > 0:
+        if mismatches:
+            details = "; ".join(
+                f"GPU {m['gpu']} {m['actual_gt_s']} GT/s (expected {m['expected_gt_s']})"
+                for m in mismatches
+            )
             self._log_event(
-                category=EventCategory.APPLICATION,
-                description=f"{amdsmitst_data.failed_test_count} failed tests running amdsmitst",
+                category=EventCategory.IO,
+                description=f"XGMI link speed is not as expected: {details}",
                 priority=EventPriority.ERROR,
                 data={
-                    "failed_test_count": amdsmitst_data.failed_test_count,
-                    "failed_tests": amdsmitst_data.failed_tests,
+                    "expected_gt_s": expected_str,
+                    "mismatches": mismatches,
                 },
                 console_log=True,
             )
@@ -815,7 +811,4 @@ def analyze_data(
                 data.xgmi_metric, expected_xgmi_speed=args.expected_xgmi_speed
             )
 
-        if data.amdsmitst_data and data.amdsmitst_data.failed_test_count > 0:
-            self.check_amdsmitst(data.amdsmitst_data)
-
         return self.result

nodescraper/plugins/inband/amdsmi/amdsmi_collector.py

@@ -29,7 +29,7 @@
 from tarfile import TarFile
 from typing import Any, Dict, List, Optional, Union
 
-from pydantic import ValidationError
+from pydantic import BaseModel, ValidationError
 
 from nodescraper.base.inbandcollectortask import InBandDataCollector
 from nodescraper.enums import EventCategory, EventPriority, ExecutionStatus, OSFamily
@@ -38,8 +38,10 @@
 from nodescraper.plugins.inband.amdsmi.amdsmidata import (
     AmdSmiDataModel,
     AmdSmiListItem,
+    AmdSmiMetric,
     AmdSmiStatic,
     AmdSmiVersion,
+    BadPages,
     EccState,
     Fw,
     FwListItem,
@@ -65,7 +67,10 @@
     StaticVbios,
     StaticVram,
     StaticXgmiPlpd,
+    Topo,
     ValueUnit,
+    XgmiLinks,
+    XgmiMetrics,
 )
 from nodescraper.plugins.inband.amdsmi.collector_args import AmdSmiCollectorArgs
 from nodescraper.utils import get_exception_traceback
@@ -87,6 +92,11 @@ class AmdSmiCollector(InBandDataCollector[AmdSmiDataModel, AmdSmiCollectorArgs])
     CMD_FIRMWARE = "firmware --json"
     CMD_STATIC = "static -g all --json"
     CMD_STATIC_GPU = "static -g {gpu_id} --json"
+    CMD_TOPOLOGY = "topology"
+    CMD_METRIC = "metric -g all"
+    CMD_BAD_PAGES = "bad-pages"
+    CMD_XGMI_METRIC = "xgmi -m"
+    CMD_XGMI_LINK = "xgmi -l"
     CMD_RAS = "ras --cper --folder={folder}"
     CMD_RAS_AFID = "ras --afid --cper-file {cper_file}"
 
@@ -317,10 +327,125 @@ def _normalize(self, val: object, default: str = "unknown", slot_type: bool = Fa
             if u == "CEM":
                 return "CEM"
             return "Unknown"
-
         return s
 
-    def _get_amdsmi_data(self) -> Optional[AmdSmiDataModel]:
+    def _build_amdsmi_sub_data(
+        self,
+        model_class: type[BaseModel],
+        json_data: Optional[Union[dict, list]],
+        *,
+        model_name: Optional[str] = None,
+    ) -> Optional[Union[list, Any]]:
+        """Build list or single instance from amd-smi JSON using a Pydantic model.
+
+        Args:
+            model_class: Pydantic model class (e.g. Topo, BadPages, AmdSmiMetric).
+            json_data: Raw dict or list from amd-smi --json.
+            model_name: Optional name for logging (defaults to model_class.__name__).
+
+        Returns:
+            List of model instances, single instance, or None on error.
+        """
+        name = model_name or model_class.__name__
+        if json_data is None:
+            return None
+        try:
+            if isinstance(json_data, list):
+                out: List[Any] = []
+                for item in json_data:
+                    if not isinstance(item, dict):
+                        continue
+                    try:
+                        out.append(model_class.model_validate(item))
+                    except ValidationError as err:
+                        self._log_event(
+                            category=EventCategory.APPLICATION,
+                            description=f"Failed to build {name} entry; skipping",
+                            data={
+                                "errors": err.errors(include_url=False),
+                                "item_keys": list(item.keys()),
+                            },
+                            priority=EventPriority.WARNING,
+                        )
+                return out
+            if isinstance(json_data, dict):
+                return model_class.model_validate(json_data)
+            return None
+        except ValidationError as err:
+            self._log_event(
+                category=EventCategory.APPLICATION,
+                description=f"Failed to build {name}",
+                data={"errors": err.errors(include_url=False)},
+                priority=EventPriority.WARNING,
+            )
+            return None
+
+    def get_topology(self) -> Optional[List[Topo]]:
+        """Get topology from amd-smi topology --json."""
+        ret = self._run_amd_smi_dict(self.CMD_TOPOLOGY)
+        if ret is None:
+            return []
+        if isinstance(ret, dict) and "gpu_data" in ret:
+            ret = ret["gpu_data"]
+        data = ret if isinstance(ret, list) else [ret]
+        built = self._build_amdsmi_sub_data(Topo, data)
+        return built if isinstance(built, list) else ([built] if built else [])
+
+    def get_bad_pages(self) -> Optional[List[BadPages]]:
+        """Get bad pages from amd-smi bad-pages --json."""
+        ret = self._run_amd_smi_dict(self.CMD_BAD_PAGES)
+        if ret is None:
+            return []
+        data = ret if isinstance(ret, list) else [ret]
+        built = self._build_amdsmi_sub_data(BadPages, data)
+        return built if isinstance(built, list) else ([built] if built else [])
+
+    def get_metric(self) -> Optional[List[AmdSmiMetric]]:
+        """Get metrics from amd-smi metric -g all --json."""
+        ret = self._run_amd_smi_dict(self.CMD_METRIC)
+        if ret is None:
+            return []
+        if isinstance(ret, dict) and "gpu_data" in ret:
+            ret = ret["gpu_data"]
+        data = ret if isinstance(ret, list) else [ret]
+        built = self._build_amdsmi_sub_data(AmdSmiMetric, data)
+        return built if isinstance(built, list) else ([built] if built else [])
+
+    def get_xgmi_data(
+        self,
+    ) -> tuple[List[XgmiMetrics], List[XgmiLinks]]:
+        """Get XGMI metric and link data from amd-smi xgmi -m and xgmi -l."""
+        xgmi_metric_raw = self._run_amd_smi_dict(self.CMD_XGMI_METRIC)
+        xgmi_metrics: Optional[List[XgmiMetrics]] = []
+        if xgmi_metric_raw is not None:
+            if isinstance(xgmi_metric_raw, dict) and "xgmi_metric" in xgmi_metric_raw:
+                xgmi_metric_raw = xgmi_metric_raw["xgmi_metric"]
+            if isinstance(xgmi_metric_raw, list) and len(xgmi_metric_raw) == 1:
+                xgmi_metric_raw = xgmi_metric_raw[0]
+            data_m = (
+                xgmi_metric_raw
+                if isinstance(xgmi_metric_raw, list)
+                else ([xgmi_metric_raw] if isinstance(xgmi_metric_raw, dict) else [])
+            )
+            built_m = self._build_amdsmi_sub_data(XgmiMetrics, data_m)
+            xgmi_metrics = built_m if isinstance(built_m, list) else ([built_m] if built_m else [])
+
+        xgmi_link_raw = self._run_amd_smi_dict(self.CMD_XGMI_LINK)
+        xgmi_links: Optional[List[XgmiLinks]] = []
+        if isinstance(xgmi_link_raw, dict) and "link_status" in xgmi_link_raw:
+            link_list = xgmi_link_raw.get("link_status")
+            if isinstance(link_list, list):
+                xgmi_links = self._build_amdsmi_sub_data(XgmiLinks, link_list)
+                xgmi_links = xgmi_links if isinstance(xgmi_links, list) else []
+        elif isinstance(xgmi_link_raw, list):
+            xgmi_links = self._build_amdsmi_sub_data(XgmiLinks, xgmi_link_raw)
+            xgmi_links = xgmi_links if isinstance(xgmi_links, list) else []
+
+        return xgmi_metrics or [], xgmi_links or []
+
+    def _get_amdsmi_data(
+        self, args: Optional[AmdSmiCollectorArgs] = None
+    ) -> Optional[AmdSmiDataModel]:
         """Fill in information for AmdSmi data model
 
         Returns:
@@ -333,6 +458,10 @@ def _get_amdsmi_data(self) -> Optional[AmdSmiDataModel]:
             firmware = self.get_firmware()
             gpu_list = self.get_gpu_list()
             statics = self.get_static()
+            topology = self.get_topology()
+            metric = self.get_metric()
+            bad_pages = self.get_bad_pages()
+            xgmi_metric, xgmi_link = self.get_xgmi_data()
             cper_data, cper_afids = self.get_cper_data()
         except Exception as e:
             self._log_event(
@@ -353,6 +482,11 @@ def _get_amdsmi_data(self) -> Optional[AmdSmiDataModel]:
                 partition=partition,
                 firmware=firmware,
                 static=statics,
+                topology=topology or [],
+                metric=metric or [],
+                bad_pages=bad_pages or [],
+                xgmi_metric=xgmi_metric or [],
+                xgmi_link=xgmi_link or [],
                 cper_data=cper_data,
                 cper_afids=cper_afids,
             )
@@ -1348,7 +1482,7 @@ def collect_data(
                 self.logger.info("amd-smi version: %s", version.version)
                 self.logger.info("ROCm version: %s", version.rocm_version)
 
-            amd_smi_data = self._get_amdsmi_data()
+            amd_smi_data = self._get_amdsmi_data(args)
 
             if amd_smi_data is None:
                 return self.result, None

nodescraper/plugins/inband/amdsmi/amdsmidata.py

@@ -471,10 +471,19 @@ class PageData(BaseModel):
     value: Optional[int]
 
 
+def _bad_pages_retired_list(v: object) -> list[PageData]:
+    """Coerce 'No bad pages found.' to empty list."""
+    if v == "No bad pages found.":
+        return []
+    return v  # type: ignore[return-value]
+
+
 class BadPages(BaseModel):
     gpu: int
     retired: list[PageData]
 
+    _retired_validator = field_validator("retired", mode="before")(_bad_pages_retired_list)
+
 
 # Metric Data
 class MetricUsage(BaseModel):
@@ -653,6 +662,8 @@ class MetricThrottleVu(BaseModel):
     value: Optional[dict[str, list[Union[int, str]]]] = Field(deprecated=True, default=None)
     unit: str = Field(deprecated=True, default="")
 
+    _value_na = field_validator("value", mode="before")(na_to_none)
+
 
 class MetricThrottle(AmdSmiBaseModel):
     accumulation_counter: Optional[Union[MetricThrottleVu, ValueUnit]] = None
@@ -806,6 +817,7 @@ class LinkStatusTable(Enum):
     UP = "U"
     DOWN = "D"
     DISABLED = "X"
+    SELF = "SELF"
 
 
 class BiDirectionalTable(Enum):
@@ -915,17 +927,6 @@ class Topo(BaseModel):
     links: list[TopoLink]
 
 
-class AmdSmiTstData(BaseModel):
-    "Summary of amdsmitst results, with list and count of passing/skipped/failed tests"
-
-    passed_tests: list[str] = Field(default_factory=list)
-    skipped_tests: list[str] = Field(default_factory=list)
-    failed_tests: list[str] = Field(default_factory=list)
-    passed_test_count: int = 0
-    skipped_test_count: int = 0
-    failed_test_count: int = 0
-
-
 class AmdSmiDataModel(DataModel):
     """Data model for amd-smi data.
 
@@ -955,7 +956,6 @@ class AmdSmiDataModel(DataModel):
     xgmi_link: Optional[list[XgmiLinks]] = Field(default_factory=list)
     cper_data: Optional[list[FileModel]] = Field(default_factory=list)
     cper_afids: dict[str, int] = Field(default_factory=dict)
-    amdsmitst_data: AmdSmiTstData = Field(default_factory=AmdSmiTstData)
 
     def get_list(self, gpu: int) -> Optional[AmdSmiListItem]:
         """Get the gpu list item for the given gpu id."""