diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index fdbf6c7ff6..9bd8c1ab83 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8737,6 +8737,24 @@ admission, token binding or removal-block behaviour. Focused proof lives in (`TestMergeHostAgentSMARTIntoDisks_AgentWearoutDoesNotHideLowPVELife` and `TestMergeHostAgentSMARTIntoDisks_AgentWearoutFillsUnreportedPVELife`). +### QEMU default disk serials are not agent disk identity + +`internal/monitoring/monitor.go` changed only so the unified physical-disk +metric sync, which writes SMART history for disks without a native writer such +as TrueNAS disks, always writes under the disk's resolved metrics target. It +previously did so only for an empty serial, so a non-empty serial that was +already rejected as a placeholder, such as `UNKNOWN`, sent the samples to the +canonical resource ID that no reader resolves. The same change makes +`diskinventory.IsUsableHardwareID` reject QEMU's default disk serials +(`drive-scsi0`, `scsi0-hd0`, `none0`, `QM00001`), so a host agent inside a +QEMU VM that reports one of them now keys that disk on its host and device +through `HostSMARTDiskSourceID` instead of on a serial every such VM shares. +Those disks change source ID and SMART metric key once. Agent registration, +enrolment, install, update, removal and report identity are unchanged. +Focused proof lives in `internal/monitoring/physical_disk_roundtrip_test.go` +(`TestNestedProxmoxDefaultQEMUSerialsStayPerNode` and +`TestTrueNASPlaceholderDiskSerialsStayPerApplianceAndShareOneHistoryKey`). + ### Windows braced MachineGuid does not abort agent startup The Windows unified agent resolves host information through gopsutil's combined diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 3295543f67..1d32c31f0a 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1522,6 +1522,39 @@ or promote a serial from a stale SMART row it matched by device path. Negative percentage-used counters remain unknown; values above 100 clamp to exhausted before deriving remaining life, so invalid or over-limit controller data cannot wrap into a fabricated healthy value. +QEMU's default disk serials are placeholders too. A virtual disk configured +without a serial reports one built from its drive ID or a per-VM counter, the +same on every VM built the same way: a SCSI disk's udev `ID_SERIAL_SHORT`, +which Proxmox's `disks/list` passes through as the serial, is `drive-scsi0` +under Proxmox, `drive-scsi0-0-0-0` under libvirt, and QEMU's automatic drive +ID for a `-drive` given none (`scsi0-hd0` for `if=scsi`, `none0` for +`if=none`), and an ATA disk's serial is `QM` plus a five-digit counter. +`diskinventory.IsUsableHardwareID` rejects exactly those shapes (`drive-scsi` +plus digits with at most three `-digits` groups, `scsi-hd`, `none`, +and `QM` plus five digits), so a real serial that only shares a prefix, such +as `DRIVE-ASSET123` or `QM0001`, stays identity. It also +strips `naa.`, `eui.`, `wwn-` and `0x` framing before its all-zero and all-F +test, so `0x0000000000000000` is a placeholder as well. Before this rule every +node of a nested Proxmox cluster minted one `machine:drive-scsi0` canonical +disk and one SMART series for its first disk, and all but one node lost that +disk from inventory (`TestNestedProxmoxDefaultQEMUSerialsStayPerNode`, +`TestIsUsableHardwareIDRejectsQEMUDefaultDiskSerials`). The TrueNAS provider +applies the same rule: a disk takes its serial as canonical identity only when +the serial is usable, and a disk without a usable serial is keyed on its +source ID by native TrueNAS disk history and by the unified SMART writer for +disks without a native writer, which is the key the registry's metrics target +resolves for it. QEMU serials used to be accepted, so those disks shared one +key. A serial that was already rejected, such as `UNKNOWN`, had the opposite +problem: native history used TrueNAS's own disk identifier and the writer used +the canonical resource ID, so neither reached the chart +(`TestTrueNASPlaceholderDiskSerialsStayPerApplianceAndShareOneHistoryKey`, +`TestProviderPlaceholderDiskSerialsKeyNeitherIdentityNorHistory`). Upgrade +effect: a disk reporting one of these values changes canonical ID and SMART +metric key once. Samples recorded under the old key are not carried over, +because that key mixed every disk that shared the value. Path-shaped alert +references (Proxmox disk health and wearout, host-agent SMART) are unaffected; +anything keyed on the old canonical ID or metric key starts again under the +new one. Proxmox cluster API polling has one configured connection authority: the operator-saved `PVEInstance.Host` and its single credential set. Auto-discovered member/corosync addresses remain ordered failover candidates and direct diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 7649b64f28..91d2c57d89 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -6035,9 +6035,11 @@ func (m *Monitor) syncUnifiedPhysicalDiskMetrics(store ResourceStoreInterface) { Collection: diskinventory.CloneStatus(resource.PhysicalDisk.Collection), LastChecked: resource.LastSeen, } - if disk.Serial == "" { - disk.ID = targetID - } + // Write under the key readers resolve. PhysicalDiskMetricID keys on a + // usable serial or WWN, which the target already equals, and + // otherwise on disk.ID, so a placeholder serial must not leave the + // canonical resource ID there. + disk.ID = targetID // Anchor to the source observation time so a read-side registry rebuild // re-issuing the same snapshot maps to one sample instead of a fresh // wall-clock row (#1966). diff --git a/internal/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go index b23736f09f..646ad979e0 100644 --- a/internal/monitoring/physical_disk_roundtrip_test.go +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -3,6 +3,7 @@ package monitoring import ( "context" "encoding/json" + "strconv" "sync" "testing" "time" @@ -11,6 +12,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/config" "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/notifications" + "github.com/rcourtman/pulse-go-rewrite/internal/truenas" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" "github.com/rcourtman/pulse-go-rewrite/pkg/metrics" @@ -914,3 +916,142 @@ func TestPreviousPhysicalDiskEvidenceRejectsSlotMatchAcrossConflictingIdentity(t }) } } + +// QEMU gives a virtual disk without a configured serial a default one built +// from its drive ID or a per-VM counter, so every VM built the same way +// reports the same value. Proxmox's disks/list serial is udev's +// ID_SERIAL_SHORT, which for a nested Proxmox node's first SCSI disk is +// "drive-scsi0" on every node. Treated as hardware identity, the registry +// minted one canonical disk for the whole cluster and every node but one lost +// its disk from inventory. +func TestNestedProxmoxDefaultQEMUSerialsStayPerNode(t *testing.T) { + now := time.Now() + snapshot := models.StateSnapshot{} + for _, node := range []string{"pve1", "pve2", "pve3"} { + snapshot.Nodes = append(snapshot.Nodes, models.Node{ + ID: "lab-" + node, Name: node, Instance: "lab", Status: "online", LastSeen: now, + }) + for _, disk := range []struct{ devPath, serial string }{ + {"/dev/sda", "drive-scsi0"}, + {"/dev/sdb", "QM00005"}, + {"/dev/sdc", "drive-scsi0-0-0-1"}, + } { + snapshot.PhysicalDisks = append(snapshot.PhysicalDisks, models.PhysicalDisk{ + ID: unifiedresources.ProxmoxPhysicalDiskSourceID("lab", node, disk.devPath, "", ""), + Instance: "lab", Node: node, DevPath: disk.devPath, Model: "QEMU HARDDISK", + Serial: disk.serial, Type: "hdd", Health: "PASSED", Size: 34359738368, LastChecked: now, + }) + } + } + + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + adapter.PopulateFromSnapshot(snapshot) + views := adapter.PhysicalDisks() + if len(views) != len(snapshot.PhysicalDisks) { + t.Fatalf("canonical physical disks = %d, want %d (one per node and device)", len(views), len(snapshot.PhysicalDisks)) + } + seenSlots := make(map[string]bool, len(views)) + seenMetricIDs := make(map[string]bool, len(views)) + for _, view := range views { + seenSlots[view.Node()+":"+view.DevPath()] = true + seenMetricIDs[view.MetricResourceID()] = true + } + if len(seenSlots) != len(snapshot.PhysicalDisks) { + t.Fatalf("canonical disks cover %d node/device slots, want %d: %v", len(seenSlots), len(snapshot.PhysicalDisks), seenSlots) + } + if len(seenMetricIDs) != len(snapshot.PhysicalDisks) { + t.Fatalf("canonical disks resolve %d metric keys, want one per disk: %v", len(seenMetricIDs), seenMetricIDs) + } + writerIDs := make(map[string]bool, len(snapshot.PhysicalDisks)) + for _, disk := range snapshot.PhysicalDisks { + writerIDs[unifiedresources.PhysicalDiskMetricID(disk)] = true + } + if len(writerIDs) != len(snapshot.PhysicalDisks) { + t.Fatalf("SMART metric series = %d, want one per disk: %v", len(writerIDs), writerIDs) + } + for writerID := range writerIDs { + if !seenMetricIDs[writerID] { + t.Fatalf("SMART writer key %q is not a key readers resolve: %v", writerID, seenMetricIDs) + } + } + + // The host agent inside each nested node reads the same ATA default + // through smartctl. + agentIDs := make(map[string]bool, 3) + for _, host := range []string{"agent-pve1", "agent-pve2", "agent-pve3"} { + agentIDs[unifiedresources.HostSMARTDiskSourceID(models.Host{ID: host}, models.HostDiskSMART{Device: "sdb", Serial: "QM00005"})] = true + } + if len(agentIDs) != 3 { + t.Fatalf("host agent disk source IDs = %v, want one per host", agentIDs) + } +} + +// A TrueNAS VM's virtual disks report QEMU's default serial, the same on +// every appliance built the same way, and other appliances report "UNKNOWN". +// Neither may merge disks across appliances, and the SMART writer must file +// history under the key the chart reader resolves, the disk's source ID, +// rather than under the canonical resource ID. +func TestTrueNASPlaceholderDiskSerialsStayPerApplianceAndShareOneHistoryKey(t *testing.T) { + previous := truenas.IsFeatureEnabled() + truenas.SetFeatureEnabled(true) + t.Cleanup(func() { truenas.SetFeatureEnabled(previous) }) + + store, err := metrics.NewStore(metrics.DefaultConfig(t.TempDir())) + if err != nil { + t.Fatalf("metrics.NewStore() error = %v", err) + } + defer func() { _ = store.Close() }() + + collectedAt := time.Now().UTC().Truncate(time.Second) + var records []unifiedresources.IngestRecord + diskCount := 0 + for _, hostname := range []string{"truenas-a", "truenas-b"} { + fixtures := truenas.DefaultFixtures() + fixtures.CollectedAt = collectedAt + fixtures.System.CollectedAt = collectedAt + fixtures.System.Hostname = hostname + fixtures.System.MachineID = hostname + "-machine-id" + for i := range fixtures.Disks { + fixtures.Disks[i].Serial = "drive-scsi" + strconv.Itoa(i) + } + fixtures.Disks[0].Serial = "UNKNOWN" + diskCount += len(fixtures.Disks) + records = append(records, truenas.NewProvider(fixtures).Records()...) + } + resourceStore := unifiedresources.NewMonitorAdapter(nil) + resourceStore.PopulateSnapshotAndSupplemental(models.StateSnapshot{}, map[unifiedresources.DataSource][]unifiedresources.IngestRecord{ + unifiedresources.SourceTrueNAS: records, + }) + monitor := &Monitor{resourceStore: resourceStore, metricsStore: store} + monitor.syncUnifiedPhysicalDiskMetrics(resourceStore) + + disks := 0 + keys := make(map[string]string, diskCount) + for _, resource := range resourceStore.GetAll() { + if resource.Type != unifiedresources.ResourceTypePhysicalDisk || resource.PhysicalDisk == nil { + continue + } + disks++ + target := resourceStore.MetricsTargetForResource(resource.ID) + if target == nil || target.ResourceType != "disk" || target.ResourceID == "" { + t.Fatalf("disk %s metrics target = %+v", resource.ID, target) + } + if other, shared := keys[target.ResourceID]; shared { + t.Fatalf("disks %s and %s share metric key %q", other, resource.ID, target.ResourceID) + } + keys[target.ResourceID] = resource.ID + if resource.PhysicalDisk.Temperature <= 0 { + continue + } + points, err := store.Query("disk", target.ResourceID, "smart_temp", collectedAt.Add(-time.Minute), time.Now().Add(time.Minute), 0) + if err != nil { + t.Fatalf("store.Query(%q) error = %v", target.ResourceID, err) + } + if len(points) == 0 { + t.Fatalf("disk %s (serial %q): no SMART history under the reader's key %q", resource.ID, resource.PhysicalDisk.Serial, target.ResourceID) + } + } + if disks != diskCount { + t.Fatalf("canonical TrueNAS disks = %d, want %d (one per appliance and device)", disks, diskCount) + } +} diff --git a/internal/truenas/provider.go b/internal/truenas/provider.go index c20af6a3be..5fcd606d57 100644 --- a/internal/truenas/provider.go +++ b/internal/truenas/provider.go @@ -13,6 +13,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/storagehealth" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" + "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" ) const ( @@ -394,10 +395,11 @@ func (p *Provider) PhysicalDiskTemperatureHistory(ctx context.Context, duration return nil, fmt.Errorf("truenas provider has no cached snapshot") } + diskScope := systemSourceID(p.connectionID, snapshot.System.Hostname) identifiers := make([]string, 0, len(snapshot.Disks)) metricIDsByIdentifier := make(map[string]string, len(snapshot.Disks)*3) for _, disk := range snapshot.Disks { - metricID := trueNASDiskMetricResourceID(disk) + metricID := trueNASDiskMetricResourceID(diskScope, disk) if metricID == "" { continue } @@ -802,18 +804,22 @@ func truenasRecordsFromSnapshot(snapshot *FixtureSnapshot, connectionID string, diskIdentity := unifiedresources.ResourceIdentity{ Hostnames: []string{snapshot.System.Hostname}, } - if disk.Serial != "" { + // A placeholder serial ("UNKNOWN", a QEMU default such as + // drive-scsi1) is shared by unrelated disks, so it must not key the + // canonical disk or disks on different appliances would merge. + hasHardwareSerial := diskinventory.IsUsableHardwareID(disk.Serial) + if hasHardwareSerial { diskIdentity.MachineID = disk.Serial } parentSourceID := systemSourceID if pool := strings.TrimSpace(disk.Pool); pool != "" { parentSourceID = scopedPoolSourceID(systemSourceID, pool) } - // Disks with a serial mint identity-keyed canonical IDs that do not - // depend on the source ID, so only serial-less disks re-key when the + // Disks with a usable serial mint identity-keyed canonical IDs that + // do not depend on the source ID, so only the rest re-key when the // system scope moves to the connection. var diskSupersededIDs []string - if disk.Serial == "" { + if !hasHardwareSerial { diskSupersededIDs = supersededChildIDs(unifiedresources.ResourceTypePhysicalDisk, scopedDiskSourceID(legacySystemSourceID, disk.Name)) } records = append(records, unifiedresources.IngestRecord{ @@ -2966,14 +2972,20 @@ func trueNASSystemMetricResourceID(connectionID string, system SystemInfo) strin } func trueNASDiskHistoryLookupKeys(disk Disk) []string { - return dedupeStrings([]string{ - strings.TrimSpace(disk.Name), - strings.TrimSpace(disk.ID), - strings.TrimSpace(disk.Serial), - }) + keys := []string{strings.TrimSpace(disk.Name), strings.TrimSpace(disk.ID)} + // A placeholder serial names no one disk, so it must not route another + // disk's native history to this one. + if diskinventory.IsUsableHardwareID(disk.Serial) { + keys = append(keys, strings.TrimSpace(disk.Serial)) + } + return dedupeStrings(keys) } -func trueNASDiskMetricResourceID(disk Disk) string { +// trueNASDiskMetricResourceID is the key native disk history is filed under. +// It must equal the metrics target the registry resolves for the disk, which +// falls back to the disk's source ID when the serial is missing or a +// placeholder, or the chart looks up a key nothing was filed under. +func trueNASDiskMetricResourceID(systemSourceID string, disk Disk) string { devPath := "" if name := strings.TrimSpace(disk.Name); name != "" { devPath = "/dev/" + name @@ -2984,11 +2996,7 @@ func trueNASDiskMetricResourceID(disk Disk) string { DiskType: strings.TrimSpace(disk.Transport), SizeBytes: disk.SizeBytes, } - fallback := strings.TrimSpace(disk.ID) - if fallback == "" { - fallback = strings.TrimSpace(disk.Name) - } - return unifiedresources.PhysicalDiskMetaMetricID(meta, fallback) + return unifiedresources.PhysicalDiskMetaMetricID(meta, scopedDiskSourceID(systemSourceID, disk.Name)) } func parentPoolFromDataset(datasetName string) string { diff --git a/internal/truenas/provider_test.go b/internal/truenas/provider_test.go index 34dc1b6451..4a2ed6bea7 100644 --- a/internal/truenas/provider_test.go +++ b/internal/truenas/provider_test.go @@ -1393,6 +1393,75 @@ func TestProviderPhysicalDiskTemperatureHistoryUsesCanonicalMetricIDs(t *testing } } +// A placeholder serial ("UNKNOWN", or QEMU's drive-scsi0 for a TrueNAS VM's +// virtual disk) names no one disk. The disk must not take it as canonical +// identity, and its native history must land on the key the registry +// resolves for it, the disk's source ID, or the chart never finds it. +func TestProviderPlaceholderDiskSerialsKeyNeitherIdentityNorHistory(t *testing.T) { + for _, serial := range []string{"drive-scsi0", "UNKNOWN"} { + t.Run(serial, func(t *testing.T) { + fixtures := DefaultFixtures() + fixtures.Disks[0].Serial = serial + diskName := fixtures.Disks[0].Name + now := time.Date(2026, 3, 29, 20, 0, 0, 0, time.UTC) + fetcher := &controllableStubFetcher{ + snapshot: &fixtures, + diskHistory: map[string][]TimeSeriesPoint{ + diskName: { + {Timestamp: now.Add(-time.Hour), Value: 31}, + {Timestamp: now, Value: 33}, + }, + }, + } + provider := NewLiveProviderForConnection(fetcher, "conn-1") + if err := provider.Refresh(context.Background()); err != nil { + t.Fatalf("Refresh() error = %v", err) + } + + records := provider.Records() + registry := unifiedresources.NewRegistry(unifiedresources.NewMemoryStore()) + registry.IngestRecords(unifiedresources.SourceTrueNAS, records) + for _, record := range records { + if record.Resource.Type != unifiedresources.ResourceTypePhysicalDisk || record.Resource.Name != diskName { + continue + } + if record.Identity.MachineID != "" { + t.Fatalf("placeholder serial %q keyed disk identity %q", serial, record.Identity.MachineID) + } + // Its canonical ID now follows the source ID, so the + // hostname-keyed ID it had before connection scoping + // must be listed as superseded, as for a serial-less disk. + if len(record.SupersededCanonicalIDs) == 0 { + t.Fatalf("placeholder serial %q disk lists no superseded canonical IDs", serial) + } + } + for _, key := range trueNASDiskHistoryLookupKeys(fixtures.Disks[0]) { + if key == serial { + t.Fatalf("placeholder serial %q routes native history lookups", serial) + } + } + resourceID := "" + for _, resource := range registry.List() { + if resource.Type == unifiedresources.ResourceTypePhysicalDisk && resource.Name == diskName { + resourceID = resource.ID + } + } + target := registry.MetricsTarget(resourceID) + if target == nil || target.ResourceID == "" || target.ResourceID == serial { + t.Fatalf("metrics target for %s = %+v, want the disk's source ID", diskName, target) + } + + history, err := provider.PhysicalDiskTemperatureHistory(context.Background(), 4*time.Hour) + if err != nil { + t.Fatalf("PhysicalDiskTemperatureHistory() error = %v", err) + } + if points := history[target.ResourceID]; len(points) != 2 { + t.Fatalf("native history under metrics target %q = %+v, history %+v", target.ResourceID, points, history) + } + }) + } +} + func TestProviderSystemMetricHistoryUsesCanonicalAgentMetricIDs(t *testing.T) { fixtures := DefaultFixtures() now := time.Date(2026, 3, 29, 20, 0, 0, 0, time.UTC) diff --git a/pkg/diskinventory/identity.go b/pkg/diskinventory/identity.go index 5f5fe8f7d8..995a4ee5a0 100644 --- a/pkg/diskinventory/identity.go +++ b/pkg/diskinventory/identity.go @@ -53,7 +53,24 @@ func IsUsableHardwareID(value string) bool { "TO BE FILLED BY O.E.M.", "0123456789": return false } - compact := strings.NewReplacer("-", "", ":", "", ".", "", " ", "").Replace(upper) + if isQEMUDefaultDiskSerial(upper) { + return false + } + // A WWN framing prefix must not hide an all-zero value: udev reports + // ID_WWN=0x0000000000000000 for some bridges, and the 0x would otherwise + // keep it from counting as all zeros. + digits := upper + for { + trimmed := digits + for _, prefix := range []string{"NAA.", "EUI.", "WWN-", "0X"} { + trimmed = strings.TrimPrefix(trimmed, prefix) + } + if trimmed == digits { + break + } + digits = trimmed + } + compact := strings.NewReplacer("-", "", ":", "", ".", "", " ", "").Replace(digits) if compact == "" { return false } @@ -66,6 +83,57 @@ func IsUsableHardwareID(value string) bool { return !allZero && !allF } +// isQEMUDefaultDiskSerial recognizes the serial QEMU reports for a virtual +// disk configured without one. Every VM built the same way reports the same +// value, so it names a slot, not a disk: each node of a nested Proxmox cluster +// lists its first disk with serial drive-scsi0. +// +// - A SCSI disk falls back to its drive ID: "drive-scsi0" under Proxmox, +// "drive-scsi0-0-0-0" under libvirt, and QEMU's automatic ID for a -drive +// given none, "scsi0-hd0" for if=scsi or "none0" for if=none. +// - An ATA disk takes a per-VM counter, "QM00001". +// +// The grammar is kept to those exact shapes so a real serial that merely +// starts with "DRIVE-" or "QM" stays usable. The argument must already be +// upper-cased. +func isQEMUDefaultDiskSerial(upper string) bool { + if counter, ok := strings.CutPrefix(upper, "QM"); ok { + return len(counter) == 5 && isASCIIDigits(counter) + } + if address, ok := strings.CutPrefix(upper, "DRIVE-SCSI"); ok { + groups := strings.Split(address, "-") + if len(groups) > 4 { + return false + } + for _, group := range groups { + if !isASCIIDigits(group) { + return false + } + } + return true + } + if bus, unit, ok := strings.Cut(upper, "-HD"); ok { + index, isSCSI := strings.CutPrefix(bus, "SCSI") + return isSCSI && isASCIIDigits(index) && isASCIIDigits(unit) + } + if unit, ok := strings.CutPrefix(upper, "NONE"); ok { + return isASCIIDigits(unit) + } + return false +} + +func isASCIIDigits(value string) bool { + if value == "" { + return false + } + for _, char := range value { + if char < '0' || char > '9' { + return false + } + } + return true +} + // normalizeHardwareID canonicalizes a serial or WWN for cross-source // comparison. Reporters disagree on framing, not identity: smartctl emits // naa./eui.-prefixed WWNs, udev emits wwn-0x tokens, and PVE surfaces bare diff --git a/pkg/diskinventory/identity_test.go b/pkg/diskinventory/identity_test.go index 73c5576127..a45e2f8a50 100644 --- a/pkg/diskinventory/identity_test.go +++ b/pkg/diskinventory/identity_test.go @@ -42,6 +42,11 @@ func TestPreferredIDRejectsPlaceholderHardwareIdentity(t *testing.T) { "DEFAULT-SERIAL", "0000-0000-0000", "FFFF:FFFF", + "0x0000000000000000", + "naa.0000000000000000", + "wwn-0x0000000000000000", + "0xffffffffffffffff", + "0x", } { if IsUsableHardwareID(placeholder) { t.Fatalf("placeholder %q was treated as usable hardware identity", placeholder) @@ -50,11 +55,65 @@ func TestPreferredIDRejectsPlaceholderHardwareIdentity(t *testing.T) { t.Fatalf("placeholder %q produced ID %q, want scoped fallback", placeholder, got) } } - if !IsUsableHardwareID("ZR5DLAYJ") || !IsUsableHardwareID("naa.5000c500abcdef01") { + if !IsUsableHardwareID("ZR5DLAYJ") || !IsUsableHardwareID("naa.5000c500abcdef01") || + !IsUsableHardwareID("0x5000c500abcdef01") { t.Fatal("real disk serial/WWN was rejected") } } +// QEMU gives a virtual disk without a configured serial one built from its +// drive ID or a per-VM counter. Observed: a Proxmox VM's virtio-scsi disk +// reports udev ID_SERIAL_SHORT=drive-scsi0 and a libvirt VM's +// drive-scsi0-0-0-0, which Proxmox's disks/list passes through as the serial. +func TestIsUsableHardwareIDRejectsQEMUDefaultDiskSerials(t *testing.T) { + for _, serial := range []string{ + "drive-scsi0", + "drive-scsi12", + "DRIVE-SCSI0", + "drive-scsi0-0-0-0", + "drive-scsi1-0-2", + "scsi0-hd0", + "scsi1-hd3", + "none0", + "none12", + "QM00001", + "qm00005", + } { + if IsUsableHardwareID(serial) { + t.Errorf("QEMU default serial %q was treated as usable hardware identity", serial) + } + if got := PreferredID(serial, "", "pve1", "/dev/sda", "", ""); got != "pve1:sda" { + t.Errorf("QEMU default serial %q produced ID %q, want scoped fallback pve1:sda", serial, got) + } + if HardwareIdentityMatch(serial, "", serial, "") { + t.Errorf("QEMU default serial %q matched itself as hardware identity", serial) + } + } + + // Anything outside those exact shapes stays usable, including real + // serials that share a prefix with them. ATA disks never surface their + // drive ID, so only SCSI drive IDs are rejected. + for _, serial := range []string{ + "DRIVE-ASSET123", + "drive-sata0", + "drive-scsi0a", + "drive-scsi0-0-0-0-0", + "drive-scsi", + "scsi0-cd0", + "ide0-hd0", + "none0a", + "nonesuch1", + "QM0001", + "QM000001", + "QM00001A", + "QMX0001", + } { + if !IsUsableHardwareID(serial) { + t.Errorf("serial %q outside QEMU's default grammar was rejected", serial) + } + } +} + func TestHardwareIdentityMatch(t *testing.T) { cases := []struct { name string