fix(pcie): stop misclassifying same-vendor non-GPU devices as GPUs
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:<vendor> 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 <model>" 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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
4ac9863353
commit
e3697c0a11
@@ -134,14 +134,11 @@ func isGPUDevice(dev schema.HardwarePCIeDevice) bool {
|
|||||||
if dev.VendorID != nil && *dev.VendorID == collector.AspeedVendorID {
|
if dev.VendorID != nil && *dev.VendorID == collector.AspeedVendorID {
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
class := trimPtr(dev.DeviceClass)
|
// AMD Instinct / Radeon and NVIDIA compute GPUs always carry VideoController,
|
||||||
// AMD Instinct / Radeon compute GPUs always carry ProcessingAccelerator or DisplayController.
|
// DisplayController, or ProcessingAccelerator. Do NOT match vendor ID alone —
|
||||||
// Do NOT match AMD vendor alone — CPU chipset PCIe devices share that vendor ID.
|
// same-vendor companion devices (NVSwitch/NVLink bridges, CPU chipset PCIe
|
||||||
if class == "VideoController" || class == "DisplayController" || class == "ProcessingAccelerator" {
|
// functions, ...) share the GPU's vendor ID without being a GPU die.
|
||||||
return true
|
return collector.IsGPUClass(trimPtr(dev.DeviceClass))
|
||||||
}
|
|
||||||
// NVIDIA devices sometimes expose class values outside the standard GPU set.
|
|
||||||
return dev.VendorID != nil && *dev.VendorID == collector.NvidiaVendorID
|
|
||||||
}
|
}
|
||||||
|
|
||||||
func formatSystemLine(board schema.HardwareBoard) string {
|
func formatSystemLine(board schema.HardwareBoard) string {
|
||||||
|
|||||||
@@ -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 <model>" 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)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -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:<vendor> 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) {
|
func TestRunNCCLTestsPassesSelectedGPUs(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
|
|||||||
@@ -340,9 +340,7 @@ func matchesGPUVendor(dev schema.HardwarePCIeDevice, vendor string) bool {
|
|||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
class := strings.TrimSpace(*dev.DeviceClass)
|
class := strings.TrimSpace(*dev.DeviceClass)
|
||||||
isGPUClass := strings.Contains(class, "Controller") || strings.Contains(class, "Accelerator") ||
|
if !collector.IsGPUClass(class) {
|
||||||
strings.Contains(class, "Display") || strings.Contains(class, "Video")
|
|
||||||
if !isGPUClass {
|
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
switch vendor {
|
switch vendor {
|
||||||
|
|||||||
@@ -87,7 +87,7 @@ func isAMDGPUDevice(dev schema.HardwarePCIeDevice) bool {
|
|||||||
if dev.DeviceClass == nil {
|
if dev.DeviceClass == nil {
|
||||||
return false
|
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) {
|
func queryAMDGPUs() (map[string]amdGPUInfo, error) {
|
||||||
|
|||||||
@@ -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) {
|
switch strings.TrimSpace(class) {
|
||||||
case "VideoController", "DisplayController", "ProcessingAccelerator":
|
case "VideoController", "DisplayController", "ProcessingAccelerator":
|
||||||
return true
|
return true
|
||||||
|
|||||||
@@ -51,7 +51,7 @@ func shouldProbePCIeSerial(dev schema.HardwarePCIeDevice) bool {
|
|||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
class := strings.TrimSpace(*dev.DeviceClass)
|
class := strings.TrimSpace(*dev.DeviceClass)
|
||||||
return isNICClass(class) || isGPUClass(class)
|
return isNICClass(class) || IsGPUClass(class)
|
||||||
}
|
}
|
||||||
|
|
||||||
func queryPCIDeviceSerial(bdf string) string {
|
func queryPCIDeviceSerial(bdf string) string {
|
||||||
|
|||||||
@@ -54,15 +54,12 @@ func topoCard(title, body string) string {
|
|||||||
// instead of importing the package for one classifier).
|
// instead of importing the package for one classifier).
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
// isNICDeviceClassDev mirrors the classification logic in hwDescribeNIC
|
// isNICDeviceClassDev applies isNICDeviceClass (pages.go) to a single device,
|
||||||
// (pages.go), applied to a single device instead of aggregated counts.
|
// with a MAC-address fallback for devices lspci doesn't classify as NIC.
|
||||||
func isNICDeviceClassDev(dev schema.HardwarePCIeDevice) bool {
|
func isNICDeviceClassDev(dev schema.HardwarePCIeDevice) bool {
|
||||||
if dev.DeviceClass != nil {
|
if dev.DeviceClass != nil && isNICDeviceClass(*dev.DeviceClass) {
|
||||||
c := strings.ToLower(strings.TrimSpace(*dev.DeviceClass))
|
|
||||||
if c == "ethernetcontroller" || c == "networkcontroller" || strings.Contains(c, "fibrechannel") {
|
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
}
|
|
||||||
return len(dev.MacAddresses) > 0
|
return len(dev.MacAddresses) > 0
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -627,8 +627,7 @@ func validateIsVendorGPU(dev schema.HardwarePCIeDevice, vendor string) bool {
|
|||||||
if dev.VendorID != nil && *dev.VendorID == pciVendorAspeed {
|
if dev.VendorID != nil && *dev.VendorID == pciVendorAspeed {
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
class := strings.ToLower(validateTrimPtr(dev.DeviceClass))
|
isGPUClass := isGPUDeviceClass(validateTrimPtr(dev.DeviceClass))
|
||||||
isGPUClass := class == "videocontroller" || class == "processingaccelerator" || class == "displaycontroller"
|
|
||||||
switch vendor {
|
switch vendor {
|
||||||
case "nvidia":
|
case "nvidia":
|
||||||
return isGPUClass && dev.VendorID != nil && *dev.VendorID == pciVendorNvidia
|
return isGPUClass && dev.VendorID != nil && *dev.VendorID == pciVendorNvidia
|
||||||
|
|||||||
@@ -560,16 +560,21 @@ func hwDescribePSU(hw schema.HardwareSnapshot) string {
|
|||||||
return fmt.Sprintf("%d× PSU", n)
|
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".
|
// hwDescribeNIC returns a summary like "2× Mellanox ConnectX-6".
|
||||||
func hwDescribeNIC(hw schema.HardwareSnapshot) string {
|
func hwDescribeNIC(hw schema.HardwareSnapshot) string {
|
||||||
counts := map[string]int{}
|
counts := map[string]int{}
|
||||||
order := []string{}
|
order := []string{}
|
||||||
for _, dev := range hw.PCIeDevices {
|
for _, dev := range hw.PCIeDevices {
|
||||||
isNIC := false
|
isNIC := dev.DeviceClass != nil && isNICDeviceClass(*dev.DeviceClass)
|
||||||
if dev.DeviceClass != nil {
|
|
||||||
c := strings.ToLower(strings.TrimSpace(*dev.DeviceClass))
|
|
||||||
isNIC = c == "ethernetcontroller" || c == "networkcontroller" || strings.Contains(c, "fibrechannel")
|
|
||||||
}
|
|
||||||
if !isNIC && len(dev.MacAddresses) == 0 {
|
if !isNIC && len(dev.MacAddresses) == 0 {
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user