Refuse a seeded disk slot mapping when the disk was replaced (#2667)

`ingestRecord` reuses an existing source-key mapping before any identity
matching. For physical disks it checked only the type and parent scope.
Proxmox keys a disk by its slot (`ProxmoxPhysicalDiskSourceID`, e.g.
`pve1-node1--dev-sdb`), so in a registry seeded from persisted unified
resources, a disk swapped into the slot mapped straight back to the
previous disk's canonical resource. Where an agent report had been
merged in, `mergeInto` also kept the old disk's serial, WWN, temperature
and FAILED health. This was found while reviewing #2658, and its
contract note said the seeded mapping was not covered.

**Change.** A Proxmox disk observation now refuses the mapping when the
two disks carry hardware identities naming different disks
(`diskinventory.HardwareIdentityConflict` from #2658):
- Only fields reported on both sides count. A missing serial or WWN,
including Proxmox's literal `unknown`, keeps the mapping.
- Differing serials do not count for SAS, because Proxmox may report the
SAS address there (#1595).
- They also do not count when the mapped disk was merged with an agent
report and Proxmox did not read the drive's own serial. This is the same
rule monitoring's `hostAgentSMARTSerialComparable` applies: NVMe on both
sides, or agent SATA under Proxmox vendor `ATA`, and never a Proxmox
`usb` disk.
- The refused key falls through to the linked-disk join, `findMatch` and
`chooseNewID`, but none of them may merge it back into the refused disk.
An ID landing on it is replaced with the key's source-specific ID.

**Reachability.** No current caller replays Proxmox disk records over a
seeded mapping. The monitor rebuilds a fresh registry each poll. The
resource API, host-continuity and Patrol registries seeded from unified
resources do not replay Proxmox disk records. This closes the registry
contract rather than a reported symptom.

**Proof.** `TestRegistrySeededSlotMappingRefusesReplacedDisk` (in
`internal/monitoring/issue1595_collection_trust_test.go`) builds the
previous disk through `IngestSnapshot` with fixtures shaped like
`monitor_pve`'s output, rehydrates with `IngestResources(List())`, polls
Proxmox, then rehydrates a second time.
- 5 replacement cases: SATA with and without a new WWN, SAS, a
USB-enclosure swap ingested Proxmox-first, and a disk Proxmox alone
reports.
- 8 same-disk controls: re-ingested, no serial, a SAS address serial,
SAS without a WWN, a USB bridge serial, a bridge serial under vendor
`ATA`, a SCSI designator, and a SCSI designator from an agent without
collection status.

Negative controls via `go test -overlay` each fail only their intended
cases: the whole fix removed, the refused-disk exclusion removed, serial
comparability always true or always false, the Proxmox-only branch
removed, a collection-status provenance rule instead, and the USB veto
removed. The combined tree of main, #2660 and this change also passes
`internal/unifiedresources` and the monitoring disk tests.

**Residuals (in the unified-resources contract).**
- Drives that share a serial they do not own, such as a USB bridge's,
still read as one disk, as they do in identity matching.
- The refused disk keeps its Proxmox facet until a rebuild, so the
slot's PVE alert reference resolves as ambiguous rather than to either
disk.
This commit is contained in:
rcourtman
2026-10-07 15:38:50 +01:00
committed by GitHub
3 changed files with 275 additions and 10 deletions
@@ -6517,9 +6517,52 @@ conflict): the agent's row may be the slot's previous occupant, retained by a
silent agent, and the join would hand the replacement that disk's serial,
readings and canonical resource. A hardware identity match still joins on its
own, and without a WWN on either side the path fallback stands. The seeded
source mapping reused after a restart is a separate path this does not cover.
`TestRegistrySASPathJoinRefusesContradictingWWN` covers the stale row and both
same-disk shapes; monitoring's SMART merge applies the matching guard first.
source mapping a rehydrated registry reuses applies the same refusal (next
section). `TestRegistrySASPathJoinRefusesContradictingWWN` covers the stale
row and both same-disk shapes; monitoring's SMART merge applies the matching
guard first.
### Seeded slot mapping refuses a replaced disk
`ingestRecord` honours a source key's existing mapping, such as one
`IngestResources` seeds from persisted unified resources, before any
identity matching. Proxmox keys a disk by its slot
(`ProxmoxPhysicalDiskSourceID`), so a disk swapped into the slot arrived
under the previous disk's key, and `mergeInto` kept that disk's canonical ID
and, where an agent report had been merged in, its serial, WWN, temperature
and failed health. A Proxmox disk observation now refuses the mapping when
the two carry hardware identities naming different disks
(`proxmoxDiskSlotHoldsAnotherDisk`: `HardwareIdentityConflict` after
`HardwareIdentityMatch`). Only fields reported on both sides count, so a
missing serial or WWN, including Proxmox's literal `unknown`, is no evidence.
Differing serials are no evidence when either side is SAS, because Proxmox
may report the SAS address there (#1595), or when the mapped disk was merged
with an agent report, whose serial `mergeInto` may have kept (collection
status does not reliably say which serial it holds), unless Proxmox read the
drive's own serial too, by the rule monitoring's
`hostAgentSMARTSerialComparable` applies: NVMe on both sides, or an agent
SATA disk under Proxmox vendor `ATA` that Proxmox does not type `usb`, not a
SCSI designator or a USB bridge's serial. A refused key falls through to the linked-disk join,
`findMatch` and `chooseNewID` like an unmapped one, except that none of them
may merge it into the refused disk: an ID that lands on it is replaced with
the key's source-specific ID. Agreement still wins, as it does in identity
matching, so drives that share a serial they do not own, such as a USB
bridge's, read as one disk: a second swap in the same enclosure joins the
first replacement, which reported the same bridge serial. That belongs to
identity matching, not this guard. The refused disk keeps its Proxmox
facet and is not updated, so until the registry is rebuilt both disks answer
to the slot's PVE alert reference, and canonical reference resolution treats
that reference as ambiguous rather than handing it to either. A later
rehydration may seed the slot key to the old disk again, and the refusal
repeats. No current caller ingests Proxmox disk records over a seeded
mapping: the monitor rebuilds a fresh registry each poll, and the resource
API, host-continuity and Patrol registries seeded from unified resources do
not replay Proxmox disk records. This closes the registry contract, not a
reported symptom. `TestRegistrySeededSlotMappingRefusesReplacedDisk` covers
SATA (with and without a new WWN), SAS, USB-enclosure and Proxmox-only
replacements, plus eight same-disk controls (including a SCSI designator
from an agent without collection status and a USB bridge serial under
Proxmox vendor `ATA`), each through two rehydrations.
### Drawer tab selection survives a transient snapshot change (#1723)
@@ -612,3 +612,164 @@ func TestRegistrySASPathJoinRefusesContradictingWWN(t *testing.T) {
})
}
}
// A registry rehydrated from a persisted unified snapshot seeds each source
// key's mapping before the next poll arrives. Proxmox keys a disk by its slot,
// so a disk swapped into the slot arrives under the previous disk's key. The
// mapping is refused when the two carry hardware identities naming different
// disks, and nothing else may merge the replacement back into the old disk,
// so it gets its own canonical resource instead of the old disk's ID, serial,
// temperature and failed health. Identity missing on one side keeps the
// mapping, and so does a serial that may not be the drive's own (a SAS
// address, a SCSI designator, a USB bridge's serial) set against the agent's.
func TestRegistrySeededSlotMappingRefusesReplacedDisk(t *testing.T) {
const slotSourceID = "pve1-node1--dev-sdb"
// Each kind is the slot's previous disk as Proxmox and the agent reported
// it; the registry merged the two reports by WWN or by path, unless the
// case has Proxmox alone report it.
kinds := map[string]struct{ agentType, vendor, proxmoxType, proxmoxSerial string }{
"sata": {agentType: "sata", vendor: "ATA", proxmoxType: "hdd", proxmoxSerial: "ZR5A0001"},
"sas": {agentType: "sas", vendor: "SEAGATE", proxmoxType: "hdd", proxmoxSerial: "5000c500aaaa0003"},
"usb": {agentType: "usb", vendor: "JMicron", proxmoxType: "usb", proxmoxSerial: "BRIDGE0001"},
"scsi": {agentType: "sata", vendor: "SEAGATE", proxmoxType: "hdd", proxmoxSerial: "Z1Z0VPD0001"},
"usb-ata": {agentType: "sata", vendor: "ATA", proxmoxType: "usb", proxmoxSerial: "BRIDGE0001"},
}
for _, tc := range []struct {
name string
kind string
proxmoxFirst bool
proxmoxOnly bool
legacyAgent bool
serial string
wwn string
replacedDrive bool
}{
{name: "replacement disk", kind: "sata", serial: "ZR5B0002", wwn: "0x5000c500bbbb0002", replacedDrive: true},
{name: "replacement disk without a WWN", kind: "sata", serial: "ZR5B0002", wwn: "unknown", replacedDrive: true},
{name: "same disk", kind: "sata", serial: "ZR5A0001", wwn: "0x5000c500aaaa0001"},
{name: "same disk without a serial", kind: "sata", serial: "unknown", wwn: "0x5000c500aaaa0001"},
{name: "SAS disk whose Proxmox serial is a SAS address", kind: "sas", serial: "5000c500aaaa0003", wwn: "0x5000c500aaaa0001"},
{name: "SAS disk without a Proxmox WWN", kind: "sas", serial: "5000c500aaaa0003", wwn: "unknown"},
{name: "USB disk whose Proxmox serial is the bridge's", kind: "usb", serial: "BRIDGE0001", wwn: "unknown"},
{name: "USB bridge serial under Proxmox vendor ATA", kind: "usb-ata", serial: "BRIDGE0001", wwn: "unknown"},
{name: "disk whose Proxmox serial is a SCSI designator", kind: "scsi", serial: "Z1Z0VPD0001", wwn: "unknown"},
{name: "SAS replacement disk", kind: "sas", serial: "5000c500bbbb0003", wwn: "0x5000c500bbbb0002", replacedDrive: true},
{name: "drive swapped in the same USB enclosure", kind: "usb", proxmoxFirst: true,
serial: "BRIDGE0001", wwn: "0x5000c500bbbb0002", replacedDrive: true},
{name: "SCSI designator from an agent without collection status", kind: "scsi", legacyAgent: true,
serial: "Z1Z0VPD0001", wwn: "unknown"},
{name: "replacement Proxmox alone reports, without a WWN", kind: "scsi", proxmoxOnly: true,
serial: "Z1Z0VPD0002", wwn: "unknown", replacedDrive: true},
} {
t.Run(tc.name, func(t *testing.T) {
kind := kinds[tc.kind]
now := time.Now()
node := models.Node{
ID: "pve1-node1", Name: "node1", Instance: "pve1", Status: "online",
LastSeen: now, LinkedAgentID: "agent-1",
}
host := models.Host{
ID: "agent-1", Hostname: "node1", LinkedNodeID: "pve1-node1", Status: "online", LastSeen: now,
Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{
Device: "/dev/sdb", Serial: "ZR5A0001", WWN: "5-c50-aaaa0001", Type: kind.agentType,
Health: "FAILED", Temperature: 55,
Collection: &diskinventory.CollectionStatus{
Serial: diskinventory.Available("smartctl"),
Temperature: diskinventory.Available("smartctl"),
},
}}},
}
// As monitor_pve builds the slot's row from disks/list.
slotDisk := func(serial, wwn, health string) models.PhysicalDisk {
disk := models.PhysicalDisk{
ID: slotSourceID, Node: "node1", Instance: "pve1", DevPath: "/dev/sdb", Vendor: kind.vendor,
Serial: proxmoxReportedDiskIdentity(serial), WWN: proxmoxReportedDiskIdentity(wwn),
Type: kind.proxmoxType, Health: health, Wearout: -1, LastChecked: now,
Collection: &diskinventory.CollectionStatus{
Serial: diskinventory.Missing("proxmox_disks", "disk serial was not reported"),
},
}
if disk.Serial != "" {
disk.Collection.Serial = diskinventory.Available("proxmox_disks")
}
return disk
}
previousDisk := slotDisk(kind.proxmoxSerial, "0x5000c500aaaa0001", "FAILED")
previous := unifiedresources.NewRegistry(nil)
if tc.proxmoxFirst {
previous.IngestSnapshot(models.StateSnapshot{
Nodes: []models.Node{node}, PhysicalDisks: []models.PhysicalDisk{previousDisk},
})
}
hosts := []models.Host{host}
if tc.proxmoxOnly {
hosts = nil
}
if tc.legacyAgent {
host.Sensors.SMART[0].Collection = nil
}
previous.IngestSnapshot(models.StateSnapshot{
Nodes: []models.Node{node}, Hosts: hosts, PhysicalDisks: []models.PhysicalDisk{previousDisk},
})
persisted := previous.ListByType(unifiedresources.ResourceTypePhysicalDisk)
if len(persisted) != 1 || persisted[0].PhysicalDisk == nil ||
!hasIssue1595Source(persisted[0].Sources, unifiedresources.SourceProxmox) ||
hasIssue1595Source(persisted[0].Sources, unifiedresources.SourceAgent) == tc.proxmoxOnly {
t.Fatalf("want one disk merged from Proxmox and the agent (Proxmox alone: %v), got %+v", tc.proxmoxOnly, persisted)
}
oldID, oldDisk := persisted[0].ID, *persisted[0].PhysicalDisk
// Restart: rehydrate from the persisted resources, then poll
// Proxmox before the agent has reported again. A second restart
// rehydrates from the result, where both disks may claim the slot.
current := slotDisk(tc.serial, tc.wwn, "PASSED")
seed := previous.List()
slotID := ""
for restart := 1; restart <= 2; restart++ {
rehydrated := unifiedresources.NewRegistry(nil)
rehydrated.IngestResources(seed)
rehydrated.IngestSnapshot(models.StateSnapshot{
Nodes: []models.Node{node}, PhysicalDisks: []models.PhysicalDisk{current},
})
seed = rehydrated.List()
var slot *unifiedresources.Resource
disks := rehydrated.ListByType(unifiedresources.ResourceTypePhysicalDisk)
for index := range disks {
for _, target := range rehydrated.SourceTargets(disks[index].ID) {
if target.Source == unifiedresources.SourceProxmox && target.SourceID == slotSourceID {
slot = &disks[index]
}
}
}
if slot == nil || slot.PhysicalDisk == nil {
t.Fatalf("restart %d: no disk holds the Proxmox slot key among %d disks", restart, len(disks))
}
if slotID != "" && slot.ID != slotID {
t.Fatalf("restart %d: slot moved from %s to %s", restart, slotID, slot.ID)
}
slotID = slot.ID
if !tc.replacedDrive {
if slot.ID != oldID || len(disks) != 1 {
t.Fatalf("restart %d: slot disk = %s with %d disks, want the persisted disk %s alone",
restart, slot.ID, len(disks), oldID)
}
continue
}
if slot.ID == oldID || len(disks) != 2 {
t.Fatalf("restart %d: replacement took the persisted disk's canonical ID %s (%d disks)", restart, oldID, len(disks))
}
if slot.PhysicalDisk.Serial != current.Serial || slot.PhysicalDisk.WWN != current.WWN ||
slot.PhysicalDisk.Temperature != 0 || slot.PhysicalDisk.Health != "PASSED" {
t.Fatalf("restart %d: replacement took the persisted disk's identity or readings: %+v", restart, slot.PhysicalDisk)
}
for _, disk := range disks {
if disk.ID == oldID && (disk.PhysicalDisk == nil ||
disk.PhysicalDisk.Serial != oldDisk.Serial || disk.PhysicalDisk.WWN != oldDisk.WWN) {
t.Fatalf("restart %d: persisted disk was rewritten: %+v", restart, disk.PhysicalDisk)
}
}
}
})
}
}
+68 -7
View File
@@ -2919,22 +2919,30 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res
// Rehydrated registries seed exact source mappings from the persisted
// unified snapshot. Honor that durable mapping before attempting weaker
// identity correlation, but fail closed if a colliding source key belongs
// to a different physical-disk parent.
// to a different physical-disk parent. A Proxmox disk key names a slot, so
// it is also refused when the slot now holds a different disk, and no
// later step may merge the observation into the refused disk either.
refusedDiskID := ""
if mappedID := rr.bySource[source][sourceID]; mappedID != "" {
if existing := rr.resources[mappedID]; existing != nil &&
existing.Type == resource.Type &&
(resource.Type != ResourceTypePhysicalDisk ||
physicalDiskMatchScopeCompatible(existing, &resource)) {
if onlyMissing {
return ""
if source == SourceProxmox && resource.Type == ResourceTypePhysicalDisk &&
proxmoxDiskSlotHoldsAnotherDisk(existing, &resource) {
refusedDiskID = existing.ID
} else {
if onlyMissing {
return ""
}
rr.mergeInto(existing, resource, source, sourceID)
return existing.ID
}
rr.mergeInto(existing, resource, source, sourceID)
return existing.ID
}
}
// Linked resources must be mutually linked to avoid one-sided/ambiguous auto-merges.
if linked := rr.resolveLinkedResource(source, sourceID, resource); linked != "" {
if linked := rr.resolveLinkedResource(source, sourceID, resource); linked != "" && linked != refusedDiskID {
existing := rr.resources[linked]
if existing != nil {
if onlyMissing {
@@ -2949,7 +2957,7 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res
candidateID := rr.sourceSpecificID(resource.Type, source, sourceID)
if resource.Type == ResourceTypeAgent || resource.Type == ResourceTypePhysicalDisk {
if match, excluded := rr.findMatch(resource, candidateID); match != nil {
if match, excluded := rr.findMatch(resource, candidateID); match != nil && match.ResourceB != refusedDiskID {
existing := rr.resources[match.ResourceB]
if existing != nil {
if onlyMissing {
@@ -2978,6 +2986,9 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res
resource.ID = rr.physicalDiskIDForMachineLocked(resource.ID, candidateID, &resource, source, onlyMissing)
}
normalizeResourceRelationships(&resource)
if resource.ID == refusedDiskID {
resource.ID = candidateID
}
if existing := rr.resources[resource.ID]; existing != nil {
if onlyMissing {
return ""
@@ -3230,6 +3241,56 @@ func physicalDiskMatchScopeCompatible(existing, incoming *Resource) bool {
return identitiesShareHostname(existing.Identity, incoming.Identity)
}
// proxmoxDiskSlotHoldsAnotherDisk reports whether a Proxmox disk observed
// under a mapped slot key (ProxmoxPhysicalDiskSourceID) names a different disk
// than the resource the key maps to: the slot's disk was swapped, and merging
// would keep the previous disk's canonical ID and, where an agent report was
// merged in, its serial, WWN and readings. Only hardware identity reported on
// both sides counts, and differing serials only where
// physicalDiskMappedSerialsComparable says both are the drive's own.
func proxmoxDiskSlotHoldsAnotherDisk(existing, incoming *Resource) bool {
if existing.PhysicalDisk == nil || incoming.PhysicalDisk == nil {
return false
}
existingSerial, existingWWN := existing.PhysicalDisk.Serial, existing.PhysicalDisk.WWN
incomingSerial, incomingWWN := incoming.PhysicalDisk.Serial, incoming.PhysicalDisk.WWN
if diskinventory.HardwareIdentityMatch(existingSerial, existingWWN, incomingSerial, incomingWWN) {
return false
}
if !physicalDiskMappedSerialsComparable(existing, incoming) {
existingSerial, incomingSerial = "", ""
}
return diskinventory.HardwareIdentityConflict(existingSerial, existingWWN, incomingSerial, incomingWWN)
}
// physicalDiskMappedSerialsComparable reports whether differing serials on a
// mapped disk and a Proxmox observation under its slot key name different
// disks. Proxmox may report a SAS disk's transport address as its serial
// (#1595). A disk Proxmox alone reports carries Proxmox's serial, compared as
// is. A disk merged with an agent report may carry the agent's serial instead
// (mergeInto restores it, and collection status does not reliably say which),
// so its serial is compared only where Proxmox read the drive's own serial
// too, as monitoring's hostAgentSMARTSerialComparable decides: an NVMe
// controller serial, or an ATA drive's IDENTIFY serial, not a SCSI designator
// or a USB bridge's serial.
func physicalDiskMappedSerialsComparable(existing, incoming *Resource) bool {
switch {
case physicalDiskTypeIs(existing, "sas") || physicalDiskTypeIs(incoming, "sas"):
return false
case !hasDataSource(existing.Sources, SourceAgent):
return true
case physicalDiskTypeIs(incoming, "nvme"):
return physicalDiskTypeIs(existing, "nvme")
case physicalDiskTypeIs(incoming, "usb"):
return false
}
return physicalDiskTypeIs(existing, "sata") && strings.EqualFold(strings.TrimSpace(incoming.PhysicalDisk.Vendor), "ATA")
}
func physicalDiskTypeIs(disk *Resource, diskType string) bool {
return strings.EqualFold(strings.TrimSpace(disk.PhysicalDisk.DiskType), diskType)
}
func (rr *ResourceRegistry) resolveLinkedResource(source DataSource, sourceID string, resource Resource) string {
if resource.Type == ResourceTypePhysicalDisk &&
(source == SourceAgent || source == SourceProxmox) {