gsadmin/pulse
1
0
Fork 0
mirror of https://github.com/rcourtman/Pulse.git synced 2026-10-23 20:25:56 +00:00
Code Issues Packages Projects Releases Wiki Activity

Refuse a seeded disk slot mapping when the disk was replaced

Browse Source 
ingestRecord reuses an existing source-key mapping before any identity
matching, and for physical disks it checked only the type and parent
scope. Proxmox keys a disk by its slot, 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 that disk's serial, WWN, temperature
and failed health.

A Proxmox disk observation now refuses the mapping when the two disks
carry hardware identities naming different disks
(diskinventory.HardwareIdentityConflict). Only fields reported on both
sides count. Differing serials do not count for SAS (Proxmox may report
the SAS address, #1595), or when the mapped disk was merged with an agent
report and Proxmox did not read the drive's own serial, by the rule
monitoring's SMART merge already uses. Collection status cannot decide
that: a legacy agent's restored serial keeps Proxmox's status.

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.
Without that, a disk keyed by a USB bridge serial re-merged at once.

No current caller replays Proxmox disk records over seeded mappings: the
monitor rebuilds a fresh registry each poll. This closes the registry
contract the seeded-mapping comment promises.
This commit is contained in:
rcourtman
2026-10-07 13:49:31 +01:00
parent a85bdcdc9a
commit a5b2a5a4c4
3 changed files with 275 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/release-control/v6/internal/subsystems/unified-resources.md
+46 -3
View File
@@ -6442,9 +6442,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)
internal/monitoring/issue1595_collection_trust_test.go
+161
View File
@@ -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)
}
}
}
})
}
}
internal/unifiedresources/registry.go
+68 -7
View File
@@ -2882,22 +2882,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 {
@@ -2912,7 +2920,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 {
@@ -2937,6 +2945,9 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res
resource.ID = rr.chooseNewID(resource.Type, identity, source, sourceID)
normalizeResourceRelationships(&resource)
if resource.ID == refusedDiskID {
resource.ID = candidateID
}
if existing := rr.resources[resource.ID]; existing != nil {
if onlyMissing {
return ""
@@ -3186,6 +3197,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) {