From 2d84ddb57750d350744936fea1d0316143033a2f Mon Sep 17 00:00:00 2001 From: Mikhail Chusavitin Date: Tue, 7 Jul 2026 17:14:03 +0300 Subject: [PATCH] webui/topo: read from techdump instead of live nvidia-smi, fix socket/NUMA mismatch, fix cross-tier SAT status merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /topo blocked the HTTP request on live nvidia-smi calls with no timeout (topo -m, nvlink -s/-e run on every page load), so a wedged driver hung the page indefinitely. CaptureTechnicalDump now persists these dumps once per audit cycle; the page reads them from techdump/ instead. buildSocketIndex mapped NUMA node number to CPU by treating dmidecode's Socket Designation (often 1-indexed, "CPU1"/"CPU2") as equal to the NUMA node number (always 0-indexed) — GPUs/NICs on NUMA node 0 fell into the "unknown" column, others attached to the wrong CPU box. Now ranks CPUs by Socket value instead of assuming a shared numbering base. Fixed a bug in ComponentStatusDB/applyComponentStatusDB where GPU SAT results were keyed per-target ("pcie:gpu:nvidia-stress") instead of per-vendor, which both broke cross-tier severity tracking (a later clean "2. Check" run and an earlier failing "3. Load" run never compared severities) and silently failed to match any real BDF, so the DB overlay never reached the topology graph at all. GPU keys are now normalized to vendor ("pcie:gpu:nvidia"/"pcie:gpu:amd"). Also skip writing to the DB when a SAT task was aborted by the user (ctx canceled), so a partial run can't stomp a previously recorded status. Co-Authored-By: Claude Sonnet 5 --- audit/internal/app/component_status_db.go | 17 ++- .../internal/app/component_status_db_test.go | 57 ++++++++ audit/internal/app/sat_overlay.go | 13 +- audit/internal/platform/techdump.go | 6 + audit/internal/webui/page_topo.go | 125 +++++++++++------- audit/internal/webui/page_topo_test.go | 13 ++ audit/internal/webui/task_runner.go | 4 +- audit/internal/webui/tasks.go | 6 +- 8 files changed, 186 insertions(+), 55 deletions(-) diff --git a/audit/internal/app/component_status_db.go b/audit/internal/app/component_status_db.go index 689cc4f..638916f 100644 --- a/audit/internal/app/component_status_db.go +++ b/audit/internal/app/component_status_db.go @@ -177,12 +177,21 @@ func ApplySATResultToDB(db *ComponentStatusDB, target, archivePath string) { source := "sat:" + target dbStatus := satStatusToDBStatus(overall) - // Map SAT target to component keys. + // Map SAT target to component keys. GPU targets are keyed by vendor, not + // by the raw target string: "nvidia" (Check tier) and "nvidia-stress" / + // "nvidia-targeted-stress" (Load/Burn tier) all exercise the same + // physical GPUs, so they must share one severity-tracked record — a + // severity-1 Check run after a severity-3 Load failure must not lose + // that failure just because it ran more recently. Recording each target + // under its own key (the previous behavior) also silently broke + // applyComponentStatusDB below, which expects "pcie:gpu:" and + // otherwise fails to match any real BDF. switch target { case "nvidia", "nvidia-targeted-stress", "nvidia-compute", "nvidia-targeted-power", "nvidia-pulse", - "nvidia-interconnect", "nvidia-bandwidth", "amd", "nvidia-stress", - "amd-stress", "amd-mem", "amd-bandwidth": - db.Record("pcie:gpu:"+target, source, dbStatus, target+" SAT: "+overall) + "nvidia-interconnect", "nvidia-bandwidth", "nvidia-stress": + db.Record("pcie:gpu:nvidia", source, dbStatus, target+" SAT: "+overall) + case "amd", "amd-stress", "amd-mem", "amd-bandwidth": + db.Record("pcie:gpu:amd", source, dbStatus, target+" SAT: "+overall) case "memory", "memory-stress", "sat-stress": db.Record("memory:all", source, dbStatus, target+" SAT: "+overall) case "cpu", "platform-stress": diff --git a/audit/internal/app/component_status_db_test.go b/audit/internal/app/component_status_db_test.go index d818bcc..ab1f89f 100644 --- a/audit/internal/app/component_status_db_test.go +++ b/audit/internal/app/component_status_db_test.go @@ -4,6 +4,8 @@ import ( "os" "path/filepath" "testing" + + "bee/audit/internal/schema" ) func TestExtractArchivePath(t *testing.T) { @@ -40,3 +42,58 @@ func TestReadSATOverallStatus_HandlesActionResultPrefix(t *testing.T) { t.Errorf("ReadSATOverallStatus(bare) = %q, want FAILED", got) } } + +func writeSATSummary(t *testing.T, overall string) string { + t.Helper() + runDir := t.TempDir() + summary := "run_at_utc=2026-07-06T17:47:22Z\noverall_status=" + overall + "\n" + if err := os.WriteFile(filepath.Join(runDir, "summary.txt"), []byte(summary), 0644); err != nil { + t.Fatal(err) + } + return runDir +} + +func TestApplySATResultToDBNormalizesGPUKeyByVendor(t *testing.T) { + // "nvidia" (Check tier) and "nvidia-stress" (Load/Burn tier) exercise the + // same physical GPUs and must collapse onto one component key so a later + // clean Check run can't erase an earlier Load-tier failure. + db, err := OpenComponentStatusDB(filepath.Join(t.TempDir(), "component-status.json")) + if err != nil { + t.Fatal(err) + } + + ApplySATResultToDB(db, "nvidia-stress", writeSATSummary(t, "FAILED")) + ApplySATResultToDB(db, "nvidia", writeSATSummary(t, "OK")) + + rec, ok := db.Get("pcie:gpu:nvidia") + if !ok { + t.Fatalf("expected pcie:gpu:nvidia record to exist") + } + if rec.Status != "Warning" { + t.Fatalf("status=%q, want Warning (FAILED) to survive the later OK Check run", rec.Status) + } +} + +func TestApplyComponentStatusDBMatchesGPUByVendor(t *testing.T) { + db, err := OpenComponentStatusDB(filepath.Join(t.TempDir(), "component-status.json")) + if err != nil { + t.Fatal(err) + } + db.Record("pcie:gpu:nvidia", "sat:nvidia-stress", "Critical", "nvidia-stress SAT: FAILED") + + class := "VideoController" + vendor := 0x10de // collector.NvidiaVendorID + snap := &schema.HardwareSnapshot{ + PCIeDevices: []schema.HardwarePCIeDevice{ + {DeviceClass: &class, VendorID: &vendor, BDF: strPtr("0000:c8:00.0")}, + }, + } + + applyComponentStatusDB(snap, db) + + if snap.PCIeDevices[0].Status == nil || *snap.PCIeDevices[0].Status != "Critical" { + t.Fatalf("expected GPU device status Critical, got %v", snap.PCIeDevices[0].Status) + } +} + +func strPtr(s string) *string { return &s } diff --git a/audit/internal/app/sat_overlay.go b/audit/internal/app/sat_overlay.go index c4d8675..50f888d 100644 --- a/audit/internal/app/sat_overlay.go +++ b/audit/internal/app/sat_overlay.go @@ -347,14 +347,19 @@ func applyComponentStatusDB(snap *schema.HardwareSnapshot, db *ComponentStatusDB ts := rec.LastChangedAt.UTC().Format("2006-01-02T15:04:05Z") switch { + case key == "pcie:gpu:nvidia" || key == "pcie:gpu:amd": + vendor := strings.TrimPrefix(key, "pcie:gpu:") + for i := range snap.PCIeDevices { + if matchesGPUVendor(snap.PCIeDevices[i], vendor) { + mergeComponentStatus(&snap.PCIeDevices[i].HardwareComponentStatus, ts, status, detail) + } + } case strings.HasPrefix(key, "pcie:"): bdf := strings.TrimPrefix(key, "pcie:") - bdf = strings.TrimPrefix(bdf, "gpu:") // strip sub-type if present - // bdf may be empty (e.g. "pcie:gpu:nvidia") — skip BDF matching - if sanitizeBDFForLookup(bdf) == "" { + normalized := sanitizeBDFForLookup(bdf) + if normalized == "" { break } - normalized := sanitizeBDFForLookup(bdf) for i := range snap.PCIeDevices { if snap.PCIeDevices[i].BDF == nil { continue diff --git a/audit/internal/platform/techdump.go b/audit/internal/platform/techdump.go index c82e524..2b95080 100644 --- a/audit/internal/platform/techdump.go +++ b/audit/internal/platform/techdump.go @@ -47,6 +47,12 @@ var techDumpNvidiaCommands = []struct { {Name: "nvidia-smi", Args: []string{"-q"}, File: "nvidia-smi-q.txt"}, {Name: "nvidia-smi", Args: []string{"--query-gpu=index,pci.bus_id,serial,vbios_version,temperature.gpu,power.draw,ecc.errors.uncorrected.aggregate.total,ecc.errors.corrected.aggregate.total,clocks_throttle_reasons.hw_slowdown", "--format=csv,noheader,nounits"}, File: "nvidia-smi-query.csv"}, {Name: "nvidia-smi", Args: []string{"conf-compute", "-q"}, File: "nvidia-smi-conf-compute-q.txt"}, + // Consumed by webui's /topo page (GPU-GPU NVLink adjacency and per-link + // status/errors) so that page can render from this persisted dump instead + // of shelling out to nvidia-smi on every HTTP request. + {Name: "nvidia-smi", Args: []string{"topo", "-m"}, File: "nvidia-smi-topo.txt"}, + {Name: "nvidia-smi", Args: []string{"nvlink", "-s"}, File: "nvidia-smi-nvlink-status.txt"}, + {Name: "nvidia-smi", Args: []string{"nvlink", "-e"}, File: "nvidia-smi-nvlink-errors.txt"}, } type lsblkDumpRoot struct { diff --git a/audit/internal/webui/page_topo.go b/audit/internal/webui/page_topo.go index 3eca577..097c5a2 100644 --- a/audit/internal/webui/page_topo.go +++ b/audit/internal/webui/page_topo.go @@ -4,7 +4,8 @@ import ( "encoding/json" "fmt" "html" - "os/exec" + "os" + "path/filepath" "regexp" "sort" "strconv" @@ -16,9 +17,11 @@ import ( // renderTopo renders the /topo page: a read-only visualization of the server // topology (CPU sockets, NUMA-affine PCIe devices, PSU/BMC) plus a separate // NVLink topology card. It is pure visualization: everything it reads either -// already exists in the audit.json contract, or is a live (non-persisted) -// query — nothing here writes to schema.HardwarePCIeDevice or any other -// contract type, and nothing here talks to Reanimator Core. +// already exists in the audit.json contract, or comes from the persisted +// techdump captured once per audit cycle (platform.CaptureTechnicalDump) — +// nothing here shells out to nvidia-smi itself, writes to +// schema.HardwarePCIeDevice or any other contract type, or talks to +// Reanimator Core. func renderTopo(opts HandlerOptions) string { data, err := loadSnapshot(opts.AuditPath) if err != nil { @@ -31,8 +34,8 @@ func renderTopo(opts HandlerOptions) string { hw := ingest.Hardware var b strings.Builder - b.WriteString(renderTopoMainDiagram(hw)) - if nv := renderTopoNVLinkCard(hw); nv != "" { + b.WriteString(renderTopoMainDiagram(hw, opts.ExportDir)) + if nv := renderTopoNVLinkCard(hw, opts.ExportDir); nv != "" { b.WriteString(nv) } return b.String() @@ -130,18 +133,36 @@ func topoEdgeColorVar(dev schema.HardwarePCIeDevice) string { // NUMA node -> CPU socket join (heuristic, no guaranteed hardware mapping) // --------------------------------------------------------------------------- -// buildSocketIndex maps a NUMA node number to the index into cpus whose -// Socket field equals that NUMA node number. This is a best-effort heuristic -// (NUMANode == Socket) documented as such in the /topo design — Linux NUMA -// node numbering and dmidecode socket designation are different numbering -// domains with no guaranteed 1:1 mapping, but in practice agree for the -// common case of N-socket boards. +// buildSocketIndex maps a NUMA node number to the index into cpus for the +// socket occupying that position in ascending Socket-designation order. +// +// Linux NUMA node numbering is always 0-based (node0, node1, ...), but +// dmidecode's "Socket Designation" is board-defined and frequently 1-based +// ("CPU1", "CPU2", ...). Mapping NUMA node N to the CPU whose Socket field +// equals N (as an earlier version of this function did) silently fails on +// any 1-indexed board: node 0 has no match (dropped into the "unknown" +// column) and node 1 wrongly maps to the first CPU. Ranking by Socket value +// instead assumes only that node order follows socket order — true for the +// common case of N-socket boards — without depending on the numbering base. func buildSocketIndex(cpus []schema.HardwareCPU) map[int]int { - idx := map[int]int{} - for i, cpu := range cpus { - if cpu.Socket != nil { - idx[*cpu.Socket] = i + order := make([]int, len(cpus)) + for i := range cpus { + order[i] = i + } + sort.SliceStable(order, func(a, b int) bool { + ca, cb := cpus[order[a]], cpus[order[b]] + sa, sb := 0, 0 + if ca.Socket != nil { + sa = *ca.Socket } + if cb.Socket != nil { + sb = *cb.Socket + } + return sa < sb + }) + idx := map[int]int{} + for numaNode, cpuIdx := range order { + idx[numaNode] = cpuIdx } return idx } @@ -245,31 +266,41 @@ func parseGPUPairAdjacency(raw string) []gpuPairLink { return pairs } -func queryGPUTopologyMatrix() (string, error) { - out, err := exec.Command("nvidia-smi", "topo", "-m").Output() +// readTopoTechDump reads a file previously captured into the persistent +// techdump directory by platform.System.CaptureTechnicalDump (run once per +// audit cycle), rather than shelling out to nvidia-smi from the HTTP request +// handler — a live call here would block page rendering on a wedged driver, +// exactly the failure mode this tool exists to diagnose. +func readTopoTechDump(exportDir, name string) (string, error) { + out, err := os.ReadFile(filepath.Join(exportDir, "techdump", name)) if err != nil { return "", err } return string(out), nil } -// queryNVIDIAIndexByBDF runs a lightweight live nvidia-smi query mapping -// PCI bus address (matching HardwarePCIeDevice.Slot) to the GPU index -// nvidia-smi/dcgmi/topo -m report, so GPU-GPU edges (keyed by index) can be -// anchored to the correct box (keyed by BDF) in the diagram. -func queryNVIDIAIndexByBDF() (map[string]int, error) { - out, err := exec.Command("nvidia-smi", "--query-gpu=index,pci.bus_id", "--format=csv,noheader").Output() +func readGPUTopologyMatrix(exportDir string) (string, error) { + return readTopoTechDump(exportDir, "nvidia-smi-topo.txt") +} + +// readNVIDIAIndexByBDF parses the persisted nvidia-smi-query.csv techdump +// (index,pci.bus_id,...) to map PCI bus address (matching +// HardwarePCIeDevice.Slot) to the GPU index nvidia-smi/topo -m reports, so +// GPU-GPU edges (keyed by index) can be anchored to the correct box (keyed +// by BDF) in the diagram. +func readNVIDIAIndexByBDF(exportDir string) (map[string]int, error) { + raw, err := readTopoTechDump(exportDir, "nvidia-smi-query.csv") if err != nil { return nil, err } result := map[string]int{} - for _, line := range strings.Split(string(out), "\n") { + for _, line := range strings.Split(raw, "\n") { line = strings.TrimSpace(line) if line == "" { continue } - parts := strings.SplitN(line, ",", 2) - if len(parts) != 2 { + parts := strings.Split(line, ",") + if len(parts) < 2 { continue } idx, err := strconv.Atoi(strings.TrimSpace(parts[0])) @@ -328,7 +359,7 @@ type topoBox struct { detailType string // "" = not clickable } -func renderTopoMainDiagram(hw schema.HardwareSnapshot) string { +func renderTopoMainDiagram(hw schema.HardwareSnapshot, exportDir string) string { socketIdx := buildSocketIndex(hw.CPUs) numCols := len(hw.CPUs) if numCols == 0 { @@ -383,11 +414,13 @@ func renderTopoMainDiagram(hw schema.HardwareSnapshot) string { totalCols++ } - // Live GPU index<->BDF map + pairwise NVLink adjacency, best-effort: - // if nvidia-smi is unavailable, GPU-GPU edges are simply omitted. - bdfToIndex, _ := queryNVIDIAIndexByBDF() + // GPU index<->BDF map + pairwise NVLink adjacency, read from the + // persisted techdump captured during the last audit cycle, best-effort: + // if the dump is missing (older audit, no NVIDIA GPUs), GPU-GPU edges are + // simply omitted. + bdfToIndex, _ := readNVIDIAIndexByBDF(exportDir) var pairs []gpuPairLink - if topoMatrix, err := queryGPUTopologyMatrix(); err == nil { + if topoMatrix, err := readGPUTopologyMatrix(exportDir); err == nil { pairs = parseGPUPairAdjacency(topoMatrix) } @@ -601,7 +634,8 @@ func truncateTopoLabel(s string, max int) string { } // --------------------------------------------------------------------------- -// Separate NVLink topology card (live query, not persisted to any contract) +// Separate NVLink topology card (read from techdump, not written to any +// ingest contract) // --------------------------------------------------------------------------- type topoNVLinkPort struct { @@ -620,12 +654,12 @@ var ( topoNVLinkErrorLineRe = regexp.MustCompile(`^Link (\d+):\s*(Replay|Recovery|CRC) Errors:\s*(\d+)`) ) -func queryTopoNVLinkStatus() (map[int][]topoNVLinkPort, error) { - out, err := exec.Command("nvidia-smi", "nvlink", "-s").Output() +func readTopoNVLinkStatus(exportDir string) (map[int][]topoNVLinkPort, error) { + raw, err := readTopoTechDump(exportDir, "nvidia-smi-nvlink-status.txt") if err != nil { return nil, err } - return parseTopoNVLinkStatus(string(out)), nil + return parseTopoNVLinkStatus(raw), nil } func parseTopoNVLinkStatus(raw string) map[int][]topoNVLinkPort { @@ -657,12 +691,12 @@ func parseTopoNVLinkStatus(raw string) map[int][]topoNVLinkPort { return result } -func queryTopoNVLinkErrors() (map[int]map[int][3]int64, error) { - out, err := exec.Command("nvidia-smi", "nvlink", "-e").Output() +func readTopoNVLinkErrors(exportDir string) (map[int]map[int][3]int64, error) { + raw, err := readTopoTechDump(exportDir, "nvidia-smi-nvlink-errors.txt") if err != nil { return nil, err } - return parseTopoNVLinkErrors(string(out)), nil + return parseTopoNVLinkErrors(raw), nil } // parseTopoNVLinkErrors returns, per GPU then link index, [replay, recovery, crc]. @@ -702,8 +736,9 @@ func parseTopoNVLinkErrors(raw string) map[int]map[int][3]int64 { } // renderTopoNVLinkCard renders the separate NVLink topology card. Returns "" -// if there are fewer than 2 NVIDIA GPUs or nvidia-smi is unavailable. -func renderTopoNVLinkCard(hw schema.HardwareSnapshot) string { +// if there are fewer than 2 NVIDIA GPUs, or the nvidia-smi nvlink techdump +// wasn't captured (older audit, or nvidia-smi unavailable on that run). +func renderTopoNVLinkCard(hw schema.HardwareSnapshot, exportDir string) string { gpuCount := 0 for _, dev := range hw.PCIeDevices { if dev.DeviceClass != nil && isGPUDeviceClass(*dev.DeviceClass) { @@ -714,13 +749,13 @@ func renderTopoNVLinkCard(hw schema.HardwareSnapshot) string { return "" } - status, err := queryTopoNVLinkStatus() + status, err := readTopoNVLinkStatus(exportDir) if err != nil || len(status) == 0 { - return topoCard("NVLink Topology", `nvidia-smi nvlink unavailable`) + return topoCard("NVLink Topology", `nvidia-smi nvlink data unavailable`) } - errors, _ := queryTopoNVLinkErrors() + errors, _ := readTopoNVLinkErrors(exportDir) - topoMatrix, _ := queryGPUTopologyMatrix() + topoMatrix, _ := readGPUTopologyMatrix(exportDir) pairs := parseGPUPairAdjacency(topoMatrix) var bodyB strings.Builder diff --git a/audit/internal/webui/page_topo_test.go b/audit/internal/webui/page_topo_test.go index 74935c9..94df8e9 100644 --- a/audit/internal/webui/page_topo_test.go +++ b/audit/internal/webui/page_topo_test.go @@ -175,6 +175,19 @@ func TestBuildSocketIndex(t *testing.T) { } } +func TestBuildSocketIndexOneIndexedSocketDesignation(t *testing.T) { + // dmidecode "Socket Designation" is frequently 1-indexed ("CPU1", "CPU2") + // while Linux NUMA nodes are always 0-indexed. NUMA node 0 must still + // resolve to the first CPU in Socket order, not fall through to the + // "unknown" column (the bug reported against the /topo page). + s1, s2 := 1, 2 + cpus := []schema.HardwareCPU{{Socket: &s1}, {Socket: &s2}} + idx := buildSocketIndex(cpus) + if idx[0] != 0 || idx[1] != 1 { + t.Fatalf("idx=%#v want {0:0, 1:1}", idx) + } +} + func TestIsRAIDControllerClass(t *testing.T) { if !isRAIDControllerClass("StorageController") || !isRAIDControllerClass("MassStorageController") { t.Fatalf("expected known RAID/storage classes to match") diff --git a/audit/internal/webui/task_runner.go b/audit/internal/webui/task_runner.go index eab4e3b..8a907f7 100644 --- a/audit/internal/webui/task_runner.go +++ b/audit/internal/webui/task_runner.go @@ -439,7 +439,9 @@ func executeTaskWithOptions(opts *HandlerOptions, t *Task, j *jobState, ctx cont if err == nil && app.ReadSATOverallStatus(archivePath) == "FAILED" { err = fmt.Errorf("SAT overall_status=FAILED (see summary.txt)") } - if opts.App != nil && opts.App.StatusDB != nil { + // See tasks.go's identical guard: a user-aborted run must not + // overwrite the component status DB with a partial result. + if opts.App != nil && opts.App.StatusDB != nil && ctx.Err() == nil { app.ApplySATResultToDB(opts.App.StatusDB, t.Target, archivePath) } } diff --git a/audit/internal/webui/tasks.go b/audit/internal/webui/tasks.go index c99cd4f..7feca99 100644 --- a/audit/internal/webui/tasks.go +++ b/audit/internal/webui/tasks.go @@ -1153,7 +1153,11 @@ func (q *taskQueue) runTask(t *Task, j *jobState, ctx context.Context) { err = fmt.Errorf("SAT overall_status=FAILED (see summary.txt)") } } - if db := q.statusDB(); db != nil { + // A user-aborted run (ctx canceled) may still have produced a partial + // archive/summary.txt — that incomplete result must not overwrite the + // component status DB, which is why this is skipped here rather than + // relying on satKeyStatus's PARTIAL/UNSUPPORTED handling below. + if db := q.statusDB(); db != nil && ctx.Err() == nil { app.ApplySATResultToDB(db, t.Target, archivePath) } }