Stop a replaced PVE disk inheriting the old disk's evidence (#2648)

## Problem

The Proxmox disk poller keeps evidence it could not collect this poll by
looking up the previous poll's record of the disk: by serial, then WWN,
then the Proxmox source ID (`instance-node-devpath`), then the device
token. The source ID and device token 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 disk's
serial. The registry keys disks on serial, so the new disk became the
old disk's canonical resource and SMART metric series, and every later
poll carried the borrowed serial forward.

Current Proxmox reports a missing serial as the literal `unknown`, which
the poller records as reported, so the serial copy needs a producer that
sends an empty serial, such as the host-agent fallback rows. The
evidence copy happens with real Proxmox data.

## Change

- `previousPhysicalDiskEvidence` matches by hardware identity through
`diskinventory.HardwareIdentityMatch` (serial first, then WWN, each
against both fields of the earlier record), and refuses a slot match
when both records carry a serial or WWN and none agrees. Missing
identity is still not a conflict: placeholders such as `unknown`, and an
empty serial, 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`,
`internal/hostagent/smartctl.go` `formatWWN`) while Proxmox reports
`0x5000c500a1b2c3d4`. `HardwareIdentityMatch` now re-pads that spelling
for WWN fields only, so one disk seen by both reporters is not taken for
a replacement. The registry's linked-disk join uses the same comparison.

## Tests

- `TestPhysicalDiskReplacementInSameSlotDoesNotInheritPreviousSerial`
drives the poller and registry across two polls after a swap; on base
the replacement inherits the old serial and canonical resource. Its
controls keep a serial-omitting same disk on its resource.
-
`TestPreviousPhysicalDiskEvidenceRejectsSlotMatchAcrossConflictingIdentity`
covers renamed devices matched only by identity, PVE vs agent WWN
spellings, SAS address serials with a matching WWN, placeholders, and
conflicting serial/WWN by source ID and by device token.
- Negative controls via `go test -overlay`: on base the replacement,
conflict and renamed-device cases fail; with the guard but without the
WWN re-padding, the same-disk PVE/agent WWN cases fail.
- `pkg/diskinventory`, `internal/unifiedresources` and
`internal/monitoring` pass in full.

Follow-ups, not in this change: the host-agent SMART merge's device-path
fallback can still fill or promote a serial from a stale SMART row; QEMU
default serials (`drive-scsi0`) collapse nested Proxmox nodes' disks
into one resource; and the registry merges disks across parents on a
canonical-ID collision.
This commit is contained in:
rcourtman
2026-10-07 12:12:23 +01:00
committed by GitHub
5 changed files with 323 additions and 12 deletions
@@ -1508,6 +1508,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.
+35 -10
View File
@@ -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 {
@@ -3,6 +3,7 @@ package monitoring
import (
"context"
"encoding/json"
"sync"
"testing"
"time"
@@ -783,3 +784,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)
}
})
}
}
+40 -2
View File
@@ -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
+26
View File
@@ -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",