diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index b1be37aa4c..9c28cda4e8 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -5325,7 +5325,8 @@ TrueNAS API and the agent on that box), and an unknown machine never splits a disk. A split (operator exclusion) recorded against any ID the disk can hold, unscoped, current or machine-scoped, keeps the observation on its source-specific ID, keyed to its machine when another machine's disk already -holds that ID (agent disk source IDs are the bare serial). Once the identity +holds that ID (an agent disk's source-specific ID hashes its bare serial or +WWN, below). Once the identity spans machines the unscoped ID no longer names one disk, so a split recorded against it applies to every copy, erring towards an extra row rather than a merge the operator forbade, and a manual link recorded against it stops @@ -5343,14 +5344,51 @@ same-serial disk first appears on another machine and back when it goes away, and alert-history rows owned under the unscoped ID do not follow a re-key; a disk two reporters share is scoped to its canonical parent when the identity first spans machines, which the fixed ingest order (snapshot sources, then -supplemental records) and source priority decide; agent SMART and Unraid disk -source IDs are the bare serial, so same-serial agent disks on different hosts -share one `SourceAgent` mapping and only the last ingested keeps the agent -source target; same-serial disks share one metrics history key; and across -reporters, hostnames that normalize alike (default TrueNAS names, short names -across domains) still join. `IdentityMatcher` keeps one resource per machine -ID, so the registry indexes disks by hardware key (`physicalDisksByHardware`) -to see every copy. +supplemental records) and source priority decide; and across reporters, +hostnames that normalize alike (default TrueNAS names, short names across +domains) still join. `IdentityMatcher` keeps one resource per machine ID, so +the registry indexes disks by hardware key (`physicalDisksByHardware`) to see +every copy. +An agent disk's source ID carries its host, because the registry keeps one +resource per source ID: when it was the bare serial, same-serial agent disks on +different hosts shared one `SourceAgent` mapping, every disk but the last +ingested lost its agent source target, and a disk only the agent reports lost +its metrics target and chart. `HostSMARTDiskSourceID` and +`HostUnraidDiskSourceID` key a disk with a usable serial or WWN as +`/physical-disk:`, which the SMART and Unraid +observations of one disk share when both report its serial; a disk without +hardware identity keeps its host/device/topology key. A registry seeded from +unified resources rebuilds one key from the agent ID the host above the disk +retains, directly or above its Unraid array or cache pool, and the merged +disk's serial or WWN (`seedAgentPhysicalDiskSourceIDLocked`), so a rehydrated +disk, which is what the resource API serves, keeps an agent source target, +though not every key its observations ingest under (a SMART row missing the +serial its Unraid row reports, or a host several agent IDs report from); with +no agent host found, a disk with hardware identity seeds its bare serial or +WWN. Canonical IDs do not move: a disk with hardware identity is keyed by it, +and `SourceSpecificID` hashes an agent disk's bare serial or WWN without the +host (`sourceSpecificIDKey`), so a disk split off onto its source-specific ID, +and the exclusion that split it, keep the IDs recorded before the host joined +the source ID. Every host's split copy shares that ID, so a split observation +takes it keyed to its machine when another machine's disk holds it, and joins +the holder on its own machine, whether the split reaches it through +`physicalDiskIDForMachineLocked` or `findMatch`. A split therefore cannot +separate observations of one disk that one machine reports under several agent +IDs: they meet on that ID, as they met on one source key before. Disk history stays keyed by +hardware identity, not host: `HostSMARTDiskMetricID`, like the Proxmox and +TrueNAS writers and `PhysicalDiskMetaMetricID`, prefers the reported serial or +WWN, so a drive keeps one SMART and I/O history across a move between hosts and +across sources that report it by the same identifier, and both hosts of a +dual-ported SAS disk chart the one drive. A host-scoped history key would +orphan the stored history of every agent disk with hardware identity for the +rare collision. Residual: distinct drives that share a usable serial on +different hosts (cloned VM disks with an explicit serial, fixed-serial USB +bridges) interleave their samples in one series, as they already merge into one +disk on a single host. Proof: +`TestSameSerialAgentDisksOnTwoHostsEachKeepTheirMetricsTarget`, +`TestHostSMARTDiskSourceIDScopesHardwareIdentityToItsHost`, +`TestRehydratedAgentDisksKeepTheirLiveSourceIDs`, +`TestSplitAgentDisksOnTwoMachinesNeverOverwriteEachOther`. That same canonical physical-disk view must also expose source-independent host context. When a disk is API-backed rather than node-backed, typed views should fall back to canonical host identity such as `identity.hostnames` instead of diff --git a/internal/monitoring/issue1595_collection_trust_test.go b/internal/monitoring/issue1595_collection_trust_test.go index 494133db65..a484609166 100644 --- a/internal/monitoring/issue1595_collection_trust_test.go +++ b/internal/monitoring/issue1595_collection_trust_test.go @@ -373,6 +373,86 @@ func TestWriteHostSMARTMetricsRecordsOnlyCollectedTemperatures(t *testing.T) { } } +// A dual-ported SAS shelf, cloned VMs with an explicit serial and fixed-serial +// USB bridges report one usable serial on several hosts. The registry keeps a +// disk per host, and each must keep its own agent source target, in the live +// registry and in one rehydrated from its resources, or a disk only the agent +// reports has no metrics target and no chart. Both targets read the series the +// agent's SMART writer files under the serial: disk history follows the drive's +// hardware identity, not the host. +func TestSameSerialAgentDisksOnTwoHostsEachKeepTheirMetricsTarget(t *testing.T) { + storeConfig := metrics.DefaultConfig(t.TempDir()) + storeConfig.FlushInterval = time.Hour + store, err := metrics.NewStore(storeConfig) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = store.Close() }) + m := &Monitor{metricsStore: store} + + now := time.Now().UTC().Truncate(time.Second) + const serial = "SHELF-SERIAL-1" + hosts := []models.Host{ + {ID: "agent-alpha", Hostname: "alpha", MachineID: "machine-alpha", Status: "online", LastSeen: now}, + {ID: "agent-beta", Hostname: "beta", MachineID: "machine-beta", Status: "online", LastSeen: now}, + } + wantSourceIDs := make(map[string]bool, len(hosts)) + for i := range hosts { + hosts[i].Sensors.SMART = []models.HostDiskSMART{{Device: "sda", Serial: serial, Model: "Shelf Disk", Type: "sas", + Health: "PASSED", Temperature: 40 + i, Collection: &diskinventory.CollectionStatus{Temperature: diskinventory.Available("smartctl")}}} + m.writeHostSMARTMetrics(hosts[i], now) + wantSourceIDs[unifiedresources.HostSMARTDiskSourceID(hosts[i], hosts[i].Sensors.SMART[0])] = true + } + store.Flush() + if len(wantSourceIDs) != len(hosts) { + t.Fatalf("agent disk source IDs = %v, want one per host", wantSourceIDs) + } + + live := unifiedresources.NewRegistry(nil) + live.IngestSnapshot(models.StateSnapshot{Hosts: hosts}) + rehydrated := unifiedresources.NewRegistry(nil) + rehydrated.IngestResources(live.List()) + for _, tc := range []struct { + name string + registry *unifiedresources.ResourceRegistry + }{{"live", live}, {"rehydrated", rehydrated}} { + disks := tc.registry.ListByType(unifiedresources.ResourceTypePhysicalDisk) + if len(disks) != len(hosts) { + t.Fatalf("%s: disks = %d, want one per host", tc.name, len(disks)) + } + gotSourceIDs := make(map[string]bool, len(disks)) + for _, disk := range disks { + for _, target := range tc.registry.SourceTargets(disk.ID) { + if target.Source == unifiedresources.SourceAgent { + gotSourceIDs[target.SourceID] = true + } + } + target := tc.registry.MetricsTarget(disk.ID) + if target == nil { + t.Fatalf("%s: disk %s has no metrics target", tc.name, disk.ID) + } + if target.ResourceType != "disk" || target.ResourceID != serial { + t.Fatalf("%s: disk %s metrics target = %+v, want the serial's disk series", tc.name, disk.ID, *target) + } + points, err := store.Query(target.ResourceType, target.ResourceID, "smart_temp", now.Add(-time.Minute), now.Add(time.Minute), 0) + if err != nil { + t.Fatal(err) + } + if len(points) == 0 { + t.Fatalf("%s: disk %s metrics target reads no SMART temperature history", tc.name, disk.ID) + } + } + if len(gotSourceIDs) != len(wantSourceIDs) { + t.Fatalf("%s: agent source targets = %v, want each host's own %v", tc.name, gotSourceIDs, wantSourceIDs) + } + for sourceID := range wantSourceIDs { + if !gotSourceIDs[sourceID] { + t.Fatalf("%s: agent source targets = %v, want each host's own %v", tc.name, gotSourceIDs, wantSourceIDs) + } + } + } +} + // 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 diff --git a/internal/unifiedresources/canonical_ids_types_test.go b/internal/unifiedresources/canonical_ids_types_test.go index b46db9f7a8..149627b6b8 100644 --- a/internal/unifiedresources/canonical_ids_types_test.go +++ b/internal/unifiedresources/canonical_ids_types_test.go @@ -483,9 +483,9 @@ func TestHostUnraidDiskSourceIDNormalizesDeviceAndPrefersSerial(t *testing.T) { want: "host-tower:sdf", }, { - name: "serial wins", + name: "serial wins, scoped to its host", disk: models.HostUnraidDisk{Device: "/dev/sdg", Serial: "SERIAL-DATA"}, - want: "SERIAL-DATA", + want: "host-tower/physical-disk:SERIAL-DATA", }, { name: "slot fallback", @@ -503,6 +503,212 @@ func TestHostUnraidDiskSourceIDNormalizesDeviceAndPrefersSerial(t *testing.T) { } } +// An agent disk's source ID carries its host, because a usable serial or WWN +// names the drive but not the machine; its metrics key is the hardware ID +// alone. Without hardware identity the two keep one host/device/topology key. +// The source-specific ID an operator split derives from still hashes the bare +// hardware ID it was derived from before the host joined the source ID. +func TestHostSMARTDiskSourceIDScopesHardwareIdentityToItsHost(t *testing.T) { + host := models.Host{ID: "host-tower"} + tests := []struct { + name string + disk models.HostDiskSMART + wantSource string + wantMetric string + }{ + { + name: "serial", + disk: models.HostDiskSMART{Device: "/dev/sda", Serial: "SERIAL-A", WWN: "5000c500a1b2c3d4"}, + wantSource: "host-tower/physical-disk:SERIAL-A", + wantMetric: "SERIAL-A", + }, + { + name: "wwn when the serial is a placeholder", + disk: models.HostDiskSMART{Device: "sdb", Serial: "To Be Filled By O.E.M.", WWN: "5000c500a1b2c3d4"}, + wantSource: "host-tower/physical-disk:5000c500a1b2c3d4", + wantMetric: "5000c500a1b2c3d4", + }, + { + name: "no hardware identity", + disk: models.HostDiskSMART{Device: "sdc [sat]", Serial: "0000000000"}, + wantSource: "host-tower:sdc", + wantMetric: "host-tower:sdc", + }, + { + name: "identity-less controller member", + disk: models.HostDiskSMART{Device: "sdd", Controller: "ctrl0", Target: "megaraid,3"}, + wantSource: "host-tower:sdd@ctrl0/megaraid,3", + wantMetric: "host-tower:sdd@ctrl0/megaraid,3", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := HostSMARTDiskSourceID(host, tt.disk); got != tt.wantSource { + t.Fatalf("HostSMARTDiskSourceID = %q, want %q", got, tt.wantSource) + } + if got := HostSMARTDiskMetricID(host, tt.disk); got != tt.wantMetric { + t.Fatalf("HostSMARTDiskMetricID = %q, want %q", got, tt.wantMetric) + } + }) + } + + sourceID := HostSMARTDiskSourceID(host, tests[0].disk) + if got, want := SourceSpecificID(ResourceTypePhysicalDisk, SourceAgent, sourceID), SourceSpecificID(ResourceTypePhysicalDisk, SourceAgent, "SERIAL-A"); got != want { + t.Fatalf("agent disk source-specific ID = %q, want the bare serial's %q", got, want) + } + rr := NewRegistry(nil) + if got, want := rr.sourceSpecificID(ResourceTypePhysicalDisk, SourceAgent, sourceID), SourceSpecificID(ResourceTypePhysicalDisk, SourceAgent, sourceID); got != want { + t.Fatalf("registry source-specific ID = %q, want SourceSpecificID's %q", got, want) + } + if got, want := SourceSpecificID(ResourceTypeStorage, SourceAgent, "host-tower/physical-disk:x"), SourceSpecificID(ResourceTypeStorage, SourceAgent, "x"); got == want { + t.Fatalf("only agent physical disks drop the host from their source-specific ID, got %q for storage", got) + } +} + +// A registry seeded from unified resources rebuilds each agent disk's source +// mapping from the resource alone, after a JSON round trip drops per-source +// parents. It must reproduce the key the agent's observation ingests under, +// for a disk on the host, behind a controller, in the Unraid array or a cache +// pool, or merged with a linked node's Proxmox row, with or without hardware +// identity, so a rehydrated disk keeps its agent source and metrics targets. +func TestRehydratedAgentDisksKeepTheirLiveSourceIDs(t *testing.T) { + now := time.Now().UTC() + tower := models.Host{ + ID: "host-tower", Hostname: "tower", MachineID: "machine-tower", Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{ + {Device: "sda", Serial: "BOOT-SERIAL", Model: "Boot SSD", Health: "PASSED"}, + {Device: "sdb", Serial: "ARRAY-SERIAL", Model: "Array Disk", Health: "PASSED"}, + {Device: "sdc", Model: "Array Disk", Health: "PASSED"}, + {Device: "nvme0n1", Serial: "CACHE-SERIAL", Model: "Cache NVMe", Health: "PASSED"}, + {Device: "sdd", Model: "RAID Member", Controller: "ctrl0", Target: "megaraid,3", Health: "PASSED"}, + }}, + Unraid: &models.HostUnraidStorage{ArrayStarted: true, ArrayState: "STARTED", Disks: []models.HostUnraidDisk{ + {Name: "disk1", Device: "sdb", Role: "data", Status: "DISK_OK", Serial: "ARRAY-SERIAL"}, + {Name: "disk2", Device: "sdc", Role: "data", Status: "DISK_OK"}, + {Name: "cache", Device: "nvme0n1", Role: "cache", Status: "DISK_OK", Serial: "CACHE-SERIAL"}, + }}, + } + for _, tc := range []struct { + name string + snapshot models.StateSnapshot + wantSourceIDs map[string]bool + }{ + { + name: "unraid host", + snapshot: models.StateSnapshot{Hosts: []models.Host{tower}}, + wantSourceIDs: map[string]bool{ + "host-tower/physical-disk:BOOT-SERIAL": true, + "host-tower/physical-disk:ARRAY-SERIAL": true, + "host-tower:sdc": true, + "host-tower/physical-disk:CACHE-SERIAL": true, + "host-tower:sdd@ctrl0/megaraid,3": true, + }, + }, + { + name: "linked proxmox node", + snapshot: sharedSerialDiskSnapshot(now, true, "pve1"), + wantSourceIDs: map[string]bool{"host-pve1/physical-disk:" + sharedDiskSerial: true}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + live := NewRegistry(nil) + live.IngestSnapshot(tc.snapshot) + payload, err := json.Marshal(live.List()) + if err != nil { + t.Fatal(err) + } + var persisted []Resource + if err := json.Unmarshal(payload, &persisted); err != nil { + t.Fatal(err) + } + rehydrated := NewRegistry(nil) + rehydrated.IngestResources(persisted) + + agentSourceID := func(rr *ResourceRegistry, diskID string) string { + for _, target := range rr.SourceTargets(diskID) { + if target.Source == SourceAgent { + return target.SourceID + } + } + return "" + } + seen := make(map[string]bool, len(tc.wantSourceIDs)) + for _, disk := range live.ListByType(ResourceTypePhysicalDisk) { + liveID := agentSourceID(live, disk.ID) + if liveID == "" { + continue + } + seen[liveID] = true + if rehydratedID := agentSourceID(rehydrated, disk.ID); rehydratedID != liveID { + t.Fatalf("disk %s rehydrated agent source ID = %q, want the live %q", disk.Name, rehydratedID, liveID) + } + if live, seeded := live.MetricsTarget(disk.ID), rehydrated.MetricsTarget(disk.ID); live == nil || seeded == nil || *live != *seeded { + t.Fatalf("disk %s metrics target live %+v, rehydrated %+v, want one non-nil target", disk.Name, live, seeded) + } + } + if len(seen) != len(tc.wantSourceIDs) { + t.Fatalf("live agent source IDs = %v, want %v", seen, tc.wantSourceIDs) + } + for id := range tc.wantSourceIDs { + if !seen[id] { + t.Fatalf("live agent source IDs = %v, want %v", seen, tc.wantSourceIDs) + } + } + }) + } +} + +// Two agent IDs can report from one machine (a re-enrolled agent whose old +// record still reports), so their disks no longer share a source key, and an +// operator split sends the second onto the source-specific ID, which hashes +// the bare serial. Another machine's split copy must then take that ID keyed +// to its own machine, never overwrite the first machine's disk. +func TestSplitAgentDisksOnTwoMachinesNeverOverwriteEachOther(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + host := func(id, machine string) models.Host { + return models.Host{ + ID: id, Hostname: machine, MachineID: "machine-" + machine, Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", Model: "SEAGATE ST4000NM0023", Serial: sharedDiskSerial, Type: "sas", Health: "PASSED", + }}}, + } + } + snapshot := models.StateSnapshot{Hosts: []models.Host{host("c1", "c"), host("c2", "c"), host("a1", "a"), host("a2", "a")}} + + unsplit := NewRegistry(nil) + unsplit.IngestSnapshot(snapshot) + machineADisk := unsplit.sourceResourceID(SourceAgent, HostSMARTDiskSourceID(snapshot.Hosts[2], snapshot.Hosts[2].Sensors.SMART[0])) + if machineADisk == "" { + t.Fatal("machine a's disk has no agent source mapping") + } + store := NewMemoryStore() + agentCandidate := SourceSpecificID(ResourceTypePhysicalDisk, SourceAgent, sharedDiskSerial) + for _, splitFrom := range []string{MachineIdentityCanonicalID(ResourceTypePhysicalDisk, sharedDiskSerial), machineADisk} { + if err := store.AddExclusion(ResourceExclusion{ResourceA: splitFrom, ResourceB: agentCandidate}); err != nil { + t.Fatal(err) + } + } + + split := NewRegistry(store) + split.IngestSnapshot(snapshot) + diskIDs := make(map[string]bool, len(snapshot.Hosts)) + for _, h := range snapshot.Hosts { + hostID := split.sourceResourceID(SourceAgent, h.ID) + diskID := split.sourceResourceID(SourceAgent, HostSMARTDiskSourceID(h, h.Sensors.SMART[0])) + disk, ok := split.Get(diskID) + if hostID == "" || !ok { + t.Fatalf("%s: host %q disk %q, want both mapped", h.ID, hostID, diskID) + } + if disk.ParentID == nil || *disk.ParentID != hostID { + t.Fatalf("%s's disk %s sits under %v, want its own machine %s", h.ID, diskID, disk.ParentID, hostID) + } + diskIDs[diskID] = true + } + if !diskIDs[agentCandidate] { + t.Fatalf("disk IDs = %v, want the first split copy on its source-specific ID %s", diskIDs, agentCandidate) + } +} + func TestIsUnsupportedLegacyResourceIDAlias(t *testing.T) { tests := []struct { name string diff --git a/internal/unifiedresources/ids.go b/internal/unifiedresources/ids.go index 7be3372e17..cce0724f5d 100644 --- a/internal/unifiedresources/ids.go +++ b/internal/unifiedresources/ids.go @@ -15,12 +15,14 @@ func normalizeSourceID(sourceID string) string { // SourceSpecificID returns the deterministic ID used for non-host resources when the registry // does not have a canonical identity to key off of. // -// This matches the ResourceRegistry's internal ID derivation: +// The ResourceRegistry derives its source-specific IDs here: // stable := fmt.Sprintf("%s:%s", source, sourceID) // id := fmt.Sprintf("%s-%s", resourceType, hex(sha256(stable)[:8])) +// An agent disk hashes its hardware ID without the host its source ID +// carries (sourceSpecificIDKey). func SourceSpecificID(resourceType ResourceType, source DataSource, sourceID string) string { resourceType = CanonicalResourceType(resourceType) - stable := fmt.Sprintf("%s:%s", source, normalizeSourceID(sourceID)) + stable := fmt.Sprintf("%s:%s", source, sourceSpecificIDKey(resourceType, source, sourceID)) hash := sha256.Sum256([]byte(stable)) return fmt.Sprintf("%s-%s", resourceType, hex.EncodeToString(hash[:8])) } diff --git a/internal/unifiedresources/metrics_targets.go b/internal/unifiedresources/metrics_targets.go index 7d301fc80b..101743e816 100644 --- a/internal/unifiedresources/metrics_targets.go +++ b/internal/unifiedresources/metrics_targets.go @@ -107,11 +107,14 @@ func BuildMetricsTarget(resource Resource, sourceTargets []SourceTarget) *Metric case ResourceTypePhysicalDisk: // The agent source must win over platform sources for physical // disks: SMART/temperature metrics are written by the host agent - // under the agent's disk source ID (HostSMARTDiskSourceID in - // monitor_agents.go). Proxmox API does not expose detailed SMART - // data, so preferring its source ID here would point store readers - // at a key nothing writes and disk metrics would appear empty - // (GitHub issue #1487). + // (HostSMARTDiskMetricID in monitor_agents.go). Proxmox API does + // not expose detailed SMART data, so preferring its source ID here + // would point store readers at a key nothing writes and disk + // metrics would appear empty (GitHub issue #1487). The agent's + // source ID is host-scoped (HostSMARTDiskSourceID), while its + // metrics key is the disk's serial or WWN, so a disk with hardware + // identity resolves to that. Only an identity-less disk falls back + // to its source ID, which keeps the shape the writer uses. if st, ok := bySource[SourceAgent]; ok { if resourceID := PhysicalDiskMetaMetricID(resource.PhysicalDisk, st.SourceID); resourceID != "" { return &MetricsTarget{ResourceType: "disk", ResourceID: resourceID} diff --git a/internal/unifiedresources/metrics_targets_test.go b/internal/unifiedresources/metrics_targets_test.go index c689df6577..f9d95c08d6 100644 --- a/internal/unifiedresources/metrics_targets_test.go +++ b/internal/unifiedresources/metrics_targets_test.go @@ -408,6 +408,29 @@ func TestBuildMetricsTarget_AgentSourceWinsForPhysicalDiskMetrics(t *testing.T) } } +// An agent disk's source ID carries its host while its metrics are written +// under its serial or WWN, so a disk only the agent reports must resolve the +// writer's key from its host-scoped source target. Without hardware identity +// the source ID is the writer's key. +func TestBuildMetricsTarget_AgentOnlyPhysicalDiskReadsTheWritersKey(t *testing.T) { + host := models.Host{ID: "agent-uuid", Hostname: "tower"} + for _, disk := range []models.HostDiskSMART{ + {Device: "sda", Serial: "WD-WCC4N0000001", WWN: "5000c500a1b2c3d4"}, + {Device: "sdb", WWN: "5000c500a1b2c3d5"}, + {Device: "sdc"}, + } { + resource, _ := resourceFromHostSMARTDisk(host, disk) + sourceID := HostSMARTDiskSourceID(host, disk) + target := BuildMetricsTarget(resource, []SourceTarget{{Source: SourceAgent, SourceID: sourceID}}) + if target == nil { + t.Fatalf("%s: BuildMetricsTarget() returned nil for agent source %q", disk.Device, sourceID) + } + if want := HostSMARTDiskMetricID(host, disk); target.ResourceType != "disk" || target.ResourceID != want { + t.Fatalf("%s: metrics target = %+v, want disk/%s", disk.Device, *target, want) + } + } +} + func TestBuildMetricsTarget_PhysicalDiskSerialWinsOverAllSources(t *testing.T) { // When the disk has a serial number, it becomes the metric ID // regardless of source, so the priority order doesn't matter. diff --git a/internal/unifiedresources/physical_disk_ids.go b/internal/unifiedresources/physical_disk_ids.go index a47ee8caa5..d4e55d46e8 100644 --- a/internal/unifiedresources/physical_disk_ids.go +++ b/internal/unifiedresources/physical_disk_ids.go @@ -22,8 +22,16 @@ func PreferredPhysicalDiskMetricID(serial, wwn, fallback string) string { } // HostSMARTDiskSourceID returns the registry source ID of a disk a host agent -// reports through SMART. +// reports through SMART. A usable serial or WWN names the drive but not the +// machine: a dual-ported SAS shelf, cloned VMs with an explicit serial and +// fixed-serial USB bridges report one identifier on several hosts, and the +// registry keeps one resource per source ID. The key therefore carries the +// reporting host. A disk without hardware identity keeps its historical +// host/device/topology key, which is also its metrics key. func HostSMARTDiskSourceID(host models.Host, disk models.HostDiskSMART) string { + if hardwareID := PreferredPhysicalDiskMetricID(disk.Serial, disk.WWN, ""); hardwareID != "" { + return hostPhysicalDiskSourceID(host.ID, hardwareID) + } return HostSMARTDiskMetricID(host, disk) } @@ -43,15 +51,91 @@ func HostSMARTDiskMetricID(host models.Host, disk models.HostDiskSMART) string { ) } +// HostUnraidDiskSourceID returns the registry source ID of a disk in a host's +// Unraid inventory. It shares HostSMARTDiskSourceID's host-scoped serial key so +// the SMART and Unraid observations of one disk keep one source mapping. func HostUnraidDiskSourceID(host models.Host, disk models.HostUnraidDisk) string { - device := normalizePhysicalDiskDeviceToken(disk.Device) - fallback := "" - if device != "" { - fallback = fmt.Sprintf("%s:%s", strings.TrimSpace(host.ID), device) - } else if strings.TrimSpace(disk.Name) != "" { - fallback = fmt.Sprintf("%s:unraid-slot:%s", strings.TrimSpace(host.ID), strings.TrimSpace(disk.Name)) + if serial := PreferredPhysicalDiskMetricID(disk.Serial, "", ""); serial != "" { + return hostPhysicalDiskSourceID(host.ID, serial) } - return PreferredPhysicalDiskMetricID(disk.Serial, "", fallback) + device := normalizePhysicalDiskDeviceToken(disk.Device) + if device != "" { + return fmt.Sprintf("%s:%s", strings.TrimSpace(host.ID), device) + } + if name := strings.TrimSpace(disk.Name); name != "" { + return fmt.Sprintf("%s:unraid-slot:%s", strings.TrimSpace(host.ID), name) + } + return "" +} + +const hostPhysicalDiskSourceIDMarker = "/physical-disk:" + +func hostPhysicalDiskSourceID(hostID, hardwareID string) string { + if hostID = strings.TrimSpace(hostID); hostID == "" { + return hardwareID + } + return hostID + hostPhysicalDiskSourceIDMarker + hardwareID +} + +// sourceSpecificIDKey returns the part of a source ID that SourceSpecificID +// hashes. An agent disk's source ID carries its host, but it used to be the +// bare serial or WWN, and the ID of a disk split from its match, like the +// operator exclusion that split it, was derived from that. The host stays out +// of the hash so both keep applying across the change. +func sourceSpecificIDKey(resourceType ResourceType, source DataSource, sourceID string) string { + sourceID = normalizeSourceID(sourceID) + if source != SourceAgent || CanonicalResourceType(resourceType) != ResourceTypePhysicalDisk { + return sourceID + } + if _, hardwareID, ok := strings.Cut(sourceID, hostPhysicalDiskSourceIDMarker); ok && hardwareID != "" { + return hardwareID + } + return sourceID +} + +// seedAgentPhysicalDiskSourceIDLocked rebuilds an agent disk's source ID for a +// registry seeded from unified resources, which carry no source mappings. It +// must reproduce the key HostSMARTDiskSourceID and HostUnraidDiskSourceID +// ingest under, or a rehydrated disk loses its agent source target, and with +// it the metrics target of a disk only the agent reports. When the reporting +// host cannot be found, a disk with hardware identity falls back to the bare +// serial or WWN its source ID used to be. +func (rr *ResourceRegistry) seedAgentPhysicalDiskSourceIDLocked(resource *Resource) string { + disk := resource.PhysicalDisk + if disk == nil { + return "" + } + hostID := rr.seedAgentDiskHostIDLocked(resource) + if hardwareID := PreferredPhysicalDiskMetricID(disk.Serial, disk.WWN, ""); hardwareID != "" { + return hostPhysicalDiskSourceID(hostID, hardwareID) + } + if hostID == "" { + return "" + } + return diskinventory.PreferredID("", "", hostID, disk.DevPath, disk.Controller, disk.Target) +} + +// seedAgentDiskHostIDLocked returns the agent ID of the host an agent-reported +// disk sits on: its parent, or the host above the Unraid array or cache pool +// that parents it. A parent that is neither yields no host. +func (rr *ResourceRegistry) seedAgentDiskHostIDLocked(resource *Resource) string { + parentID := resource.ParentID + for depth := 0; parentID != nil && depth < 4; depth++ { + parent := rr.resources[CanonicalResourceID(strings.TrimSpace(*parentID))] + if parent == nil { + return "" + } + if parent.Agent != nil { + if agentID := strings.TrimSpace(parent.Agent.AgentID); agentID != "" { + return agentID + } + } + if CanonicalResourceType(parent.Type) != ResourceTypeStorage { + return "" + } + parentID = parent.ParentID + } + return "" } // ProxmoxPhysicalDiskSourceID returns the source-native ID used for physical diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index bad50d3eff..043eee072a 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -947,20 +947,7 @@ func (rr *ResourceRegistry) seedSourceIDForResourceLocked(resource *Resource, so } } case ResourceTypePhysicalDisk: - if resource.PhysicalDisk == nil { - return "" - } - if parentSourceID := rr.seedParentSourceIDLocked(resource, SourceAgent); parentSourceID != "" { - return diskinventory.PreferredID( - resource.PhysicalDisk.Serial, - resource.PhysicalDisk.WWN, - parentSourceID, - resource.PhysicalDisk.DevPath, - resource.PhysicalDisk.Controller, - resource.PhysicalDisk.Target, - ) - } - return PreferredPhysicalDiskMetricID(resource.PhysicalDisk.Serial, resource.PhysicalDisk.WWN, "") + return rr.seedAgentPhysicalDiskSourceIDLocked(resource) } case SourceDocker: if resource.Docker == nil { @@ -2958,6 +2945,21 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res } } else if excluded { resource.ID = candidateID + if resource.Type == ResourceTypePhysicalDisk { + // An agent disk's source-specific ID hashes its bare serial + // (sourceSpecificIDKey), so another machine's split copy may + // hold it, and one machine's observations may meet on it. + resource.ID = rr.physicalDiskFallbackIDLocked(candidateID, &resource, source) + if existing := rr.resources[resource.ID]; existing != nil && existing.Type == ResourceTypePhysicalDisk { + if onlyMissing { + return "" + } + rr.mergeInto(existing, resource, source, sourceID) + rr.bySource[source][sourceID] = existing.ID + rr.matcher.Add(existing.ID, existing.Identity) + return existing.ID + } + } if onlyMissing && rr.resources[resource.ID] != nil { return "" } @@ -5448,8 +5450,9 @@ func (rr *ResourceRegistry) physicalDiskScopedIDLocked(disk *Resource) string { } // physicalDiskFallbackIDLocked returns the observation's source-specific ID, -// keyed to its machine when another machine's disk already holds it: agent -// disk source IDs are the bare serial, so one candidate ID serves every host. +// keyed to its machine when another machine's disk already holds it: an agent +// disk's source-specific ID hashes its bare serial (sourceSpecificIDKey), so +// one candidate ID serves every host. func (rr *ResourceRegistry) physicalDiskFallbackIDLocked(candidateID string, incoming *Resource, source DataSource) string { holder := rr.resources[candidateID] if holder == nil || (holder.Type == ResourceTypePhysicalDisk && !rr.physicalDiskMachinesConflictLocked(holder, incoming, source)) { @@ -5608,8 +5611,7 @@ func (rr *ResourceRegistry) canonicalIDFromIdentity(resourceType ResourceType, i } func (rr *ResourceRegistry) sourceSpecificID(resourceType ResourceType, source DataSource, sourceID string) string { - stable := fmt.Sprintf("%s:%s", source, normalizeSourceID(sourceID)) - return buildHashID(resourceType, stable) + return SourceSpecificID(resourceType, source, sourceID) } func buildHashID(resourceType ResourceType, stable string) string {