From ff67e38968c02a7cec2bf3dd72d9afe2febc8227 Mon Sep 17 00:00:00 2001 From: Richard Courtman Date: Wed, 7 Oct 2026 11:15:08 +0100 Subject: [PATCH] Pair a merged disk's temperature with its rows' own states The registry merges a physical disk's rows one step at a time, choosing the temperature by source preference and merging collection states separately. The pairing rule from #2618 was exact for two rows only: with three or more, an earlier step's single merged state stood in for its rows. A silent agent, an offline Unraid inventory and a stale Proxmox copy could leave a withdrawn 41 labelled collected, and a legacy agent's stateless reading could pick up another row's "unsupported". The merged disk now keeps each row's current reading and state (merge-time only, never serialized), and the shown value takes its state from the readings of that value: a collected reading first, then the agent's own report over a copy. A copy of an agent source's state gives way to the agent's own withdrawal, including a legacy agent's sourceless one, and a value source preference keeps after its row reported again keeps its presented state under that row's later withdrawal. Value selection is unchanged. --- .../v6/internal/subsystems/agent-lifecycle.md | 6 + .../v6/internal/subsystems/monitoring.md | 11 +- .../internal/subsystems/storage-recovery.md | 8 + .../internal/subsystems/unified-resources.md | 56 +- internal/monitoring/monitor.go | 20 +- .../physical_disk_roundtrip_test.go | 68 ++- internal/unifiedresources/registry.go | 303 +++++++++- .../registry_merge_policy_test.go | 567 +++++++++++++++++- internal/unifiedresources/types.go | 5 + pkg/diskinventory/status.go | 23 + pkg/diskinventory/status_test.go | 21 + 11 files changed, 1005 insertions(+), 83 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index fdbf6c7ff6..dd1d022b02 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -755,6 +755,12 @@ 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. +A legacy agent's report carries no collection state, so its expiry records +`unavailable` without a source; monitoring's Proxmox-disk copy and the +unified-resources registry both read that through +`diskinventory.LegacyHostAgentStatus` (the `host_agent` source), so the +withdrawal still supersedes the copy a Proxmox disk keeps once its host stops +being polled. 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 diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 1806e08bbc..5ef5a18add 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -3860,7 +3860,16 @@ state supersede the availability it supplied earlier (`diskinventory.MergeReportedStatus`), so neither a skipped disk poll nor a host that is never disk-polled again can carry it forward. Expiry is compare-and-set on the report time the evaluation judged stale, so a report -accepted in between is never expired. Every SMART temperature history writer +accepted in between is never expired. +A legacy agent (from before collection provenance) withdraws its readings +without a source, so `hostAgentSMARTCollection` stamps its reading copied onto +the Proxmox disk through `diskinventory.LegacyHostAgentStatus`, the same rule +the unified-resources registry applies to match that withdrawal to the copy: a +silent legacy agent's retained temperature is not collected on the canonical +disk either, even when no disk poll refreshes the copy. Proof: +`TestSilentLegacyAgentCanonicalDiskFollowsItsWithdrawal` in +`internal/monitoring/physical_disk_roundtrip_test.go`. +Every SMART temperature history writer records a temperature only when its current collection state is available (or 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 diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 446b898071..66185fb88d 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -3820,6 +3820,14 @@ values may remain visible as history, but their current collection state must remain explicit. In particular, members behind a shared controller must not inherit aggregate I/O counters, and a disk drawer must hide live-I/O charts when per-member collection is unavailable or unsupported. +On a disk merged from several rows (Proxmox, host agent SMART, Unraid +inventory, TrueNAS), the temperature state these surfaces read is the state of +the reading the disk shows (`pairPhysicalDiskTemperatureState` in +`internal/unifiedresources/registry.go`), so a retained value never arrives +under another row's `available`. The merge keeps each row's reading in an +unexported record on `PhysicalDiskMeta` (`internal/unifiedresources/types.go`) +that is never serialized; storage surfaces read only the presented temperature +and its state. A retained disk temperature reads as last known, never as a live reading. The Physical Disks table, the disk drawer and the pool drawer's linked-disk list diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index d810dc84e0..fd37419cf2 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -3553,19 +3553,49 @@ 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. +Each merge step then presents the shown temperature under a state that belongs +to it (`pairPhysicalDiskTemperatureState`). The value is still chosen by source +preference and the field states merged as above, so the shown value could +otherwise sit under another row's state: a silent agent's retained reading +under a Proxmox row's current `available`, or a legacy agent's stateless +reading under a Proxmox inventory row's `unsupported`. One merged state per +field cannot say whose reading is shown once a disk has three or more rows, so +the registry keeps each row's own temperature reading and state on the merged +disk (merge-time only, never serialized) and derives the state from the +readings of the shown value. Each counts its own state, except that a state a +non-agent row copied from an agent source gives way to an agent row's own +withdrawal of that source (a legacy agent's sourceless withdrawal matches its +`host_agent` copy through `diskinventory.LegacyHostAgentStatus`, the rule +monitoring stamps the copy with); that withdrawal is the only state taken from +a reading of another value, and it never grants `available`. A collected +reading is presented over one that is not and an agent's own report over a +copy, so while a current reading holds the shown value, the order in which +different rows arrive does not change the state. A second agent reporting the +same disk is not a copy: its withdrawal leaves another agent's own collected +reading collected. A row that reports again replaces its reading. When source +preference keeps showing a value no current reading holds (the row that +reported it went into standby, or reported again without the SMART attributes +that made its value preferred), the value keeps the state it was presented with +and the row that state belonged to, under that row's later withdrawal, and the +agent's withdrawal of its source still supersedes it; a row's earlier report +never competes as current evidence. Such a kept state depends on the history of +reports, as the kept value itself does, and stays `available` when its row now +reports a different collected value. A disk that arrives without the record, or +no longer presents the value and state it was built for (edited), stands as one +reading of unknown rows: an incoming agent row's withdrawal of the source of +its state supersedes it, and when the agent is among its sources its own +withdrawal still supersedes an incoming copy. A disk entering the registry as a +new row is that row alone. Two boundaries lose rows. JSON keeps only the +presented state, so a withdrawal it does not show cannot supersede a copy +merged after a round trip; the production rebuild ingests every row into a +fresh registry and never merges across JSON. A manual link keeps the primary +disk's readings and drops the other disk's, so rows merged after the link +(supplemental records) meet only the primary's. +`registry_merge_policy_test.go` +(`TestPhysicalDiskMergePairsEveryShownTemperatureWithItsRowState`) checks every +ingest order of a catalog of agent, Unraid, Proxmox and TrueNAS rows against +this rule, and `TestPhysicalDiskMergeTemperatureReadingsAcrossIngestBoundaries` +pins the boundaries. Unraid array-inventory rows carry no per-field provenance, so the adapter derives the state of a positive temperature taken from one (a row without a temperature claims no state): `unavailable` from `unraid` diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 7649b64f28..f91fb9fc3f 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -661,25 +661,19 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No return updated } -// hostAgentLegacySource is the provenance recorded for a temperature from an -// agent that predates collection provenance (before 6.2). Those agents only -// send readings they collected. -const hostAgentLegacySource = "host_agent" - // hostAgentSMARTCollection returns the agent's collection state for one SMART -// row, giving a provenance-less temperature the legacy agent source so that -// history and later lease expiry treat it like any other agent reading. +// row, giving a provenance-less temperature the legacy agent source +// (diskinventory.LegacyHostAgentStatus) so that history and later lease expiry +// treat it like any other agent reading. func hostAgentSMARTCollection(smart models.HostDiskSMART) *diskinventory.CollectionStatus { collection := diskinventory.CloneStatus(smart.Collection) if collection == nil { collection = &diskinventory.CollectionStatus{} } - switch { - case collection.Temperature.State == "" && smart.Temperature > 0 && !smart.Standby: - collection.Temperature = diskinventory.Available(hostAgentLegacySource) - case collection.Temperature.State != "" && strings.TrimSpace(collection.Temperature.Source) == "": - collection.Temperature.Source = hostAgentLegacySource - } + collection.Temperature = diskinventory.LegacyHostAgentStatus( + collection.Temperature, + smart.Temperature > 0 && !smart.Standby, + ) return collection } diff --git a/internal/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go index 733e7d092c..1103a36445 100644 --- a/internal/monitoring/physical_disk_roundtrip_test.go +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -533,7 +533,7 @@ func TestMergeHostAgentSMARTIntoDisks_LegacyAgentTemperatureFollowsLease(t *test }}}}) reporting := mergeHostAgentSMARTIntoDisks([]models.PhysicalDisk{proxmoxDisk}, nodes, state.GetHosts())[0] - if reporting.Temperature != 38 || reporting.Collection.Temperature != diskinventory.Available(hostAgentLegacySource) { + if reporting.Temperature != 38 || reporting.Collection.Temperature != diskinventory.Available(diskinventory.LegacyHostAgentSource) { t.Fatalf("legacy agent temperature not recorded as collected: temp=%d collection=%+v", reporting.Temperature, reporting.Collection) } if !diskinventory.TemperatureCollected(reporting.Temperature, reporting.Collection) { @@ -555,6 +555,72 @@ func TestMergeHostAgentSMARTIntoDisks_LegacyAgentTemperatureFollowsLease(t *test } } +// A legacy agent (before collection provenance) withdraws its readings without +// a source when its lease expires, while monitoring stamps its reading copied +// onto the Proxmox disk with the legacy agent source. When no disk poll +// refreshes that copy (the host is down), the canonical disk merged from the +// agent's row and the Proxmox row must still follow the agent's withdrawal, +// and collect the reading again once the agent reports. +func TestSilentLegacyAgentCanonicalDiskFollowsItsWithdrawal(t *testing.T) { + state := models.NewState() + state.UpdateNodesForInstance("pve1", []models.Node{{ + ID: "pve1-node1", Name: "node1", Instance: "pve1", Status: "online", + LastSeen: time.Now(), LinkedAgentID: "agent-1", + }}) + report := func(seen time.Time) { + state.UpsertHost(models.Host{ + ID: "agent-1", Hostname: "node1", LinkedNodeID: "pve1-node1", Status: "online", + IntervalSeconds: 30, LastSeen: seen, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sdb", Serial: "LEGACY2", Type: "sata", Health: "PASSED", Temperature: 38, + }}}, + }) + } + lastReport := time.Now() + report(lastReport) + inventory := models.PhysicalDisk{ + ID: "pve1-node1-sdb", Node: "node1", Instance: "pve1", DevPath: "/dev/sdb", Serial: "LEGACY2", + Type: "sata", Health: "PASSED", Wearout: -1, LastChecked: time.Now(), + Collection: &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature"), + }, + } + // The disk poll while the agent reported is the copy the Proxmox row + // keeps once its host stops being polled. + state.UpdatePhysicalDisks("pve1", mergeHostAgentSMARTIntoDisks( + []models.PhysicalDisk{inventory}, state.GetSnapshot().Nodes, state.GetHosts())) + canonical := func() (int, *diskinventory.CollectionStatus) { + t.Helper() + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + adapter.PopulateFromSnapshot(state.GetSnapshot()) + disks := adapter.PhysicalDisks() + if len(disks) != 1 || disks[0].Serial() != "LEGACY2" { + t.Fatalf("canonical disks = %d, want the agent and Proxmox rows merged into one", len(disks)) + } + return disks[0].Temperature(), disks[0].Collection() + } + + if temperature, collection := canonical(); temperature != 38 || !diskinventory.TemperatureCollected(temperature, collection) { + t.Fatalf("reporting legacy agent: temperature=%d collection=%+v, want 38 collected", temperature, collection) + } + + if _, expired := state.ExpireHostTelemetry("agent-1", lastReport); !expired { + t.Fatal("the agent's lease did not expire") + } + temperature, collection := canonical() + if temperature != 38 || diskinventory.TemperatureCollected(temperature, collection) { + t.Fatalf("silent legacy agent: temperature=%d collection=%+v, want the retained 38 not collected", temperature, collection) + } + if collection.Temperature.Reason != models.HostAgentStoppedReportingReason { + t.Fatalf("silent legacy agent: temperature state %+v, want the agent's own withdrawal", collection.Temperature) + } + + report(time.Now()) + if temperature, collection := canonical(); temperature != 38 || !diskinventory.TemperatureCollected(temperature, collection) { + t.Fatalf("resumed legacy agent: temperature=%d collection=%+v, want 38 collected again", temperature, collection) + } +} + // A disk that goes into standby keeps its pre-sleep temperature as retained // evidence (preserveUnavailablePhysicalDiskEvidence), with the agent's standby // state. Full disk polls during standby must not record that value again. diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 48bc5b03cd..6c9a786cad 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -2891,7 +2891,7 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res if onlyMissing { return "" } - rr.mergeInto(existing, resource, source) + rr.mergeInto(existing, resource, source, sourceID) return existing.ID } } @@ -2903,7 +2903,7 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res if onlyMissing { return "" } - rr.mergeInto(existing, resource, source) + rr.mergeInto(existing, resource, source, sourceID) rr.bySource[source][sourceID] = existing.ID return existing.ID } @@ -2918,7 +2918,7 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res if onlyMissing { return "" } - rr.mergeInto(existing, resource, source) + rr.mergeInto(existing, resource, source, sourceID) rr.bySource[source][sourceID] = existing.ID return existing.ID } @@ -2927,6 +2927,7 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res if onlyMissing && rr.resources[resource.ID] != nil { return "" } + stampPhysicalDiskTemperatureReading(&resource, source, sourceID) rr.resources[resource.ID] = &resource rr.bySource[source][sourceID] = resource.ID rr.matcher.Add(resource.ID, identity) @@ -2940,11 +2941,12 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res if onlyMissing { return "" } - rr.mergeInto(existing, resource, source) + rr.mergeInto(existing, resource, source, sourceID) rr.bySource[source][sourceID] = existing.ID rr.matcher.Add(existing.ID, existing.Identity) return existing.ID } + stampPhysicalDiskTemperatureReading(&resource, source, sourceID) rr.resources[resource.ID] = &resource rr.bySource[source][sourceID] = resource.ID rr.matcher.Add(resource.ID, identity) @@ -3127,7 +3129,7 @@ func (rr *ResourceRegistry) mergeLinkedKubernetesNode( resource.Identity = identity resource.Type = CanonicalResourceType(resource.Type) - rr.mergeInto(existing, resource, SourceK8s) + rr.mergeInto(existing, resource, SourceK8s, sourceID) rr.bySource[SourceK8s][sourceID] = existing.ID rr.matcher.Add(existing.ID, existing.Identity) return true @@ -3690,7 +3692,7 @@ func identitiesShareHostname(a, b ResourceIdentity) bool { return false } -func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, source DataSource) { +func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, source DataSource, sourceID string) { if existing == nil { return } @@ -3707,6 +3709,12 @@ func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, sou mergedPhysicalDisk := incoming.PhysicalDisk != nil if mergedPhysicalDisk { previous := existing.PhysicalDisk + incomingReading := physicalDiskRowTemperatureReading(incoming.PhysicalDisk, source, sourceID) + readings := withPhysicalDiskTemperatureReading( + physicalDiskTemperatureReadingsOf(previous, existing.Sources), + incomingReading, + ) + kept := keptPhysicalDiskTemperature(previous, incomingReading) existing.PhysicalDisk = mergePhysicalDiskData(existing.PhysicalDisk, incoming.PhysicalDisk) if source == SourceProxmox && previous != nil && hasDataSource(existing.Sources, SourceAgent) { if diskinventory.IsUsableHardwareID(previous.Serial) && @@ -3752,7 +3760,7 @@ func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, sou incoming.PhysicalDisk.Collection, ) } - pairPhysicalDiskTemperatureState(existing.PhysicalDisk, previous, incoming.PhysicalDisk) + pairPhysicalDiskTemperatureState(existing.PhysicalDisk, readings, kept) } if existing.PhysicalDisk != nil && (mergedPhysicalDisk || len(incoming.Incidents) > 0) { existing.PhysicalDisk.Risk = physicalDiskRiskFromMeta(existing.PhysicalDisk, existing.Incidents) @@ -4028,37 +4036,270 @@ 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) { +// physicalDiskTemperatureReadings records, for one merged disk, the +// temperature reading each of its rows reported with that row's own +// collection state. The merge chooses the shown value by source preference +// and merges field states separately, and with three or more rows one merged +// state per field cannot say whose reading the value is, so the registry keeps +// every row's reading and derives the state of the shown value from them +// (pairPhysicalDiskTemperatureState). It is merge-time state, never +// serialized: a disk read back from JSON starts again from its presented value +// and state. A record is never mutated once built, so clones share it, and a +// seeded disk continues from it only while it still presents the value and +// state the record was built for. A disk that enters the registry as a new row +// is that row alone, whatever record it carries. +type physicalDiskTemperatureReadings struct { + // value and state are what the disk presented when this record was + // built. A disk that no longer presents them does not trust the record. + value int + state diskinventory.FieldStatus + // row is the row whose reading the presented state belongs to, empty + // when the state is not a reading's. + row string + // rows holds each row's current reading, sorted by row. + rows []physicalDiskTemperatureReading +} + +type physicalDiskTemperatureReading struct { + // row identifies the reporting row (source and source ID), so that the + // row's next report replaces its reading. It is empty for a reading that + // stands in for a disk the registry did not merge itself. + row string + value int + state diskinventory.FieldStatus + // withdraws is set when a state on this reading saying it is not collected + // is the agent's own word on that source. A host agent's own row is the + // later word on the fields it supplied. + withdraws bool + // copied is set when a state on this reading from an agent source may be + // a copy of an earlier agent report, which the agent's own withdrawal of + // that source supersedes: true for every reading but the agent's own row. + copied bool +} + +// physicalDiskRowTemperatureReading is one ingested row's own reading. +func physicalDiskRowTemperatureReading(meta *PhysicalDiskMeta, source DataSource, sourceID string) physicalDiskTemperatureReading { + reading := physicalDiskTemperatureReading{ + row: string(source) + "\x00" + normalizeSourceID(sourceID), + withdraws: source == SourceAgent, + copied: source != SourceAgent, + } + if meta != nil { + reading.value = meta.Temperature + reading.state = physicalDiskTemperatureState(meta) + } + return reading +} + +// physicalDiskTemperatureReadingsOf returns the readings behind a disk's +// presented temperature. A disk the registry has not merged itself stands as +// one reading of its presented value and state whose rows are unknown, so it +// is taken conservatively both ways: an incoming agent row's withdrawal +// supersedes the state it presents, and when the agent is among its sources +// its own withdrawal still supersedes an incoming copy. +func physicalDiskTemperatureReadingsOf(meta *PhysicalDiskMeta, sources []DataSource) []physicalDiskTemperatureReading { + if meta == nil { + return nil + } + if record := physicalDiskTemperatureRecord(meta); record != nil { + return record.rows + } + return []physicalDiskTemperatureReading{{ + value: meta.Temperature, + state: physicalDiskTemperatureState(meta), + withdraws: hasDataSource(sources, SourceAgent), + copied: true, + }} +} + +// withPhysicalDiskTemperatureReading returns rows with reading in place of +// any earlier reading from the same row, sorted by row, in a new slice. +func withPhysicalDiskTemperatureReading( + rows []physicalDiskTemperatureReading, + reading physicalDiskTemperatureReading, +) []physicalDiskTemperatureReading { + out := make([]physicalDiskTemperatureReading, 0, len(rows)+1) + for _, existing := range rows { + if existing.row != reading.row { + out = append(out, existing) + } + } + out = append(out, reading) + sort.Slice(out, func(i, j int) bool { return out[i].row < out[j].row }) + return out +} + +// physicalDiskTemperatureRecord returns the disk's readings record while the +// disk still presents the value and state it was built for. +func physicalDiskTemperatureRecord(meta *PhysicalDiskMeta) *physicalDiskTemperatureReadings { + if meta == nil { + return nil + } + record := meta.temperatureReadings + if record == nil || record.value != meta.Temperature || record.state != physicalDiskTemperatureState(meta) { + return nil + } + return record +} + +// keptPhysicalDiskTemperature is the reading behind a value source preference +// keeps showing after the row that reported it has reported again (an agent +// row going into standby reports no temperature, a row without SMART +// attributes cannot displace one with them). The kept value is always the +// previously presented one, so it keeps the state it was presented with and +// the row that state belonged to, under that row's new withdrawal when it +// reports one. It counts as a copy: the agent's withdrawal of its source still +// supersedes it. +func keptPhysicalDiskTemperature(previous *PhysicalDiskMeta, incoming physicalDiskTemperatureReading) physicalDiskTemperatureReading { + kept := physicalDiskTemperatureReading{copied: true} + if previous == nil { + return kept + } + kept.value = previous.Temperature + kept.state = physicalDiskTemperatureState(previous) + if record := physicalDiskTemperatureRecord(previous); record != nil { + kept.row = record.row + } + if kept.row != "" && kept.row == incoming.row && + incoming.state.State != "" && incoming.state.State != diskinventory.FieldAvailable { + kept.state = incoming.state + } + return kept +} + +// stampPhysicalDiskTemperatureReading gives a disk entering the registry +// without a merge the record of its own row's reading. +func stampPhysicalDiskTemperatureReading(resource *Resource, source DataSource, sourceID string) { + if resource == nil || resource.PhysicalDisk == nil { return } - var shown *PhysicalDiskMeta - switch merged.Temperature { - case incoming.Temperature: - shown = incoming - case previous.Temperature: - shown = previous - default: + meta := *resource.PhysicalDisk + reading := physicalDiskRowTemperatureReading(&meta, source, sourceID) + meta.temperatureReadings = &physicalDiskTemperatureReadings{ + value: meta.Temperature, + state: physicalDiskTemperatureState(&meta), + row: reading.row, + rows: []physicalDiskTemperatureReading{reading}, + } + resource.PhysicalDisk = &meta +} + +func physicalDiskTemperatureState(meta *PhysicalDiskMeta) diskinventory.FieldStatus { + if meta == nil || meta.Collection == nil { + return diskinventory.FieldStatus{} + } + return meta.Collection.Temperature +} + +// physicalDiskTemperatureSourceKey normalizes the source of a temperature +// state for matching an agent's withdrawal to another row's copy of it. A +// host agent from before collection provenance withdraws its readings without +// a source, while monitoring stamps its readings copied onto a Proxmox row +// with the legacy agent source; diskinventory.LegacyHostAgentStatus is that +// one rule, applied to the agent's own word. +func physicalDiskTemperatureSourceKey(status diskinventory.FieldStatus, agentWord bool) string { + if agentWord { + status = diskinventory.LegacyHostAgentStatus(status, false) + } + return strings.ToLower(strings.TrimSpace(status.Source)) +} + +// pairPhysicalDiskTemperatureState presents the merged disk's shown +// temperature under a state that belongs to it, and records the readings for +// the next merge step. Among the readings of the shown value, each counts its +// own state, except that a state another row copied from an agent source +// gives way to the agent's own withdrawal of that source (a lease expiry, a +// standby): the agent's own row is the later word on its sources, so neither +// a copied "available" nor a copied older withdrawal stands against it. That +// withdrawal is the only state taken from a reading of another value; it is +// never "available", so availability is never borrowed. A collected reading +// is presented over one that is not, an agent's own report over a copy, and a +// reported row over a disk's stand-in, so for the same shown value and the +// same row reports, the order in which different rows arrive does not change +// the state. Source preference still chooses the value +// (mergePhysicalDiskData and the Proxmox carry in mergeInto); this never +// changes it. A value no current reading holds is the kept one +// (keptPhysicalDiskTemperature). With no temperature shown the merged state +// stays as the field-state rules left it. +func pairPhysicalDiskTemperatureState( + merged *PhysicalDiskMeta, + rows []physicalDiskTemperatureReading, + kept physicalDiskTemperatureReading, +) { + if merged == nil { return } - if diskinventory.TemperatureCollected(shown.Temperature, shown.Collection) { + owner := "" + defer func() { + merged.temperatureReadings = &physicalDiskTemperatureReadings{ + value: merged.Temperature, + state: physicalDiskTemperatureState(merged), + row: owner, + rows: rows, + } + }() + if merged.Temperature <= 0 { return } - if merged.Collection == nil { - merged.Collection = &diskinventory.CollectionStatus{} + // A reported row's withdrawal is later than the one a stand-in presents. + type withdrawal struct { + state diskinventory.FieldStatus + standIn bool } - merged.Collection.Temperature = shown.Collection.Temperature + withdrawn := make(map[string]withdrawal) + for _, row := range rows { + key := physicalDiskTemperatureSourceKey(row.state, row.withdraws) + if !row.withdraws || key == "" || row.state.State == "" || row.state.State == diskinventory.FieldAvailable { + continue + } + if current, seen := withdrawn[key]; !seen || current.standIn { + withdrawn[key] = withdrawal{state: row.state, standIn: row.row == ""} + } + } + rank := func(row physicalDiskTemperatureReading, state diskinventory.FieldStatus) int { + score := 0 + if !diskinventory.TemperatureCollected(row.value, &diskinventory.CollectionStatus{Temperature: state}) { + score += 4 + } + if row.copied { + score += 2 + } + if row.row == "" { + score++ + } + return score + } + effective := func(row physicalDiskTemperatureReading) diskinventory.FieldStatus { + if row.copied && row.state.State != "" { + if agentWord, ok := withdrawn[physicalDiskTemperatureSourceKey(row.state, row.withdraws)]; ok { + return agentWord.state + } + } + return row.state + } + found, bestRank := false, 0 + var state diskinventory.FieldStatus + for _, row := range rows { + if row.value != merged.Temperature { + continue + } + rowState := effective(row) + if rowRank := rank(row, rowState); !found || rowRank < bestRank { + found, bestRank, state, owner = true, rowRank, rowState, row.row + } + } + if !found && kept.value == merged.Temperature { + found, state, owner = true, effective(kept), kept.row + } + if !found || (merged.Collection == nil && state == (diskinventory.FieldStatus{})) { + return + } + collection := diskinventory.CloneStatus(merged.Collection) + if collection == nil { + collection = &diskinventory.CollectionStatus{} + } + collection.Temperature = state + merged.Collection = collection } func mergePhysicalDiskData(existing *PhysicalDiskMeta, incoming *PhysicalDiskMeta) *PhysicalDiskMeta { diff --git a/internal/unifiedresources/registry_merge_policy_test.go b/internal/unifiedresources/registry_merge_policy_test.go index ecf99d30e4..5b7a92adb6 100644 --- a/internal/unifiedresources/registry_merge_policy_test.go +++ b/internal/unifiedresources/registry_merge_policy_test.go @@ -2,6 +2,7 @@ package unifiedresources import ( "encoding/json" + "strconv" "strings" "testing" "time" @@ -1321,6 +1322,10 @@ func TestPhysicalDiskMergePairsTemperatureWithItsCollectionState(t *testing.T) { 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") + // An agent from before collection provenance withdraws without a source; + // monitoring copies its readings onto the Proxmox row as host_agent. + legacyWithdrawn := diskinventory.Unavailable("", "host agent stopped reporting") + legacyCopy := diskinventory.Available(diskinventory.LegacyHostAgentSource) for _, tc := range []struct { name string @@ -1336,6 +1341,8 @@ func TestPhysicalDiskMergePairsTemperatureWithItsCollectionState(t *testing.T) { {"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}, + {"resumed agent, same value as a Proxmox copy of its withdrawal with SMART attributes", disk(41, smartctl), withSMART(disk(41, withdrawn)), 41, smartctl}, + {"legacy agent past its lease, Proxmox copy with SMART attributes", disk(72, legacyWithdrawn), withSMART(disk(41, legacyCopy)), 0, legacyWithdrawn}, {"standby agent, Proxmox copy of its pre-sleep reading", disk(0, standby), disk(41, smartctl), 41, standby}, } { for _, proxmoxFirst := range []bool{true, false} { @@ -1368,11 +1375,13 @@ func TestPhysicalDiskMergePairsTemperatureWithItsCollectionState(t *testing.T) { } } -// 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. +// Three rows for one disk, 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, including when the Unraid inventory and the Proxmox copy happen +// to hold the same value, so that an earlier merge step's single state cannot +// tell whose reading is shown. func TestPhysicalDiskMergeNeverPresentsAWithdrawnTemperatureAsCollected(t *testing.T) { disk := func(temperature int, status diskinventory.FieldStatus) Resource { return Resource{ @@ -1384,26 +1393,536 @@ func TestPhysicalDiskMergeNeverPresentsAWithdrawnTemperatureAsCollected(t *testi } } 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) + for _, unraidTemperature := range []int{37, 41} { + rows := []physicalDiskMergeRow{ + {SourceAgent, "agent-1-sdb", disk(72, diskinventory.Unavailable("smartctl", "host agent stopped reporting"))}, + {SourceAgent, "agent-1-unraid-sdb", disk(unraidTemperature, diskinventory.Unavailable("unraid", "host agent stopped reporting"))}, + {SourceProxmox, "pve1-node1-sdb", disk(41, diskinventory.Available("smartctl"))}, } - 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) + for _, order := range physicalDiskMergeOrders(len(rows)) { + got := mergePhysicalDiskRows(t, rows, order, identity) + if diskinventory.TemperatureCollected(got.Temperature, got.Collection) { + t.Errorf("unraid %d, order %v: withdrawn temperature presented as collected: temperature=%d collection=%+v", + unraidTemperature, order, got.Temperature, got.Collection) + } } } } + +// A legacy agent (before collection provenance) sends only readings it +// collected, with no state. Its reading must stay collected whatever other +// rows the disk has: a Proxmox inventory row that cannot read temperatures +// must not lend it "unsupported", and a second Proxmox row collecting a +// different value must not change which state belongs to which value. +func TestPhysicalDiskMergeKeepsALegacyAgentReadingCollected(t *testing.T) { + disk := func(temperature int, status *diskinventory.FieldStatus) Resource { + meta := &PhysicalDiskMeta{DevPath: "/dev/sdb", Serial: "WD-LEGACY1", Temperature: temperature} + if status != nil { + meta.Collection = &diskinventory.CollectionStatus{Temperature: *status} + } + return Resource{Type: ResourceTypePhysicalDisk, Name: "WDC", Status: StatusOnline, PhysicalDisk: meta} + } + unsupported := diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature") + nodeSMART := diskinventory.Available("proxmox_node_smart") + identity := ResourceIdentity{MachineID: "WD-LEGACY1", Hostnames: []string{"node1"}} + legacy := physicalDiskMergeRow{SourceAgent, "agent-1-sdb", disk(50, nil)} + inventory := physicalDiskMergeRow{SourceProxmox, "pve1-node1-sdb", disk(0, &unsupported)} + otherNode := physicalDiskMergeRow{SourceProxmox, "pve2-node2-sdb", disk(40, &nodeSMART)} + + for _, rows := range [][]physicalDiskMergeRow{{legacy, inventory}, {legacy, inventory, otherNode}} { + for _, order := range physicalDiskMergeOrders(len(rows)) { + got := mergePhysicalDiskRows(t, rows, order, identity) + var state diskinventory.FieldStatus + if got.Collection != nil { + state = got.Collection.Temperature + } + want := map[int]diskinventory.FieldStatus{50: {}, 40: nodeSMART} + if wantState, ok := want[got.Temperature]; !ok || state != wantState { + t.Errorf("%d rows, order %v: temperature=%d state=%+v, want 50 without state or 40 with %+v", + len(rows), order, got.Temperature, state, nodeSMART) + } + } + } +} + +// Whatever rows a disk has and whatever order they arrive in, the merged +// temperature must carry a state that belongs to a row holding that value, +// after the agent's own withdrawal of a source supersedes another row's copy +// of that source's earlier "available" state. The merge may still choose +// which row's value to show by source preference; it may not pair the value +// with another row's state. The catalog covers a reporting, silent, standby +// and legacy agent, a SMART row with and without attributes, the Unraid +// inventory (collected, past the lease, spun down), and Proxmox rows that +// read nothing, collect their own reading, or carry a fresh or stale copy of +// the agent's. +func TestPhysicalDiskMergePairsEveryShownTemperatureWithItsRowState(t *testing.T) { + disk := func(temperature int, status *diskinventory.FieldStatus, smart bool) Resource { + meta := &PhysicalDiskMeta{DevPath: "/dev/sdb", Serial: "WD-CATALOG1", Temperature: temperature} + if status != nil { + meta.Collection = &diskinventory.CollectionStatus{Temperature: *status} + } + if smart { + powerOnHours := int64(1000) + meta.SMART = &SMARTMeta{PowerOnHours: &powerOnHours} + } + return Resource{Type: ResourceTypePhysicalDisk, Name: "WDC", Status: StatusOnline, PhysicalDisk: meta} + } + state := func(status diskinventory.FieldStatus) *diskinventory.FieldStatus { return &status } + present := func(resource Resource) *Resource { return &resource } + stopped := "host agent stopped reporting" + agentRows := []Resource{ + disk(41, state(diskinventory.Available("smartctl")), false), + disk(50, state(diskinventory.Available("smartctl")), true), + disk(72, state(diskinventory.Unavailable("smartctl", stopped)), false), + disk(41, state(diskinventory.Unavailable("smartctl", stopped)), true), + disk(0, state(diskinventory.Unavailable("smartctl", "disk is in standby")), false), + disk(50, nil, false), + disk(72, state(diskinventory.Unavailable("host_agent", stopped)), false), + disk(72, state(diskinventory.Unavailable("", stopped)), false), + } + unraidRows := []*Resource{ + nil, + present(disk(41, state(diskinventory.Unavailable("unraid", stopped)), false)), + present(disk(37, state(diskinventory.Available("unraid")), false)), + present(disk(41, state(diskinventory.Unavailable("unraid", "disk is reported spun down")), false)), + } + proxmoxRows := []Resource{ + disk(0, state(diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature")), false), + disk(40, state(diskinventory.Available("proxmox_node_smart")), false), + disk(41, state(diskinventory.Available("smartctl")), false), + disk(41, state(diskinventory.Available("smartctl")), true), + disk(41, state(diskinventory.Unavailable("smartctl", stopped)), true), + disk(72, state(diskinventory.Available("host_agent")), false), + disk(41, state(diskinventory.Available("host_agent")), true), + } + // TrueNAS rows carry no collection state. + trueNASRows := []*Resource{nil, present(disk(40, nil, true))} + identity := ResourceIdentity{MachineID: "WD-CATALOG1", Hostnames: []string{"node1"}} + + for _, agent := range agentRows { + for _, unraid := range unraidRows { + for _, proxmox := range proxmoxRows { + for _, trueNAS := range trueNASRows { + rows := []physicalDiskMergeRow{ + {SourceAgent, "agent-1-sdb", agent}, + {SourceProxmox, "pve1-node1-sdb", proxmox}, + } + if unraid != nil { + rows = append(rows, physicalDiskMergeRow{SourceAgent, "agent-1-unraid-sdb", *unraid}) + } + if trueNAS != nil { + rows = append(rows, physicalDiskMergeRow{SourceTrueNAS, "truenas-1-sdb", *trueNAS}) + } + for _, order := range physicalDiskMergeOrders(len(rows)) { + assertPhysicalDiskTemperaturePairedWithItsRows(t, rows, order, mergePhysicalDiskRows(t, rows, order, identity)) + } + } + } + } + } +} + +// assertPhysicalDiskTemperaturePairedWithItsRows checks a merged disk against +// its rows: a shown temperature carries the state of a row reporting that +// value, preferring a collected reading, then the agent's own report over a +// copy. +func assertPhysicalDiskTemperaturePairedWithItsRows(t *testing.T, rows []physicalDiskMergeRow, order []int, got *PhysicalDiskMeta) { + t.Helper() + if got.Temperature <= 0 { + return + } + var shown diskinventory.FieldStatus + if got.Collection != nil { + shown = got.Collection.Temperature + } + type candidate struct { + state diskinventory.FieldStatus + rank int + } + var candidates []candidate + bestRank := -1 + for _, row := range rows { + if row.resource.PhysicalDisk.Temperature != got.Temperature { + continue + } + state := physicalDiskRowTemperatureStateAfterWithdrawals(row, rows) + rank := 0 + if !diskinventory.TemperatureCollected(got.Temperature, &diskinventory.CollectionStatus{Temperature: state}) { + rank += 2 + } + if row.source != SourceAgent { + rank++ + } + candidates = append(candidates, candidate{state, rank}) + if bestRank < 0 || rank < bestRank { + bestRank = rank + } + } + for _, candidate := range candidates { + if candidate.rank == bestRank && candidate.state == shown { + return + } + } + t.Errorf("rows %s, order %v: temperature %d shown under %+v, but its rows say %+v (best rank %d)", + describePhysicalDiskMergeRows(rows), order, got.Temperature, shown, candidates, bestRank) +} + +type physicalDiskMergeRow struct { + source DataSource + sourceID string + resource Resource +} + +// physicalDiskRowTemperatureStateAfterWithdrawals is the test oracle: a row's +// own temperature state, unless the row is not the agent's own report and the +// agent withdrew the source of the state it copied, which then gives way to +// that withdrawal. An agent withdrawal without a source is a legacy agent's, +// whose copies carry the legacy agent source. +func physicalDiskRowTemperatureStateAfterWithdrawals(row physicalDiskMergeRow, rows []physicalDiskMergeRow) diskinventory.FieldStatus { + own := func(r physicalDiskMergeRow) diskinventory.FieldStatus { + if r.resource.PhysicalDisk.Collection == nil { + return diskinventory.FieldStatus{} + } + return r.resource.PhysicalDisk.Collection.Temperature + } + state := own(row) + if row.source == SourceAgent || state.State == "" { + return state + } + for _, other := range rows { + withdrawal := own(other) + source := withdrawal.Source + if source == "" { + source = diskinventory.LegacyHostAgentSource + } + if other.source == SourceAgent && withdrawal.State != "" && withdrawal.State != diskinventory.FieldAvailable && + strings.EqualFold(source, state.Source) { + return withdrawal + } + } + return state +} + +func mergePhysicalDiskRows(t *testing.T, rows []physicalDiskMergeRow, order []int, identity ResourceIdentity) *PhysicalDiskMeta { + t.Helper() + registry := NewRegistry(nil) + for _, i := range order { + row := rows[i] + resource := row.resource + resource.PhysicalDisk = clonePhysicalDiskMeta(row.resource.PhysicalDisk) + registry.ingest(row.source, row.sourceID, resource, identity) + } + disks := registry.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 1 || disks[0].PhysicalDisk == nil { + t.Fatalf("rows %s, order %v: expected one merged disk, got %d", describePhysicalDiskMergeRows(rows), order, len(disks)) + } + return disks[0].PhysicalDisk +} + +// physicalDiskMergeOrders returns every ingest order of n rows. +func physicalDiskMergeOrders(n int) [][]int { + if n == 0 { + return [][]int{{}} + } + var orders [][]int + for _, rest := range physicalDiskMergeOrders(n - 1) { + for position := 0; position <= len(rest); position++ { + order := append(append(append([]int{}, rest[:position]...), n-1), rest[position:]...) + orders = append(orders, order) + } + } + return orders +} + +func describePhysicalDiskMergeRows(rows []physicalDiskMergeRow) string { + parts := make([]string, 0, len(rows)) + for _, row := range rows { + meta := row.resource.PhysicalDisk + state := "none" + if meta.Collection != nil { + state = string(meta.Collection.Temperature.State) + "(" + meta.Collection.Temperature.Source + ")" + } + smart := "" + if meta.SMART != nil { + smart = "+smart" + } + parts = append(parts, string(row.source)+":"+strconv.Itoa(meta.Temperature)+"/"+state+smart) + } + return "[" + strings.Join(parts, " ") + "]" +} + +// The readings behind a merged disk's temperature describe the rows that +// registry merged. A disk cloned out and seeded back continues from them; a +// disk known only by its presented value and state (read back from JSON, or +// edited) stands as one reading of unknown rows, which an agent's later word +// supersedes and which still withdraws for the agent; a disk that enters as a +// new row is that row alone; and a row that reports again replaces its own +// earlier reading. A second agent reporting the same disk is not a copy of the +// first, so its withdrawal leaves the other agent's own reading collected. +func TestPhysicalDiskMergeTemperatureReadingsAcrossIngestBoundaries(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-BOUNDARY1", Temperature: temperature, + Collection: &diskinventory.CollectionStatus{Temperature: status}, + }, + } + } + identity := ResourceIdentity{MachineID: "WD-BOUNDARY1", Hostnames: []string{"node1"}} + smartctl := diskinventory.Available("smartctl") + standby := diskinventory.Unavailable("smartctl", "disk is in standby") + stopped := diskinventory.Unavailable("smartctl", "host agent stopped reporting") + spunDown := diskinventory.Unavailable("unraid", "disk is reported spun down") + only := func(t *testing.T, registry *ResourceRegistry) *PhysicalDiskMeta { + t.Helper() + disks := registry.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 1 || disks[0].PhysicalDisk == nil { + t.Fatalf("expected one merged disk, got %d", len(disks)) + } + return disks[0].PhysicalDisk + } + stateOf := func(meta *PhysicalDiskMeta) diskinventory.FieldStatus { + if meta.Collection == nil { + return diskinventory.FieldStatus{} + } + return meta.Collection.Temperature + } + // presented is a merged disk known only by what it presents. + presented := func(temperature int, status diskinventory.FieldStatus) Resource { + resource := disk(temperature, status) + resource.ID = "physical-disk:WD-BOUNDARY1" + resource.Identity = identity + resource.Sources = []DataSource{SourceAgent, SourceProxmox} + return resource + } + expect := func(t *testing.T, registry *ResourceRegistry, temperature int, state diskinventory.FieldStatus) { + t.Helper() + if got := only(t, registry); got.Temperature != temperature || stateOf(got) != state { + t.Fatalf("temperature=%d state=%+v, want %d under %+v", got.Temperature, stateOf(got), temperature, state) + } + } + + t.Run("a cloned disk seeded back continues from its rows", func(t *testing.T) { + first := NewRegistry(nil) + first.ingest(SourceAgent, "agent-1-sdb", disk(0, standby), identity) + first.ingest(SourceAgent, "agent-1-unraid-sdb", disk(0, spunDown), identity) + second := NewRegistry(nil) + second.IngestResources(first.ListByType(ResourceTypePhysicalDisk)) + second.ingest(SourceProxmox, "pve1-node1-sdb", disk(41, smartctl), identity) + expect(t, second, 41, standby) + }) + + t.Run("an edited clone starts again from what it presents", func(t *testing.T) { + first := NewRegistry(nil) + first.ingest(SourceAgent, "agent-1-sdb", disk(50, smartctl), identity) + first.ingest(SourceProxmox, "pve1-node1-sdb", disk(40, diskinventory.Available("proxmox_node_smart")), identity) + resources := first.ListByType(ResourceTypePhysicalDisk) + if len(resources) != 1 || resources[0].PhysicalDisk.Temperature != 50 { + t.Fatalf("expected one merged disk showing the agent's 50, got %+v", resources) + } + resources[0].PhysicalDisk.Collection.Temperature = stopped + second := NewRegistry(nil) + second.IngestResources(resources) + second.ingest(SourceProxmox, "pve1-node1-sdb", disk(50, smartctl), identity) + if got := only(t, second); diskinventory.TemperatureCollected(got.Temperature, got.Collection) { + t.Fatalf("temperature=%d state=%+v, want the edited withdrawal to stand over the Proxmox copy", got.Temperature, stateOf(got)) + } + }) + + t.Run("a presented disk's withdrawal still supersedes a copy", func(t *testing.T) { + registry := NewRegistry(nil) + registry.IngestResources([]Resource{presented(50, stopped)}) + registry.ingest(SourceProxmox, "pve1-node1-sdb", disk(50, smartctl), identity) + if got := only(t, registry); diskinventory.TemperatureCollected(got.Temperature, got.Collection) { + t.Fatalf("temperature=%d state=%+v, want the presented withdrawal to stand over the Proxmox copy", got.Temperature, stateOf(got)) + } + }) + + t.Run("an agent's later word supersedes a presented disk's availability", func(t *testing.T) { + registry := NewRegistry(nil) + registry.IngestResources([]Resource{presented(50, smartctl)}) + registry.ingest(SourceAgent, "agent-1-sdb", disk(50, stopped), identity) + expect(t, registry, 50, stopped) + }) + + t.Run("an agent's later word is presented over a presented disk's state", func(t *testing.T) { + registry := NewRegistry(nil) + registry.IngestResources([]Resource{presented(41, stopped)}) + registry.ingest(SourceAgent, "agent-1-sdb", disk(41, standby), identity) + expect(t, registry, 41, standby) + }) + + t.Run("a merged disk ingested again as a Proxmox row is a copy", func(t *testing.T) { + first := NewRegistry(nil) + first.ingest(SourceAgent, "agent-1-sdb", disk(50, smartctl), identity) + first.ingest(SourceProxmox, "pve1-node1-sdb", disk(40, diskinventory.Available("proxmox_node_smart")), identity) + resources := first.ListByType(ResourceTypePhysicalDisk) + if len(resources) != 1 { + t.Fatalf("expected one merged disk, got %d", len(resources)) + } + second := NewRegistry(nil) + second.IngestRecords(SourceProxmox, []IngestRecord{{SourceID: "pve1-node1-sdb", Resource: resources[0], Identity: identity}}) + second.ingest(SourceAgent, "agent-1-sdb", disk(0, standby), identity) + expect(t, second, 50, standby) + }) + + t.Run("an agent row reporting again replaces its own reading", func(t *testing.T) { + registry := NewRegistry(nil) + registry.ingest(SourceAgent, "agent-1-sdb", disk(41, smartctl), identity) + registry.ingest(SourceAgent, "agent-1-sdb", disk(0, standby), identity) + registry.ingest(SourceProxmox, "pve1-node1-sdb", disk(41, smartctl), identity) + expect(t, registry, 41, standby) + }) + + t.Run("a value kept after its row reports again keeps the row's later word", func(t *testing.T) { + rows := map[string]physicalDiskMergeRow{ + "silent": {SourceAgent, "agent-1-sdb", disk(72, stopped)}, + "standby": {SourceAgent, "agent-1-sdb", disk(0, standby)}, + "proxmox": {SourceProxmox, "pve1-node1-sdb", disk(40, diskinventory.Available("proxmox_node_smart"))}, + } + for _, sequence := range [][]string{{"silent", "standby", "proxmox"}, {"silent", "proxmox", "standby"}} { + registry := NewRegistry(nil) + for _, name := range sequence { + registry.ingest(rows[name].source, rows[name].sourceID, rows[name].resource, identity) + } + if got := only(t, registry); got.Temperature != 72 || stateOf(got) != standby { + t.Errorf("%v: temperature=%d state=%+v, want the retained 72 under the agent's standby", sequence, got.Temperature, stateOf(got)) + } + } + }) + + t.Run("a value kept from another source takes its row's later withdrawal", func(t *testing.T) { + // The SMART row first borrowed the Unraid inventory's reading, then + // reported its own standby without a temperature. + proxmox := func(registry *ResourceRegistry) { + registry.ingest(SourceProxmox, "pve1-node1-sdb", disk(40, diskinventory.Available("proxmox_node_smart")), identity) + } + for _, proxmoxFirst := range []bool{false, true} { + registry := NewRegistry(nil) + if proxmoxFirst { + proxmox(registry) + } + registry.ingest(SourceAgent, "agent-1-sdb", disk(72, diskinventory.Available("unraid")), identity) + registry.ingest(SourceAgent, "agent-1-sdb", disk(0, standby), identity) + if !proxmoxFirst { + proxmox(registry) + } + expect(t, registry, 72, standby) + } + }) + + t.Run("a kept value stays paired however often its row reports again", func(t *testing.T) { + withSMART := func(r Resource) Resource { + powerOnHours := int64(1000) + r.PhysicalDisk.SMART = &SMARTMeta{PowerOnHours: &powerOnHours} + return r + } + registry := NewRegistry(nil) + registry.ingest(SourceAgent, "agent-1-sdb", withSMART(disk(72, stopped)), identity) + registry.ingest(SourceAgent, "agent-1-sdb", disk(41, smartctl), identity) + registry.ingest(SourceAgent, "agent-1-sdb", disk(42, smartctl), identity) + expect(t, registry, 72, stopped) + }) + + t.Run("a kept legacy reading takes its row's later withdrawal", func(t *testing.T) { + legacy := func(temperature int, smart bool) Resource { + r := Resource{ + Type: ResourceTypePhysicalDisk, Name: "WDC", Status: StatusOnline, + PhysicalDisk: &PhysicalDiskMeta{DevPath: "/dev/sdb", Serial: "WD-BOUNDARY1", Temperature: temperature}, + } + if smart { + powerOnHours := int64(1000) + r.PhysicalDisk.SMART = &SMARTMeta{PowerOnHours: &powerOnHours} + } + return r + } + legacyStopped := diskinventory.Unavailable("", "host agent stopped reporting") + registry := NewRegistry(nil) + registry.ingest(SourceAgent, "agent-1-sdb", legacy(72, true), identity) + registry.ingest(SourceAgent, "agent-1-sdb", legacy(41, false), identity) + registry.ingest(SourceAgent, "agent-1-sdb", disk(0, legacyStopped), identity) + expect(t, registry, 72, legacyStopped) + }) + + t.Run("a kept copy gives way to the agent's later withdrawal", func(t *testing.T) { + withSMART := func(r Resource) Resource { + powerOnHours := int64(1000) + r.PhysicalDisk.SMART = &SMARTMeta{PowerOnHours: &powerOnHours} + return r + } + registry := NewRegistry(nil) + registry.ingest(SourceProxmox, "pve1-node1-sdb", withSMART(disk(72, smartctl)), identity) + registry.ingest(SourceProxmox, "pve1-node1-sdb", disk(40, smartctl), identity) + registry.ingest(SourceAgent, "agent-1-sdb", disk(0, standby), identity) + expect(t, registry, 72, standby) + }) + + t.Run("a row's earlier value is not current evidence", func(t *testing.T) { + withSMART := func(r Resource) Resource { + powerOnHours := int64(1000) + r.PhysicalDisk.SMART = &SMARTMeta{PowerOnHours: &powerOnHours} + return r + } + nodeSMART := diskinventory.Available("proxmox_node_smart") + registry := NewRegistry(nil) + registry.ingest(SourceProxmox, "pve1-node1-sdb", disk(72, nodeSMART), identity) + registry.ingest(SourceProxmox, "pve1-node1-sdb", disk(40, nodeSMART), identity) + registry.ingest(SourceAgent, "agent-1-sdb", withSMART(disk(72, stopped)), identity) + expect(t, registry, 72, stopped) + }) + + t.Run("a reported row is presented over a presented disk's stand-in", func(t *testing.T) { + registry := NewRegistry(nil) + registry.IngestResources([]Resource{presented(41, diskinventory.Unavailable("unraid", "host agent stopped reporting"))}) + registry.ingest(SourceAgent, "agent-1-sdb", disk(0, standby), identity) + registry.ingest(SourceProxmox, "pve1-node1-sdb", disk(41, smartctl), identity) + expect(t, registry, 41, standby) + }) + + t.Run("an agent's later withdrawal supersedes a presented disk's older one", func(t *testing.T) { + for _, withCopy := range []bool{false, true} { + registry := NewRegistry(nil) + registry.IngestResources([]Resource{presented(41, stopped)}) + registry.ingest(SourceAgent, "agent-1-sdb", disk(0, standby), identity) + if withCopy { + registry.ingest(SourceProxmox, "pve1-node1-sdb", disk(41, smartctl), identity) + } + if got := only(t, registry); got.Temperature != 41 || stateOf(got) != standby { + t.Errorf("with Proxmox copy=%v: temperature=%d state=%+v, want 41 under the agent's standby", withCopy, got.Temperature, stateOf(got)) + } + } + }) + + t.Run("two agents' withdrawals of one source settle the same in any order", func(t *testing.T) { + rows := []physicalDiskMergeRow{ + {SourceAgent, "agent-1-sdb", disk(0, standby)}, + {SourceAgent, "agent-2-sdb", disk(0, stopped)}, + {SourceProxmox, "pve1-node1-sdb", disk(41, smartctl)}, + } + var first diskinventory.FieldStatus + for i, order := range physicalDiskMergeOrders(len(rows)) { + got := mergePhysicalDiskRows(t, rows, order, identity) + if got.Temperature != 41 || diskinventory.TemperatureCollected(got.Temperature, got.Collection) { + t.Fatalf("order %v: temperature=%d state=%+v, want the copied 41 withdrawn", order, got.Temperature, stateOf(got)) + } + if i == 0 { + first = stateOf(got) + } else if stateOf(got) != first { + t.Errorf("order %v: state=%+v, but another order gave %+v", order, stateOf(got), first) + } + } + }) + + t.Run("a second agent's withdrawal is not the first agent's", func(t *testing.T) { + rows := []physicalDiskMergeRow{ + {SourceAgent, "agent-1-sdb", disk(41, smartctl)}, + {SourceAgent, "agent-2-sdb", disk(41, stopped)}, + } + for _, order := range physicalDiskMergeOrders(len(rows)) { + if got := mergePhysicalDiskRows(t, rows, order, identity); got.Temperature != 41 || stateOf(got) != smartctl { + t.Errorf("order %v: temperature=%d state=%+v, want the first agent's collected 41", order, got.Temperature, stateOf(got)) + } + } + }) +} diff --git a/internal/unifiedresources/types.go b/internal/unifiedresources/types.go index dd55de384d..fe8acee7c7 100644 --- a/internal/unifiedresources/types.go +++ b/internal/unifiedresources/types.go @@ -547,6 +547,11 @@ type PhysicalDiskMeta struct { Collection *diskinventory.CollectionStatus `json:"collection,omitempty"` SMART *SMARTMeta `json:"smart,omitempty"` Risk *PhysicalDiskRisk `json:"risk,omitempty"` + + // temperatureReadings is the registry's merge-time record of each row's + // own temperature reading. It is never serialized; see + // physicalDiskTemperatureReadings. + temperatureReadings *physicalDiskTemperatureReadings } // PhysicalDiskIOMeta preserves the cumulative kernel counters attributed to a diff --git a/pkg/diskinventory/status.go b/pkg/diskinventory/status.go index f4db88f0c6..a20575e8d4 100644 --- a/pkg/diskinventory/status.go +++ b/pkg/diskinventory/status.go @@ -15,6 +15,29 @@ const ( FieldMissing FieldState = "missing" ) +// LegacyHostAgentSource is the provenance recorded for a host agent field +// reported before collection provenance existed (agents before 6.2). Those +// agents send only readings they collected. +const LegacyHostAgentSource = "host_agent" + +// LegacyHostAgentStatus returns a host agent field's collection state with +// the legacy agent's provenance filled in. A report from before collection +// provenance carries no state, so a present reading was collected from +// LegacyHostAgentSource, and a state recorded for such a report without a +// source (its lease-expiry withdrawal) is that source's too. Monitoring +// stamps the legacy source on the Proxmox disk's copy of the reading and the +// unified-resources registry matches the agent's withdrawal to that copy, so +// both apply this one rule. +func LegacyHostAgentStatus(status FieldStatus, hasReading bool) FieldStatus { + switch { + case status.State == "" && hasReading: + return Available(LegacyHostAgentSource) + case status.State != "" && strings.TrimSpace(status.Source) == "": + status.Source = LegacyHostAgentSource + } + return status +} + // FieldStatus carries collection state and provenance for one disk signal. type FieldStatus struct { State FieldState `json:"state"` diff --git a/pkg/diskinventory/status_test.go b/pkg/diskinventory/status_test.go index b418eac9da..d3df00ecfb 100644 --- a/pkg/diskinventory/status_test.go +++ b/pkg/diskinventory/status_test.go @@ -87,3 +87,24 @@ func TestTemperatureCollectedOnlyForAReadingTakenByThisObservation(t *testing.T) } } } + +func TestLegacyHostAgentStatusFillsTheLegacyAgentProvenance(t *testing.T) { + stopped := "host agent stopped reporting" + for _, tc := range []struct { + name string + status FieldStatus + hasReading bool + want FieldStatus + }{ + {"stateless reading", FieldStatus{}, true, Available(LegacyHostAgentSource)}, + {"no reading", FieldStatus{}, false, FieldStatus{}}, + {"sourceless withdrawal", Unavailable("", stopped), true, Unavailable(LegacyHostAgentSource, stopped)}, + {"sourceless withdrawal without a reading", Unavailable("", stopped), false, Unavailable(LegacyHostAgentSource, stopped)}, + {"current agent state", Unavailable("smartctl", "disk is in standby"), false, Unavailable("smartctl", "disk is in standby")}, + {"current agent reading", Available("smartctl"), true, Available("smartctl")}, + } { + if got := LegacyHostAgentStatus(tc.status, tc.hasReading); got != tc.want { + t.Errorf("%s: LegacyHostAgentStatus = %+v, want %+v", tc.name, got, tc.want) + } + } +}