diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index faec9ec737..1e47117003 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8806,6 +8806,25 @@ in `internal/monitoring/monitor_host_agents_test.go` `internal/monitoring/issue1595_collection_trust_test.go` (`TestAgentDiskHistoryFollowsTheSerialItsUnraidRowReports`). +### Unraid temperatures chart agent disk history + +`internal/monitoring/monitor_agents.go` changed only so the SMART history +writer charts the temperature a host agent's disk shows. A SMART row without +its own reading shows the one the host's Unraid inventory reports for the disk, +when that inventory row describes the disk (`HostSMARTDiskTemperature`), and a +disk only the Unraid inventory reports, +such as a member the agent's SMART collection skips under `--disk-exclude`, +shows that row's reading (`HostUnraidDiskTemperature`). Both are now written +as `smart_temp` under the key the disk's metrics target reads +(`HostSMARTDiskMetricID`, `HostUnraidDiskMetricID`); a reading the disk does +not show as collected, a spun-down disk's or an expired host's, is not. Agent +registration, enrolment, install, update, removal, report identity and disk +source IDs are unchanged. Focused proof lives in +`internal/monitoring/monitor_host_agents_test.go` +(`TestApplyHostReportChartsUnraidTemperatureOfDiskWithoutSMART`) and +`internal/monitoring/issue1595_collection_trust_test.go` +(`TestAgentDiskChartsTheUnraidTemperatureItShows`). + ### 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 796fc46ca0..1285052caa 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -4800,8 +4800,10 @@ timeline instead of creating a second drawer-only or mock-only disk history path. The SMART-resolved id is the serial the disk resource carries: when a SMART row reports none, `HostSMARTDiskMetricID` takes the one the host's Unraid inventory -reports for that disk, as the unified-resources adapter does, unless several -SMART rows share the disk's device path, and `hostDiskIOMetricResourceID` keys +reports for that disk, as the unified-resources adapter does, unless the Unraid +row does not describe the disk (`unraidDiskDescribesSMARTRow`: a controller +member, or several SMART rows on the disk's device path), and +`hostDiskIOMetricResourceID` keys a device with no non-standby SMART row by its Unraid serial (`HostUnraidDeviceMetricID`, which refuses a device several Unraid rows name) before the linked Proxmox node's disks and the `:` fallback. @@ -4812,6 +4814,21 @@ their counter under no member. Proof: `TestAgentDiskHistoryFollowsTheSerialItsUnraidRowReports` and `TestHostDiskIOMetricResourceIDFallbacks`. +The agent's `smart_temp` series holds the reading the disk resource shows, +not only the SMART row's own. `writeHostSMARTMetrics` writes +`HostSMARTDiskTemperature`, which falls back to the host's Unraid inventory +reading when the row has none, and `writeHostUnraidDiskTemperatures` writes +`HostUnraidDiskTemperature` under `HostUnraidDiskMetricID` for each Unraid row +whose key is not one of the host's SMART rows' keys, such as a `--disk-exclude` +member, so an Unraid reading never overwrites a SMART row's own at the same +timestamp. +Unraid rows sharing a key are one disk in the registry, which shows the latest +of their readings, so the last row with a reading decides that key's single +sample. `diskinventory.TemperatureCollected` keeps a spun-down disk's or an +expired host's leftover reading out of history. +Proof: +`TestAgentDiskChartsTheUnraidTemperatureItShows` and +`TestApplyHostReportChartsUnraidTemperatureOfDiskWithoutSMART`. That same monitoring-owned disk-health boundary also includes shared storage risk assessment in `internal/storagehealth/`. When providers or host agents emit structured storage topology such as Unraid per-disk state, the shared diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 84269fbcce..826a91d7c7 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -5432,6 +5432,26 @@ A row's own serial is kept even when it is a placeholder, as the adapter keeps it, and the Unraid row then stays a disk of its own. Proof: `TestHostSMARTDiskMetricIDTakesTheSerialItsUnraidRowReports`, `TestAgentDiskHistoryFollowsTheSerialItsUnraidRowReports`. +The adapters' disk temperature rule is shared with the agent disk history +writer. `HostSMARTDiskTemperature` returns the temperature and collection state +a SMART row's disk shows: the row's own reading, or, when it has none, its +Unraid row's under `unraidDiskTemperatureStatus`. `matchUnraidDisk` finds that +Unraid row by the SMART row's usable serial anywhere in the inventory before +any device path, and a placeholder serial matches nothing. The fallback, like +the serial a row without one takes (`hostSMARTDiskSerial`), comes only from an +Unraid row that describes the row's disk (`unraidDiskDescribesSMARTRow`): one +carrying the row's usable serial, or one naming the row's device path when the +row is that whole kernel block device. A controller member, even the only one +reported, and a row sharing its path with other SMART rows are not, because +the Unraid row describes the block device as a whole. `HostUnraidDiskTemperature` +returns what a disk built from an Unraid row alone shows, and +`HostUnraidDiskMetricID` returns the key that disk's metrics target reads: its +usable serial, else its `HostUnraidDiskSourceID`, and nothing for a row without +a device, which is not ingested. The writer used to take only the SMART row's +own reading, so a temperature shown from the Unraid inventory, for a disk whose +SMART probe returned none or one excluded from SMART collection, was never +charted. Proof: `TestAgentDiskChartsTheUnraidTemperatureItShows` and +`TestHostUnraidDiskMetricIDMatchesItsDiskMetricsTarget`. 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 7486e493af..0da88b9821 100644 --- a/internal/monitoring/issue1595_collection_trust_test.go +++ b/internal/monitoring/issue1595_collection_trust_test.go @@ -681,6 +681,167 @@ func TestAgentDiskHistoryFollowsTheSerialItsUnraidRowReports(t *testing.T) { } } +// An Unraid host's disk can show a temperature only its Unraid inventory +// reports: a member the agent's SMART collection skips (--disk-exclude), or +// one Unraid reads with its own per-disk SMART settings while the agent's +// probe returns no temperature. The chart must hold the reading the disk +// shows, under the key its metrics target reads, and nothing for a reading the +// disk does not show as collected (a spun-down disk, a host past its lease) or +// does not show at all (a controller member behind the device Unraid reads). +func TestAgentDiskChartsTheUnraidTemperatureItShows(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, rateTracker: NewRateTracker()} + + now := time.Now().UTC().Truncate(time.Second) + host := models.Host{ID: "host-tower", Hostname: "tower", MachineID: "machine-tower", Status: "online", LastSeen: now} + host.Sensors.SMART = []models.HostDiskSMART{ + // The agent's identity-only row for a disk its probe got nothing from. + {Device: "sdb", Serial: "SER-B", Health: "UNKNOWN", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "SMART probe returned no usable temperature data")}}, + // A row with its own reading keeps it; the Unraid one is not written too. + {Device: "sdc", WWN: "0x5000c500aaaa0003", Health: "PASSED", Temperature: 41, Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Available("smartctl")}}, + // The agent's native standby row for a disk Unraid reports spun down. + {Device: "sdd", Serial: "SER-D", Health: "UNKNOWN", Standby: true, Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("unraid", "disk is reported spun down")}}, + // Controller members behind one kernel block device, which the Unraid + // row describes as a whole: neither member takes its temperature. + {Device: "sdh", Controller: "sdh", Target: "megaraid,0", WWN: "0x5000c500bbbb0001", Health: "PASSED", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "SMART probe returned no usable temperature data")}}, + {Device: "sdh", Controller: "sdh", Target: "megaraid,1", WWN: "0x5000c500bbbb0002", Health: "PASSED", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "SMART probe returned no usable temperature data")}}, + // A member whose own serial is the Unraid row's is the disk that row + // describes, though its path is shared. + {Device: "sdi", Controller: "sdi", Target: "megaraid,0", Serial: "SER-I", Health: "UNKNOWN", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "SMART probe returned no usable temperature data")}}, + {Device: "sdi", Controller: "sdi", Target: "megaraid,1", Serial: "SER-I2", Health: "UNKNOWN", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "SMART probe returned no usable temperature data")}}, + // A placeholder serial is no evidence of which member the row is. + {Device: "sdn", Controller: "sdn", Target: "megaraid,0", Serial: "UNKNOWN", WWN: "0x5000c500cccc0001", Health: "UNKNOWN", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "SMART probe returned no usable temperature data")}}, + {Device: "sdn", Controller: "sdn", Target: "megaraid,1", Serial: "UNKNOWN", WWN: "0x5000c500cccc0002", Health: "UNKNOWN", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "SMART probe returned no usable temperature data")}}, + // A placeholder serial matches no Unraid row; the row's path finds its own. + {Device: "sdo", Serial: "0123456789", WWN: "0x5000c500eeee0001", Health: "UNKNOWN", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "SMART probe returned no usable temperature data")}}, + // The only reported member behind a controller path is still a member. + {Device: "sdq", Controller: "sdq", Target: "megaraid,0", WWN: "0x5000c500ffff0001", Health: "UNKNOWN", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "SMART probe returned no usable temperature data")}}, + // Without a serial or WWN, a member charts its own reading under its + // topology key. + {Device: "sdr", Controller: "sdr", Target: "megaraid,0", Health: "PASSED", Temperature: 35, Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Available("smartctl")}}, + // A SMART row's key is its own: the merged disk shows the row's reading, + // here not collected, and the Unraid one is not charted over it. + {Device: "sdm", WWN: "0x5000c500dddd0001", Health: "PASSED", Temperature: 40, Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", "disk is in standby")}}, + } + host.Unraid = &models.HostUnraidStorage{ArrayStarted: true, Disks: []models.HostUnraidDisk{ + {Device: "sdb", Serial: "SER-B", Name: "disk1", Role: "data", Status: "online", Temperature: 38}, + {Device: "sdc", Serial: "SER-C", Name: "disk2", Role: "data", Status: "online", Temperature: 39}, + {Device: "sdd", Serial: "SER-D", Name: "disk3", Role: "data", Status: "online", Temperature: 35, SpunDown: true}, + // Members with no SMART row at all. + {Device: "sde", Serial: "SER-E", Name: "disk4", Role: "data", Status: "online", Temperature: 36}, + {Device: "sdf", Serial: "SER-F", Name: "disk5", Role: "data", Status: "online", Temperature: 33, SpunDown: true}, + {Device: "sdg", Name: "disk6", Role: "data", Status: "online", Temperature: 37}, + {Device: "sdh", Serial: "SER-H", Name: "disk7", Role: "data", Status: "online", Temperature: 42}, + {Device: "sdi", Serial: "SER-I", Name: "disk8", Role: "data", Status: "online", Temperature: 44}, + // Rows sharing one history key are one disk, showing the latest reading. + {Device: "sdj", Name: "disk9", Role: "data", Status: "online"}, + {Device: "sdj", Name: "disk10", Role: "data", Status: "online", Temperature: 43}, + {Device: "sdk", Name: "disk11", Role: "data", Status: "online", Temperature: 38}, + {Device: "sdk", Name: "disk12", Role: "data", Status: "online", Temperature: 39}, + {Device: "sdl", Name: "disk13", Role: "data", Status: "online", Temperature: 40}, + {Device: "sdl", Name: "disk14", Role: "data", Status: "online", Temperature: 32, SpunDown: true}, + {Device: "sdn", Serial: "UNKNOWN", Name: "disk15", Role: "data", Status: "online", Temperature: 45}, + {Device: "sdm", Serial: "SER-M", Name: "disk16", Role: "data", Status: "online", Temperature: 37}, + {Device: "sdp", Serial: "0123456789", Name: "disk17", Role: "data", Status: "online", Temperature: 30}, + {Device: "sdo", Serial: "0123456789", Name: "disk18", Role: "data", Status: "online", Temperature: 46}, + {Device: "sdq", Serial: "SER-Q", Name: "disk19", Role: "data", Status: "online", Temperature: 47}, + {Device: "sdr", Serial: "SER-R", Name: "disk20", Role: "data", Status: "online", Temperature: 36}, + }} + // Disks by usable serial, else WWN, else device path, and the reading each + // shows as collected. + want := map[string]float64{ + "SER-B": 38, "SER-C": 41, "SER-D": 0, "SER-E": 36, "SER-F": 0, "sdg": 37, + "SER-H": 42, "0x5000c500bbbb0001": 0, "0x5000c500bbbb0002": 0, + "SER-I": 44, "SER-I2": 0, "sdj": 43, "sdk": 39, "sdl": 0, + "sdn": 45, "0x5000c500cccc0001": 0, "0x5000c500cccc0002": 0, "SER-M": 0, + "0x5000c500eeee0001": 46, "sdo": 46, "sdp": 30, "0x5000c500ffff0001": 0, "SER-Q": 47, + "sdr": 35, "SER-R": 36, + } + + m.writeHostSMARTMetrics(host, now) + // Once the host's lease expires its last report is kept, offline, as + // context; nothing it carries is a current reading. + reported := host + reported.Sensors.SMART = append([]models.HostDiskSMART(nil), host.Sensors.SMART...) + state := &models.State{Hosts: []models.Host{reported}} + offline, expired := state.ExpireHostTelemetry(host.ID, host.LastSeen) + if !expired { + t.Fatal("host telemetry did not expire") + } + m.writeHostSMARTMetrics(offline, now.Add(30*time.Second)) + store.Flush() + + live := unifiedresources.NewRegistry(nil) + live.IngestSnapshot(models.StateSnapshot{Hosts: []models.Host{host}}) + 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(want) { + t.Fatalf("%s: disks = %d, want %d", tc.name, len(disks), len(want)) + } + for _, disk := range disks { + device := disk.PhysicalDisk.Serial + if !diskinventory.IsUsableHardwareID(device) { + device = "" + } + if device == "" { + device = disk.PhysicalDisk.WWN + } + if device == "" { + device = disk.PhysicalDisk.DevPath + } + wantTemp, ok := want[device] + if !ok { + t.Fatalf("%s: unexpected disk %s at %q", tc.name, disk.ID, device) + } + shown := 0.0 + if diskinventory.TemperatureCollected(disk.PhysicalDisk.Temperature, disk.PhysicalDisk.Collection) { + shown = float64(disk.PhysicalDisk.Temperature) + } + if shown != wantTemp { + t.Fatalf("%s: disk %s shows %v°C as collected, want %v°C", tc.name, device, shown, wantTemp) + } + target := tc.registry.MetricsTarget(disk.ID) + if target == nil { + t.Fatalf("%s: disk %s has no metrics target", tc.name, device) + } + 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) + } + switch { + case wantTemp == 0 && len(points) != 0: + t.Errorf("%s: disk %s shows no collected temperature but target %+v charts %+v", tc.name, device, *target, points) + case wantTemp > 0 && (len(points) != 1 || points[0].Value != wantTemp || !points[0].Timestamp.Equal(now)): + t.Errorf("%s: disk %s shows %v°C but target %+v charts %+v, want one sample at the online report", tc.name, device, wantTemp, *target, points) + } + } + } +} + // 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/monitoring/monitor_agents.go b/internal/monitoring/monitor_agents.go index 85599d39e7..598c6b3bd5 100644 --- a/internal/monitoring/monitor_agents.go +++ b/internal/monitoring/monitor_agents.go @@ -4081,14 +4081,18 @@ func (m *Monitor) writeHostSMARTMetrics(host models.Host, now time.Time) { return } + smartMetricIDs := make(map[string]struct{}, len(host.Sensors.SMART)) for _, disk := range host.Sensors.SMART { resourceID := unifiedresources.HostSMARTDiskMetricID(host, disk) if resourceID == "" { continue } + smartMetricIDs[resourceID] = struct{}{} - if diskinventory.TemperatureCollected(disk.Temperature, disk.Collection) { - m.metricsStore.Write("disk", resourceID, "smart_temp", float64(disk.Temperature), now) + // Chart the reading the disk shows: a row without its own carries the + // host's Unraid reading for the disk. + if temperature, collection := unifiedresources.HostSMARTDiskTemperature(host, disk); diskinventory.TemperatureCollected(temperature, collection) { + m.metricsStore.Write("disk", resourceID, "smart_temp", float64(temperature), now) } attrs := disk.Attributes @@ -4127,6 +4131,50 @@ func (m *Monitor) writeHostSMARTMetrics(host models.Host, now time.Time) { m.metricsStore.Write("disk", resourceID, "smart_unsafe_shutdowns", float64(*attrs.UnsafeShutdowns), now) } } + + m.writeHostUnraidDiskTemperatures(host, smartMetricIDs, now) +} + +// writeHostUnraidDiskTemperatures charts the temperature of each disk only the +// host's Unraid inventory reports, such as a member the agent's SMART +// collection skips under --disk-exclude. smartMetricIDs holds the history keys +// of the host's SMART rows. A disk under one of those keys is that row's to +// chart, and the row already carries the Unraid reading when it has none of +// its own. Unraid rows sharing a key are one disk in the registry, which keeps +// the latest of their readings, so the last row with a reading decides that +// key's sample. +func (m *Monitor) writeHostUnraidDiskTemperatures(host models.Host, smartMetricIDs map[string]struct{}, now time.Time) { + if host.Unraid == nil { + return + } + type reading struct { + temperature int + collection *diskinventory.CollectionStatus + } + var keys []string + readings := make(map[string]reading, len(host.Unraid.Disks)) + for _, disk := range host.Unraid.Disks { + resourceID := unifiedresources.HostUnraidDiskMetricID(host, disk) + if resourceID == "" { + continue + } + if _, ok := smartMetricIDs[resourceID]; ok { + continue + } + temperature, collection := unifiedresources.HostUnraidDiskTemperature(host, disk) + if temperature <= 0 { + continue + } + if _, ok := readings[resourceID]; !ok { + keys = append(keys, resourceID) + } + readings[resourceID] = reading{temperature: temperature, collection: collection} + } + for _, resourceID := range keys { + if r := readings[resourceID]; diskinventory.TemperatureCollected(r.temperature, r.collection) { + m.metricsStore.Write("disk", resourceID, "smart_temp", float64(r.temperature), now) + } + } } func normalizeHostDiskDevice(device string) string { diff --git a/internal/monitoring/monitor_host_agents_test.go b/internal/monitoring/monitor_host_agents_test.go index baef427792..600be586b6 100644 --- a/internal/monitoring/monitor_host_agents_test.go +++ b/internal/monitoring/monitor_host_agents_test.go @@ -3017,6 +3017,71 @@ func TestApplyHostReportPersistsSMARTMetricsForAgentDisks(t *testing.T) { } } +// An Unraid member the agent sends no SMART row for (a --disk-exclude match) +// still shows the temperature its Unraid inventory row reports, so the report +// charts it under the disk's serial. A disk with its own SMART reading keeps +// it, and a spun-down member's leftover reading is not charted. +func TestApplyHostReportChartsUnraidTemperatureOfDiskWithoutSMART(t *testing.T) { + storeCfg := metrics.DefaultConfig(t.TempDir()) + storeCfg.WriteBufferSize = 1 + store, err := metrics.NewStore(storeCfg) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + defer store.Close() + + monitor := &Monitor{ + state: models.NewState(), + alertManager: alerts.NewManager(), + hostTokenBindings: make(map[string]string), + config: &config.Config{}, + rateTracker: NewRateTracker(), + metricsStore: store, + } + t.Cleanup(func() { monitor.alertManager.Stop() }) + + report := agentshost.Report{ + Agent: agentshost.AgentInfo{ID: "agent-tower", Version: "1.0.0", IntervalSeconds: 30}, + Host: agentshost.HostInfo{ID: "machine-tower", Hostname: "tower", MachineID: "machine-tower"}, + Metrics: agentshost.Metrics{ + Memory: agentshost.MemoryMetric{TotalBytes: 1024, UsedBytes: 512, FreeBytes: 512, Usage: 50}, + }, + Sensors: agentshost.Sensors{ + SMART: []agentshost.DiskSMART{ + {Device: "/dev/sda", Model: "IronWolf", Serial: "SERIAL-TOWER-1", Temperature: 41}, + }, + }, + Unraid: &agentshost.UnraidStorage{ArrayStarted: true, Disks: []agentshost.UnraidDisk{ + {Name: "disk1", Device: "sda", Role: "data", Status: "online", Serial: "SERIAL-TOWER-1", Temperature: 40}, + {Name: "disk2", Device: "sdb", Role: "data", Status: "online", Serial: "SERIAL-TOWER-2", Temperature: 36}, + {Name: "disk3", Device: "sdc", Role: "data", Status: "online", Serial: "SERIAL-TOWER-3", Temperature: 34, SpunDown: true}, + }}, + Timestamp: time.Now().UTC(), + } + + if _, err := monitor.ApplyHostReport(report, nil); err != nil { + t.Fatalf("ApplyHostReport: %v", err) + } + store.Flush() + + points := waitForStoredDiskMetric(t, store, "SERIAL-TOWER-2", "smart_temp") + if len(points) != 1 || points[0].Value != 36 { + t.Fatalf("SERIAL-TOWER-2 smart_temp = %+v, want the Unraid reading 36", points) + } + points = waitForStoredDiskMetric(t, store, "SERIAL-TOWER-1", "smart_temp") + if len(points) != 1 || points[0].Value != 41 { + t.Fatalf("SERIAL-TOWER-1 smart_temp = %+v, want its own SMART reading 41", points) + } + now := time.Now().UTC() + points, err = store.Query("disk", "SERIAL-TOWER-3", "smart_temp", now.Add(-time.Hour), now.Add(time.Hour), 60) + if err != nil { + t.Fatalf("Query smart_temp: %v", err) + } + if len(points) != 0 { + t.Fatalf("spun-down SERIAL-TOWER-3 smart_temp = %+v, want none", points) + } +} + func TestApplyHostReportPersistsAgentTemperatureMetric(t *testing.T) { t.Helper() diff --git a/internal/unifiedresources/adapters.go b/internal/unifiedresources/adapters.go index 167af4c6b1..e5220c3800 100644 --- a/internal/unifiedresources/adapters.go +++ b/internal/unifiedresources/adapters.go @@ -806,10 +806,7 @@ func resourceFromHostUnraidPhysicalDisk(host models.Host, disk models.HostUnraid health := unraidPhysicalDiskHealth(disk) assessment := assessUnraidPhysicalDisk(disk) sizeBytes := unraidDiskSizeBytes(host, disk) - var collection *diskinventory.CollectionStatus - if status := unraidDiskTemperatureStatus(host, disk); status.State != "" { - collection = &diskinventory.CollectionStatus{Temperature: status} - } + temperature, collection := HostUnraidDiskTemperature(host, disk) resource := Resource{ Type: ResourceTypePhysicalDisk, Name: name, @@ -824,7 +821,7 @@ func resourceFromHostUnraidPhysicalDisk(host models.Host, disk models.HostUnraid SizeBytes: sizeBytes, Health: health, Wearout: -1, - Temperature: disk.Temperature, + Temperature: temperature, Used: unraidDiskMountPath(disk), StorageRole: unraidDiskRole(&disk), StorageGroup: unraidDiskGroup(&disk), @@ -869,6 +866,46 @@ func unraidDiskTemperatureStatus(host models.Host, disk models.HostUnraidDisk) d } } +// HostUnraidDiskTemperature returns the temperature the disk resource built +// from a host's Unraid inventory row shows, with its collection state. The +// agent disk history writer charts this reading. +func HostUnraidDiskTemperature(host models.Host, disk models.HostUnraidDisk) (int, *diskinventory.CollectionStatus) { + status := unraidDiskTemperatureStatus(host, disk) + if status.State == "" { + return disk.Temperature, nil + } + return disk.Temperature, &diskinventory.CollectionStatus{Temperature: status} +} + +// HostSMARTDiskTemperature returns the temperature the disk resource built +// from a host agent's SMART row shows, with its collection state. The agent +// disk history writer charts this reading. +func HostSMARTDiskTemperature(host models.Host, disk models.HostDiskSMART) (int, *diskinventory.CollectionStatus) { + return hostSMARTDiskTemperature(host, disk, matchUnraidDisk(host.Unraid, disk)) +} + +// hostSMARTDiskTemperature returns the temperature the disk resource built +// from a host agent's SMART row shows, with the row's collection state: the +// row's own reading, or, when the row has none, the one the host's Unraid +// inventory reports for the disk. Unraid reads its members' temperatures with +// its own per-disk SMART settings, so it can have one when the agent's probe +// returned none. The reading is taken only from an Unraid row that describes +// the row's disk (unraidDiskDescribesSMARTRow). +func hostSMARTDiskTemperature(host models.Host, disk models.HostDiskSMART, unraidDisk *models.HostUnraidDisk) (int, *diskinventory.CollectionStatus) { + collection := diskinventory.CloneStatus(disk.Collection) + if disk.Temperature > 0 || !unraidDiskDescribesSMARTRow(host, disk, unraidDisk) { + return disk.Temperature, collection + } + // The reading is now the Unraid inventory's, and so is its state. + if status := unraidDiskTemperatureStatus(host, *unraidDisk); status.State != "" { + if collection == nil { + collection = &diskinventory.CollectionStatus{} + } + collection.Temperature = status + } + return unraidDisk.Temperature, collection +} + func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Resource, ResourceIdentity) { name := strings.TrimSpace(disk.Model) if name == "" { @@ -907,8 +944,7 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res model := strings.TrimSpace(disk.Model) serial := hostSMARTDiskSerial(host, disk, unraidDisk) diskType := strings.TrimSpace(disk.Type) - temperature := disk.Temperature - collection := diskinventory.CloneStatus(disk.Collection) + temperature, collection := hostSMARTDiskTemperature(host, disk, unraidDisk) health := strings.TrimSpace(disk.Health) if unraidDisk != nil { if model == "" { @@ -920,16 +956,6 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res if sizeBytes <= 0 { sizeBytes = unraidDiskSizeBytes(host, *unraidDisk) } - if temperature <= 0 { - temperature = unraidDisk.Temperature - // The reading is now the Unraid inventory's, and so is its state. - if status := unraidDiskTemperatureStatus(host, *unraidDisk); status.State != "" { - if collection == nil { - collection = &diskinventory.CollectionStatus{} - } - collection.Temperature = status - } - } if health == "" || strings.EqualFold(health, "UNKNOWN") { health = unraidPhysicalDiskHealth(*unraidDisk) } @@ -1219,21 +1245,30 @@ func maxInt64() int64 { return int64(^uint64(0) >> 1) } +// matchUnraidDisk returns the host's Unraid row for a SMART row: the one +// carrying the row's usable serial, else the one at its device path. A serial +// names the drive and a path only where the kernel put it, so a serial match +// anywhere in the inventory outranks a path match. A placeholder serial, which +// several bridges report alike, is no match at all. func matchUnraidDisk(unraid *models.HostUnraidStorage, disk models.HostDiskSMART) *models.HostUnraidDisk { if unraid == nil || len(unraid.Disks) == 0 { return nil } - normalizedDevice := strings.ToLower(normalizePhysicalDiskDeviceToken(disk.Device)) - normalizedSerial := strings.TrimSpace(strings.ToLower(disk.Serial)) - for i := range unraid.Disks { - candidate := &unraid.Disks[i] - if normalizedSerial != "" && strings.EqualFold(strings.TrimSpace(candidate.Serial), normalizedSerial) { - return candidate + if serial := strings.TrimSpace(disk.Serial); diskinventory.IsUsableHardwareID(serial) { + for i := range unraid.Disks { + if strings.EqualFold(strings.TrimSpace(unraid.Disks[i].Serial), serial) { + return &unraid.Disks[i] + } } - candidateDevice := strings.ToLower(normalizePhysicalDiskDeviceToken(candidate.Device)) - if normalizedDevice != "" && candidateDevice != "" && candidateDevice == normalizedDevice { - return candidate + } + normalizedDevice := strings.ToLower(normalizePhysicalDiskDeviceToken(disk.Device)) + if normalizedDevice == "" { + return nil + } + for i := range unraid.Disks { + if strings.ToLower(normalizePhysicalDiskDeviceToken(unraid.Disks[i].Device)) == normalizedDevice { + return &unraid.Disks[i] } } return nil diff --git a/internal/unifiedresources/canonical_ids_types_test.go b/internal/unifiedresources/canonical_ids_types_test.go index c8ed1bedb5..edd8be0863 100644 --- a/internal/unifiedresources/canonical_ids_types_test.go +++ b/internal/unifiedresources/canonical_ids_types_test.go @@ -503,6 +503,50 @@ func TestHostUnraidDiskSourceIDNormalizesDeviceAndPrefersSerial(t *testing.T) { } } +// An Unraid inventory row's history key is the one the metrics target of the +// disk resource built from it reads: its usable serial, unscoped like every +// physical-disk history key, or else its host/device source ID. A row without +// a device is not ingested as a disk and has no key. +func TestHostUnraidDiskMetricIDMatchesItsDiskMetricsTarget(t *testing.T) { + host := models.Host{ID: "host-tower", Hostname: "tower"} + tests := []struct { + name string + disk models.HostUnraidDisk + want string + }{ + {"serial", models.HostUnraidDisk{Device: "/dev/sdb", Serial: "SERIAL-DATA"}, "SERIAL-DATA"}, + {"placeholder serial", models.HostUnraidDisk{Device: "sdc", Serial: "N/A"}, "host-tower:sdc"}, + {"no serial", models.HostUnraidDisk{Device: "sdd [sat]"}, "host-tower:sdd"}, + {"no device", models.HostUnraidDisk{Name: "disk9", Serial: "SERIAL-MISSING"}, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := HostUnraidDiskMetricID(host, tt.disk) + if got != tt.want { + t.Fatalf("HostUnraidDiskMetricID(%+v) = %q, want %q", tt.disk, got, tt.want) + } + if tt.want == "" { + return + } + disk := tt.disk + disk.Name, disk.Role, disk.Status = "disk1", "data", "online" + registry := NewRegistry(nil) + registry.IngestSnapshot(models.StateSnapshot{Hosts: []models.Host{{ + ID: host.ID, Hostname: host.Hostname, Status: "online", + Unraid: &models.HostUnraidStorage{ArrayStarted: true, Disks: []models.HostUnraidDisk{disk}}, + }}}) + disks := registry.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 1 { + t.Fatalf("disks = %d, want 1", len(disks)) + } + target := registry.MetricsTarget(disks[0].ID) + if target == nil || target.ResourceType != "disk" || target.ResourceID != got { + t.Fatalf("metrics target = %+v, want disk %q", target, got) + } + }) + } +} + // 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. diff --git a/internal/unifiedresources/physical_disk_ids.go b/internal/unifiedresources/physical_disk_ids.go index 4ba7cefa98..9d210e9e8c 100644 --- a/internal/unifiedresources/physical_disk_ids.go +++ b/internal/unifiedresources/physical_disk_ids.go @@ -61,17 +61,37 @@ func hostSMARTDiskKey(host models.Host, disk models.HostDiskSMART, serial, wwn s // row's own, or, when the row reports none, the one the host's Unraid // inventory reports for the disk. smartctl reports no serial for a disk in // standby, and the Unraid row still names it. A row without a serial matches -// its Unraid row by device path alone, so it takes no serial from a path -// several of the host's rows share: controller members behind one kernel block -// device, which the Unraid row describes as a whole. +// its Unraid row by device path alone, so it takes the serial only where that +// row describes its disk (unraidDiskDescribesSMARTRow). func hostSMARTDiskSerial(host models.Host, disk models.HostDiskSMART, unraidDisk *models.HostUnraidDisk) string { serial := strings.TrimSpace(disk.Serial) - if serial != "" || unraidDisk == nil || hostSMARTDevicePathShared(host, disk) { + if serial != "" || !unraidDiskDescribesSMARTRow(host, disk, unraidDisk) { return serial } return strings.TrimSpace(unraidDisk.Serial) } +// unraidDiskDescribesSMARTRow reports whether the Unraid row matched to a SMART +// row (matchUnraidDisk) describes that row's disk. An Unraid row describes a +// kernel block device as a whole, so it describes the row when it carries the +// row's usable serial, or when it names the row's device path and the row is +// that whole device: not a controller member behind it, even the only one +// reported, nor one of several rows sharing the path. +func unraidDiskDescribesSMARTRow(host models.Host, disk models.HostDiskSMART, unraidDisk *models.HostUnraidDisk) bool { + if unraidDisk == nil { + return false + } + if serial := strings.TrimSpace(disk.Serial); diskinventory.IsUsableHardwareID(serial) && + strings.EqualFold(serial, strings.TrimSpace(unraidDisk.Serial)) { + return true + } + device := strings.ToLower(normalizePhysicalDiskDeviceToken(disk.Device)) + if device == "" || device != strings.ToLower(normalizePhysicalDiskDeviceToken(unraidDisk.Device)) { + return false + } + return !diskinventory.IsControllerMemberTarget(disk.Target) && !hostSMARTDevicePathShared(host, disk) +} + // hostSMARTDevicePathShared reports whether more than one of the host's SMART // rows, the row itself among them, names the row's device path. func hostSMARTDevicePathShared(host models.Host, disk models.HostDiskSMART) bool { @@ -130,6 +150,17 @@ func HostUnraidDiskSourceID(host models.Host, disk models.HostUnraidDisk) string return "" } +// HostUnraidDiskMetricID returns the history key of the disk resource built +// from a host's Unraid inventory row: the key its metrics target reads, the +// row's usable serial or else its source ID. It returns "" for a row the +// registry does not ingest as a disk, one without a device. +func HostUnraidDiskMetricID(host models.Host, disk models.HostUnraidDisk) string { + if strings.TrimSpace(disk.Device) == "" { + return "" + } + return PreferredPhysicalDiskMetricID(disk.Serial, "", HostUnraidDiskSourceID(host, disk)) +} + const hostPhysicalDiskSourceIDMarker = "/physical-disk:" func hostPhysicalDiskSourceID(hostID, hardwareID string) string {