diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 368015df21..e812d7a3fe 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -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) diff --git a/internal/monitoring/issue1595_collection_trust_test.go b/internal/monitoring/issue1595_collection_trust_test.go index 7a52f71d8b..06e72b6a29 100644 --- a/internal/monitoring/issue1595_collection_trust_test.go +++ b/internal/monitoring/issue1595_collection_trust_test.go @@ -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) + } + } + } + }) + } +} diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index efabca099f..a799e8182f 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -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) {