Read controller-member disk history at its written key

A physical disk behind a RAID controller (target such as megaraid,3)
with no usable serial or WWN had its SMART history written under one
key and read under another, so its temperature and SMART charts stayed
empty. The host agent writes under HostSMARTDiskMetricID and Proxmox
under PhysicalDiskMetricID, and both keys already scope the member
(host-pve:sdd@ctrl0/megaraid,3). BuildMetricsTarget passed that source
ID to PhysicalDiskMetaMetricID, which appended the topology again
(host-pve:sdd@ctrl0/megaraid,3:sdd@ctrl0/megaraid,3).

The reader now does what the Proxmox writer does: a fallback that
already names the member is returned unchanged, and only a fallback
without the topology gets it appended. That covers a Proxmox source ID
from before members were scoped and the canonical resource ID a view
falls back to. TrueNAS disks carry no controller target and are
unaffected.

SMART history needs no migration: no SMART writer used the doubled
key, so samples already stored become readable. A linked node's agent
with no SMART reading for a member, such as a standby row, files the
device's I/O under the Proxmox disk's metrics target, which was the
doubled key. That I/O now lands beside the member's SMART history, and
samples already stored under the doubled key are left to age out.

This carries forward the fix from PR 2661, whose branch conflicts with
the disk ID stack it now sits on, and proves it against the real SMART
and I/O writers in the live and rehydrated registries.

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

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

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

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

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)
 }