From bd20069c60107bc52efc74eef9ad15e6023fdedc Mon Sep 17 00:00:00 2001 From: rcourtman Date: Fri, 29 May 2026 19:55:53 +0100 Subject: [PATCH] fix(disks): report authoritative disk size and namespace devpath from the host agent On a Proxmox node, physical disks collected by the host agent were keyed by the NVMe controller (e.g. "nvme0 [nvme]") instead of the namespace, reported sizeBytes 0 (or a stale filesystem-usage value), and flickered as the agent reading intermittently replaced the authoritative Proxmox disks/list reading. Root causes: - smartctl --scan-open reports NVMe disks by their controller char device (/dev/nvme0), and that scan label became the reported devPath. - DiskSMART carried no capacity, so the server backfilled size by matching the SMART device against host filesystem-usage entries, which never match a whole partitioned/LVM/ZFS disk, leaving size 0. - The unified-resource merge let the agent's controller label overwrite the canonical Proxmox /dev/... devPath. Fixes: - The agent now reports the canonical block device (an NVMe controller resolves to its namespace) and the authoritative capacity from /sys/block, with the smartctl user_capacity / nvme_total_capacity as a cross-platform fallback. Disks behind multiplexing controllers (megaraid, cciss, areca) keep their disambiguating label and smartctl-reported size. - SizeBytes flows through the agent report, host model, and adapter; the filesystem-usage match is demoted to a legacy fallback. - The merge keeps a canonical /dev/ devPath and never downgrades it to a scan label, so an un-updated agent can no longer corrupt Proxmox data. Refs #1483. --- internal/hostagent/agent.go | 1 + internal/hostagent/smartctl.go | 150 ++++++++++++++- internal/hostagent/smartctl_size_test.go | 211 +++++++++++++++++++++ internal/models/models.go | 19 +- internal/monitoring/monitor_helpers.go | 1 + internal/unifiedresources/adapters.go | 10 +- internal/unifiedresources/adapters_test.go | 53 ++++++ internal/unifiedresources/registry.go | 33 +++- internal/unifiedresources/registry_test.go | 87 +++++++++ pkg/agents/host/report.go | 19 +- 10 files changed, 554 insertions(+), 30 deletions(-) create mode 100644 internal/hostagent/smartctl_size_test.go diff --git a/internal/hostagent/agent.go b/internal/hostagent/agent.go index d9292c165..3bb395ece 100644 --- a/internal/hostagent/agent.go +++ b/internal/hostagent/agent.go @@ -1388,6 +1388,7 @@ func (a *Agent) collectSMARTData(ctx context.Context, diskExclude []string) []ag Serial: disk.Serial, WWN: disk.WWN, Type: disk.Type, + SizeBytes: disk.SizeBytes, Temperature: disk.Temperature, Health: disk.Health, Standby: disk.Standby, diff --git a/internal/hostagent/smartctl.go b/internal/hostagent/smartctl.go index d71dca464..e030674aa 100644 --- a/internal/hostagent/smartctl.go +++ b/internal/hostagent/smartctl.go @@ -42,14 +42,15 @@ var ( // DiskSMART represents S.M.A.R.T. data for a single disk. type DiskSMART struct { - Device string `json:"device"` // Device path (e.g., /dev/sda) - Model string `json:"model,omitempty"` // Disk model - Serial string `json:"serial,omitempty"` // Serial number - WWN string `json:"wwn,omitempty"` // World Wide Name - Type string `json:"type,omitempty"` // Transport type: sata, sas, nvme - Temperature int `json:"temperature"` // Temperature in Celsius - Health string `json:"health,omitempty"` // PASSED, FAILED, UNKNOWN - Standby bool `json:"standby,omitempty"` // True if disk was in standby + Device string `json:"device"` // Block device name (e.g., sda, nvme0n1) + Model string `json:"model,omitempty"` // Disk model + Serial string `json:"serial,omitempty"` // Serial number + WWN string `json:"wwn,omitempty"` // World Wide Name + Type string `json:"type,omitempty"` // Transport type: sata, sas, nvme + SizeBytes int64 `json:"sizeBytes,omitempty"` // Capacity in bytes (0 when unknown) + Temperature int `json:"temperature"` // Temperature in Celsius + Health string `json:"health,omitempty"` // PASSED, FAILED, UNKNOWN + Standby bool `json:"standby,omitempty"` // True if disk was in standby Attributes *SMARTAttributes `json:"attributes,omitempty"` LastUpdated time.Time `json:"lastUpdated"` // When this reading was taken } @@ -157,7 +158,11 @@ type smartctlJSON struct { OUI uint64 `json:"oui"` ID uint64 `json:"id"` } `json:"wwn"` - SmartStatus *struct { + UserCapacity struct { + Bytes int64 `json:"bytes"` + } `json:"user_capacity"` + NVMeTotalCapacity int64 `json:"nvme_total_capacity"` + SmartStatus *struct { Passed bool `json:"passed"` } `json:"smart_status,omitempty"` Temperature struct { @@ -250,6 +255,7 @@ func CollectSMARTLocal(ctx context.Context, diskExclude []string) ([]DiskSMART, continue } if smart != nil { + refineLinuxBlockDeviceIdentity(smart, target) results = append(results, *smart) } } @@ -710,6 +716,123 @@ func isFreeBSDDiskDeviceName(name string) bool { return false } +// refineLinuxBlockDeviceIdentity rewrites a freshly collected SMART reading so +// that its device identity and size reflect the underlying block device rather +// than the smartctl scan target. smartctl --scan-open reports NVMe disks by their +// controller char device (/dev/nvme0), but the stable, user-visible identity is +// the namespace block device (/dev/nvme0n1) — the same name Proxmox's disks/list +// and /sys/block expose. It also backfills the capacity from /sys/block, the +// authoritative size source, so the agent no longer depends on a fragile +// filesystem-usage match on the server side. +func refineLinuxBlockDeviceIdentity(smart *DiskSMART, target smartctlTarget) { + if smart == nil || runtimeGOOS != "linux" { + return + } + // Disks addressed behind a multiplexing controller (megaraid,7; cciss,1; + // areca,1/1; ...) all share a single /dev path, so the smartctl scan label is + // the only thing that disambiguates them and /sys/block describes the array, + // not the member. Leave those as-is and trust the smartctl-reported capacity. + if isMultiplexedDeviceType(target.DeviceType) { + return + } + block := canonicalBlockDeviceForScanPath(target.Path) + if block == "" { + return + } + smart.Device = block + if size := blockDeviceSizeBytes(block); size > 0 { + smart.SizeBytes = size + } +} + +// isMultiplexedDeviceType reports whether a smartctl -d type addresses a member +// disk behind a controller (e.g. "megaraid,7"), as opposed to a directly +// attached device ("", "sat", "nvme", "scsi", "sat,auto"). +func isMultiplexedDeviceType(deviceType string) bool { + idx := strings.IndexByte(deviceType, ',') + if idx < 0 || idx+1 >= len(deviceType) { + return false + } + next := deviceType[idx+1] + return next >= '0' && next <= '9' +} + +// canonicalBlockDeviceForScanPath maps a smartctl scan target to its canonical +// /sys/block device name. NVMe controllers (nvmeN) resolve to their first +// namespace (nvmeNnM); every other device keeps its basename. +func canonicalBlockDeviceForScanPath(scanPath string) string { + name := filepath.Base(strings.TrimSpace(scanPath)) + if name == "" || name == "." || name == string(filepath.Separator) { + return "" + } + if isNVMeControllerName(name) { + if ns := firstNVMeNamespace(name); ns != "" { + return ns + } + } + return name +} + +// isNVMeControllerName reports whether name is an NVMe controller char device +// (e.g. "nvme0") rather than a namespace block device (e.g. "nvme0n1"). +func isNVMeControllerName(name string) bool { + return hasNumericSuffix(name, "nvme") +} + +// firstNVMeNamespace returns the lowest-numbered namespace block device for an +// NVMe controller (e.g. "nvme0" -> "nvme0n1"), or "" when none is found. +func firstNVMeNamespace(controller string) string { + entries, err := readDir("/sys/block") + if err != nil { + return "" + } + prefix := controller + "n" + best := "" + for _, entry := range entries { + name := strings.TrimSpace(entry.Name()) + if !strings.HasPrefix(name, prefix) { + continue + } + // Require a pure namespace (nvme0n1), not a partition (nvme0n1p1). + if suffix := name[len(prefix):]; suffix == "" || !isAllDigits(suffix) { + continue + } + if best == "" || name < best { + best = name + } + } + return best +} + +// blockDeviceSizeBytes reads /sys/block//size, which the kernel always +// reports in 512-byte sectors regardless of the physical block size. +func blockDeviceSizeBytes(name string) int64 { + if name == "" { + return 0 + } + data, err := smartctlReadFile(filepath.Join("/sys/block", name, "size")) + if err != nil { + return 0 + } + sectors, err := strconv.ParseInt(strings.TrimSpace(string(data)), 10, 64) + if err != nil || sectors <= 0 { + return 0 + } + return sectors * 512 +} + +func isAllDigits(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} + func hasNumericSuffix(name, prefix string) bool { if !strings.HasPrefix(name, prefix) || len(name) == len(prefix) { return false @@ -1002,6 +1125,15 @@ func parseSMARTOutput(output []byte, target smartctlTarget) (*DiskSMART, error) result.WWN = formatWWN(smartData.WWN.NAA, smartData.WWN.OUI, smartData.WWN.ID) } + // Capacity straight from the device smartctl just queried. On Linux this is + // refined to the authoritative /sys/block value in CollectSMARTLocal; here it + // is the cross-platform fallback so non-Linux hosts still report a size. + if smartData.NVMeTotalCapacity > 0 { + result.SizeBytes = smartData.NVMeTotalCapacity + } else if smartData.UserCapacity.Bytes > 0 { + result.SizeBytes = smartData.UserCapacity.Bytes + } + if smartData.Temperature.Current > 0 { result.Temperature = smartData.Temperature.Current } else if smartData.NVMeSmartHealthInformationLog != nil && smartData.NVMeSmartHealthInformationLog.Temperature > 0 { diff --git a/internal/hostagent/smartctl_size_test.go b/internal/hostagent/smartctl_size_test.go new file mode 100644 index 000000000..05b0a62a9 --- /dev/null +++ b/internal/hostagent/smartctl_size_test.go @@ -0,0 +1,211 @@ +package hostagent + +import ( + "io/fs" + "os" + "testing" +) + +func TestParseSMARTOutputParsesNVMeCapacity(t *testing.T) { + output := []byte(`{ + "device": {"name": "/dev/nvme0", "type": "nvme", "protocol": "NVMe"}, + "model_name": "KINGSTON SNV3S2000G", + "serial_number": "50026B7282A0FB69", + "nvme_total_capacity": 2000398934016, + "smart_status": {"passed": true}, + "temperature": {"current": 37} + }`) + + result, err := parseSMARTOutput(output, smartctlTarget{Path: "/dev/nvme0", DeviceType: "nvme"}) + if err != nil { + t.Fatalf("parseSMARTOutput returned error: %v", err) + } + if result.SizeBytes != 2000398934016 { + t.Errorf("SizeBytes = %d, want 2000398934016", result.SizeBytes) + } +} + +func TestParseSMARTOutputParsesUserCapacity(t *testing.T) { + output := []byte(`{ + "device": {"name": "/dev/sda", "type": "sat", "protocol": "ATA"}, + "model_name": "INTEL SSDSC2BW240A4", + "serial_number": "CVDA000000000000", + "user_capacity": {"bytes": 240057409536}, + "smart_status": {"passed": true}, + "temperature": {"current": 30} + }`) + + result, err := parseSMARTOutput(output, smartctlTarget{Path: "/dev/sda", DeviceType: "sat"}) + if err != nil { + t.Fatalf("parseSMARTOutput returned error: %v", err) + } + if result.SizeBytes != 240057409536 { + t.Errorf("SizeBytes = %d, want 240057409536", result.SizeBytes) + } +} + +func TestIsNVMeControllerName(t *testing.T) { + cases := map[string]bool{ + "nvme0": true, + "nvme11": true, + "nvme0n1": false, // namespace, not controller + "nvme0n1p1": false, // partition + "sda": false, + "nvme": false, + "": false, + } + for name, want := range cases { + if got := isNVMeControllerName(name); got != want { + t.Errorf("isNVMeControllerName(%q) = %v, want %v", name, got, want) + } + } +} + +func TestCanonicalBlockDeviceForScanPath(t *testing.T) { + origReadDir := readDir + t.Cleanup(func() { readDir = origReadDir }) + + readDir = func(string) ([]os.DirEntry, error) { + return []os.DirEntry{ + fakeDirEntry{name: "nvme0n1"}, + fakeDirEntry{name: "nvme0n1p1"}, // partition must be ignored + fakeDirEntry{name: "nvme1n1"}, + fakeDirEntry{name: "sda"}, + }, nil + } + + cases := map[string]string{ + "/dev/nvme0": "nvme0n1", // controller resolves to its namespace + "/dev/nvme1": "nvme1n1", + "/dev/nvme0n1": "nvme0n1", // already a namespace + "/dev/sda": "sda", + } + for scanPath, want := range cases { + if got := canonicalBlockDeviceForScanPath(scanPath); got != want { + t.Errorf("canonicalBlockDeviceForScanPath(%q) = %q, want %q", scanPath, got, want) + } + } +} + +func TestCanonicalBlockDeviceControllerWithoutNamespaceKeepsName(t *testing.T) { + origReadDir := readDir + t.Cleanup(func() { readDir = origReadDir }) + + readDir = func(string) ([]os.DirEntry, error) { + return []os.DirEntry{fakeDirEntry{name: "nvme0n1p1"}}, nil // only a partition exists + } + + if got := canonicalBlockDeviceForScanPath("/dev/nvme0"); got != "nvme0" { + t.Errorf("canonicalBlockDeviceForScanPath(/dev/nvme0) = %q, want fallback nvme0", got) + } +} + +func TestBlockDeviceSizeBytes(t *testing.T) { + origReadFile := smartctlReadFile + t.Cleanup(func() { smartctlReadFile = origReadFile }) + + smartctlReadFile = func(path string) ([]byte, error) { + switch path { + case "/sys/block/nvme0n1/size": + return []byte("3907029168\n"), nil + case "/sys/block/sda/size": + return []byte("468862128\n"), nil + default: + return nil, fs.ErrNotExist + } + } + + if got := blockDeviceSizeBytes("nvme0n1"); got != 2000398934016 { + t.Errorf("blockDeviceSizeBytes(nvme0n1) = %d, want 2000398934016", got) + } + if got := blockDeviceSizeBytes("sda"); got != 240057409536 { + t.Errorf("blockDeviceSizeBytes(sda) = %d, want 240057409536", got) + } + if got := blockDeviceSizeBytes("missing"); got != 0 { + t.Errorf("blockDeviceSizeBytes(missing) = %d, want 0", got) + } +} + +func TestRefineLinuxBlockDeviceIdentity(t *testing.T) { + origGOOS := runtimeGOOS + origReadDir := readDir + origReadFile := smartctlReadFile + t.Cleanup(func() { + runtimeGOOS = origGOOS + readDir = origReadDir + smartctlReadFile = origReadFile + }) + + runtimeGOOS = "linux" + readDir = func(string) ([]os.DirEntry, error) { + return []os.DirEntry{fakeDirEntry{name: "nvme0n1"}, fakeDirEntry{name: "sda"}}, nil + } + smartctlReadFile = func(path string) ([]byte, error) { + switch path { + case "/sys/block/nvme0n1/size": + return []byte("3907029168"), nil + default: + return nil, fs.ErrNotExist + } + } + + // NVMe controller scan target rewrites to the namespace and gains the size. + smart := &DiskSMART{Device: "nvme0 [nvme]"} + refineLinuxBlockDeviceIdentity(smart, smartctlTarget{Path: "/dev/nvme0", DeviceType: "nvme"}) + if smart.Device != "nvme0n1" { + t.Errorf("Device = %q, want nvme0n1", smart.Device) + } + if smart.SizeBytes != 2000398934016 { + t.Errorf("SizeBytes = %d, want 2000398934016", smart.SizeBytes) + } + + // A smartctl-reported capacity is preserved when /sys/block has no size. + smart = &DiskSMART{Device: "sda [sat]", SizeBytes: 240057409536} + refineLinuxBlockDeviceIdentity(smart, smartctlTarget{Path: "/dev/sda", DeviceType: "sat"}) + if smart.Device != "sda" { + t.Errorf("Device = %q, want sda", smart.Device) + } + if smart.SizeBytes != 240057409536 { + t.Errorf("SizeBytes = %d, want fallback 240057409536", smart.SizeBytes) + } + + // Disks behind a multiplexing controller keep their disambiguating label. + smart = &DiskSMART{Device: "sda [megaraid,7]", SizeBytes: 480103981056} + refineLinuxBlockDeviceIdentity(smart, smartctlTarget{Path: "/dev/sda", DeviceType: "megaraid,7"}) + if smart.Device != "sda [megaraid,7]" { + t.Errorf("Device = %q, want unchanged megaraid label", smart.Device) + } + if smart.SizeBytes != 480103981056 { + t.Errorf("SizeBytes = %d, want preserved smartctl capacity", smart.SizeBytes) + } +} + +func TestIsMultiplexedDeviceType(t *testing.T) { + cases := map[string]bool{ + "megaraid,7": true, + "cciss,1": true, + "areca,1/1": true, + "aacraid,0": true, + "sat": false, + "nvme": false, + "": false, + "sat,auto": false, + } + for dt, want := range cases { + if got := isMultiplexedDeviceType(dt); got != want { + t.Errorf("isMultiplexedDeviceType(%q) = %v, want %v", dt, got, want) + } + } +} + +func TestRefineLinuxBlockDeviceIdentityNonLinuxNoOp(t *testing.T) { + origGOOS := runtimeGOOS + t.Cleanup(func() { runtimeGOOS = origGOOS }) + runtimeGOOS = "darwin" + + smart := &DiskSMART{Device: "nvme0 [nvme]"} + refineLinuxBlockDeviceIdentity(smart, smartctlTarget{Path: "/dev/nvme0", DeviceType: "nvme"}) + if smart.Device != "nvme0 [nvme]" { + t.Errorf("Device = %q, want unchanged on non-linux", smart.Device) + } +} diff --git a/internal/models/models.go b/internal/models/models.go index 155c4e3ec..b0399c1b7 100644 --- a/internal/models/models.go +++ b/internal/models/models.go @@ -360,15 +360,16 @@ func (s HostSensorSummary) NormalizeCollections() HostSensorSummary { // HostDiskSMART represents S.M.A.R.T. data for a disk from a host agent. type HostDiskSMART struct { - Device string `json:"device"` // Device name (e.g., sda) - Model string `json:"model,omitempty"` // Disk model - Serial string `json:"serial,omitempty"` // Serial number - WWN string `json:"wwn,omitempty"` // World Wide Name - Type string `json:"type,omitempty"` // Transport type: sata, sas, nvme - Temperature int `json:"temperature"` // Temperature in Celsius - Health string `json:"health,omitempty"` // PASSED, FAILED, UNKNOWN - Standby bool `json:"standby,omitempty"` // True if disk was in standby - Pool string `json:"pool,omitempty"` // ZFS pool this disk belongs to (empty if not a ZFS member) + Device string `json:"device"` // Block device name (e.g., sda, nvme0n1) + Model string `json:"model,omitempty"` // Disk model + Serial string `json:"serial,omitempty"` // Serial number + WWN string `json:"wwn,omitempty"` // World Wide Name + Type string `json:"type,omitempty"` // Transport type: sata, sas, nvme + SizeBytes int64 `json:"sizeBytes,omitempty"` // Capacity in bytes (0 when unknown) + Temperature int `json:"temperature"` // Temperature in Celsius + Health string `json:"health,omitempty"` // PASSED, FAILED, UNKNOWN + Standby bool `json:"standby,omitempty"` // True if disk was in standby + Pool string `json:"pool,omitempty"` // ZFS pool this disk belongs to (empty if not a ZFS member) Attributes *SMARTAttributes `json:"attributes,omitempty"` } diff --git a/internal/monitoring/monitor_helpers.go b/internal/monitoring/monitor_helpers.go index 4ee894bad..80ebdb31a 100644 --- a/internal/monitoring/monitor_helpers.go +++ b/internal/monitoring/monitor_helpers.go @@ -563,6 +563,7 @@ func convertAgentSMARTToModels(smart []agentshost.DiskSMART) []models.HostDiskSM Serial: disk.Serial, WWN: disk.WWN, Type: disk.Type, + SizeBytes: disk.SizeBytes, Temperature: disk.Temperature, Health: disk.Health, Standby: disk.Standby, diff --git a/internal/unifiedresources/adapters.go b/internal/unifiedresources/adapters.go index b18c0724b..226137dce 100644 --- a/internal/unifiedresources/adapters.go +++ b/internal/unifiedresources/adapters.go @@ -624,10 +624,16 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res } } - sizeBytes := int64(0) + // The agent reports the authoritative disk capacity (from /sys/block); the + // filesystem-usage match is only a last-resort fallback for older agents that + // predate the SizeBytes field. matchedDisk.Total is a filesystem size, not a + // whole-disk size, so it is never preferred over a real capacity. + sizeBytes := disk.SizeBytes used := "" if matchedDisk != nil { - sizeBytes = matchedDisk.Total + if sizeBytes <= 0 { + sizeBytes = matchedDisk.Total + } used = strings.TrimSpace(matchedDisk.Mountpoint) } unraidDisk := matchUnraidDisk(host.Unraid, disk) diff --git a/internal/unifiedresources/adapters_test.go b/internal/unifiedresources/adapters_test.go index c5f021693..3a7657252 100644 --- a/internal/unifiedresources/adapters_test.go +++ b/internal/unifiedresources/adapters_test.go @@ -662,6 +662,59 @@ func TestResourceFromHostSMARTDiskNormalizesLegacyUnraidKiBSize(t *testing.T) { } } +func TestResourceFromHostSMARTDiskPrefersReportedSize(t *testing.T) { + const diskBytes = int64(2000398934016) + host := models.Host{ + ID: "pve-host", + Hostname: "pve", + LastSeen: time.Now().UTC(), + // A filesystem-usage entry whose Total is NOT the whole-disk size. The + // adapter must not pick this up over the agent-reported capacity. + Disks: []models.Disk{ + {Device: "/dev/nvme0n1p2", Mountpoint: "/", Total: 100_000_000_000}, + }, + Sensors: models.HostSensorSummary{ + SMART: []models.HostDiskSMART{ + {Device: "nvme0n1", Serial: "50026B72FB69", Type: "nvme", Health: "PASSED", SizeBytes: diskBytes}, + }, + }, + } + + resource, _ := resourceFromHostSMARTDisk(host, host.Sensors.SMART[0]) + if resource.PhysicalDisk == nil { + t.Fatal("expected physical disk metadata") + } + if resource.PhysicalDisk.SizeBytes != diskBytes { + t.Fatalf("sizeBytes = %d, want reported capacity %d", resource.PhysicalDisk.SizeBytes, diskBytes) + } +} + +func TestResourceFromHostSMARTDiskFallsBackToFilesystemSize(t *testing.T) { + const fsBytes = int64(240057409536) + host := models.Host{ + ID: "pve-host", + Hostname: "pve", + LastSeen: time.Now().UTC(), + Disks: []models.Disk{ + {Device: "/dev/sda", Mountpoint: "/data", Total: fsBytes}, + }, + Sensors: models.HostSensorSummary{ + SMART: []models.HostDiskSMART{ + // Legacy agent with no SizeBytes: fall back to the filesystem match. + {Device: "sda", Serial: "INTEL-1", Type: "sata", Health: "PASSED"}, + }, + }, + } + + resource, _ := resourceFromHostSMARTDisk(host, host.Sensors.SMART[0]) + if resource.PhysicalDisk == nil { + t.Fatal("expected physical disk metadata") + } + if resource.PhysicalDisk.SizeBytes != fsBytes { + t.Fatalf("sizeBytes = %d, want filesystem fallback %d", resource.PhysicalDisk.SizeBytes, fsBytes) + } +} + func TestResourceFromKubernetesDeployment_PopulatesMetricsUnderMockMode(t *testing.T) { mockruntime.SetEnabled(true) t.Cleanup(func() { mockruntime.SetEnabled(false) }) diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 440f3c74d..db797a250 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -2353,7 +2353,7 @@ func mergePhysicalDiskData(existing *PhysicalDiskMeta, incoming *PhysicalDiskMet } merged := *existing - if incoming.DevPath != "" { + if shouldReplacePhysicalDiskDevPath(merged.DevPath, incoming.DevPath) { merged.DevPath = incoming.DevPath } if incoming.Model != "" { @@ -2423,6 +2423,37 @@ func mergePhysicalDiskData(existing *PhysicalDiskMeta, incoming *PhysicalDiskMet return &merged } +// shouldReplacePhysicalDiskDevPath decides whether an incoming devPath should +// overwrite the existing one when two sources describe the same disk. A canonical +// /dev/ path (as produced by the Proxmox disks/list poll) must never be +// downgraded to a non-canonical scan label such as the host agent's +// "nvme0 [nvme]" controller display name. When both are equally canonical the +// incoming value wins, preserving the previous last-writer behaviour. +func shouldReplacePhysicalDiskDevPath(existing, incoming string) bool { + incoming = strings.TrimSpace(incoming) + if incoming == "" { + return false + } + existing = strings.TrimSpace(existing) + if existing == "" { + return true + } + if isCanonicalDevPath(existing) && !isCanonicalDevPath(incoming) { + return false + } + return true +} + +// isCanonicalDevPath reports whether devPath is a clean /dev/ path with +// no whitespace or scan-type suffix (e.g. "/dev/nvme0n1", not "nvme0 [nvme]"). +func isCanonicalDevPath(devPath string) bool { + devPath = strings.TrimSpace(devPath) + if !strings.HasPrefix(devPath, "/dev/") { + return false + } + return !strings.ContainsAny(devPath, " \t[") +} + func shouldReplacePhysicalDiskHealth(existing, incoming string) bool { incoming = strings.ToUpper(strings.TrimSpace(incoming)) if incoming == "" { diff --git a/internal/unifiedresources/registry_test.go b/internal/unifiedresources/registry_test.go index 0018e3888..ab94287ac 100644 --- a/internal/unifiedresources/registry_test.go +++ b/internal/unifiedresources/registry_test.go @@ -1891,6 +1891,93 @@ func TestResourceRegistry_IngestSnapshotMergesAgentAndProxmoxPhysicalDisksByIden } } +// Regression for issue #1483: a legacy host agent reports an NVMe disk keyed by +// its controller ("nvme0 [nvme]") with no size, while the Proxmox disks/list poll +// reports the canonical namespace ("/dev/nvme0n1") with the real capacity. The +// merge must keep the authoritative Proxmox devPath and size, and only enrich +// with the agent's SMART temperature. +func TestResourceRegistry_AgentDiskDoesNotClobberProxmoxDevPath(t *testing.T) { + rr := NewRegistry(nil) + now := time.Date(2026, 5, 29, 12, 0, 0, 0, time.UTC) + const nvmeSize = int64(2000398934016) + + rr.IngestSnapshot(models.StateSnapshot{ + PhysicalDisks: []models.PhysicalDisk{ + { + ID: "pve-disk-1", + Node: "pve", + Instance: "pve", + DevPath: "/dev/nvme0n1", + Model: "KINGSTON SNV3S2000G", + Serial: "SERIAL-NVME-0", + Type: "nvme", + Size: nvmeSize, + Health: "PASSED", + LastChecked: now, + }, + }, + Hosts: []models.Host{ + { + ID: "host-pve", + Hostname: "pve", + Status: "online", + LastSeen: now, + Sensors: models.HostSensorSummary{ + SMART: []models.HostDiskSMART{ + { + Device: "nvme0 [nvme]", // legacy controller label, no size + Model: "KINGSTON SNV3S2000G", + Serial: "SERIAL-NVME-0", + Type: "nvme", + Temperature: 37, + Health: "PASSED", + }, + }, + }, + }, + }, + }) + + disks := rr.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 1 { + t.Fatalf("expected 1 merged physical disk resource, got %d", len(disks)) + } + disk := disks[0] + if disk.PhysicalDisk == nil { + t.Fatal("expected physical disk metadata") + } + if disk.PhysicalDisk.DevPath != "/dev/nvme0n1" { + t.Fatalf("devPath = %q, want canonical /dev/nvme0n1 (agent label must not clobber)", disk.PhysicalDisk.DevPath) + } + if disk.PhysicalDisk.SizeBytes != nvmeSize { + t.Fatalf("sizeBytes = %d, want authoritative Proxmox capacity %d", disk.PhysicalDisk.SizeBytes, nvmeSize) + } + if disk.PhysicalDisk.Temperature != 37 { + t.Fatalf("temperature = %d, want enriched agent SMART value 37", disk.PhysicalDisk.Temperature) + } +} + +func TestShouldReplacePhysicalDiskDevPath(t *testing.T) { + cases := []struct { + name string + existing string + incoming string + want bool + }{ + {"canonical not downgraded by scan label", "/dev/nvme0n1", "nvme0 [nvme]", false}, + {"canonical not downgraded by bare token", "/dev/nvme0n1", "nvme0n1", false}, + {"scan label upgraded to canonical", "nvme0 [nvme]", "/dev/nvme0n1", true}, + {"empty existing always replaced", "", "nvme0 [nvme]", true}, + {"empty incoming never replaces", "/dev/nvme0n1", "", false}, + {"both canonical last writer wins", "/dev/sda", "/dev/sdb", true}, + } + for _, tc := range cases { + if got := shouldReplacePhysicalDiskDevPath(tc.existing, tc.incoming); got != tc.want { + t.Errorf("%s: shouldReplacePhysicalDiskDevPath(%q, %q) = %v, want %v", tc.name, tc.existing, tc.incoming, got, tc.want) + } + } +} + func TestResourceRegistry_IngestSnapshotPropagatesUnraidDiskRole(t *testing.T) { rr := NewRegistry(nil) now := time.Date(2026, 3, 7, 12, 0, 0, 0, time.UTC) diff --git a/pkg/agents/host/report.go b/pkg/agents/host/report.go index 243637b3e..d8ff89b46 100644 --- a/pkg/agents/host/report.go +++ b/pkg/agents/host/report.go @@ -121,15 +121,16 @@ type Sensors struct { // DiskSMART represents S.M.A.R.T. data for a single disk. type DiskSMART struct { - Device string `json:"device"` // Device path (e.g., sda) - Model string `json:"model,omitempty"` // Disk model - Serial string `json:"serial,omitempty"` // Serial number - WWN string `json:"wwn,omitempty"` // World Wide Name - Type string `json:"type,omitempty"` // Transport type: sata, sas, nvme - Temperature int `json:"temperature"` // Temperature in Celsius - Health string `json:"health,omitempty"` // PASSED, FAILED, UNKNOWN - Standby bool `json:"standby,omitempty"` // True if disk was in standby - Pool string `json:"pool,omitempty"` // ZFS pool this disk belongs to (empty if not a ZFS member) + Device string `json:"device"` // Block device name (e.g., sda, nvme0n1) + Model string `json:"model,omitempty"` // Disk model + Serial string `json:"serial,omitempty"` // Serial number + WWN string `json:"wwn,omitempty"` // World Wide Name + Type string `json:"type,omitempty"` // Transport type: sata, sas, nvme + SizeBytes int64 `json:"sizeBytes,omitempty"` // Capacity in bytes (0 when unknown) + Temperature int `json:"temperature"` // Temperature in Celsius + Health string `json:"health,omitempty"` // PASSED, FAILED, UNKNOWN + Standby bool `json:"standby,omitempty"` // True if disk was in standby + Pool string `json:"pool,omitempty"` // ZFS pool this disk belongs to (empty if not a ZFS member) Attributes *SMARTAttributes `json:"attributes,omitempty"` }