From 1e4ec513e1e1838c789d4cfc5f89a5321aa71496 Mon Sep 17 00:00:00 2001 From: Mikhail Chusavitin Date: Tue, 18 Aug 2026 14:24:49 +0300 Subject: [PATCH] fix(collector): dedup GPUs and resolve model text on xFusion boards without PCI identity A re-run against the G5500 V7 (previous fix commit a3567dd) surfaced two follow-on bugs once the GPUs started being classified correctly: - gpuDocDedupKey fell straight through to the full @odata.id path when a GPU had neither SerialNumber nor BDF, so the same physical H100 exposed under both Systems/1/PCIeDevices and Chassis/1/PCIeDevices (identical resource Id, different collection root) was kept twice, doubling the reported GPU count. Added an Id + resolved VendorID/DeviceID fallback tier that collapses same-Id duplicates without collapsing genuinely distinct GPUs that happen to share a vendor/device pair. - isMissingOrRawPCIModel/isGenericRedfishInventoryName didn't recognize xFusion's generic "PCIeCardN"/"OCPCardN" slot labels, so when the doc's actual Model field was empty and GPU.Model fell back to the slot-like Name field, the pci.ids VendorId/DeviceId resolution never fired and the GPU surfaced with a meaningless model like "PCIeCard1" instead of the real chip name. Co-Authored-By: Claude Sonnet 5 --- internal/collector/redfish.go | 13 ++++++ internal/collector/redfish_test.go | 70 ++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/internal/collector/redfish.go b/internal/collector/redfish.go index f4d0ad4..73498c9 100644 --- a/internal/collector/redfish.go +++ b/internal/collector/redfish.go @@ -4591,6 +4591,10 @@ func isGenericRedfishInventoryName(value string) bool { return true case value == "pciedevice", strings.HasPrefix(value, "pciedevice_"), strings.HasPrefix(value, "pciedevice "): return true + // xFusion (and similar OEM BMCs) label generic PCIe/OCP slots "PCIeCardN"/ + // "OCPCardN" instead of a real model — these are slot labels, not model text. + case strings.HasPrefix(value, "pciecard"), strings.HasPrefix(value, "ocpcard"): + return true case value == "pciefunction", strings.HasPrefix(value, "pciefunction_"), strings.HasPrefix(value, "pciefunction "): return true case value == "ethernetinterface", strings.HasPrefix(value, "ethernetinterface_"), strings.HasPrefix(value, "ethernetinterface "): @@ -4818,6 +4822,15 @@ func gpuDocDedupKey(doc map[string]interface{}, gpu models.GPU) string { if bdf := strings.TrimSpace(gpu.BDF); bdf != "" { return bdf } + // Some BMCs (e.g. xFusion) expose the same physical GPU under both + // Systems/{id}/PCIeDevices and Chassis/{id}/PCIeDevices with neither + // serial nor BDF populated in either copy. The resource Id is identical + // across both trees for the same device, so combine it with the resolved + // VendorID/DeviceID (stable, collection-independent) to still collapse + // them without risking collisions between distinct GPUs. + if id := strings.ToLower(strings.TrimSpace(asString(doc["Id"]))); id != "" && (gpu.VendorID != 0 || gpu.DeviceID != 0) { + return fmt.Sprintf("id:%s|vd:%04x:%04x", id, gpu.VendorID, gpu.DeviceID) + } if path := normalizeRedfishPath(asString(doc["@odata.id"])); path != "" { return "path:" + path } diff --git a/internal/collector/redfish_test.go b/internal/collector/redfish_test.go index 98398b1..cc4e17d 100644 --- a/internal/collector/redfish_test.go +++ b/internal/collector/redfish_test.go @@ -3908,6 +3908,76 @@ func TestLooksLikeGPU_ResolvesVendorDeviceIDWhenModelTextMissing(t *testing.T) { } } +// TestGpuDocDedupKey_CollapsesSameIdAcrossCollectionRootsWithoutIdentity +// covers xFusion BMCs (e.g. G5500 V7) where the same physical GPU is +// enumerated under both Systems/{id}/PCIeDevices and Chassis/{id}/PCIeDevices +// with neither SerialNumber nor BDF populated in either copy. Without a +// fallback keyed on the shared resource Id + resolved VendorID/DeviceID, +// collectGPUs would keep both copies and double the reported GPU count. +func TestGpuDocDedupKey_CollapsesSameIdAcrossCollectionRootsWithoutIdentity(t *testing.T) { + gpu := models.GPU{VendorID: 0x10de, DeviceID: 0x2330} + + systemsDoc := map[string]interface{}{"@odata.id": "/redfish/v1/Systems/1/PCIeDevices/PCIeCard1", "Id": "PCIeCard1"} + chassisDoc := map[string]interface{}{"@odata.id": "/redfish/v1/Chassis/1/PCIeDevices/PCIeCard1", "Id": "PCIeCard1"} + + key1 := gpuDocDedupKey(systemsDoc, gpu) + key2 := gpuDocDedupKey(chassisDoc, gpu) + if key1 == "" || key2 == "" { + t.Fatalf("expected non-empty dedup keys, got %q and %q", key1, key2) + } + if key1 != key2 { + t.Fatalf("expected same physical GPU under different collection roots to dedup to the same key, got %q vs %q", key1, key2) + } +} + +// TestGpuDocDedupKey_DistinctIdsStayDistinct guards against the Id+VendorID/ +// DeviceID fallback over-collapsing genuinely different GPUs that share a +// vendor/device pair but have distinct slot Ids. +func TestGpuDocDedupKey_DistinctIdsStayDistinct(t *testing.T) { + gpu := models.GPU{VendorID: 0x10de, DeviceID: 0x2330} + + doc1 := map[string]interface{}{"@odata.id": "/redfish/v1/Chassis/1/PCIeDevices/PCIeCard1", "Id": "PCIeCard1"} + doc3 := map[string]interface{}{"@odata.id": "/redfish/v1/Chassis/1/PCIeDevices/PCIeCard3", "Id": "PCIeCard3"} + + if gpuDocDedupKey(doc1, gpu) == gpuDocDedupKey(doc3, gpu) { + t.Fatal("expected distinct GPU slot Ids to produce distinct dedup keys") + } +} + +// TestIsMissingOrRawPCIModel_TreatsGenericSlotLabelsAsMissing covers xFusion +// BMCs that label generic PCIe/OCP slots "PCIeCardN"/"OCPCardN" instead of a +// real model — these must be treated as missing so pci.ids VendorId/DeviceId +// resolution fills in the real GPU model instead of leaving the slot label. +func TestIsMissingOrRawPCIModel_TreatsGenericSlotLabelsAsMissing(t *testing.T) { + for _, model := range []string{"PCIeCard1", "PCIeCard3", "OCPCard1", "pciecard1"} { + if !isMissingOrRawPCIModel(model) { + t.Errorf("expected %q to be treated as a missing/raw model", model) + } + } + if isMissingOrRawPCIModel("H100 SXM5 80GB") { + t.Fatal("expected a real model name not to be treated as missing/raw") + } +} + +// TestParseGPUWithSupplementalDocs_ResolvesModelWhenOnlySlotLabelPresent +// reproduces the G5500 V7 case end-to-end: the PCIeDevice doc has no Model, +// only Name="PCIeCard1" (a slot label), and identity comes solely from the +// linked PCIeFunction's raw VendorId/DeviceId. The parsed GPU's Model must +// resolve to the real chip name via pci.ids instead of surfacing "PCIeCard1". +func TestParseGPUWithSupplementalDocs_ResolvesModelWhenOnlySlotLabelPresent(t *testing.T) { + doc := map[string]interface{}{ + "Id": "PCIeCard1", + "Name": "PCIeCard1", + } + functionDocs := []map[string]interface{}{ + {"VendorId": "0x10de", "DeviceId": "0x2330"}, + } + gpu := parseGPUWithSupplementalDocs(doc, functionDocs, nil, 1) + if gpu.Model == "PCIeCard1" || strings.TrimSpace(gpu.Model) == "" { + t.Fatalf("expected GPU model to be resolved via pci.ids, got %q", gpu.Model) + } +} + func TestFirmwareInventoryDeviceName_PrefersIDForGenericSoftwareInventory(t *testing.T) { doc := map[string]interface{}{ "Id": "HGX_FW_NVSwitch_0",