diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index f1d7dce1a5..796fc46ca0 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -4104,6 +4104,14 @@ ambiguous. Direct SATA, SAS, and NVMe device fallback IDs retain their legacy shape, while multiple controller members behind one block path add their controller target to the fallback identity. Per-member I/O must never inherit an aggregate controller counter. +A linked node's agent with no SMART reading for a device, such as a member's +standby row, files that device's I/O under the Proxmox disk's metrics target +(`proxmoxPhysicalDiskMatchesForLinkedNode` reads `MetricResourceID`). For a +member merged with its Proxmox row that target is the member's own SMART key, +scoped to the member once (`PhysicalDiskMetaMetricID`), so its SMART and I/O +history share one series. The target used to carry the member topology twice, +and I/O already stored under that doubled key is left to age out rather than +migrated. Proof: `TestIdentitylessControllerMembersReadTheirWritersHistory`. The same rules apply to SATA and NVMe inventory: direct-disk source IDs keep their historical shape, controller-member IDs add their member target, and cross-source correlation is scoped to the canonical parent node. A successful diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index c91656d065..84269fbcce 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -3554,6 +3554,24 @@ remain preferred over device fallback. Direct SATA, SAS, and NVMe fallbacks retain their historical device-scoped shape; controller members sharing one block path add controller/target scope so they cannot collapse into one resource. Topology correlation is parent-scoped and ambiguity fails closed. +That scope is added once. A controller member without a usable serial or WWN +keeps its history under a source ID that already names the member: the host +agent's `HostSMARTDiskMetricID`, and the Proxmox `PhysicalDiskMetricID`, which +returns `ProxmoxPhysicalDiskSourceID`. `PhysicalDiskMetaMetricID` therefore +returns a fallback that already ends in the disk's own device, controller and +target unchanged as the metrics target. It appends that topology only to a +fallback without it: a Proxmox source ID from before members were scoped, +which `PhysicalDiskMetricID` scopes the same way, or the canonical resource ID +`PhysicalDiskView.MetricResourceID` falls back to. The suffix survives a merge +because a member target only reaches a disk together with the controller the +agent reported beside it (the multiplexed smartctl path sets both), and the +Proxmox row copies both from that agent row. A registry rehydrated from +persisted resources rebuilds the agent source ID in the same scoped shape +(`seedAgentPhysicalDiskSourceIDLocked`). TrueNAS disks carry no controller +target and keep their fallback. The reader used to append the topology to an +already scoped source ID as well, so such a member's chart read a key no +writer used. Proof: `TestPhysicalDiskMetaMetricIDScopesAControllerMemberOnce`, +`TestIdentitylessControllerMembersReadTheirWritersHistory`. Host reports, monitoring models, registry merges, typed physical-disk views, API resources, websocket/REST snapshot merges, and storage presentation must diff --git a/internal/monitoring/issue1595_collection_trust_test.go b/internal/monitoring/issue1595_collection_trust_test.go index b79ac63f09..7486e493af 100644 --- a/internal/monitoring/issue1595_collection_trust_test.go +++ b/internal/monitoring/issue1595_collection_trust_test.go @@ -373,6 +373,146 @@ func TestWriteHostSMARTMetricsRecordsOnlyCollectedTemperatures(t *testing.T) { } } +// A controller member without a usable serial or WWN keys its history by its +// source ID, which already names the member behind the shared block path: the +// host agent writes under HostSMARTDiskMetricID, Proxmox under +// PhysicalDiskMetricID, which returns ProxmoxPhysicalDiskSourceID. The metrics +// target a chart reads must be that key, in the live registry and in one +// rehydrated from persisted resources, not the key with the member's topology +// appended a second time, or every sample those writers store goes unread. The +// agent's I/O for a lone member is filed under the member's SMART key. A linked +// node's agent files the I/O of a member it reads no SMART for through the +// Proxmox disk's metrics target, so that I/O must share the member's SMART key. +func TestIdentitylessControllerMembersReadTheirWritersHistory(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() }) + state := models.NewState() + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + m := &Monitor{state: state, resourceStore: adapter, metricsStore: store, rateTracker: NewRateTracker()} + + now := time.Now().UTC().Truncate(time.Second) + collected := func() *diskinventory.CollectionStatus { + return &diskinventory.CollectionStatus{Temperature: diskinventory.Available("smartctl")} + } + state.UpdateNodesForInstance("pve", []models.Node{ + {ID: "pve-node1", Name: "node1", Instance: "pve", Status: "online", LastSeen: now}, + {ID: "pve-node2", Name: "node2", Instance: "pve", Status: "online", LastSeen: now, LinkedAgentID: "agent-node2"}, + }) + standalone := models.Host{ID: "host-pve", Hostname: "standalone", MachineID: "machine-standalone", Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{ + {Device: "sdd", Controller: "ctrl0", Target: "megaraid,3", Type: "sas", Temperature: 33, Collection: collected()}, + {Device: "sdd", Controller: "ctrl0", Target: "megaraid,4", Type: "sas", Temperature: 34, Collection: collected()}, + {Device: "/dev/sde", Controller: "ctrl0", Target: "megaraid,5", Type: "sas", Temperature: 35, Collection: collected()}, + }}, + DiskIO: []models.DiskIO{{Device: "sde", ReadBytes: 1 << 20, WriteBytes: 1 << 20, IOTime: 100}}, + } + linked := models.Host{ID: "agent-node2", Hostname: "node2", MachineID: "machine-node2", LinkedNodeID: "pve-node2", Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{ + {Device: "sdc", Controller: "ctrl1", Target: "megaraid,1", Type: "sas", Standby: true, Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "disk is in standby")}}, + }}, + DiskIO: []models.DiskIO{{Device: "sdc", ReadBytes: 1 << 20, WriteBytes: 1 << 20, IOTime: 100}}, + } + state.UpsertHost(standalone) + state.UpsertHost(linked) + pveMember := models.PhysicalDisk{ + ID: unifiedresources.ProxmoxPhysicalDiskSourceID("pve", "node1", "/dev/sdx", "", "megaraid,0"), + Instance: "pve", Node: "node1", DevPath: "/dev/sdx", Target: "megaraid,0", Type: "sas", Temperature: 40, LastChecked: now, + } + pveBlockPath := models.PhysicalDisk{ + ID: unifiedresources.ProxmoxPhysicalDiskSourceID("pve", "node2", "/dev/sdc", "", ""), + Instance: "pve", Node: "node2", DevPath: "/dev/sdc", Serial: "unknown", Type: "sas", LastChecked: now, + } + state.UpdatePhysicalDisks("pve", []models.PhysicalDisk{pveMember, pveBlockPath}) + adapter.PopulateFromSnapshot(state.GetSnapshot()) + + hosts := []models.Host{standalone, linked} + for _, host := range hosts { + m.writeHostPhysicalDiskIOMetrics(host, now.Add(-30*time.Second)) + } + for _, host := range hosts { + for i := range host.DiskIO { + host.DiskIO[i].ReadBytes += 1 << 20 + host.DiskIO[i].WriteBytes += 1 << 20 + host.DiskIO[i].IOTime += 1000 + } + m.writeHostPhysicalDiskIOMetrics(host, now) + m.writeHostSMARTMetrics(host, now) + } + m.writeSMARTMetrics(pveMember, now) + store.Flush() + + smartTemp := []string{"smart_temp"} + diskIO := []string{"diskread", "diskwrite", "disk"} + want := map[string]struct { + key string + metrics []string + }{ + "megaraid,3": {unifiedresources.HostSMARTDiskMetricID(standalone, standalone.Sensors.SMART[0]), smartTemp}, + "megaraid,4": {unifiedresources.HostSMARTDiskMetricID(standalone, standalone.Sensors.SMART[1]), smartTemp}, + "megaraid,5": {unifiedresources.HostSMARTDiskMetricID(standalone, standalone.Sensors.SMART[2]), append(smartTemp, diskIO...)}, + "megaraid,0": {unifiedresources.PhysicalDiskMetricID(pveMember), smartTemp}, + "megaraid,1": {unifiedresources.HostSMARTDiskMetricID(linked, linked.Sensors.SMART[0]), diskIO}, + } + for target, member := range map[string]string{ + "megaraid,3": "host-pve:sdd@ctrl0/megaraid,3", + "megaraid,5": "host-pve:sde@ctrl0/megaraid,5", + "megaraid,0": "pve-node1--dev-sdx:sdx@/megaraid,0", + "megaraid,1": "agent-node2:sdc@ctrl1/megaraid,1", + } { + if want[target].key != member { + t.Fatalf("%s: writer key = %q, want the member scoped once %q", target, want[target].key, member) + } + } + + live := unifiedresources.NewRegistry(nil) + live.IngestSnapshot(state.GetSnapshot()) + payload, err := json.Marshal(live.List()) + if err != nil { + t.Fatal(err) + } + var persisted []unifiedresources.Resource + if err := json.Unmarshal(payload, &persisted); err != nil { + t.Fatal(err) + } + rehydrated := unifiedresources.NewRegistry(nil) + rehydrated.IngestResources(persisted) + for _, tc := range []struct { + name string + registry *unifiedresources.ResourceRegistry + }{{"live", live}, {"rehydrated", rehydrated}} { + disks := tc.registry.ListByType(unifiedresources.ResourceTypePhysicalDisk) + if len(disks) != len(want) { + t.Fatalf("%s: disks = %d, want %d with the standby member merged into its Proxmox row", tc.name, len(disks), len(want)) + } + for _, disk := range disks { + member, ok := want[disk.PhysicalDisk.Target] + if !ok { + t.Fatalf("%s: unexpected disk %s at %q target %q", tc.name, disk.ID, disk.PhysicalDisk.DevPath, disk.PhysicalDisk.Target) + } + target := tc.registry.MetricsTarget(disk.ID) + if target == nil || target.ResourceType != "disk" || target.ResourceID != member.key { + t.Errorf("%s: %s metrics target = %+v, want the writer's disk/%s", tc.name, disk.PhysicalDisk.Target, target, member.key) + continue + } + for _, metric := range member.metrics { + points, err := store.Query(target.ResourceType, target.ResourceID, metric, now.Add(-time.Minute), now.Add(time.Minute), 0) + if err != nil { + t.Fatal(err) + } + if len(points) == 0 { + t.Errorf("%s: %s metrics target %s reads no %s history", tc.name, disk.PhysicalDisk.Target, target.ResourceID, metric) + } + } + } + } +} + // 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 diff --git a/internal/unifiedresources/canonical_ids_types_test.go b/internal/unifiedresources/canonical_ids_types_test.go index 4be51f17b9..c8ed1bedb5 100644 --- a/internal/unifiedresources/canonical_ids_types_test.go +++ b/internal/unifiedresources/canonical_ids_types_test.go @@ -824,6 +824,84 @@ func TestSplitAgentDisksOnTwoMachinesNeverOverwriteEachOther(t *testing.T) { } } +// The metrics reader scopes a controller member's fallback key to the member +// once, as the writers do. A source ID that already names the member is the +// writer's key and stays unchanged. A Proxmox source ID from before members +// were scoped, and the canonical resource ID a view falls back to, get the +// member topology appended. A disk that is no controller member keeps its +// fallback. +func TestPhysicalDiskMetaMetricIDScopesAControllerMemberOnce(t *testing.T) { + agent := models.Host{ID: "host-pve"} + agentMember := models.HostDiskSMART{Device: "sdd", Controller: "ctrl0", Target: "megaraid,3"} + labelledMember := models.HostDiskSMART{Device: "sdc [megaraid,1]", Controller: "sdc", Target: "megaraid,1"} + pveMember := models.PhysicalDisk{ + ID: ProxmoxPhysicalDiskSourceID("pve", "node1", "/dev/sdx", "", "megaraid,0"), + Instance: "pve", Node: "node1", DevPath: "/dev/sdx", Target: "megaraid,0", + } + legacyPVEMember := pveMember + legacyPVEMember.ID = "pve-node1--dev-sdx" + tests := []struct { + name string + meta PhysicalDiskMeta + fallback string + want string + }{ + { + name: "agent member source ID", + meta: PhysicalDiskMeta{DevPath: "sdd", Controller: "ctrl0", Target: "megaraid,3"}, + fallback: HostSMARTDiskSourceID(agent, agentMember), + want: HostSMARTDiskMetricID(agent, agentMember), + }, + { + name: "agent member reported under its smartctl label, merged with its Proxmox path", + meta: PhysicalDiskMeta{DevPath: "/dev/sdc", Controller: "sdc", Target: "megaraid,1"}, + fallback: HostSMARTDiskSourceID(agent, labelledMember), + want: HostSMARTDiskMetricID(agent, labelledMember), + }, + { + name: "proxmox member source ID", + meta: PhysicalDiskMeta{DevPath: "/dev/sdx", Target: "megaraid,0"}, + fallback: pveMember.ID, + want: PhysicalDiskMetricID(pveMember), + }, + { + name: "proxmox member source ID from before members were scoped", + meta: PhysicalDiskMeta{DevPath: "/dev/sdx", Target: "megaraid,0"}, + fallback: legacyPVEMember.ID, + want: PhysicalDiskMetricID(legacyPVEMember), + }, + { + name: "canonical resource ID", + meta: PhysicalDiskMeta{DevPath: "/dev/sdx", Target: "megaraid,0"}, + fallback: "physical_disk-0123456789abcdef", + want: "physical_disk-0123456789abcdef:sdx@/megaraid,0", + }, + { + name: "no controller member", + meta: PhysicalDiskMeta{DevPath: "/dev/sda"}, + fallback: "truenas-system:disk:sda", + want: "truenas-system:disk:sda", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := PhysicalDiskMetaMetricID(&tt.meta, tt.fallback); got != tt.want { + t.Fatalf("PhysicalDiskMetaMetricID(%q) = %q, want %q", tt.fallback, got, tt.want) + } + }) + } + for name, key := range map[string]string{ + "agent member": HostSMARTDiskMetricID(agent, agentMember), + "proxmox member": PhysicalDiskMetricID(pveMember), + "legacy proxmox member": PhysicalDiskMetricID(legacyPVEMember), + "labelled agent member": HostSMARTDiskMetricID(agent, labelledMember), + } { + if strings.Count(key, "@") != 1 { + t.Fatalf("%s writer key %q, want the member topology once", name, key) + } + } +} + func TestIsUnsupportedLegacyResourceIDAlias(t *testing.T) { tests := []struct { name string diff --git a/internal/unifiedresources/physical_disk_ids.go b/internal/unifiedresources/physical_disk_ids.go index 3e96433b1a..4ba7cefa98 100644 --- a/internal/unifiedresources/physical_disk_ids.go +++ b/internal/unifiedresources/physical_disk_ids.go @@ -292,6 +292,14 @@ func PhysicalDiskMetricID(disk models.PhysicalDisk) string { return strings.TrimSpace(fallback) } +// PhysicalDiskMetaMetricID returns the history key metrics readers use for a +// physical disk resource. fallback keys a disk without a usable serial or WWN. +// The writers key a controller member's history by a source ID that already +// names the member (HostSMARTDiskMetricID, ProxmoxPhysicalDiskSourceID), so, +// as PhysicalDiskMetricID does, such a fallback is returned unchanged. Only a +// fallback without the member's topology gets it appended: a Proxmox source ID +// from before members were scoped, or the canonical resource ID a view falls +// back to. func PhysicalDiskMetaMetricID(disk *PhysicalDiskMeta, fallback string) string { if disk == nil { return strings.TrimSpace(fallback) @@ -302,15 +310,19 @@ func PhysicalDiskMetaMetricID(disk *PhysicalDiskMeta, fallback string) string { if wwn := strings.TrimSpace(disk.WWN); diskinventory.IsUsableHardwareID(wwn) { return wwn } - if diskinventory.IsControllerMemberTarget(disk.Target) && strings.TrimSpace(fallback) != "" { - return diskinventory.PreferredID( + if fallback = strings.TrimSpace(fallback); diskinventory.IsControllerMemberTarget(disk.Target) && fallback != "" { + scoped := diskinventory.PreferredID( "", "", - strings.TrimSpace(fallback), + fallback, disk.DevPath, disk.Controller, disk.Target, ) + if member := strings.TrimPrefix(scoped, fallback); member != "" && strings.HasSuffix(fallback, member) { + return fallback + } + return scoped } return PreferredPhysicalDiskMetricID(disk.Serial, disk.WWN, fallback) }