diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 6813b04986..4d1c57bcac 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -734,6 +734,11 @@ agent's SMART temperature and I/O counters as last-known values but marks them report replaces them. Expiry is compare-and-set on the report time the offline sweep judged stale, so a report admitted between that judgement and the expiry is never expired. +Unraid array-inventory temperatures follow the same lease: while the host is +offline the unified-resources adapter reports them `unavailable` with the same +reason (`models.HostAgentStoppedReportingReason`) and leaves them out of the +disk's risk, and the metrics-history API does not return any such retained +disk temperature as a live point. An enabled availability target assigned to a host agent creates an agent-lifecycle lease for that exact target/agent pairing. First assignment diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 8454240e42..80f345f31f 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -6308,6 +6308,17 @@ windows. `/api/metrics-store/history` must accept `resourceType=disk`, keep `MetricsTarget.ResourceID` that unified resources already expose, instead of leaving storage drawers or other callers to fork a disk-local history route or invent an alternate disk identity. +When a disk range has no samples, the `smart_temp` live fallback (single-metric +and all-metrics) is served only for a temperature that is collected +(`diskinventory.TemperatureCollected`). A temperature that normalization kept +under a non-available collection state, such as a disk in standby or a host +agent past its reporting lease, is omitted, so the range stays empty instead of +returning that value at `now` with `source: "live"`. Mock mode derives a +synthetic disk temperature series from the disk's reading, or pads a sparse +stored one out to `now`, only from a temperature collected now; without one the +stored samples are returned as they are. An empty mock range can still fall +through to the monitor's generic demo chart series for that disk ID, which is +not derived from the disk's reading. That same metrics-history contract also owns Kubernetes pod identity normalization. `/api/metrics-store/history` must accept legacy bare pod IDs such as `cluster-1:pod:pod-1`, canonicalize them onto the unified pod metrics diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 6522d39af6..94971c3220 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -3778,6 +3778,12 @@ predates collection state), so a retained reading has to keep its non-available state to stay out of history; a path that relabels a carried reading as available, such as a node-temperature carry stamped `proxmox_node_smart`, is not covered by this rule. +`diskinventory.TemperatureCollected` is the shared form of that test for +consumers outside the history writers: the metrics-history API serves a disk +`smart_temp` live point, and mock mode derives a synthetic disk temperature +series from the disk's reading or pads a stored one, only from a collected +temperature, so a retained standby or silent-agent reading never reappears as a +point at the current time. Unified-resource physical-disk round trips must retain named `StorageGroup` membership rather than degrading it to the generic `Used` filesystem label. diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index a2dc2b0c6e..3bd46e4a70 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -3065,3 +3065,11 @@ paused-delivery banner strings in English, German and Spanish. This is alert presentation copy only: no API token name, scope preset, privacy disclosure, telemetry statement or security term changes, and no request, storage or authorization path moves. + +### Retained disk temperatures are not served as live history + +`/api/metrics-store/history` now omits a disk's `smart_temp` live point, and +the mock-mode synthetic series derived from that reading, when the temperature +was not collected by the current observation. This only withholds a value the +caller was already authorized to read; the route, its authentication and scope +checks, tenant resolution and response shape are unchanged. diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 626b08f763..f3dc92fb2b 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6739,3 +6739,12 @@ blank header is removed on the next save. Export and import pass through the same normalization. No new file, retention window, backup, migration or recovery authority is introduced; `TestWebhookMaskedValuesResolvePerKey` in `internal/api/alerting/notifications_test.go` covers the write path. + +### Retained disk temperatures open no storage or recovery path + +The metrics-history handler in `internal/api/router.go` serves a disk +temperature live point only when `diskinventory.TemperatureCollected` holds, +and the Unraid physical-disk adapter marks an inventory temperature kept past +the host agent's lease as no longer collected. Both change presentation of +values Pulse already holds: no metrics write, retention window, backup, +migration or recovery authority is added or moved. diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 5c1dfdb1bb..f90e5ef236 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -3576,6 +3576,20 @@ state the Proxmox row still carries from the same source past its reporting lease must not stay "collected" through the Proxmox row's copy of the agent's own state, including when its host is down and no disk poll refreshes that copy. +Unraid array-inventory rows carry no per-field provenance, so the adapter +derives the state of a temperature taken from one: `unavailable` from `unraid` +with "host agent stopped reporting" once `State.ExpireHostTelemetry` has marked +the host offline, or with "disk is reported spun down" for a spun-down disk, +and otherwise `available`. The value is kept as last-known context; it is +collected again only when a reporting host sends a positive reading for a disk +that is no longer spun down. A SMART row that falls back to +the inventory reading because it has none of its own takes that state with it. +The Unraid-native disk row records only the withdrawing states, never +`available`: the registry chooses a merged disk's temperature and its state +separately, and an `available` claim from the provenance-less row would +outrank the SMART row's explicit state for the same disk when the SMART +reading is the one shown. A withdrawn inventory temperature also stops +counting as heat in the disk's risk assessment. Cross-source correlation compares normalized serial and WWN values across fields without truncation, allowing a PVE bare-hex array-volume serial to join diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 32bfb19a12..b2cd38ac74 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -65,6 +65,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/pkg/audit" authpkg "github.com/rcourtman/pulse-go-rewrite/pkg/auth" "github.com/rcourtman/pulse-go-rewrite/pkg/cloudauth" + "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" "github.com/rcourtman/pulse-go-rewrite/pkg/edition" pkglicensing "github.com/rcourtman/pulse-go-rewrite/pkg/licensing" licensetestsupport "github.com/rcourtman/pulse-go-rewrite/pkg/licensing/testsupport" @@ -12546,6 +12547,129 @@ func TestContract_MetricsHistoryAgentTemperatureLiveFallback(t *testing.T) { } } +// Normalization may keep a disk temperature it did not collect this time: a +// host agent past its reporting lease, or a disk in standby. The history +// endpoint must not return that retained value as a live point at "now", and +// mock mode must not grow a synthetic series that ends on it. A collected +// temperature still answers an empty range with a live point. +func TestContract_MetricsHistoryDiskLivePointOnlyForCollectedTemperature(t *testing.T) { + state := models.NewState() + lastReport := time.Now().Add(-time.Hour) + state.UpsertHost(models.Host{ + ID: "agent-silent", Hostname: "silent", Status: "online", IntervalSeconds: 30, LastSeen: lastReport, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sdb", Serial: "WD-SILENT-LIVE", Type: "sata", Health: "PASSED", Temperature: 41, + Collection: &diskinventory.CollectionStatus{Temperature: diskinventory.Available("smartctl")}, + }}}, + }) + if _, changed := state.ExpireHostTelemetry("agent-silent", lastReport); !changed { + t.Fatal("the silent agent's lease expiry did not mark its SMART readings") + } + registry := unifiedresources.NewRegistry(nil) + registry.IngestSnapshot(state.GetSnapshot()) + var resources []unifiedresources.Resource + for _, resource := range unifiedresources.NewMonitorAdapter(registry).GetAll() { + if resource.Type == unifiedresources.ResourceTypePhysicalDisk { + resources = append(resources, resource) + } + } + if len(resources) != 1 || resources[0].PhysicalDisk.Temperature != 41 { + t.Fatalf("silent agent disk resources = %+v, want one disk retaining 41", resources) + } + staticDisk := func(serial string, temperature int, status diskinventory.FieldStatus) unifiedresources.Resource { + return unifiedresources.Resource{ + ID: "physical-disk:" + serial, + Type: unifiedresources.ResourceTypePhysicalDisk, + PhysicalDisk: &unifiedresources.PhysicalDiskMeta{ + Serial: serial, + Temperature: temperature, + Collection: &diskinventory.CollectionStatus{Temperature: status}, + }, + MetricsTarget: &unifiedresources.MetricsTarget{ResourceType: "disk", ResourceID: serial}, + } + } + resources = append(resources, + staticDisk("WD-STANDBY-LIVE", 38, diskinventory.Unavailable("smartctl", "disk is in standby")), + staticDisk("WD-AWAKE-LIVE", 36, diskinventory.Available("smartctl")), + staticDisk("WD-ASLEEP-LIVE", 0, diskinventory.Unavailable("smartctl", "disk is in standby")), + ) + + store, err := metrics.NewStore(metrics.DefaultConfig(t.TempDir())) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = store.Close() }) + monitor := &monitoring.Monitor{} + monitor.SetResourceStore(staticPhysicalDiskResourceStore{resources: resources}) + setUnexportedField(t, monitor, "metricsHistory", monitoring.NewMetricsHistory(10, time.Hour)) + setUnexportedField(t, monitor, "metricsStore", store) + router := &Router{monitor: monitor} + + history := func(query string) (metricsHistoryResponse, map[string]json.RawMessage) { + t.Helper() + rec := httptest.NewRecorder() + router.handleMetricsHistory(rec, httptest.NewRequest(http.MethodGet, "/api/metrics-store/history?resourceType=disk&"+query, nil)) + if rec.Code != http.StatusOK { + t.Fatalf("%s: status = %d, body=%s", query, rec.Code, rec.Body.String()) + } + var single metricsHistoryResponse + var all struct { + Metrics map[string]json.RawMessage `json:"metrics"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &single); err != nil { + t.Fatalf("%s: decode: %v", query, err) + } + if err := json.Unmarshal(rec.Body.Bytes(), &all); err != nil { + t.Fatalf("%s: decode metrics: %v", query, err) + } + return single, all.Metrics + } + + for _, serial := range []string{"WD-SILENT-LIVE", "WD-STANDBY-LIVE"} { + resp, _ := history("resourceId=" + serial + "&metric=smart_temp&range=5m") + if resp.Source == "live" || len(resp.Points) != 0 { + t.Fatalf("%s: retained temperature served as current: source=%q points=%+v", serial, resp.Source, resp.Points) + } + if _, all := history("resourceId=" + serial + "&range=5m"); all["smart_temp"] != nil { + t.Fatalf("%s: retained temperature served in the all-metrics response: %s", serial, all["smart_temp"]) + } + } + resp, _ := history("resourceId=WD-AWAKE-LIVE&metric=smart_temp&range=5m") + if resp.Source != "live" || len(resp.Points) != 1 || resp.Points[0].Value != 36 { + t.Fatalf("collected temperature not served as a live point: source=%q points=%+v", resp.Source, resp.Points) + } + + // Mock mode pads a sparse stored series out to now from the disk's current + // temperature. A retained temperature is not current, so the stored + // samples stay as they are. + // Without a current reading (retained, absent, or no such disk) there is + // nothing to pad with, so the last stored sample is not extended either. + setMockModeForTest(t, true) + now := time.Now() + for _, serial := range []string{"WD-STANDBY-LIVE", "WD-ASLEEP-LIVE", "WD-REMOVED-LIVE"} { + store.Write("disk", serial, "smart_temp", 38, now.Add(-50*time.Minute)) + store.Write("disk", serial, "smart_temp", 38, now.Add(-40*time.Minute)) + } + store.Flush() + for _, serial := range []string{"WD-STANDBY-LIVE", "WD-ASLEEP-LIVE", "WD-REMOVED-LIVE"} { + resp, _ = history("resourceId=" + serial + "&metric=smart_temp&range=1h") + if resp.Source != "store" || len(resp.Points) != 2 { + t.Fatalf("%s: mock mode padded stored temperature history to now without a current reading: source=%q points=%d", serial, resp.Source, len(resp.Points)) + } + } + // With nothing stored, mock mode must not seed its synthetic series from a + // retained temperature either. (The monitor's generic demo series for the + // disk ID is not derived from the disk's reading.) + resp, _ = history("resourceId=WD-SILENT-LIVE&metric=smart_temp&range=1h") + if resp.Source == "mock_synthetic" { + t.Fatalf("mock mode synthesized history from a silent agent's retained temperature: points=%+v", resp.Points) + } + resp, _ = history("resourceId=WD-AWAKE-LIVE&metric=smart_temp&range=1h") + if resp.Source != "mock_synthetic" || len(resp.Points) < 2 || resp.Points[len(resp.Points)-1].Value != 36 { + t.Fatalf("mock mode stopped synthesizing a collected temperature: source=%q points=%+v", resp.Source, resp.Points) + } +} + func TestContract_MetricsHistoryCanonicalizesLegacyKubernetesPodIDs(t *testing.T) { mh := monitoring.NewMetricsHistory(1000, time.Hour) now := time.Now().UTC().Truncate(time.Second) diff --git a/internal/api/router.go b/internal/api/router.go index ceec1ce918..6f77a32878 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -6723,7 +6723,9 @@ func (r *Router) handleMetricsHistory(w http.ResponseWriter, req *http.Request) return points } pd := disk.PhysicalDisk - if pd.Temperature > 0 { + // A retained temperature (standby, a silent host agent) is not a + // reading taken now. + if diskinventory.TemperatureCollected(pd.Temperature, pd.Collection) { points["smart_temp"] = monitoring.MetricPoint{Timestamp: now, Value: float64(pd.Temperature)} } if pd.SMART != nil { @@ -6802,7 +6804,8 @@ func (r *Router) handleMetricsHistory(w http.ResponseWriter, req *http.Request) if mock.IsMockEnabled() && runtimeResourceType == "disk" { current := 0.0 - if disk := findDisk(resourceID); disk != nil && disk.PhysicalDisk != nil && metricType == "smart_temp" { + if disk := findDisk(resourceID); disk != nil && disk.PhysicalDisk != nil && metricType == "smart_temp" && + diskinventory.TemperatureCollected(disk.PhysicalDisk.Temperature, disk.PhysicalDisk.Collection) { current = float64(disk.PhysicalDisk.Temperature) } if current > 0 || metricType == "disk" || metricType == "diskread" || metricType == "diskwrite" { @@ -7189,7 +7192,12 @@ func (r *Router) handleMetricsHistory(w http.ResponseWriter, req *http.Request) if len(points) > 0 && len(points) < targetPoints { current := points[len(points)-1].Value if metricType == "smart_temp" { - if disk := findDisk(resourceID); disk != nil && disk.PhysicalDisk != nil && disk.PhysicalDisk.Temperature > 0 { + // Pad a temperature series to now only with a reading + // collected now. Without one (retained, absent, or no + // such disk) the stored samples stay as they are. + current = 0 + if disk := findDisk(resourceID); disk != nil && disk.PhysicalDisk != nil && + diskinventory.TemperatureCollected(disk.PhysicalDisk.Temperature, disk.PhysicalDisk.Collection) { current = float64(disk.PhysicalDisk.Temperature) } } diff --git a/internal/models/models.go b/internal/models/models.go index 30aaa665e6..38ae9c18f3 100644 --- a/internal/models/models.go +++ b/internal/models/models.go @@ -5695,7 +5695,9 @@ func (s *State) ExpireHostTelemetry(hostID string, lastSeen time.Time) (Host, bo return Host{}, false } -const hostAgentStoppedReportingReason = "host agent stopped reporting" +// HostAgentStoppedReportingReason is the collection-state reason recorded on +// readings a host agent supplied once its reporting lease has expired. +const HostAgentStoppedReportingReason = "host agent stopped reporting" // expireHostSMARTReadings marks the temperature and I/O counters of one SMART // row as no longer collected, keeping their values as last-known evidence. @@ -5725,7 +5727,7 @@ func expireCollectedReading(status *diskinventory.FieldStatus, hasValue bool) bo default: return false } - *status = diskinventory.Unavailable(status.Source, hostAgentStoppedReportingReason) + *status = diskinventory.Unavailable(status.Source, HostAgentStoppedReportingReason) return true } diff --git a/internal/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go index 74bca727c3..0c07448d64 100644 --- a/internal/monitoring/physical_disk_roundtrip_test.go +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -633,3 +633,88 @@ func TestStandbyDiskRetainedTemperatureIsNotRecordedAsHistory(t *testing.T) { } } } + +// An Unraid agent reports array disk temperatures in its Unraid inventory, +// which carries no per-field provenance, and a SMART row without a temperature +// falls back to that inventory reading. Once the agent's reporting lease +// expires both rows stay as last-known context, so the canonical disks must say +// the retained temperature is no longer collected. The next report collects +// it again. +func TestSilentUnraidAgentDiskTemperatureIsRetainedButNotCollected(t *testing.T) { + now := time.Now() + state := models.NewState() + report := func(seen time.Time, temperature int) { + state.UpsertHost(models.Host{ + ID: "agent-tower", Hostname: "tower", Status: "online", IntervalSeconds: 30, LastSeen: seen, + Unraid: &models.HostUnraidStorage{ArrayStarted: true, Disks: []models.HostUnraidDisk{ + {Name: "disk1", Device: "sdc", Role: "data", Status: "online", Serial: "UNRAID-ONLY1", Temperature: temperature}, + {Name: "disk2", Device: "sdd", Role: "data", Status: "online", Serial: "UNRAID-SMART2", Temperature: temperature + 2}, + {Name: "disk3", Device: "sde", Role: "data", Status: "online", Serial: "UNRAID-SAS3", Temperature: temperature + 4}, + }}, + // The SMART rows for disk2 and disk3 carry no temperature of their + // own; disk3's smartctl could not read one at all. + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{ + {Device: "/dev/sdd", Serial: "UNRAID-SMART2", Type: "sata", Health: "PASSED"}, + {Device: "/dev/sde", Serial: "UNRAID-SAS3", Type: "sas", Health: "PASSED", Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unsupported("smartctl", "no temperature attribute"), + }}, + }}, + }) + } + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + m := &Monitor{state: state, resourceStore: adapter, startTime: now.Add(-time.Hour)} + temperatures := func() map[string]diskinventory.FieldStatus { + t.Helper() + adapter.PopulateFromSnapshot(state.GetSnapshot()) + got := make(map[string]diskinventory.FieldStatus) + for _, view := range adapter.PhysicalDisks() { + status := diskinventory.FieldStatus{} + if collection := view.Collection(); collection != nil { + status = collection.Temperature + } + if _, seen := got[view.Serial()]; seen { + t.Fatalf("disk %s surfaced twice", view.Serial()) + } + got[view.Serial()] = status + wantTemperature := map[string]int{"UNRAID-ONLY1": 37, "UNRAID-SMART2": 39, "UNRAID-SAS3": 41}[view.Serial()] + if view.Temperature() != wantTemperature { + t.Fatalf("disk %s temperature = %d, want %d", view.Serial(), view.Temperature(), wantTemperature) + } + } + if len(got) != 3 { + t.Fatalf("canonical disks = %v, want the three Unraid disks", got) + } + return got + } + + // A reporting agent's readings are collected. The inventory-only disk + // keeps no state of its own (it counts as collected); the SMART rows that + // borrow the inventory reading take its provenance. + collected := map[string]diskinventory.FieldStatus{ + "UNRAID-ONLY1": {}, + "UNRAID-SMART2": diskinventory.Available("unraid"), + "UNRAID-SAS3": diskinventory.Available("unraid"), + } + report(now, 37) + for serial, status := range temperatures() { + if status != collected[serial] { + t.Fatalf("reporting agent: disk %s temperature status = %+v, want %+v", serial, status, collected[serial]) + } + } + + state.TouchHost("agent-tower", now.Add(-hostAgentHealthWindow(30)-time.Minute)) + m.evaluateHostAgents(now) + for serial, status := range temperatures() { + if status != diskinventory.Unavailable("unraid", "host agent stopped reporting") { + t.Fatalf("silent agent: disk %s retained temperature presented as collected: %+v", serial, status) + } + } + + report(time.Now(), 37) + m.evaluateHostAgents(time.Now()) + for serial, status := range temperatures() { + if status != collected[serial] { + t.Fatalf("resumed agent: disk %s temperature status = %+v, want collected again (%+v)", serial, status, collected[serial]) + } + } +} diff --git a/internal/unifiedresources/adapter_coverage_test.go b/internal/unifiedresources/adapter_coverage_test.go index 12d961af3e..154ca7e4aa 100644 --- a/internal/unifiedresources/adapter_coverage_test.go +++ b/internal/unifiedresources/adapter_coverage_test.go @@ -7,6 +7,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/storagehealth" + "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" ) func strPointer(v string) *string { @@ -1045,3 +1046,119 @@ func TestMetricPercent(t *testing.T) { }) } } + +// Unraid inventory temperatures have no agent provenance, so the adapter +// derives their state from the host's reporting lease and the disk's spin +// state. A native row records only a state that withdraws its reading, a SMART +// row that borrows the inventory reading takes the inventory's state, and a +// SMART row with its own reading keeps the agent's state. A withdrawn reading +// never counts as heat in the disk's risk. +func TestUnraidDiskTemperatureFollowsTheHostLease(t *testing.T) { + smartWithout := models.HostDiskSMART{Device: "/dev/sdc", Serial: "UNRAID-1", Collection: &diskinventory.CollectionStatus{ + Serial: diskinventory.Available("smartctl"), + Temperature: diskinventory.Unsupported("smartctl", "no temperature attribute"), + }} + smartWith := models.HostDiskSMART{Device: "/dev/sdc", Serial: "UNRAID-1", Temperature: 40, Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("smartctl", models.HostAgentStoppedReportingReason), + }} + stopped := diskinventory.Unavailable("unraid", models.HostAgentStoppedReportingReason) + spunDown := diskinventory.Unavailable("unraid", "disk is reported spun down") + hotRisk := func(risk *PhysicalDiskRisk) bool { + if risk == nil { + return false + } + for _, reason := range risk.Reasons { + if reason.Code == "temperature_high" { + return true + } + } + return false + } + for _, tc := range []struct { + name string + status string + spunDown bool + native diskinventory.FieldStatus + borrowed diskinventory.FieldStatus + }{ + {"reporting", "online", false, diskinventory.FieldStatus{}, diskinventory.Available("unraid")}, + {"silent agent", "offline", false, stopped, stopped}, + {"spun down", "online", true, spunDown, spunDown}, + } { + inventory := models.HostUnraidDisk{Name: "disk1", Device: "sdc", Role: "data", Status: "online", Serial: "UNRAID-1", + Temperature: 72, SpunDown: tc.spunDown} + host := models.Host{ID: "agent-tower", Hostname: "tower", Status: tc.status, + Unraid: &models.HostUnraidStorage{Disks: []models.HostUnraidDisk{inventory}}} + collected := tc.borrowed.State == diskinventory.FieldAvailable + + native, _ := resourceFromHostUnraidPhysicalDisk(host, inventory) + got := native.PhysicalDisk + var nativeStatus diskinventory.FieldStatus + if got.Collection != nil { + nativeStatus = got.Collection.Temperature + } + if got.Temperature != 72 || nativeStatus != tc.native { + t.Fatalf("%s: Unraid row temperature=%d state=%+v, want 72 with %+v", tc.name, got.Temperature, nativeStatus, tc.native) + } + if hotRisk(got.Risk) != collected { + t.Fatalf("%s: Unraid row temperature risk = %v, want %v: %+v", tc.name, hotRisk(got.Risk), collected, got.Risk) + } + + borrowed, _ := resourceFromHostSMARTDisk(host, smartWithout) + got = borrowed.PhysicalDisk + if got.Temperature != 72 || got.Collection.Temperature != tc.borrowed || + got.Collection.Serial != diskinventory.Available("smartctl") { + t.Fatalf("%s: SMART row borrowing the inventory reading: temperature=%d collection=%+v", tc.name, got.Temperature, got.Collection) + } + if hotRisk(got.Risk) != collected { + t.Fatalf("%s: borrowed temperature risk = %v, want %v: %+v", tc.name, hotRisk(got.Risk), collected, got.Risk) + } + if smartWithout.Collection.Temperature.State != diskinventory.FieldUnsupported { + t.Fatal("the adapter mutated the SMART row's own collection state") + } + + own, _ := resourceFromHostSMARTDisk(host, smartWith) + if got := own.PhysicalDisk; got.Temperature != 40 || got.Collection.Temperature != smartWith.Collection.Temperature { + t.Fatalf("%s: SMART row with its own reading lost the agent's state: temperature=%d collection=%+v", tc.name, got.Temperature, got.Collection) + } + } + + cold := models.HostUnraidDisk{Name: "disk2", Device: "sdd", Serial: "UNRAID-2", SpunDown: true} + native, _ := resourceFromHostUnraidPhysicalDisk(models.Host{ID: "agent-tower", Status: "online"}, cold) + if native.PhysicalDisk.Collection != nil { + t.Fatalf("an Unraid row without a temperature claimed temperature state: %+v", native.PhysicalDisk.Collection) + } +} + +// The registry merges an Unraid inventory row and the agent's SMART row for the +// same disk, choosing the temperature and the collection state separately. A +// reporting host's inventory row must not lend the merged disk an "available" +// state when the reading it shows is the SMART row's, and that row says it was +// not collected. +func TestUnraidInventoryRowDoesNotMaskTheSMARTRowState(t *testing.T) { + state := models.NewState() + state.UpsertHost(models.Host{ + ID: "agent-tower", Hostname: "tower", Status: "online", + Unraid: &models.HostUnraidStorage{Disks: []models.HostUnraidDisk{ + {Name: "disk1", Device: "sdc", Role: "data", Status: "online", Serial: "UNRAID-MERGE1", Temperature: 37}, + }}, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sdc", Serial: "UNRAID-MERGE1", Temperature: 40, + Collection: &diskinventory.CollectionStatus{Temperature: diskinventory.Unavailable("smartctl", "disk is in standby")}, + }}}, + }) + registry := NewRegistry(nil) + registry.IngestSnapshot(state.GetSnapshot()) + var disks []*PhysicalDiskMeta + for _, resource := range registry.List() { + if resource.Type == ResourceTypePhysicalDisk { + disks = append(disks, resource.PhysicalDisk) + } + } + if len(disks) != 1 { + t.Fatalf("physical disks = %d, want the inventory and SMART rows merged into one", len(disks)) + } + if got := disks[0]; got.Temperature != 40 || diskinventory.TemperatureCollected(got.Temperature, got.Collection) { + t.Fatalf("merged disk presents the SMART row's uncollected reading as collected: temperature=%d collection=%+v", got.Temperature, got.Collection) + } +} diff --git a/internal/unifiedresources/adapters.go b/internal/unifiedresources/adapters.go index 67cfe887ab..5196aab935 100644 --- a/internal/unifiedresources/adapters.go +++ b/internal/unifiedresources/adapters.go @@ -804,8 +804,16 @@ func resourceFromHostUnraidPhysicalDisk(host models.Host, disk models.HostUnraid model := strings.TrimSpace(disk.Model) name := firstNonEmpty(model, disk.Name, disk.Device, host.Hostname) health := unraidPhysicalDiskHealth(disk) - assessment := assessUnraidPhysicalDisk(disk) + assessment := assessUnraidPhysicalDisk(host, disk) sizeBytes := unraidDiskSizeBytes(host, disk) + // The row records only a state that withdraws its reading. A positive + // claim from this provenance-less row would outrank another source's + // explicit state for the same disk when the registry merges them, even + // where that source's temperature is the one shown. + var collection *diskinventory.CollectionStatus + if status := unraidDiskTemperatureStatus(host, disk); status.State == diskinventory.FieldUnavailable { + collection = &diskinventory.CollectionStatus{Temperature: status} + } resource := Resource{ Type: ResourceTypePhysicalDisk, Name: name, @@ -829,6 +837,7 @@ func resourceFromHostUnraidPhysicalDisk(host models.Host, disk models.HostUnraid ReadCount: disk.ReadCount, WriteCount: disk.WriteCount, ErrorCount: disk.ErrorCount, + Collection: collection, Risk: physicalDiskRiskFromAssessment(assessment), }, } @@ -841,6 +850,29 @@ func resourceFromHostUnraidPhysicalDisk(host models.Host, disk models.HostUnraid return resource, identity } +// unraidTemperatureSource is the provenance of a temperature read from an +// Unraid agent's array inventory; the agent's standby SMART rows use it too. +const unraidTemperatureSource = "unraid" + +// unraidDiskTemperatureStatus returns the collection state of the temperature +// an Unraid inventory row carries. The inventory has no per-field provenance: +// each accepted report replaces it, and once the host's reporting lease +// expires State.ExpireHostTelemetry keeps it as last-known context and marks +// the host offline. A temperature kept past that point, or one a spun-down +// disk still carries, is not collected. +func unraidDiskTemperatureStatus(host models.Host, disk models.HostUnraidDisk) diskinventory.FieldStatus { + switch { + case disk.Temperature <= 0: + return diskinventory.FieldStatus{} + case strings.EqualFold(strings.TrimSpace(host.Status), "offline"): + return diskinventory.Unavailable(unraidTemperatureSource, models.HostAgentStoppedReportingReason) + case disk.SpunDown: + return diskinventory.Unavailable(unraidTemperatureSource, "disk is reported spun down") + default: + return diskinventory.Available(unraidTemperatureSource) + } +} + func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Resource, ResourceIdentity) { name := strings.TrimSpace(disk.Model) if name == "" { @@ -880,6 +912,7 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res serial := strings.TrimSpace(disk.Serial) diskType := strings.TrimSpace(disk.Type) temperature := disk.Temperature + collection := diskinventory.CloneStatus(disk.Collection) health := strings.TrimSpace(disk.Health) if unraidDisk != nil { if model == "" { @@ -896,6 +929,13 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res } 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) @@ -903,7 +943,7 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res } assessment := storagehealth.AssessHostSMARTDisk(disk) if unraidDisk != nil { - assessment = storagehealth.SummarizeAssessments(assessment, assessUnraidPhysicalDisk(*unraidDisk)) + assessment = storagehealth.SummarizeAssessments(assessment, assessUnraidPhysicalDisk(host, *unraidDisk)) } wearout := physicalDiskWearoutFromSMARTAttributes(disk.Attributes) @@ -939,7 +979,7 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res WriteCount: unraidDiskCounter(unraidDisk, "write"), ErrorCount: unraidDiskCounter(unraidDisk, "error"), IO: physicalDiskIOToMeta(disk.IO), - Collection: diskinventory.CloneStatus(disk.Collection), + Collection: collection, SMART: convertSMARTAttributes(disk.Attributes), Risk: physicalDiskRiskFromAssessment(assessment), }, @@ -1288,11 +1328,16 @@ func unraidPhysicalDiskHealth(disk models.HostUnraidDisk) string { } } -func assessUnraidPhysicalDisk(disk models.HostUnraidDisk) storagehealth.Assessment { +func assessUnraidPhysicalDisk(host models.Host, disk models.HostUnraidDisk) storagehealth.Assessment { + // A retained inventory temperature is not current evidence of heat. + temperature := 0 + if status := unraidDiskTemperatureStatus(host, disk); status.State == diskinventory.FieldAvailable { + temperature = disk.Temperature + } assessment := storagehealth.AssessSample(storagehealth.Sample{ Model: disk.Model, Health: unraidPhysicalDiskHealth(disk), - Temperature: disk.Temperature, + Temperature: temperature, Wearout: -1, }) addReason := func(code string, severity storagehealth.RiskLevel, summary string) { diff --git a/pkg/diskinventory/status.go b/pkg/diskinventory/status.go index 7041ab9883..f4db88f0c6 100644 --- a/pkg/diskinventory/status.go +++ b/pkg/diskinventory/status.go @@ -71,6 +71,23 @@ func CloneStatus(status *CollectionStatus) *CollectionStatus { return &clone } +// TemperatureCollected reports whether a disk temperature was collected by the +// observation that carries it. Normalization may keep a last-known temperature +// it did not collect (a disk in standby, a host agent past its reporting +// lease) under a non-available state; that value must not be recorded or +// presented as a current reading. A temperature without collection state +// predates this contract and counts as collected. +func TemperatureCollected(temperature int, status *CollectionStatus) bool { + if temperature <= 0 { + return false + } + if status == nil { + return true + } + state := status.Temperature.State + return state == "" || state == FieldAvailable +} + // MergeStatus keeps an available observation over a weaker state while still // allowing a current available observation to replace older provenance. func MergeStatus(existing, incoming *CollectionStatus) *CollectionStatus { diff --git a/pkg/diskinventory/status_test.go b/pkg/diskinventory/status_test.go index 238e4b7097..b418eac9da 100644 --- a/pkg/diskinventory/status_test.go +++ b/pkg/diskinventory/status_test.go @@ -65,3 +65,25 @@ func TestMergeReportedStatusLetsTheSourceWithdrawItsOwnAvailability(t *testing.T t.Fatal("MergeReportedStatus mutated its input") } } + +func TestTemperatureCollectedOnlyForAReadingTakenByThisObservation(t *testing.T) { + for _, tc := range []struct { + name string + temperature int + status *CollectionStatus + want bool + }{ + {"no reading", 0, &CollectionStatus{Temperature: Available("smartctl")}, false}, + {"collected", 41, &CollectionStatus{Temperature: Available("smartctl")}, true}, + {"predates collection state", 41, nil, true}, + {"no temperature state", 41, &CollectionStatus{Serial: Available("smartctl")}, true}, + {"retained in standby", 41, &CollectionStatus{Temperature: Unavailable("smartctl", "disk is in standby")}, false}, + {"retained past the agent lease", 41, &CollectionStatus{Temperature: Unavailable("unraid", "host agent stopped reporting")}, false}, + {"unsupported", 41, &CollectionStatus{Temperature: Unsupported("proxmox_disks", "not exposed")}, false}, + {"missing", 41, &CollectionStatus{Temperature: Missing("smartctl", "attribute absent")}, false}, + } { + if got := TemperatureCollected(tc.temperature, tc.status); got != tc.want { + t.Errorf("%s: TemperatureCollected = %v, want %v", tc.name, got, tc.want) + } + } +}