From 9b644b3d49275bd6c693653287749fc84df34987 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 11:23:34 +0100 Subject: [PATCH] Stop a replaced PVE disk inheriting the old disk's evidence The disk poller keeps evidence it could not collect this poll by looking up the previous record, first by serial and WWN, then by the Proxmox source ID and device token. Those last two name a slot, not a disk, and a replacement in the same slot keeps both. So a replacement reporting a different serial or WWN took the old disk's retained temperature, controller, pool and I/O. A replacement record with an empty serial also took the old serial. The registry keys disks on serial, so the new disk became the old disk's canonical resource and SMART metric series, and each later poll carried the borrowed serial forward. Match on hardware identity through diskinventory.HardwareIdentityMatch, serial first then WWN, and refuse a slot match when both records carry a serial or WWN that share no value. Missing identity is still not a conflict, so Proxmox's literal "unknown" and other placeholders keep the slot's evidence as before. The registry's merged disk view keeps the host agent's WWN, which the agent spells as unpadded NAA 5 fields ("5-c50-a1b2c3d4") while Proxmox reports "0x5000c500a1b2c3d4". HardwareIdentityMatch now re-pads that spelling for WWN fields, so the same disk seen by both reporters is not mistaken for a replacement. --- .../v6/internal/subsystems/monitoring.md | 25 +++ internal/monitoring/monitor_pve.go | 45 +++- .../physical_disk_roundtrip_test.go | 197 ++++++++++++++++++ pkg/diskinventory/identity.go | 42 +++- pkg/diskinventory/identity_test.go | 26 +++ 5 files changed, 323 insertions(+), 12 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 1806e08bbc..3295543f67 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1494,6 +1494,31 @@ later coarse healthy value and lets explicit SMART endurance replace contradictory Proxmox wearout. Missing permission, ambiguous identity, standby, and absent SMART fields remain neutral rather than borrowing telemetry from another disk. +Retaining evidence a disk poll could not collect follows the same identity +rule. `previousPhysicalDiskEvidence` looks up the earlier record by serial, then +by WWN, each compared through `diskinventory.HardwareIdentityMatch` against +both fields of the earlier record. `HardwareIdentityMatch`, which the +registry's linked-disk join also uses, may additionally re-pad a WWN field +written in the host agent's unpadded NAA 5 `naa-oui-id` spelling +(`5-c50-a1b2c3d4`, from smartctl's WWN fields) to the 16-digit form udev and +Proxmox report; serials are never re-padded. The registry's merged disk view +keeps the agent's WWN spelling, so without it one disk seen by both reporters +would read as two. The Proxmox source ID and the device token +name a slot, not a disk, so a slot match is refused when both records carry a +serial or WWN and no value is shared between them. Missing identity is not +conflicting identity: Proxmox's literal `unknown` and other placeholders count +as unreported, and a record whose serial arrives empty keeps the slot's last +known serial, still marked `missing`. Before this rule, a replacement in the +same slot took the previous occupant's retained temperature, controller, pool +and I/O, and a replacement record with an empty serial also took the old +serial, which keyed it to the old disk's canonical resource and SMART metric +series, so every later poll carried the borrowed serial forward +(`TestPhysicalDiskReplacementInSameSlotDoesNotInheritPreviousSerial`, +`TestPreviousPhysicalDiskEvidenceRejectsSlotMatchAcrossConflictingIdentity`). +This lookup is the only place the rule applies: a replacement that reports no +usable serial or WWN cannot be told apart from the same disk and still +inherits, and the linked agent's SMART merge, which runs first, can still fill +or promote a serial from a stale SMART row it matched by device path. Negative percentage-used counters remain unknown; values above 100 clamp to exhausted before deriving remaining life, so invalid or over-limit controller data cannot wrap into a fabricated healthy value. diff --git a/internal/monitoring/monitor_pve.go b/internal/monitoring/monitor_pve.go index 581307f263..4456d56f65 100644 --- a/internal/monitoring/monitor_pve.go +++ b/internal/monitoring/monitor_pve.go @@ -1284,6 +1284,15 @@ func cloneDiskIO(in *models.DiskIO) *models.DiskIO { return &out } +// previousPhysicalDiskEvidence finds the previous poll's record of the disk +// now observed, so evidence the current poll could not collect can be +// retained. Stable hardware identity is tried first. The source ID and device +// token name a slot, not a disk: a replacement in the same slot keeps both, so +// a slot match is rejected when the two records carry hardware identities that +// disagree. Without that guard, a replacement inherits the previous +// occupant's retained readings, and one whose record arrives without a serial +// inherits the old serial too, which keys it to the old disk's canonical +// resource, and every later poll carries that serial forward. func previousPhysicalDiskEvidence(current models.PhysicalDisk, previous []models.PhysicalDisk) (models.PhysicalDisk, bool) { sameScope := func(candidate models.PhysicalDisk) bool { return strings.EqualFold(strings.TrimSpace(candidate.Instance), strings.TrimSpace(current.Instance)) && @@ -1305,25 +1314,27 @@ func previousPhysicalDiskEvidence(current models.PhysicalDisk, previous []models return matched, found } - if serial := strings.TrimSpace(current.Serial); diskinventory.IsUsableHardwareID(serial) { + // Serial before WWN, as before; each may match either field of the + // earlier record because reporters disagree on which one holds the + // durable identifier. + for _, identity := range [][2]string{{current.Serial, ""}, {"", current.WWN}} { if matched, ok := uniqueMatch(func(candidate models.PhysicalDisk) bool { - return candidate.Serial != "" && strings.EqualFold(strings.TrimSpace(candidate.Serial), serial) + return diskinventory.HardwareIdentityMatch(identity[0], identity[1], candidate.Serial, candidate.WWN) }); ok { return matched, true } } - if wwn := strings.TrimSpace(current.WWN); diskinventory.IsUsableHardwareID(wwn) { - if matched, ok := uniqueMatch(func(candidate models.PhysicalDisk) bool { - return candidate.WWN != "" && strings.EqualFold(strings.TrimSpace(candidate.WWN), wwn) - }); ok { - return matched, true + sameSlotOccupant := func(matched models.PhysicalDisk, ok bool) (models.PhysicalDisk, bool) { + if !ok || physicalDiskHardwareIdentitiesConflict(current, matched) { + return models.PhysicalDisk{}, false } + return matched, true } if id := strings.TrimSpace(current.ID); id != "" { if matched, ok := uniqueMatch(func(candidate models.PhysicalDisk) bool { return strings.TrimSpace(candidate.ID) == id }); ok { - return matched, true + return sameSlotOccupant(matched, ok) } } @@ -1331,7 +1342,7 @@ func previousPhysicalDiskEvidence(current models.PhysicalDisk, previous []models if device == "" { return models.PhysicalDisk{}, false } - return uniqueMatch(func(candidate models.PhysicalDisk) bool { + return sameSlotOccupant(uniqueMatch(func(candidate models.PhysicalDisk) bool { return normalizeSMARTDeviceIdentifier(candidate.DevPath) == device && diskTopologyCompatible( current.Controller, @@ -1339,7 +1350,21 @@ func previousPhysicalDiskEvidence(current models.PhysicalDisk, previous []models candidate.Controller, candidate.Target, ) - }) + })) +} + +// physicalDiskHardwareIdentitiesConflict reports whether both records carry a +// serial or WWN and none of them names the same disk. Missing identity is not +// conflicting identity, so a disk whose identity went unreported this poll +// still matches its slot. Matching a record against itself applies the same +// normalization as the comparison, so a placeholder such as a 0x-prefixed +// all-zero WWN counts as unreported rather than as a different disk. +func physicalDiskHardwareIdentitiesConflict(left, right models.PhysicalDisk) bool { + hasID := func(disk models.PhysicalDisk) bool { + return diskinventory.HardwareIdentityMatch(disk.Serial, disk.WWN, disk.Serial, disk.WWN) + } + return hasID(left) && hasID(right) && + !diskinventory.HardwareIdentityMatch(left.Serial, left.WWN, right.Serial, right.WWN) } func preserveUnavailablePhysicalDiskEvidence(current, previous models.PhysicalDisk) models.PhysicalDisk { diff --git a/internal/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go index 733e7d092c..b23736f09f 100644 --- a/internal/monitoring/physical_disk_roundtrip_test.go +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -3,6 +3,7 @@ package monitoring import ( "context" "encoding/json" + "sync" "testing" "time" @@ -717,3 +718,199 @@ func TestSilentUnraidAgentDiskTemperatureIsRetainedButNotCollected(t *testing.T) } } } + +type slotDiskPVEClient struct { + fakeStorageClient + mu sync.Mutex + disk proxmox.Disk +} + +func (client *slotDiskPVEClient) setDisk(disk proxmox.Disk) { + client.mu.Lock() + defer client.mu.Unlock() + client.disk = disk +} + +func (client *slotDiskPVEClient) GetDisks(context.Context, string) ([]proxmox.Disk, error) { + client.mu.Lock() + defer client.mu.Unlock() + return []proxmox.Disk{client.disk}, nil +} + +// A disk swapped into the same slot keeps the Proxmox source ID, which is +// path-shaped. When a disk record arrives with the replacement's WWN but an +// empty serial, the poller must not copy the previous occupant's serial onto +// it: the registry keys the disk on that serial, so the replacement would take +// over the old disk's canonical resource, and every later poll would copy the +// borrowed serial forward again. Current Proxmox spells a missing serial as +// "unknown", which the poller records as reported; an empty serial comes from +// producers that omit the field, such as the host-agent fallback rows. The +// fake client below stands in for any of them. +func TestPhysicalDiskReplacementInSameSlotDoesNotInheritPreviousSerial(t *testing.T) { + t.Setenv("PULSE_DATA_DIR", t.TempDir()) + const ( + oldSerial = "ZR5OLD0001" + oldWWN = "0x5000c500aaaa0001" + newWWN = "0x5000c500bbbb0002" + ) + type pollResult struct { + disk models.PhysicalDisk + view *unifiedresources.PhysicalDiskView + } + newHarness := func(t *testing.T) (*slotDiskPVEClient, func() pollResult) { + t.Helper() + state := models.NewState() + state.UpdateNodesForInstance("pve1", []models.Node{{ + ID: "pve1-node1", Name: "node1", Instance: "pve1", Status: "online", LastSeen: time.Now(), + }}) + alertManager := alerts.NewManager() + t.Cleanup(alertManager.Stop) + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + m := &Monitor{ + state: state, resourceStore: adapter, alertManager: alertManager, + startTime: time.Now().Add(-time.Hour), lastPhysicalDiskPoll: make(map[string]time.Time), + } + client := &slotDiskPVEClient{} + poll := func() pollResult { + t.Helper() + adapter.PopulateFromSnapshot(state.GetSnapshot()) + started := time.Now() + delete(m.lastPhysicalDiskPoll, "pve1") + m.maybePollPhysicalDisksAsync(context.Background(), "pve1", &config.PVEInstance{}, client, + []proxmox.Node{{Node: "node1", Status: "online"}}, map[string]string{"node1": "online"}, nil) + deadline := time.Now().Add(3 * time.Second) + for { + if disks := state.GetSnapshot().PhysicalDisks; len(disks) == 1 && !disks[0].LastChecked.Before(started) { + adapter.PopulateFromSnapshot(state.GetSnapshot()) + views := adapter.PhysicalDisks() + if len(views) != 1 { + t.Fatalf("canonical physical disks = %d, want one", len(views)) + } + return pollResult{disk: disks[0], view: views[0]} + } + if time.Now().After(deadline) { + t.Fatal("physical disk poll did not land in state") + } + time.Sleep(10 * time.Millisecond) + } + } + return client, poll + } + pveDisk := func(serial, wwn string) proxmox.Disk { + return proxmox.Disk{ + DevPath: "/dev/sdb", Model: "ST4000NM000A", Serial: serial, WWN: wwn, + Type: "hdd", Health: "PASSED", Wearout: 100, Size: 4000787030016, + } + } + + t.Run("replacement reporting a different WWN", func(t *testing.T) { + client, poll := newHarness(t) + client.setDisk(pveDisk(oldSerial, oldWWN)) + first := poll() + if first.disk.Serial != oldSerial || first.view.Serial() != oldSerial { + t.Fatalf("original disk serial: state %q, canonical %q", first.disk.Serial, first.view.Serial()) + } + + client.setDisk(pveDisk("", newWWN)) + for round := 1; round <= 2; round++ { + got := poll() + if got.disk.Serial != "" || got.view.Serial() != "" { + t.Fatalf("poll %d: replacement inherited serial: state %q, canonical %q", round, got.disk.Serial, got.view.Serial()) + } + if got.disk.WWN != newWWN { + t.Fatalf("poll %d: replacement WWN = %q, want %q", round, got.disk.WWN, newWWN) + } + if got.view.ID() == first.view.ID() { + t.Fatalf("poll %d: replacement took over the old disk's canonical resource %q", round, got.view.ID()) + } + if got.disk.Collection == nil || got.disk.Collection.Serial.State != diskinventory.FieldMissing { + t.Fatalf("poll %d: replacement serial collection = %+v, want missing", round, got.disk.Collection) + } + } + }) + + t.Run("same disk keeps its serial while Proxmox omits it", func(t *testing.T) { + for name, missing := range map[string]proxmox.Disk{ + "WWN still reported": pveDisk("", oldWWN), + "no identity at all": pveDisk("", ""), + } { + t.Run(name, func(t *testing.T) { + client, poll := newHarness(t) + client.setDisk(pveDisk(oldSerial, oldWWN)) + first := poll() + + client.setDisk(missing) + for round := 1; round <= 2; round++ { + got := poll() + if got.disk.Serial != oldSerial || got.view.ID() != first.view.ID() { + t.Fatalf("poll %d: same disk lost its identity: serial %q, resource %q (was %q)", + round, got.disk.Serial, got.view.ID(), first.view.ID()) + } + if got.disk.Collection == nil || got.disk.Collection.Serial.State != diskinventory.FieldMissing { + t.Fatalf("poll %d: retained serial presented as collected: %+v", round, got.disk.Collection) + } + } + }) + } + }) +} + +// Previous evidence matches a disk by stable hardware identity first and by +// slot (source ID, then device token) only when no reported serial or WWN +// says the slot now holds a different disk. The earlier record is the +// registry's merged view, which keeps a linked agent's WWN spelling; Proxmox +// reports a missing serial or WWN as the literal "unknown". +func TestPreviousPhysicalDiskEvidenceRejectsSlotMatchAcrossConflictingIdentity(t *testing.T) { + const slotID = "pve1-node1--dev-sdb" + previous := models.PhysicalDisk{ + ID: slotID, Instance: "pve1", Node: "node1", DevPath: "/dev/sdb", + Serial: "ZR5OLD0001", WWN: "5-c50-aaaa0001", Temperature: 38, StorageGroup: "tank", + } + // Identity cases sit on another device path, so only identity can match. + moved := func(serial, wwn string) models.PhysicalDisk { + return models.PhysicalDisk{ID: "pve1-node1--dev-sdc", DevPath: "/dev/sdc", Serial: serial, WWN: wwn} + } + inSlot := func(serial, wwn string) models.PhysicalDisk { + return models.PhysicalDisk{ID: slotID, DevPath: "/dev/sdb", Serial: serial, WWN: wwn} + } + for _, tc := range []struct { + name string + current models.PhysicalDisk + want bool + wantSerial string + }{ + {"renamed device, same serial", moved("ZR5OLD0001", "unknown"), true, "ZR5OLD0001"}, + {"renamed device, PVE spelling of the agent WWN", moved("unknown", "0x5000c500aaaa0001"), true, "unknown"}, + {"renamed device, old serial reported as WWN", moved("", "ZR5OLD0001"), true, "ZR5OLD0001"}, + {"renamed device, different disk", moved("ZR5NEW0002", "0x5000c500bbbb0002"), false, ""}, + {"same slot, PVE spelling of the agent WWN", inSlot("unknown", "0x5000c500aaaa0001"), true, "unknown"}, + {"same slot, SAS address serial with matching WWN", inSlot("5000c500aaaa0003", "0x5000c500aaaa0001"), true, "5000c500aaaa0003"}, + {"same slot, identity unreported", inSlot("unknown", "unknown"), true, "unknown"}, + {"same slot, serial omitted by producer", inSlot("", ""), true, "ZR5OLD0001"}, + {"same slot, placeholder WWN", inSlot("", "0x0000000000000000"), true, "ZR5OLD0001"}, + {"same slot, different WWN", inSlot("", "0x5000c500bbbb0002"), false, ""}, + {"same slot, different serial", inSlot("ZR5NEW0002", "unknown"), false, ""}, + {"device token only, different WWN", models.PhysicalDisk{ID: "agent-fallback-id", DevPath: "/dev/sdb", WWN: "0x5000c500bbbb0002"}, false, ""}, + } { + t.Run(tc.name, func(t *testing.T) { + current := tc.current + current.Instance, current.Node = "pve1", "node1" + current.Collection = &diskinventory.CollectionStatus{ + Serial: diskinventory.Missing("proxmox_disks", "disk serial was not reported"), + Temperature: diskinventory.Unsupported("proxmox_disks", "no temperature"), + Pool: diskinventory.Unavailable("proxmox_zfs", "query failed"), + } + matched, ok := previousPhysicalDiskEvidence(current, []models.PhysicalDisk{previous}) + if ok != tc.want { + t.Fatalf("previous evidence match = %v, want %v", ok, tc.want) + } + if !ok { + return + } + got := preserveUnavailablePhysicalDiskEvidence(current, matched) + if got.Serial != tc.wantSerial || got.Temperature != previous.Temperature || got.StorageGroup != previous.StorageGroup { + t.Fatalf("same disk evidence: serial %q (want %q), temperature %d, pool %q", got.Serial, tc.wantSerial, got.Temperature, got.StorageGroup) + } + }) + } +} diff --git a/pkg/diskinventory/identity.go b/pkg/diskinventory/identity.go index 36ef7cba64..5f5fe8f7d8 100644 --- a/pkg/diskinventory/identity.go +++ b/pkg/diskinventory/identity.go @@ -93,14 +93,52 @@ func normalizeHardwareID(value string) string { return value } +// normalizeHardwareWWN is normalizeHardwareID for a value reported as a WWN. +// The host agent also spells smartctl's NAA 5 WWN as unpadded naa-oui-id hex +// fields, which only a WWN field can carry, so only WWNs are re-padded. +func normalizeHardwareWWN(value string) string { + value = normalizeHardwareID(value) + if expanded, ok := expandNAA5FieldWWN(value); ok { + return expanded + } + return value +} + +// expandNAA5FieldWWN rewrites the host agent's "5-c50-a1b2c3d4" spelling of a +// NAA 5 WWN (4-bit NAA, 24-bit OUI, 36-bit vendor ID, each printed as unpadded +// hex) into the 16-digit form udev and PVE report, "5000c500a1b2c3d4". +// Anything that does not fit those field widths is left alone. +func expandNAA5FieldWWN(value string) (string, bool) { + fields := strings.Split(value, "-") + if len(fields) != 3 || fields[0] != "5" || + len(fields[1]) > 6 || len(fields[2]) > 9 || + !isLowerHex(fields[1]) || !isLowerHex(fields[2]) { + return "", false + } + return "5" + strings.Repeat("0", 6-len(fields[1])) + fields[1] + + strings.Repeat("0", 9-len(fields[2])) + fields[2], true +} + +func isLowerHex(value string) bool { + if value == "" { + return false + } + for _, char := range value { + if (char < '0' || char > '9') && (char < 'a' || char > 'f') { + return false + } + } + return true +} + // HardwareIdentityMatch reports whether two disk observations carry the same // stable hardware identity. Serial and WWN are folded together because // sources disagree on which field holds the durable identifier: PVE reports a // RAID array volume's NAA identifier as its serial while smartctl reports the // same value as a naa.-prefixed WWN with no serial at all. func HardwareIdentityMatch(leftSerial, leftWWN, rightSerial, rightWWN string) bool { - left := [2]string{normalizeHardwareID(leftSerial), normalizeHardwareID(leftWWN)} - right := [2]string{normalizeHardwareID(rightSerial), normalizeHardwareID(rightWWN)} + left := [2]string{normalizeHardwareID(leftSerial), normalizeHardwareWWN(leftWWN)} + right := [2]string{normalizeHardwareID(rightSerial), normalizeHardwareWWN(rightWWN)} for _, l := range left { if l == "" { continue diff --git a/pkg/diskinventory/identity_test.go b/pkg/diskinventory/identity_test.go index f3477f7384..73c5576127 100644 --- a/pkg/diskinventory/identity_test.go +++ b/pkg/diskinventory/identity_test.go @@ -87,6 +87,32 @@ func TestHardwareIdentityMatch(t *testing.T) { bWWN: "0025385b91501234", want: true, }, + { + // internal/hostagent formatWWN spells smartctl's NAA 5 triple + // with unpadded fields; PVE reports udev's ID_WWN. + name: "agent naa-oui-id wwn matches pve 0x wwn", + aWWN: "5-c50-a1b2c3d4", + bWWN: "0x5000c500a1b2c3d4", + want: true, + }, + { + name: "agent naa-oui-id wwn of another disk stays distinct", + aWWN: "5-c50-a1b2c3d5", + bWWN: "0x5000c500a1b2c3d4", + want: false, + }, + { + name: "serial shaped like the field form is not re-padded", + aSerial: "5-c50-a1b2c3d4", + bWWN: "0x5000c500a1b2c3d4", + want: false, + }, + { + name: "field form wider than NAA 5 is not re-padded", + aWWN: "5-c50-123456789abc", + bWWN: "0x5000c50123456789abc", + want: false, + }, { name: "truncated udev wwn never matches full sibling identifier", aWWN: "0x61866da053481f00",