diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 79650fb3d1..3815c3ddf8 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -3507,20 +3507,30 @@ 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. +Each merge step also stops the shown temperature borrowing another row's +availability (`pairPhysicalDiskTemperatureState`). The value is still chosen +by source preference and the states merged as above, so when the two rows of a +step carry different temperatures the shown value could sit under the other +row's `available` state, for example a silent agent's retained reading carried +over a Proxmox row that collected its own reading now. When the merged state +says collected but the row the shown value came from says it was not, the +merged state becomes that row's own. The rule never changes a value and never +grants availability; rows with the same value keep the merged state. It sees +only the two rows of one step: the merged disk keeps one state per field, so +with three or more rows for one disk a withdrawal an earlier step replaced is +not visible to a later step, and the earlier step's merged state stands in for +its row. Tracking each reading's own state across merges would close that. Unraid array-inventory rows carry no per-field provenance, so the adapter -derives the state of a temperature taken from one: `unavailable` from `unraid` +derives the state of a positive temperature taken from one (a row without a +temperature claims no state): `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. +that is no longer spun down. The Unraid-native disk row carries that state, and +so does a SMART row that falls back to the inventory reading because it has +none of its own. The adapter's risk assessment of an Unraid row no longer counts +a withdrawn inventory temperature as heat. 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/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go index 0c07448d64..38c45b8963 100644 --- a/internal/monitoring/physical_disk_roundtrip_test.go +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -687,11 +687,10 @@ func TestSilentUnraidAgentDiskTemperatureIsRetainedButNotCollected(t *testing.T) 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. + // A reporting agent's inventory readings are collected, including where + // a SMART row borrows them. collected := map[string]diskinventory.FieldStatus{ - "UNRAID-ONLY1": {}, + "UNRAID-ONLY1": diskinventory.Available("unraid"), "UNRAID-SMART2": diskinventory.Available("unraid"), "UNRAID-SAS3": diskinventory.Available("unraid"), } diff --git a/internal/unifiedresources/adapter_coverage_test.go b/internal/unifiedresources/adapter_coverage_test.go index 154ca7e4aa..8953fb4a49 100644 --- a/internal/unifiedresources/adapter_coverage_test.go +++ b/internal/unifiedresources/adapter_coverage_test.go @@ -1049,10 +1049,9 @@ 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. +// state. The native row and a SMART row that borrows the inventory reading +// both carry it, 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"), @@ -1081,7 +1080,7 @@ func TestUnraidDiskTemperatureFollowsTheHostLease(t *testing.T) { native diskinventory.FieldStatus borrowed diskinventory.FieldStatus }{ - {"reporting", "online", false, diskinventory.FieldStatus{}, diskinventory.Available("unraid")}, + {"reporting", "online", false, diskinventory.Available("unraid"), diskinventory.Available("unraid")}, {"silent agent", "offline", false, stopped, stopped}, {"spun down", "online", true, spunDown, spunDown}, } { @@ -1131,10 +1130,8 @@ func TestUnraidDiskTemperatureFollowsTheHostLease(t *testing.T) { } // 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. +// same disk. When they disagree, the inventory's "available" never lends +// itself to the SMART row's uncollected reading. func TestUnraidInventoryRowDoesNotMaskTheSMARTRowState(t *testing.T) { state := models.NewState() state.UpsertHost(models.Host{ @@ -1158,7 +1155,8 @@ func TestUnraidInventoryRowDoesNotMaskTheSMARTRowState(t *testing.T) { 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) + if got := disks[0]; got.Temperature != 40 || got.Collection == nil || + got.Collection.Temperature != diskinventory.Unavailable("smartctl", "disk is in standby") { + t.Fatalf("merged disk presents the SMART row's uncollected reading under another state: temperature=%d collection=%+v", got.Temperature, got.Collection) } } diff --git a/internal/unifiedresources/adapters.go b/internal/unifiedresources/adapters.go index 5196aab935..da65047d35 100644 --- a/internal/unifiedresources/adapters.go +++ b/internal/unifiedresources/adapters.go @@ -806,12 +806,8 @@ func resourceFromHostUnraidPhysicalDisk(host models.Host, disk models.HostUnraid health := unraidPhysicalDiskHealth(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 { + if status := unraidDiskTemperatureStatus(host, disk); status.State != "" { collection = &diskinventory.CollectionStatus{Temperature: status} } resource := Resource{ diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index cb2f10a48a..fc932ff604 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -3732,6 +3732,7 @@ func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, sou incoming.PhysicalDisk.Collection, ) } + pairPhysicalDiskTemperatureState(existing.PhysicalDisk, previous, incoming.PhysicalDisk) } if existing.PhysicalDisk != nil && (mergedPhysicalDisk || len(incoming.Incidents) > 0) { existing.PhysicalDisk.Risk = physicalDiskRiskFromMeta(existing.PhysicalDisk, existing.Incidents) @@ -4006,6 +4007,39 @@ func mergeTrueNASData(existing *TrueNASData, incoming *TrueNASData) *TrueNASData return &merged } +// pairPhysicalDiskTemperatureState stops one merge step presenting the shown +// temperature under the other row's availability. The merge rules choose the +// value by source preference and merge field states separately, so when the +// two rows carry different temperatures the shown value can sit under the +// other row's "available" state: a silent agent's retained reading beside a +// Proxmox row that collected its own reading now, for example. When the +// merged state says collected but the row the shown value came from says it +// was not, the merged state becomes that row's own. Values never change, and a +// state is only ever withdrawn here, never granted. previous may already be an +// earlier step's merge, whose single state per field stands in for its rows. +func pairPhysicalDiskTemperatureState(merged, previous, incoming *PhysicalDiskMeta) { + if merged == nil || previous == nil || incoming == nil || previous.Temperature == incoming.Temperature || + !diskinventory.TemperatureCollected(merged.Temperature, merged.Collection) { + return + } + var shown *PhysicalDiskMeta + switch merged.Temperature { + case incoming.Temperature: + shown = incoming + case previous.Temperature: + shown = previous + default: + return + } + if diskinventory.TemperatureCollected(shown.Temperature, shown.Collection) { + return + } + if merged.Collection == nil { + merged.Collection = &diskinventory.CollectionStatus{} + } + merged.Collection.Temperature = shown.Collection.Temperature +} + func mergePhysicalDiskData(existing *PhysicalDiskMeta, incoming *PhysicalDiskMeta) *PhysicalDiskMeta { if existing == nil { return incoming diff --git a/internal/unifiedresources/registry_merge_policy_test.go b/internal/unifiedresources/registry_merge_policy_test.go index 83c8fda61a..ac3b15ec22 100644 --- a/internal/unifiedresources/registry_merge_policy_test.go +++ b/internal/unifiedresources/registry_merge_policy_test.go @@ -634,3 +634,120 @@ func TestPhysicalDiskMergeFollowsAgentWithdrawalOfItsOwnReadings(t *testing.T) { }) } } + +// A merged disk must never present its temperature under a state the +// reading's own row does not claim. A silent agent's retained reading stays as +// last-known context under the agent's own state, whatever the Proxmox row +// collected or copied; a resumed agent's fresh reading stays collected over a +// Proxmox copy of its earlier withdrawal; a standby withdrawal keeps the +// pre-sleep reading as last-known. +func TestPhysicalDiskMergePairsTemperatureWithItsCollectionState(t *testing.T) { + disk := func(temperature int, status diskinventory.FieldStatus) Resource { + return Resource{ + Type: ResourceTypePhysicalDisk, Name: "WDC", Status: StatusOnline, + PhysicalDisk: &PhysicalDiskMeta{ + DevPath: "/dev/sdb", Serial: "WD-PAIR1", Temperature: temperature, + Collection: &diskinventory.CollectionStatus{Temperature: status}, + }, + } + } + // The Proxmox row can also carry the SMART attributes copied from the + // agent's earlier report, which changes whose value the merge shows. + withSMART := func(r Resource) Resource { + powerOnHours := int64(1000) + r.PhysicalDisk.SMART = &SMARTMeta{PowerOnHours: &powerOnHours} + return r + } + identity := ResourceIdentity{MachineID: "WD-PAIR1", Hostnames: []string{"node1"}} + retained := diskinventory.Unavailable("host_agent", "host agent stopped reporting") + nodeSMART := diskinventory.Available("proxmox_node_smart") + smartctl := diskinventory.Available("smartctl") + withdrawn := diskinventory.Unavailable("smartctl", "host agent stopped reporting") + standby := diskinventory.Unavailable("smartctl", "disk is in standby") + noProxmoxTemp := diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature") + + for _, tc := range []struct { + name string + agent, proxmox Resource + // wantTemperature 0 accepts either row's value: source preference, + // not this rule, decides which one is shown. + wantTemperature int + wantState diskinventory.FieldStatus + }{ + {"silent agent, Proxmox collected", disk(72, retained), disk(40, nodeSMART), 72, retained}, + {"silent agent, no Proxmox reading", disk(72, retained), disk(0, noProxmoxTemp), 72, retained}, + {"reporting agent, Proxmox collected", disk(41, smartctl), disk(40, nodeSMART), 41, smartctl}, + {"silent agent, Proxmox copy of its earlier reading", disk(72, withdrawn), disk(41, smartctl), 72, withdrawn}, + {"silent agent, Proxmox copy with SMART attributes", disk(72, withdrawn), withSMART(disk(41, smartctl)), 0, withdrawn}, + {"resumed agent, Proxmox copy of its withdrawal", disk(50, smartctl), disk(41, withdrawn), 50, smartctl}, + {"standby agent, Proxmox copy of its pre-sleep reading", disk(0, standby), disk(41, smartctl), 41, standby}, + } { + for _, proxmoxFirst := range []bool{true, false} { + registry := NewRegistry(nil) + ingestProxmox := func() { registry.ingest(SourceProxmox, "pve1-node1-sdb", tc.proxmox, identity) } + ingestAgent := func() { registry.ingest(SourceAgent, "agent-1-sdb", tc.agent, identity) } + if proxmoxFirst { + ingestProxmox() + ingestAgent() + } else { + ingestAgent() + ingestProxmox() + } + disks := registry.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 1 || disks[0].PhysicalDisk == nil { + t.Fatalf("%s (proxmox first=%v): expected one merged disk, got %+v", tc.name, proxmoxFirst, disks) + } + got := disks[0].PhysicalDisk + var state diskinventory.FieldStatus + if got.Collection != nil { + state = got.Collection.Temperature + } + valueOK := got.Temperature == tc.wantTemperature || + (tc.wantTemperature == 0 && (got.Temperature == tc.agent.PhysicalDisk.Temperature || got.Temperature == tc.proxmox.PhysicalDisk.Temperature)) + if !valueOK || state != tc.wantState { + t.Errorf("%s (proxmox first=%v): temperature=%d state=%+v, want %d with %+v", + tc.name, proxmoxFirst, got.Temperature, state, tc.wantTemperature, tc.wantState) + } + } + } +} + +// Three rows for one disk with different values, none collected now: the +// agent's SMART row withdrew its reading, the Unraid inventory row is past the +// same lease, and the Proxmox row still carries a copy of the agent's earlier +// "available" state. Whatever the ingest order, the merged disk must not +// present a reading as collected. +func TestPhysicalDiskMergeNeverPresentsAWithdrawnTemperatureAsCollected(t *testing.T) { + disk := func(temperature int, status diskinventory.FieldStatus) Resource { + return Resource{ + Type: ResourceTypePhysicalDisk, Name: "WDC", Status: StatusOnline, + PhysicalDisk: &PhysicalDiskMeta{ + DevPath: "/dev/sdb", Serial: "WD-THREE1", Temperature: temperature, + Collection: &diskinventory.CollectionStatus{Temperature: status}, + }, + } + } + identity := ResourceIdentity{MachineID: "WD-THREE1", Hostnames: []string{"tower"}} + rows := []struct { + source DataSource + sourceID string + resource Resource + }{ + {SourceAgent, "agent-1-sdb", disk(72, diskinventory.Unavailable("smartctl", "host agent stopped reporting"))}, + {SourceAgent, "agent-1-unraid-sdb", disk(37, diskinventory.Unavailable("unraid", "host agent stopped reporting"))}, + {SourceProxmox, "pve1-node1-sdb", disk(41, diskinventory.Available("smartctl"))}, + } + for _, order := range [][]int{{0, 1, 2}, {0, 2, 1}, {1, 0, 2}, {1, 2, 0}, {2, 0, 1}, {2, 1, 0}} { + registry := NewRegistry(nil) + for _, i := range order { + registry.ingest(rows[i].source, rows[i].sourceID, rows[i].resource, identity) + } + disks := registry.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 1 || disks[0].PhysicalDisk == nil { + t.Fatalf("order %v: expected one merged disk, got %d", order, len(disks)) + } + if got := disks[0].PhysicalDisk; diskinventory.TemperatureCollected(got.Temperature, got.Collection) { + t.Errorf("order %v: withdrawn temperature presented as collected: temperature=%d collection=%+v", order, got.Temperature, got.Collection) + } + } +}