Stop a stale agent SMART row lending a replaced disk its identity

The linked host agent's SMART merge falls back to matching a Proxmox
disk by device path, and a path names a slot, not a disk. When a disk
is swapped before the agent reports again, or after the agent went
silent and kept its last rows, the row for the old disk matched the
replacement. It filled or, for SAS, promoted the old serial onto it,
which keyed the replacement to the old disk's canonical resource and
metric history, and lent it the old disk's temperature and failed
health. For SAS the registry's same-path join did the same at the
canonical level even when monitoring kept the rows apart.

Both now refuse a row whose identity contradicts the disk's, judged
only on what both producers report like for like. WWNs count for every
disk. Serials count only where Proxmox's serial is the drive's own, as
smartctl's is: NVMe namespaces, and disks presented with SCSI vendor
"ATA", whose serial udev's ata_id reads from IDENTIFY. Elsewhere
Proxmox's serial may be a SAS address, a SCSI designator or a USB
bridge's own serial (#1595). Reporter spellings are not disagreement:
udev writes whitespace in serials as underscores, its T10 designator
for an ATA drive puts "ATA" and the model before the drive serial, and
its ID_WWN keeps only the first 64 bits of an NAA 6 identifier. The WWN match step compares normalized identity too,
so the agent's 5-c50-a1b2c3d4 spelling finds the Proxmox
0x5000c500a1b2c3d4 disk.

Proxmox's disk inventory reports a serial or WWN udev cannot read as
the literal "unknown". The poller recorded it as a collected serial, so
the collection state hid the gap and the agent's serial never filled
it. It is now recorded as empty, and retained evidence carries only a
usable previous serial so an older record cannot restore it. Identity
keys already ignored the placeholder.
This commit is contained in:
rcourtman
2026-10-07 12:13:49 +01:00
parent 41cea2ffab
commit 8d00d54740
12 changed files with 643 additions and 20 deletions
@@ -237,6 +237,17 @@ state; the monitoring readback requires an actual Proxmox source observation.
This leaves Agent source identity, report admission and command authority
unchanged. The repeated-cycle regression is
`TestPhysicalDiskSkippedPollDoesNotPromoteAgentOnlySMARTToPVEInventory`.
A linked Agent's SMART row that matches a PVE disk only by device path is
refused when its WWN contradicts the disk's, or its serial does where both
producers read the drive's own serial (NVMe, and disks presented with SCSI
vendor `ATA`),
and the registry's SAS path join refuses a contradicting WWN, so a silent
Agent's retained row for a swapped-out disk lends the replacement neither its
serial, and with it the old disk's canonical resource, nor its readings.
Without comparable identity (a SAS, SCSI or USB replacement with no WWN on one
side) the row still matches. Report admission and the SMART wire contract are
unchanged; the regression is
`TestHostAgentSMARTRowForSwappedOutDiskDoesNotLendItsIdentity`.
Assistant historical metric wiring uses the current monitor's retained store
and registry metrics coordinates. Historical reads do not alter enrollment,
@@ -1508,6 +1508,51 @@ 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.
The device-path match is the last resort, and a path names a slot, not a
disk: the row there may describe the slot's previous occupant, because the
agent has not reported since a swap or went silent and keeps its last rows.
`mergeHostAgentSMARTIntoDisks` refuses such a row when its identity
contradicts the Proxmox disk's (`diskinventory.HardwareIdentityConflict`):
nothing agrees across serial and WWN, and a field both producers report like
for like disagrees. WWNs are compared for every disk. Serials are compared
only where Proxmox's serial is the drive's own, as smartctl's is: a Proxmox
disk typed `nvme` against an NVMe row, and one presented with SCSI vendor
`ATA` (libata, or a SAS HBA's SCSI-ATA translation, where udev's `ata_id`
reads IDENTIFY) and not typed `usb` or `sas` against an ATA row. Any other
Proxmox serial may be a SAS address or SCSI designator (#1595) or a USB
bridge's own serial, so it counts toward agreement only; a USB bridge that
does expose the drive's serial is excluded too unless it reports vendor `ATA`.
Reporter spellings are not disagreement: serials compare on letters and digits
only (udev writes whitespace as underscores), a T10 vendor designator for an
ATA drive (`ATA`, the model, then the serial) is no evidence against the serial
it ends with, and a WWN that begins with the other is no evidence (udev's `ID_WWN` keeps only
the first 64 bits of a 128-bit NAA 6 identifier). The refusal comes after the
unique-path check, so a second row on the path still makes it ambiguous. The
WWN match itself compares through `HardwareIdentityMatch`, so the agent's
`5-c50-a1b2c3d4` spelling finds the Proxmox `0x5000c500a1b2c3d4` disk. The
registry's SAS path join applies the same WWN refusal (unified-resources).
Proxmox's disk inventory (`PVE::Diskmanage`) writes the literal `unknown` for
a serial or WWN udev cannot read; the poller records it as empty, so the
serial's collection state reads `missing` and the agent's serial or WWN can
fill it, where the placeholder used to read as a collected serial. Retained
evidence carries a previous serial only when it is a usable identity, so a
record from before this change cannot restore the placeholder. Identity keys
already ignored the placeholder, so the empty value keys the same; a disk whose
serial the linked agent now fills keys its Proxmox SMART samples on that serial
instead of its WWN or path, as the agent's own samples already were. Before the
guard, the retained row of a swapped-out disk filled or promoted the old serial
onto the replacement, which keyed it to the old disk's canonical resource and
history, and lent it the old disk's temperature and failed health
(`TestHostAgentSMARTRowForSwappedOutDiskDoesNotLendItsIdentity`,
`TestHostAgentSMARTMergeRefusesPathMatchAcrossContradictingIdentity`,
`TestRegistrySASPathJoinRefusesContradictingWWN`,
`TestProxmoxUnknownDiskIdentityIsRecordedAsUnreported`,
`TestHardwareIdentityConflict`). Without comparable evidence there is nothing
to act on: a row without a usable serial or WWN still matches its path, and a
replacement whose serial is not comparable (SAS, SCSI, USB) still takes a
stale row's identity when either record lacks a WWN. The registry's seeded
source mapping, which a restart reuses by the slot-shaped Proxmox source ID,
does not apply this guard yet.
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
@@ -1529,10 +1574,9 @@ 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.
A replacement that reports no usable serial or WWN cannot be told apart from
the same disk and still inherits. The linked agent's SMART merge, which runs
first, refuses a contradicting device-path row on its own terms (above).
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.
@@ -6431,6 +6431,22 @@ ambiguity test rejects multiple opposite-source candidates in both directions.
These are synthetic source proofs, not appliance acceptance or a claim about
which collector produced a reporter's row.
### Linked SAS path join refuses a contradicting WWN
`resolveLinkedPhysicalDisk` falls back to joining a Proxmox disk to its linked
agent's disk on the same path when the agent reports SAS, because Proxmox may
report a SAS address as the serial (#1595). That path fallback is refused when
both sides carry WWNs that name different disks
(`diskinventory.HardwareIdentityConflict`, under which the agent's NAA field
spelling equals the Proxmox WWN and udev's 64-bit NAA 6 prefix is no
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.
### Drawer tab selection survives a transient snapshot change (#1723)
The shared resource detail drawer keeps the operator's selected tab while a
@@ -1,16 +1,21 @@
package monitoring
import (
"context"
"encoding/json"
"os"
"path/filepath"
"strings"
"testing"
"time"
"github.com/rcourtman/pulse-go-rewrite/internal/alerts"
"github.com/rcourtman/pulse-go-rewrite/internal/config"
"github.com/rcourtman/pulse-go-rewrite/internal/models"
"github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources"
"github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory"
"github.com/rcourtman/pulse-go-rewrite/pkg/metrics"
"github.com/rcourtman/pulse-go-rewrite/pkg/proxmox"
)
type issue1595TopologyFixture struct {
@@ -367,3 +372,243 @@ func TestWriteHostSMARTMetricsRecordsOnlyCollectedTemperatures(t *testing.T) {
}
}
}
// A SMART row found only by device path may describe the slot's previous
// occupant. It is refused when its identity contradicts the Proxmox disk's,
// judged only on what both producers report like for like: WWNs for every
// disk, serials only where Proxmox's serial is the drive's own, for NVMe and
// for disks the kernel reaches through libata (vendor "ATA"). A SAS disk's
// Proxmox serial may be a transport address and a USB bridge's is its own.
// Reporter spellings of one identity (udev's underscores, a T10 designator
// ending with the drive serial, the agent's NAA fields, udev's 64-bit NAA 6
// prefix) are not disagreement.
func TestHostAgentSMARTMergeRefusesPathMatchAcrossContradictingIdentity(t *testing.T) {
type mergeCase struct {
name string
disk models.PhysicalDisk
smart models.HostDiskSMART
// others are further rows on the same path, which make it ambiguous.
others []models.HostDiskSMART
merged bool
serial string
}
smartctlSerial := &diskinventory.CollectionStatus{Serial: diskinventory.Available("smartctl")}
pveDisk := func(device, vendor, serial, wwn, diskType string) models.PhysicalDisk {
return models.PhysicalDisk{
ID: "pve1-node1-" + device, Node: "node1", Instance: "pve1", DevPath: "/dev/" + device,
Vendor: vendor, Serial: serial, WWN: wwn, Type: diskType, Health: "PASSED",
}
}
smartRow := func(device, serial, wwn, diskType string) models.HostDiskSMART {
return models.HostDiskSMART{
Device: "/dev/" + device, Serial: serial, WWN: wwn, Type: diskType,
Health: "FAILED", Temperature: 47, Collection: diskinventory.CloneStatus(smartctlSerial),
}
}
for _, tc := range []mergeCase{
{
name: "SAS replacement beside a stale row with another WWN",
disk: pveDisk("sda", "SEAGATE", "5000c500b0000001", "0x5000c500b0000001", "unknown"),
smart: smartRow("sda", "ZR5OLD0001", "naa.5000c500a0000001", "sas"),
},
{
name: "SATA replacement beside a stale row with another serial",
disk: pveDisk("sdb", "ATA", "WD-NEW0001", "", "hdd"),
smart: smartRow("sdb", "WD-OLD0001", "", "sata"),
},
{
name: "same SAS disk whose agent row has no WWN",
disk: pveDisk("sdc", "SEAGATE", "5000c500a0000003", "0x5000c500a0000003", "unknown"),
smart: smartRow("sdc", "ZR5TESTA0003", "", "sas"),
merged: true, serial: "ZR5TESTA0003",
},
{
name: "same SAS disk renamed, agent WWN in smartctl NAA fields",
disk: pveDisk("sdd", "SEAGATE", "5000c500a0000004", "0x5000c500a0000004", "unknown"),
smart: smartRow("sdx", "ZR5TESTA0004", "5-c50-a0000004", "sas"),
merged: true, serial: "ZR5TESTA0004",
},
{
name: "same disk with udev's spelling of the serial",
disk: pveDisk("sde", "ATA", "WD-WX_1234", "", "hdd"),
smart: smartRow("sde", "WD-WX 1234", "", "sata"),
merged: true, serial: "WD-WX_1234",
},
{
name: "NVMe replacement whose serial looks like a SAS address",
disk: pveDisk("nvme0n1", "", "50026B7282A0FB69", "", "nvme"),
smart: smartRow("nvme0n1", "50026B7282A0FB11", "", "nvme"),
},
{
name: "ambiguous path where only one row contradicts",
disk: pveDisk("sdg", "ATA", "WD-NEW0007", "", "hdd"),
smart: smartRow("sdg", "WD-OLD0007", "", "sata"),
others: []models.HostDiskSMART{
smartRow("sdg", "", "", "sata"),
},
},
{
name: "same disk in a USB enclosure whose serial udev reports",
disk: pveDisk("sdh", "JMicron", "000000000000000F1E2D", "", "usb"),
smart: smartRow("sdh", "WD-WX0008", "", "sata"),
merged: true, serial: "000000000000000F1E2D",
},
{
name: "same SSD in a USB enclosure Proxmox types ssd",
disk: pveDisk("sdj", "ASMT", "00000000000000000009", "", "ssd"),
smart: smartRow("sdj", "S3Z9NB0K123456X", "", "sata"),
merged: true, serial: "00000000000000000009",
},
{
name: "same disk behind a T10 designator ending with its serial",
disk: pveDisk("sdi", "ATA", "ATA_ST4000NM000A-2HZ_ZC1ABCDE", "", "unknown"),
smart: smartRow("sdi", "ZC1ABCDE", "", "sata"),
merged: true, serial: "ATA_ST4000NM000A-2HZ_ZC1ABCDE",
},
{
name: "same NAA 6 volume with udev's 64-bit WWN",
disk: pveDisk("sdf", "HP", "PDNLH0BRH8V0AB", "0x600508b1001c4d5e", "hdd"),
smart: smartRow("sdf", "", "naa.600508b1001c4d5e7f8a9b0c1d2e3f40", "scsi"),
merged: true, serial: "PDNLH0BRH8V0AB",
},
} {
t.Run(tc.name, func(t *testing.T) {
got := mergeHostAgentSMARTIntoDisks(
[]models.PhysicalDisk{tc.disk},
[]models.Node{{Name: "node1", LinkedAgentID: "agent-1"}},
[]models.Host{{ID: "agent-1", Sensors: models.HostSensorSummary{
SMART: append([]models.HostDiskSMART{tc.smart}, tc.others...),
}}},
)[0]
if !tc.merged {
if got.Serial != tc.disk.Serial || got.WWN != tc.disk.WWN || got.Type != tc.disk.Type ||
got.Temperature != 0 || got.Health != "PASSED" {
t.Fatalf("contradicting row was merged into the disk: %+v", got)
}
return
}
if got.Serial != tc.serial || got.Temperature != 47 || got.Health != "FAILED" {
t.Fatalf("same disk's row was not merged: serial %q (want %q), temperature %d, health %q",
got.Serial, tc.serial, got.Temperature, got.Health)
}
})
}
}
// Proxmox's disk inventory reports a serial or WWN udev cannot read as the
// literal "unknown". The poller records it as unreported rather than as a
// collected serial; the disk's identity keys were already blind to it.
func TestProxmoxUnknownDiskIdentityIsRecordedAsUnreported(t *testing.T) {
t.Setenv("PULSE_DATA_DIR", t.TempDir())
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)
m := &Monitor{
state: state, alertManager: alertManager,
startTime: time.Now().Add(-time.Hour), lastPhysicalDiskPoll: make(map[string]time.Time),
}
// A record from before the placeholder was recorded as unreported must
// not carry it back in as the disk's last known serial.
state.UpdatePhysicalDisks("pve1", []models.PhysicalDisk{{
ID: unifiedresources.ProxmoxPhysicalDiskSourceID("pve1", "node1", "/dev/sdb", "", ""),
Node: "node1", Instance: "pve1", DevPath: "/dev/sdb", Serial: "unknown", WWN: "unknown",
Collection: &diskinventory.CollectionStatus{Serial: diskinventory.Available("proxmox_disks")},
}})
client := &slotDiskPVEClient{}
client.setDisk(proxmox.Disk{
DevPath: "/dev/sdb", Model: "USB3.0 Bridge", Serial: "unknown", WWN: "unknown",
Type: "hdd", Health: "PASSED", Wearout: 100, Size: 1000204886016,
})
started := time.Now()
m.maybePollPhysicalDisksAsync(context.Background(), "pve1", &config.PVEInstance{}, client,
[]proxmox.Node{{Node: "node1", Status: "online"}}, map[string]string{"node1": "online"}, nil)
deadline := started.Add(3 * time.Second)
for disks := state.GetSnapshot().PhysicalDisks; len(disks) != 1 || disks[0].LastChecked.Before(started); disks = state.GetSnapshot().PhysicalDisks {
if time.Now().After(deadline) {
t.Fatal("physical disk poll did not land in state")
}
time.Sleep(10 * time.Millisecond)
}
got := state.GetSnapshot().PhysicalDisks[0]
if got.Serial != "" || got.WWN != "" {
t.Fatalf("placeholder identity recorded as reported: serial %q, wwn %q", got.Serial, got.WWN)
}
if got.Collection == nil || got.Collection.Serial.State != diskinventory.FieldMissing {
t.Fatalf("serial collection = %+v, want missing", got.Collection)
}
placeholder := got
placeholder.Serial, placeholder.WWN = "unknown", "unknown"
if unifiedresources.PhysicalDiskMetricID(got) != unifiedresources.PhysicalDiskMetricID(placeholder) {
t.Fatalf("metric key moved: %q, was %q",
unifiedresources.PhysicalDiskMetricID(got), unifiedresources.PhysicalDiskMetricID(placeholder))
}
}
// The registry joins a Proxmox disk to its linked agent's disk on the same
// path when the agent reports SAS, because Proxmox puts the SAS address in
// the serial (#1595). A WWN naming another disk refuses that join: the agent's
// row may be the slot's previous occupant, retained by a silent agent, and the
// join used to hand the replacement that disk's serial, readings and canonical
// resource.
func TestRegistrySASPathJoinRefusesContradictingWWN(t *testing.T) {
const replacementWWN = "0x5000c500bbbb0002"
for _, tc := range []struct {
name string
agentWWN string
joined bool
}{
{name: "retained row of the previous occupant", agentWWN: "5-c50-aaaa0001"},
{name: "same disk in the agent's WWN spelling", agentWWN: "5-c50-bbbb0002", joined: true},
{name: "same disk without an agent WWN", agentWWN: "", joined: true},
} {
t.Run(tc.name, func(t *testing.T) {
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: "ZR5AGENT0001", WWN: tc.agentWWN, Type: "sas",
Health: "FAILED", Temperature: 52,
Collection: &diskinventory.CollectionStatus{
Serial: diskinventory.Available("smartctl"),
Temperature: diskinventory.Available("smartctl"),
},
}}},
}
replacement := models.PhysicalDisk{
ID: "pve1-node1--dev-sdb", Node: "node1", Instance: "pve1", DevPath: "/dev/sdb",
Serial: strings.TrimPrefix(replacementWWN, "0x"), WWN: replacementWWN, Type: "unknown",
Health: "PASSED", Wearout: -1, LastChecked: now,
}
registry := unifiedresources.NewRegistry(nil)
registry.IngestSnapshot(models.StateSnapshot{
Nodes: []models.Node{node}, Hosts: []models.Host{host}, PhysicalDisks: []models.PhysicalDisk{replacement},
})
var proxmoxDisk *unifiedresources.Resource
disks := registry.ListByType(unifiedresources.ResourceTypePhysicalDisk)
for index := range disks {
if hasIssue1595Source(disks[index].Sources, unifiedresources.SourceProxmox) {
proxmoxDisk = &disks[index]
}
}
if proxmoxDisk == nil || proxmoxDisk.PhysicalDisk == nil {
t.Fatalf("no canonical Proxmox disk among %d disks", len(disks))
}
joined := hasIssue1595Source(proxmoxDisk.Sources, unifiedresources.SourceAgent)
if joined != tc.joined || (tc.joined && len(disks) != 1) || (!tc.joined && len(disks) != 2) {
t.Fatalf("joined = %v with %d disks, want joined = %v", joined, len(disks), tc.joined)
}
if !tc.joined && (proxmoxDisk.PhysicalDisk.Serial == "ZR5AGENT0001" ||
proxmoxDisk.PhysicalDisk.Temperature != 0 || proxmoxDisk.PhysicalDisk.Health == "FAILED") {
t.Fatalf("replacement took the retained row's identity or readings: %+v", proxmoxDisk.PhysicalDisk)
}
})
}
}
+3 -3
View File
@@ -435,7 +435,7 @@ func TestMergeHostAgentSMARTIntoDisks_PreservesExistingProxmoxIdentity(t *testin
DevPath: "/dev/sdb",
Model: "Proxmox Model",
Serial: "PVE-SERIAL",
WWN: "pve-wwn",
WWN: "0x5000c500a1b2c3d4",
Type: "sas",
Size: 100,
Health: "PASSED",
@@ -453,7 +453,7 @@ func TestMergeHostAgentSMARTIntoDisks_PreservesExistingProxmoxIdentity(t *testin
Device: "/dev/sdb",
Model: "Agent Model",
Serial: "AGENT-SERIAL",
WWN: "agent-wwn",
WWN: "5-c50-a1b2c3d4", // the same disk, in smartctl's spelling
Type: "sata",
SizeBytes: 200,
Temperature: 35,
@@ -466,7 +466,7 @@ func TestMergeHostAgentSMARTIntoDisks_PreservesExistingProxmoxIdentity(t *testin
}
got := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts)[0]
if got.Model != "Proxmox Model" || got.Serial != "PVE-SERIAL" || got.WWN != "pve-wwn" || got.Type != "sas" {
if got.Model != "Proxmox Model" || got.Serial != "PVE-SERIAL" || got.WWN != "0x5000c500a1b2c3d4" || got.Type != "sas" {
t.Fatalf("existing Proxmox identity was overwritten: %+v", got)
}
if got.Size != 100 || got.Temperature != 41 || got.StorageGroup != "api-pool" {
+53 -4
View File
@@ -560,11 +560,12 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No
return &smartData[matchIndex]
}
// Try to match by WWN (most reliable)
// Try to match by WWN (most reliable). PVE and the agent spell one WWN
// differently (0x5000c500a1b2c3d4 against smartctl's 5-c50-a1b2c3d4),
// so compare the normalized identity, not the reported string.
if diskinventory.IsUsableHardwareID(updated[i].WWN) {
matched = uniqueMatch(func(candidate models.HostDiskSMART) bool {
return diskinventory.IsUsableHardwareID(candidate.WWN) &&
strings.EqualFold(candidate.WWN, updated[i].WWN) &&
return diskinventory.HardwareIdentityMatch("", updated[i].WWN, "", candidate.WWN) &&
diskTopologyCompatible(updated[i].Controller, updated[i].Target, candidate.Controller, candidate.Target)
})
}
@@ -578,7 +579,12 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No
})
}
// Last resort: match by device path
// Last resort: match by device path. A path names a slot, not a disk,
// so the row there may describe the slot's previous occupant: the agent
// has not reported since the swap, or it went silent and its last rows
// are retained. A row whose identity contradicts the disk's is refused,
// or it would fill or promote the old serial onto the replacement,
// which would then take the old disk's canonical resource and history.
if matched == nil {
normalizedDevPath := normalizeSMARTDeviceIdentifier(updated[i].DevPath)
matched = uniqueMatch(func(candidate models.HostDiskSMART) bool {
@@ -587,6 +593,15 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No
normalizedDiskDev == normalizedDevPath &&
diskTopologyCompatible(updated[i].Controller, updated[i].Target, candidate.Controller, candidate.Target)
})
if matched != nil && hostAgentSMARTIdentityConflicts(updated[i], *matched) {
log.Debug().
Str("node", updated[i].Node).
Str("device", updated[i].DevPath).
Str("serial", updated[i].Serial).
Str("smartSerial", matched.Serial).
Msg("Skipping host agent SMART row whose disk identity contradicts the Proxmox disk at the same path")
matched = nil
}
}
if matched == nil {
@@ -703,6 +718,40 @@ func diskTopologyCompatible(leftController, leftTarget, rightController, rightTa
return true
}
// hostAgentSMARTIdentityConflicts reports whether a SMART row found only by
// device path names a different disk than the Proxmox disk at that path.
// WWNs are compared for every disk; serials only where both producers read
// the drive's own serial (hostAgentSMARTSerialComparable). Any other Proxmox
// serial still counts toward agreement.
func hostAgentSMARTIdentityConflicts(disk models.PhysicalDisk, smart models.HostDiskSMART) bool {
if diskinventory.HardwareIdentityMatch(disk.Serial, disk.WWN, smart.Serial, smart.WWN) {
return false
}
serial := ""
if hostAgentSMARTSerialComparable(disk, smart) {
serial = disk.Serial
}
return diskinventory.HardwareIdentityConflict(serial, disk.WWN, smart.Serial, smart.WWN)
}
// hostAgentSMARTSerialComparable reports whether the Proxmox serial was read
// from the drive itself, as smartctl's is. Proxmox takes the serial from udev:
// for an NVMe namespace that is the controller's serial, and for a disk
// presented with SCSI vendor "ATA" (libata, or a SAS HBA's SCSI-ATA
// translation) udev's ata_id reads it from IDENTIFY. Any other disk's Proxmox
// serial may be a SAS address or SCSI designator from VPD page 0x83 (#1595),
// or a USB bridge's own serial.
func hostAgentSMARTSerialComparable(disk models.PhysicalDisk, smart models.HostDiskSMART) bool {
agentType := strings.ToLower(strings.TrimSpace(smart.Type))
switch strings.ToLower(strings.TrimSpace(disk.Type)) {
case "nvme":
return agentType == "nvme"
case "usb", "sas":
return false
}
return agentType == "sata" && strings.EqualFold(strings.TrimSpace(disk.Vendor), "ATA")
}
func shouldPromoteHostAgentSerial(disk models.PhysicalDisk, smart models.HostDiskSMART) bool {
if smart.Collection != nil &&
strings.EqualFold(strings.TrimSpace(smart.Type), "sas") &&
@@ -1,6 +1,7 @@
package monitoring
import (
"context"
"encoding/json"
"errors"
"fmt"
@@ -23,6 +24,7 @@ import (
"github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources"
agentsdocker "github.com/rcourtman/pulse-go-rewrite/pkg/agents/docker"
agentshost "github.com/rcourtman/pulse-go-rewrite/pkg/agents/host"
"github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory"
"github.com/rcourtman/pulse-go-rewrite/pkg/metrics"
"github.com/rcourtman/pulse-go-rewrite/pkg/proxmox"
)
@@ -6945,3 +6947,137 @@ func TestSilentLinkedAgentStopsFeedingNodeDisk(t *testing.T) {
t.Fatalf("agent reporting again: node disk = %+v from %q, agent %q; want the agent's summary and online", disk, source, host.Status)
}
}
// A disk swapped into a slot must not take its identity from the linked host
// agent's row for the slot's previous occupant. A silent agent keeps its last
// rows, and that row used to match the replacement by device path alone: it
// filled a serial Proxmox reported as "unknown" (recorded as unreported, so
// the agent can fill it) or, for SAS, promoted its serial over the SAS address
// Proxmox reports, and the registry joined a SAS replacement to the agent's
// disk on the same path. Either way the replacement took the old disk's
// serial, canonical resource and history, temperature and failed health.
func TestHostAgentSMARTRowForSwappedOutDiskDoesNotLendItsIdentity(t *testing.T) {
const (
oldSerial = "ZA1OLD0001"
oldAgentWWN = "5-c50-aaaa0001" // smartctl's NAA field spelling
oldPVEWWN = "0x5000c500aaaa0001"
newPVEWWN = "0x5000c500bbbb0002"
)
for _, transport := range []struct {
agentType string
// pveSerial is what Proxmox reports for a disk with the given WWN.
pveSerial func(wwn string) string
// replacementSerial is the serial the replacement's record keeps.
replacementSerial string
serialState diskinventory.FieldState
}{
{
agentType: "sata",
pveSerial: func(string) string { return "unknown" },
replacementSerial: "",
serialState: diskinventory.FieldMissing,
},
{
agentType: "sas",
pveSerial: func(wwn string) string { return strings.TrimPrefix(wwn, "0x") },
replacementSerial: strings.TrimPrefix(newPVEWWN, "0x"),
serialState: diskinventory.FieldAvailable,
},
} {
t.Run(transport.agentType, func(t *testing.T) {
t.Setenv("PULSE_DATA_DIR", t.TempDir())
now := time.Now()
state := models.NewState()
state.UpdateNodesForInstance("pve1", []models.Node{{
ID: "pve1-node1", Name: "node1", Instance: "pve1", Status: "online",
LastSeen: now, LinkedAgentID: "agent-1",
}})
state.UpsertHost(models.Host{
ID: "agent-1", Hostname: "node1", LinkedNodeID: "pve1-node1",
Status: "online", IntervalSeconds: 30, LastSeen: now,
Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{
Device: "/dev/sdb", Serial: oldSerial, WWN: oldAgentWWN, Type: transport.agentType,
Health: "FAILED", Temperature: 52,
Collection: &diskinventory.CollectionStatus{
Serial: diskinventory.Available("smartctl"),
Temperature: diskinventory.Available("smartctl"),
},
}}},
})
alertManager := alerts.NewManager()
t.Cleanup(alertManager.Stop)
adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil))
m := &Monitor{
state: state, resourceStore: adapter, alertManager: alertManager,
metricsHistory: NewMetricsHistory(100, time.Hour),
startTime: now.Add(-time.Hour), lastPhysicalDiskPoll: make(map[string]time.Time),
}
client := &slotDiskPVEClient{}
pveDisk := func(wwn string) proxmox.Disk {
return proxmox.Disk{
DevPath: "/dev/sdb", Model: "ST4000NM000A", Serial: transport.pveSerial(wwn), WWN: wwn,
Type: "hdd", Health: "PASSED", Wearout: 100, Size: 4000787030016,
}
}
poll := func() (models.PhysicalDisk, *unifiedresources.PhysicalDiskView) {
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())
var proxmoxViews []*unifiedresources.PhysicalDiskView
for _, view := range adapter.PhysicalDisks() {
if _, ok := view.SourceStatus(unifiedresources.SourceProxmox); ok {
proxmoxViews = append(proxmoxViews, view)
}
}
if len(proxmoxViews) != 1 {
t.Fatalf("canonical Proxmox physical disks = %d, want one", len(proxmoxViews))
}
return disks[0], proxmoxViews[0]
}
if time.Now().After(deadline) {
t.Fatal("physical disk poll did not land in state")
}
time.Sleep(10 * time.Millisecond)
}
}
client.setDisk(pveDisk(oldPVEWWN))
original, originalView := poll()
if original.Serial != oldSerial || originalView.Serial() != oldSerial ||
original.Collection == nil || original.Collection.Serial.State != diskinventory.FieldAvailable {
t.Fatalf("agent serial not merged into the disk it describes: state %q, canonical %q, collection %+v",
original.Serial, originalView.Serial(), original.Collection)
}
// The agent stops reporting, keeping its rows, and the disk is replaced.
state.TouchHost("agent-1", now.Add(-hostAgentHealthWindow(30)-time.Minute))
m.evaluateHostAgents(now)
client.setDisk(pveDisk(newPVEWWN))
for round := 1; round <= 2; round++ {
got, view := poll()
if got.Serial != transport.replacementSerial || view.Serial() == oldSerial {
t.Fatalf("poll %d: replacement took the retained row's serial: state %q, canonical %q",
round, got.Serial, view.Serial())
}
if view.ID() == originalView.ID() {
t.Fatalf("poll %d: replacement took over the old disk's canonical resource %q", round, view.ID())
}
if got.Temperature != 0 || got.Health != "PASSED" || view.Temperature() != 0 || view.Health() != "PASSED" {
t.Fatalf("poll %d: replacement took the old disk's readings: state %d/%q, canonical %d/%q",
round, got.Temperature, got.Health, view.Temperature(), view.Health())
}
if got.WWN != newPVEWWN ||
got.Collection == nil || got.Collection.Serial.State != transport.serialState {
t.Fatalf("poll %d: replacement wwn %q, serial collection %+v", round, got.WWN, got.Collection)
}
}
})
}
}
+23 -4
View File
@@ -1105,8 +1105,8 @@ func (m *Monitor) maybePollPhysicalDisksAsync(
DevPath: disk.DevPath,
Model: disk.Model,
Vendor: disk.Vendor,
Serial: disk.Serial,
WWN: disk.WWN,
Serial: proxmoxReportedDiskIdentity(disk.Serial),
WWN: proxmoxReportedDiskIdentity(disk.WWN),
Type: disk.Type,
Size: disk.Size,
Health: normalizeProxmoxDiskHealth(disk.Health),
@@ -1121,7 +1121,7 @@ func (m *Monitor) maybePollPhysicalDisksAsync(
},
LastChecked: time.Now(),
}
if strings.TrimSpace(disk.Serial) != "" {
if strings.TrimSpace(physicalDisk.Serial) != "" {
physicalDisk.Collection.Serial = diskinventory.Available("proxmox_disks")
} else {
physicalDisk.Collection.Serial = diskinventory.Missing("proxmox_disks", "disk serial was not reported")
@@ -1220,6 +1220,22 @@ func normalizeProxmoxDiskHealth(health string) string {
}
}
// proxmoxUnreportedDiskIdentity is what PVE's disk inventory
// (PVE::Diskmanage) writes into a disk's serial and wwn when udev has none.
const proxmoxUnreportedDiskIdentity = "unknown"
// proxmoxReportedDiskIdentity records PVE's placeholder for an unreported
// serial or WWN as empty. Kept verbatim, the placeholder read as a collected
// serial, so the disk's collection state hid the gap and the linked host
// agent's serial or WWN never filled it. Identity keys are unaffected:
// diskinventory.IsUsableHardwareID already rejected the placeholder.
func proxmoxReportedDiskIdentity(value string) string {
if strings.EqualFold(strings.TrimSpace(value), proxmoxUnreportedDiskIdentity) {
return ""
}
return value
}
// physicalDisksFromHostAgentSMART builds PhysicalDisk entries for a node from
// its linked host agent's SMART inventory. This is the fallback when the
// Proxmox disks/list query fails: PVE probes SMART per disk inside that call,
@@ -1371,8 +1387,11 @@ func preserveUnavailablePhysicalDiskEvidence(current, previous models.PhysicalDi
if current.Collection == nil {
return current
}
// Only an identity is worth keeping: a record from before Proxmox's
// "unknown" was recorded as unreported would otherwise restore it.
if current.Collection.Serial.State != diskinventory.FieldAvailable &&
strings.TrimSpace(current.Serial) == "" {
strings.TrimSpace(current.Serial) == "" &&
diskinventory.IsUsableHardwareID(previous.Serial) {
current.Serial = previous.Serial
}
if current.Collection.Temperature.State != diskinventory.FieldAvailable &&
@@ -809,9 +809,9 @@ func (client *slotDiskPVEClient) GetDisks(context.Context, string) ([]proxmox.Di
// 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.
// "unknown", which the poller records as empty, as it does for 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 (
+8 -2
View File
@@ -3272,8 +3272,14 @@ func (rr *ResourceRegistry) resolveLinkedPhysicalDisk(source DataSource, incomin
existing.PhysicalDisk.Serial, existing.PhysicalDisk.WWN,
)
existingDevice := strings.ToLower(normalizePhysicalDiskDeviceToken(existing.PhysicalDisk.DevPath))
agentReportsSAS := (source == SourceProxmox && strings.EqualFold(existing.PhysicalDisk.DiskType, "sas")) ||
(source == SourceAgent && strings.EqualFold(incoming.PhysicalDisk.DiskType, "sas"))
// Proxmox may put a SAS disk's transport address in its serial, so
// the serials cannot refuse this path join, but a WWN naming another
// disk can: the agent's row may be the slot's previous occupant,
// retained by a silent agent, and joining it hands the replacement
// that disk's serial, readings and canonical resource.
agentReportsSAS := ((source == SourceProxmox && strings.EqualFold(existing.PhysicalDisk.DiskType, "sas")) ||
(source == SourceAgent && strings.EqualFold(incoming.PhysicalDisk.DiskType, "sas"))) &&
!diskinventory.HardwareIdentityConflict("", incomingWWN, "", existing.PhysicalDisk.WWN)
deviceMatch := (agentReportsSAS || physicalDiskMissingIdentityPathCompatible(incoming.PhysicalDisk, existing.PhysicalDisk)) &&
incomingDevice != "" &&
incomingDevice == existingDevice &&
+61
View File
@@ -0,0 +1,61 @@
package diskinventory
import "strings"
// HardwareIdentityConflict reports whether two observations of one disk slot
// carry hardware identities that name different disks: nothing agrees across
// serial and WWN (HardwareIdentityMatch), and at least one field reported on
// both sides disagrees. A field missing or unusable on either side is no
// evidence, so an observation without identity never conflicts.
//
// Reporters spell one identity differently, and spelling is not disagreement:
// - serials are compared on letters and digits only, because udev rewrites
// whitespace and unusual characters in a serial as underscores while
// smartctl keeps them, and a T10 vendor designator for an ATA drive
// ("ATA", the model, then the drive serial) is no evidence against the
// serial it ends with, because udev reports one when a SCSI-ATA
// translation layer hides IDENTIFY from it;
// - a WWN that begins with the other is no evidence, because udev's ID_WWN
// carries only the first 64 bits of a 128-bit NAA 6 identifier that sysfs
// and smartctl report in full.
//
// A caller blanks a field its producer may not report as disk identity, such
// as a SAS disk's serial, where Proxmox may report a SAS address, and checks
// HardwareIdentityMatch with the field kept if it may still show agreement.
func HardwareIdentityConflict(leftSerial, leftWWN, rightSerial, rightWWN string) bool {
if HardwareIdentityMatch(leftSerial, leftWWN, rightSerial, rightWWN) {
return false
}
return serialsDisagree(leftSerial, rightSerial) || wwnsDisagree(leftWWN, rightWWN)
}
func serialsDisagree(left, right string) bool {
left, right = compactSerial(normalizeHardwareID(left)), compactSerial(normalizeHardwareID(right))
return left != "" && right != "" && left != right &&
!isATADesignatorOf(left, right) && !isATADesignatorOf(right, left)
}
// isATADesignatorOf reports whether designator, compacted, is the T10 vendor
// designator of the ATA drive whose serial is given: the "ata" vendor field,
// a model, then that serial.
func isATADesignatorOf(designator, serial string) bool {
const vendor = "ata"
return len(designator) > len(vendor)+len(serial) &&
strings.HasPrefix(designator, vendor) && strings.HasSuffix(designator, serial)
}
func compactSerial(value string) string {
var builder strings.Builder
for _, char := range value {
if (char >= 'a' && char <= 'z') || (char >= '0' && char <= '9') {
builder.WriteRune(char)
}
}
return builder.String()
}
func wwnsDisagree(left, right string) bool {
left, right = normalizeHardwareWWN(left), normalizeHardwareWWN(right)
return left != "" && right != "" &&
!strings.HasPrefix(left, right) && !strings.HasPrefix(right, left)
}
@@ -0,0 +1,36 @@
package diskinventory
import "testing"
func TestHardwareIdentityConflict(t *testing.T) {
for _, tc := range []struct {
name string
leftSerial, leftWWN string
rightSerial, rightWWN string
want bool
}{
{name: "different serials", leftSerial: "WD-NEW0001", rightSerial: "WD-OLD0001", want: true},
{name: "different WWNs", leftWWN: "0x5000c500b0000001", rightWWN: "naa.5000c500a0000001", want: true},
{name: "WWN disagrees while serial is absent on one side", leftSerial: "PVE-ONLY", leftWWN: "0x5000c500b0000001", rightWWN: "5-c50-a0000001", want: true},
{name: "same serial", leftSerial: "WD-WX1234", rightSerial: "wd-wx1234"},
{name: "udev underscores for whitespace", leftSerial: "WD-WX_1234", rightSerial: "WD-WX 1234"},
{name: "T10 designator ending with the drive serial", leftSerial: "ATA_ST4000NM000A-2HZ_ZC1ABCDE", rightSerial: "ZC1ABCDE"},
{name: "serial that only begins with the other", leftSerial: "SN1234", rightSerial: "SN123", want: true},
{name: "bare serial that ends with the other", leftSerial: "001234567890", rightSerial: "1234567890", want: true},
{name: "agent NAA fields against PVE hex", leftSerial: "5000c500a0000001", leftWWN: "0x5000c500a0000001", rightSerial: "ZR5TESTA0001", rightWWN: "5-c50-a0000001"},
{name: "udev 64-bit prefix of an NAA 6 WWN", leftWWN: "0x600508b1001c4d5e", rightWWN: "naa.600508b1001c4d5e7f8a9b0c1d2e3f40"},
{name: "RAID volume NAA as serial against WWN", leftSerial: "600508b1001c4d5e7f8a9b0c1d2e3f40", leftWWN: "0x600508b1001c4d5e", rightSerial: "OTHER", rightWWN: "naa.600508b1001c4d5e7f8a9b0c1d2e3f40"},
{name: "serial against WWN only is no evidence", leftSerial: "WD-NEW0001", rightWWN: "naa.5000c500a0000001"},
{name: "placeholders are no evidence", leftSerial: "unknown", leftWWN: "0x0000000000000000", rightSerial: "WD-OLD0001", rightWWN: "naa.5000c500a0000001"},
{name: "one side without identity", rightSerial: "WD-OLD0001", rightWWN: "naa.5000c500a0000001"},
} {
t.Run(tc.name, func(t *testing.T) {
if got := HardwareIdentityConflict(tc.leftSerial, tc.leftWWN, tc.rightSerial, tc.rightWWN); got != tc.want {
t.Fatalf("HardwareIdentityConflict = %v, want %v", got, tc.want)
}
if got := HardwareIdentityConflict(tc.rightSerial, tc.rightWWN, tc.leftSerial, tc.leftWWN); got != tc.want {
t.Fatalf("HardwareIdentityConflict (swapped) = %v, want %v", got, tc.want)
}
})
}
}