From e3697c0a1155cd3c7b841a1ceaedf3d3cff5c77a Mon Sep 17 00:00:00 2001 From: Mikhail Chusavitin Date: Mon, 17 Aug 2026 12:50:50 +0300 Subject: [PATCH] fix(pcie): stop misclassifying same-vendor non-GPU devices as GPUs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit matchesGPUVendor (sat_overlay.go) matched any PCIe DeviceClass containing the substring "Controller" instead of the actual GPU classes, so a same-vendor non-GPU device (e.g. an NVIDIA-branded NIC/storage controller) with a degraded PCIe link surfaced as a pcie:gpu: alarm on the Hardware Summary/webui. isGPUDevice (app_format.go) had the same bug via a different path: an "any NVIDIA-vendor device is a GPU" fallback that could inflate the "GPU: N x " audit summary line with non-GPU companion devices (NVSwitch/NVLink bridges, etc). Both now defer to collector.IsGPUClass (exported from the previously unexported isGPUClass), the canonical exact-match classifier already used by amdgpu.go — removing the duplicated/incorrect class checks instead of reimplementing them. Also dedup two more locally-duplicated classifiers within webui (which doesn't import collector by convention): NIC-class matching (pages.go/page_topo.go) and GPU-class matching in page_validate.go now share one copy per package instead of being reimplemented per file. Co-Authored-By: Claude Sonnet 5 --- audit/internal/app/app_format.go | 13 ++++----- audit/internal/app/app_format_test.go | 32 +++++++++++++++++++++++ audit/internal/app/app_test.go | 32 +++++++++++++++++++++++ audit/internal/app/sat_overlay.go | 4 +-- audit/internal/collector/amdgpu.go | 2 +- audit/internal/collector/contract.go | 6 ++++- audit/internal/collector/pcie_identity.go | 2 +- audit/internal/webui/page_topo.go | 11 +++----- audit/internal/webui/page_validate.go | 3 +-- audit/internal/webui/pages.go | 15 +++++++---- 10 files changed, 92 insertions(+), 28 deletions(-) create mode 100644 audit/internal/app/app_format_test.go diff --git a/audit/internal/app/app_format.go b/audit/internal/app/app_format.go index b9ad60e..880884d 100644 --- a/audit/internal/app/app_format.go +++ b/audit/internal/app/app_format.go @@ -134,14 +134,11 @@ func isGPUDevice(dev schema.HardwarePCIeDevice) bool { if dev.VendorID != nil && *dev.VendorID == collector.AspeedVendorID { return false } - class := trimPtr(dev.DeviceClass) - // AMD Instinct / Radeon compute GPUs always carry ProcessingAccelerator or DisplayController. - // Do NOT match AMD vendor alone — CPU chipset PCIe devices share that vendor ID. - if class == "VideoController" || class == "DisplayController" || class == "ProcessingAccelerator" { - return true - } - // NVIDIA devices sometimes expose class values outside the standard GPU set. - return dev.VendorID != nil && *dev.VendorID == collector.NvidiaVendorID + // AMD Instinct / Radeon and NVIDIA compute GPUs always carry VideoController, + // DisplayController, or ProcessingAccelerator. Do NOT match vendor ID alone — + // same-vendor companion devices (NVSwitch/NVLink bridges, CPU chipset PCIe + // functions, ...) share the GPU's vendor ID without being a GPU die. + return collector.IsGPUClass(trimPtr(dev.DeviceClass)) } func formatSystemLine(board schema.HardwareBoard) string { diff --git a/audit/internal/app/app_format_test.go b/audit/internal/app/app_format_test.go new file mode 100644 index 0000000..2bcf159 --- /dev/null +++ b/audit/internal/app/app_format_test.go @@ -0,0 +1,32 @@ +package app + +import ( + "testing" + + "bee/audit/internal/schema" +) + +// TestFormatGPULineIgnoresNonGPUSameVendorDevice guards the bug where +// isGPUDevice treated any NVIDIA-vendor PCIe device as a GPU regardless of +// class (a fallback for "NVIDIA devices sometimes expose class values +// outside the standard GPU set"). That fallback misclassified same-vendor +// companion devices — e.g. NVSwitch/NVLink bridges, or an NVIDIA-branded NIC +// — as compute GPUs, inflating the "GPU: N x " audit summary line. +func TestFormatGPULineIgnoresNonGPUSameVendorDevice(t *testing.T) { + vendor := 0x10de // collector.NvidiaVendorID + bridgeClass := "Bridge" + bridgeModel := "NVSwitch" + gpuClass := "VideoController" + gpuModel := "H100" + + devices := []schema.HardwarePCIeDevice{ + {DeviceClass: &bridgeClass, VendorID: &vendor, Model: &bridgeModel}, + {DeviceClass: &gpuClass, VendorID: &vendor, Model: &gpuModel}, + } + + got := formatGPULine(devices) + want := "GPU: 1 x H100" + if got != want { + t.Fatalf("formatGPULine() = %q, want %q", got, want) + } +} diff --git a/audit/internal/app/app_test.go b/audit/internal/app/app_test.go index 5c35337..d4c3376 100644 --- a/audit/internal/app/app_test.go +++ b/audit/internal/app/app_test.go @@ -391,6 +391,38 @@ func TestWritePCIeGPUStatusesToDBSurfacesLinkSpeedDegradation(t *testing.T) { } } +// TestWritePCIeGPUStatusesToDBIgnoresNonGPUSameVendorDevice guards the bug +// where a PCIe link-speed degradation on a non-GPU device that merely shares +// the GPU's PCI vendor ID (e.g. an NVIDIA-branded NIC/storage/NVLink-bridge +// companion device) was misclassified as a GPU fault and surfaced as a +// pcie:gpu: alarm, because matchesGPUVendor matched any class +// containing the substring "Controller" instead of the actual GPU classes. +func TestWritePCIeGPUStatusesToDBIgnoresNonGPUSameVendorDevice(t *testing.T) { + db, err := OpenComponentStatusDB(filepath.Join(t.TempDir(), "component-status.json")) + if err != nil { + t.Fatal(err) + } + + class := "NetworkController" + vendor := 0x10de // collector.NvidiaVendorID + status := "Warning" + desc := "PCIe link speed degraded: running at Gen1, capable of Gen5" + devices := []schema.HardwarePCIeDevice{ + { + HardwareComponentStatus: schema.HardwareComponentStatus{Status: &status, ErrorDescription: &desc}, + DeviceClass: &class, + VendorID: &vendor, + BDF: strPtr("0000:02:00.0"), + }, + } + + writePCIeGPUStatusesToDB(db, devices) + + if _, ok := db.Get("pcie:gpu:nvidia"); ok { + t.Fatal("expected non-GPU same-vendor device to not write pcie:gpu:nvidia") + } +} + func TestRunNCCLTestsPassesSelectedGPUs(t *testing.T) { t.Parallel() diff --git a/audit/internal/app/sat_overlay.go b/audit/internal/app/sat_overlay.go index 211de80..87f857c 100644 --- a/audit/internal/app/sat_overlay.go +++ b/audit/internal/app/sat_overlay.go @@ -340,9 +340,7 @@ func matchesGPUVendor(dev schema.HardwarePCIeDevice, vendor string) bool { return false } class := strings.TrimSpace(*dev.DeviceClass) - isGPUClass := strings.Contains(class, "Controller") || strings.Contains(class, "Accelerator") || - strings.Contains(class, "Display") || strings.Contains(class, "Video") - if !isGPUClass { + if !collector.IsGPUClass(class) { return false } switch vendor { diff --git a/audit/internal/collector/amdgpu.go b/audit/internal/collector/amdgpu.go index 3ed8abe..1f38eeb 100644 --- a/audit/internal/collector/amdgpu.go +++ b/audit/internal/collector/amdgpu.go @@ -87,7 +87,7 @@ func isAMDGPUDevice(dev schema.HardwarePCIeDevice) bool { if dev.DeviceClass == nil { return false } - return dev.VendorID != nil && *dev.VendorID == AMDVendorID && isGPUClass(strings.TrimSpace(*dev.DeviceClass)) + return dev.VendorID != nil && *dev.VendorID == AMDVendorID && IsGPUClass(strings.TrimSpace(*dev.DeviceClass)) } func queryAMDGPUs() (map[string]amdGPUInfo, error) { diff --git a/audit/internal/collector/contract.go b/audit/internal/collector/contract.go index 0a40ab9..c6236a9 100644 --- a/audit/internal/collector/contract.go +++ b/audit/internal/collector/contract.go @@ -48,7 +48,11 @@ func isNICClass(class string) bool { } } -func isGPUClass(class string) bool { +// IsGPUClass reports whether a normalized PCIe device class (see +// mapPCIeDeviceClass) identifies a GPU/accelerator die itself, as opposed to +// a same-vendor companion device (NIC, storage controller, NVLink bridge, +// audio/USB function, ...) that happens to share the GPU's PCI vendor ID. +func IsGPUClass(class string) bool { switch strings.TrimSpace(class) { case "VideoController", "DisplayController", "ProcessingAccelerator": return true diff --git a/audit/internal/collector/pcie_identity.go b/audit/internal/collector/pcie_identity.go index 12fae74..1101667 100644 --- a/audit/internal/collector/pcie_identity.go +++ b/audit/internal/collector/pcie_identity.go @@ -51,7 +51,7 @@ func shouldProbePCIeSerial(dev schema.HardwarePCIeDevice) bool { return false } class := strings.TrimSpace(*dev.DeviceClass) - return isNICClass(class) || isGPUClass(class) + return isNICClass(class) || IsGPUClass(class) } func queryPCIDeviceSerial(bdf string) string { diff --git a/audit/internal/webui/page_topo.go b/audit/internal/webui/page_topo.go index ee8c69b..f1b6449 100644 --- a/audit/internal/webui/page_topo.go +++ b/audit/internal/webui/page_topo.go @@ -54,14 +54,11 @@ func topoCard(title, body string) string { // instead of importing the package for one classifier). // --------------------------------------------------------------------------- -// isNICDeviceClassDev mirrors the classification logic in hwDescribeNIC -// (pages.go), applied to a single device instead of aggregated counts. +// isNICDeviceClassDev applies isNICDeviceClass (pages.go) to a single device, +// with a MAC-address fallback for devices lspci doesn't classify as NIC. func isNICDeviceClassDev(dev schema.HardwarePCIeDevice) bool { - if dev.DeviceClass != nil { - c := strings.ToLower(strings.TrimSpace(*dev.DeviceClass)) - if c == "ethernetcontroller" || c == "networkcontroller" || strings.Contains(c, "fibrechannel") { - return true - } + if dev.DeviceClass != nil && isNICDeviceClass(*dev.DeviceClass) { + return true } return len(dev.MacAddresses) > 0 } diff --git a/audit/internal/webui/page_validate.go b/audit/internal/webui/page_validate.go index 20b175a..c06f39d 100644 --- a/audit/internal/webui/page_validate.go +++ b/audit/internal/webui/page_validate.go @@ -627,8 +627,7 @@ func validateIsVendorGPU(dev schema.HardwarePCIeDevice, vendor string) bool { if dev.VendorID != nil && *dev.VendorID == pciVendorAspeed { return false } - class := strings.ToLower(validateTrimPtr(dev.DeviceClass)) - isGPUClass := class == "videocontroller" || class == "processingaccelerator" || class == "displaycontroller" + isGPUClass := isGPUDeviceClass(validateTrimPtr(dev.DeviceClass)) switch vendor { case "nvidia": return isGPUClass && dev.VendorID != nil && *dev.VendorID == pciVendorNvidia diff --git a/audit/internal/webui/pages.go b/audit/internal/webui/pages.go index ef7e6d5..1dd8724 100644 --- a/audit/internal/webui/pages.go +++ b/audit/internal/webui/pages.go @@ -560,16 +560,21 @@ func hwDescribePSU(hw schema.HardwareSnapshot) string { return fmt.Sprintf("%d× PSU", n) } +// isNICDeviceClass reports whether a normalized PCIe DeviceClass string +// (see collector.mapPCIeDeviceClass) identifies a NIC/HBA-class device. +// webui does not import collector (see the "Classification helpers" note +// in page_topo.go), so this mirrors collector.isNICClass locally. +func isNICDeviceClass(class string) bool { + c := strings.ToLower(strings.TrimSpace(class)) + return c == "ethernetcontroller" || c == "networkcontroller" || strings.Contains(c, "fibrechannel") +} + // hwDescribeNIC returns a summary like "2× Mellanox ConnectX-6". func hwDescribeNIC(hw schema.HardwareSnapshot) string { counts := map[string]int{} order := []string{} for _, dev := range hw.PCIeDevices { - isNIC := false - if dev.DeviceClass != nil { - c := strings.ToLower(strings.TrimSpace(*dev.DeviceClass)) - isNIC = c == "ethernetcontroller" || c == "networkcontroller" || strings.Contains(c, "fibrechannel") - } + isNIC := dev.DeviceClass != nil && isNICDeviceClass(*dev.DeviceClass) if !isNIC && len(dev.MacAddresses) == 0 { continue }