diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index a089e0f234..2625c04957 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -401,6 +401,20 @@ wire contract and Agent authority are unchanged; `internal/monitoring/physical_disk_roundtrip_test.go` and `TestPhysicalDiskAgentSMARTReportedStaysInternal` in `internal/models/deepcopy_test.go` pin the marker. +A PVE disk the operator split from the Agent's disk (report-merge on the merged +disk) is paired with no SMART row of the linked Agent and, from the first full +disk poll that matches the Agent's row, shows none of the Agent's temperature, +including the node sensor reading the node poll took from that Agent's report +while it is reporting, so the Agent's `CheckHost` owns the temperature alert of +the Agent's own disk only and the PVE disk check has no Agent-supplied reading +of the Proxmox disk to alert on beside it: the split disk is marked `AgentSMARTSplit` (internal poll +evidence on `models.PhysicalDisk`, never serialized) instead of +`AgentSMARTReported`. Report admission, the SMART wire contract and Agent +authority are unchanged: the Agent still reports the row, and the split only +decides whose disk it is on the server; +`TestOperatorSplitKeepsAgentSMARTOffTheProxmoxDisk` in +`internal/monitoring/physical_disk_roundtrip_test.go` and +`TestPhysicalDiskAgentSMARTReportedStaysInternal` pin it. Assistant historical metric wiring uses the current monitor's retained store and registry metrics coordinates. Historical reads do not alter enrollment, diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index df749fdb7f..d74eb5f09d 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -2434,6 +2434,48 @@ merge, so on a mock estate every hot disk on an online node alerts. `internal/monitoring/physical_disk_roundtrip_test.go` pin the marker, the collected reading, agent ownership and exclusion. +A disk the operator split from the agent's disk (report-merge on the merged +disk) is not paired with the agent's row. `mergeHostAgentSMARTIntoDisks` asks +the resource store, through `diskAgentSplitStore` +(`unifiedresources.MonitorAdapter.ProxmoxDiskAgentSMARTSplit`), once per disk +it matches a row to, on the full poll and on the skipped polls between, and a +split disk takes nothing from the row: no attributes, I/O, health, identity, +type or temperature, and no `AgentSMARTReported`. It takes no agent-supplied +node-sensor temperature either: the node poll takes the node's SMART list from +the linked agent when the agent's report carries a usable temperature +(`mergeTemperatureData`; `hostSuppliesNodeSMARTTemperatures` mirrors that +rule), and its legacy NVMe list is matched by order, so +`dropNodeSensorTemperature` removes a reading `mergeNVMeTempsIntoDisks` took +from either, leaving the PVE disk check no such reading to judge beside the +agent's own alert, while a reading from the SSH collector's list stays (the +rule needs the agent still reporting). The +split disk is still evaluated by `checkPhysicalDiskAlerts` whenever its node +is polled. It is marked +`AgentSMARTSplit` (internal poll evidence, never serialized), so the poll's +retention of unavailable evidence restores nothing into it. The decider is +asked about the observation under its own ID, as the registry holds it. The +Proxmox-query-failure fallback omits the rows the operator split from the disk +in their slot, so while the query fails that disk's Proxmox record is not +listed (a node every row of which is split keeps its last records, and is not +evaluated by `checkPhysicalDiskAlerts`, as any unqueried node). A store that holds no +decisions, or none for the pair, leaves the merge as it was. The decision +comes from the registry generation the monitor last built, so a split applies +from the first full disk poll that collects the node from Proxmox and matches +the agent's row after the rebuild that loaded it; the skipped polls before then +mark the record and drop its node-sensor temperature but leave what an earlier +poll merged into it. +`TestMergeHostAgentSMARTIntoDisksLeavesDisksTheOperatorSplitAlone`, +`TestHostAgentSMARTFallbackSkipsRowsTheOperatorSplit`, +`TestFailedDiskQueryListsOnlyTheRowsTheOperatorDidNotSplit`, +`TestMergeHostAgentSMARTIntoDisksKeepsSSHNodeSensorReadingOfSplitDisk`, +`TestDropNodeSensorTemperatureRemovesOnlyAgentSuppliedReadings`, +`TestMergeNVMeTempsLabelsReadingsWithTheNodeSensorSources`, +`TestHostSuppliesNodeSMARTTemperaturesFollowsUsableReadings` and +`TestOperatorSplitKeepsAgentSMARTOffTheProxmoxDisk` in +`internal/monitoring/physical_disk_roundtrip_test.go` pin the merge, the +fallback and the poller end to end, and `TestProxmoxDiskAlertsRunOnMergedDiskState` +in `canonical_guardrails_test.go` pins the signature and call. + Switching mock mode fences the alert evaluations that read mode-dependent data (`mockModeFence`, `internal/monitoring/mock_mode_fence.go`). `GetState`, the fixture graph, the unified read view, the recovery rollups and the connection diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 33200622df..eb6b8fd3a7 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -7252,13 +7252,68 @@ every merged source. The monitor rebuild and a resources API registry built from snapshot apply the split on their next build; the registry seeded from the monitor's listing follows once the monitor has rebuilt. -What the split does not reach. The PVE disk poller's -`mergeHostAgentSMARTIntoDisks` pairs the linked agent's SMART rows with -Proxmox disks by WWN, serial or path before the registry sees them and reads -no exclusions, so the split Proxmox row still carries the agent's SMART -attributes and I/O, its health where it reports a failure or Proxmox none, -and any serial, WWN, type or temperature it filled, and the agent keeps the -disk's temperature alert while it reports (`AgentSMARTReported`). Two split +The PVE disk poller honours the split too. `mergeHostAgentSMARTIntoDisks` +pairs the linked agent's SMART rows with Proxmox disks by WWN, serial or path +before the registry sees either, and asks +`MonitorAdapter.ProxmoxDiskAgentSMARTSplit` +(`internal/unifiedresources/physical_disk_agent_split.go`) about each pair it +is about to make. The answer is `physicalDiskSplitLocked`'s, applied to the +agent's disk as the registry generation holds it (as its row would key it +when the generation does not hold it) and to the Proxmox observation by its +slot's source-specific candidate ID, so the exclusions that keep the +registry's rows apart keep the poller's pair apart, in the same forms: one +naming only the agent's candidate splits nothing, a replacement in the slot +inherits no split recorded against the disk it replaced, and a manual link +that leaves the generation holding both observations as one disk is the +operator's later decision, so the poller pairs them again while it stands. The +decision reads the exclusions and that mapping, never what the pairing would +fill into the Proxmox row, whose serial it re-keys. A split Proxmox row takes +nothing from the agent's row: not its SMART attributes or I/O, health, serial, +WWN, type, controller, pool or temperature, whether from the row or from the +node's sensor lists. The node poll takes the SMART list from the linked agent +while the agent is reporting and its report carries a usable temperature (the +SSH collector's otherwise), and `mergeNVMeTempsIntoDisks` matches a list's rows +to the disk by the same identity, so `dropNodeSensorTemperature` drops a +reading taken from the agent's list, and the legacy NVMe list's guess by order, +which names no disk, while a reading from the SSH collector's list stays. The +rule is read from the agent's report as the disk poll sees it, so a report that +changed since the node poll can misjudge one poll. Nor does the disk +keep the agent's claim on its temperature alert (`AgentSMARTReported`): the +agent's `CheckHost` alerts on the agent's own disk and the PVE disk check has +no agent-supplied reading of the split disk to judge, so one reading never +alerts twice, and Proxmox's own health and wearout verdicts of the disk are +evaluated as for any disk. A refused row never makes another row match. The +record is marked `AgentSMARTSplit`, so the poll's evidence retention +(`preserveUnavailablePhysicalDiskEvidence`) restores nothing into it: nothing +records which retained value Proxmox collected and which the agent filled, so +its serial or ZFS pool is absent for a poll that cannot collect it and returns +with the next that can, and a Proxmox serial that goes unreported for a poll +can move the disk to a WWN- or slot-keyed ID, and its history to that key, +until it returns. The decider is asked once per matched row, about the +observation under its own ID, the key the registry holds it by, so the poller +follows the registry exactly: a split recorded under another key of the same +disk (a controller member's stand-in is keyed by its target, a Proxmox poll's +record by device alone) names an observation the other key's rows do not +carry, as for the registry. When the Proxmox disk query fails, +`physicalDisksFromHostAgentSMART` stands in for the node's disks except for +the rows the operator split from the Proxmox disk in their slot, judged under +the stand-in's own ID. Such a disk gets no stand-in, which would be the +agent's disk again, so while the query fails its Proxmox record is not listed +unless every row of the node is split, when the node keeps its last records as +any node Proxmox cannot be queried for does. + +What the split does not reach. The poller applies a split on the first full +disk poll that collects the node from Proxmox and matches the agent's row +after the registry generation holds it. Until then a Proxmox record already +merged keeps the agent's data (the next full poll, five minutes by default, +`PhysicalDiskPollingMinutes`): the skipped polls between mark the record split +and drop its node-sensor temperature but leave the rest as it is, a node +Proxmox cannot be queried for keeps its last records, and a poll in which +the agent does not list the disk marks nothing, so retention restores the +earlier record's temperature, I/O, controller, target, serial and pool as +last-known evidence. The query-failure fallback judges its stand-in row by the +row's own identity, so it does not apply a split that names identity only the +Proxmox disk carries (an agent disk in standby reports no serial). Two split rows that share a serial read one serial-keyed history (`PhysicalDiskMetaMetricID`), and a PVE disk alert lifecycle row whose recorded identity matches both, with no WWN only one of them shares, stays @@ -7287,7 +7342,13 @@ the rebuilt listing or built from the snapshot, plain and for presentation; then a relink of the split rows, the exclusions report-merge records on the relinked disk (the link's own pair and both candidates), the agent's disk in standby, a same-serial disk appearing on another machine, and a split -recorded while such a disk was present that holds once it leaves. +recorded while such a disk was present that holds once it leaves. At each of +those steps the test also asks `ProxmoxDiskAgentSMARTSplit` what the poller +would, and `TestOperatorSplitKeepsAgentSMARTOffTheProxmoxDisk` in +`internal/monitoring/physical_disk_roundtrip_test.go` drives the poller itself +through the same four requests for a SAS disk paired by path and an NVMe disk +paired by serial, with an open PVE temperature alert, the health alert the +agent's failure raised, and a failing Proxmox disk query. ### Seeded slot mapping refuses a replaced disk diff --git a/internal/models/deepcopy_test.go b/internal/models/deepcopy_test.go index e412d86406..9cbaaf038f 100644 --- a/internal/models/deepcopy_test.go +++ b/internal/models/deepcopy_test.go @@ -515,21 +515,24 @@ func TestCloneHostXCPNGInventoryIsolation(t *testing.T) { } } -// AgentSMARTReported is internal poll evidence: a state snapshot copy keeps it -// for the alert check, and it never reaches an API or websocket payload. +// AgentSMARTReported and AgentSMARTSplit are internal poll evidence: a state +// snapshot copy keeps them for the alert check and the poll's evidence +// retention, and neither reaches an API or websocket payload. func TestPhysicalDiskAgentSMARTReportedStaysInternal(t *testing.T) { - disk := PhysicalDisk{ID: "lab-pve1-sda", DevPath: "/dev/sda", AgentSMARTReported: true} - if clone := clonePhysicalDisk(disk); !clone.AgentSMARTReported { - t.Fatalf("clone dropped AgentSMARTReported") + disk := PhysicalDisk{ID: "lab-pve1-sda", DevPath: "/dev/sda", AgentSMARTReported: true, AgentSMARTSplit: true} + if clone := clonePhysicalDisk(disk); !clone.AgentSMARTReported || !clone.AgentSMARTSplit { + t.Fatalf("clone dropped AgentSMARTReported/AgentSMARTSplit: %+v", clone) } - if clones := clonePhysicalDisks([]PhysicalDisk{disk}); !clones[0].AgentSMARTReported { - t.Fatalf("slice clone dropped AgentSMARTReported") + if clones := clonePhysicalDisks([]PhysicalDisk{disk}); !clones[0].AgentSMARTReported || !clones[0].AgentSMARTSplit { + t.Fatalf("slice clone dropped AgentSMARTReported/AgentSMARTSplit: %+v", clones[0]) } payload, err := json.Marshal(disk) if err != nil { t.Fatal(err) } - if strings.Contains(strings.ToLower(string(payload)), "agentsmartreported") { - t.Fatalf("AgentSMARTReported leaked into JSON: %s", payload) + for _, field := range []string{"agentsmartreported", "agentsmartsplit"} { + if strings.Contains(strings.ToLower(string(payload)), field) { + t.Fatalf("%s leaked into JSON: %s", field, payload) + } } } diff --git a/internal/models/models.go b/internal/models/models.go index ceee0c30f3..6196eb682a 100644 --- a/internal/models/models.go +++ b/internal/models/models.go @@ -2859,8 +2859,13 @@ type PhysicalDisk struct { // AgentSMARTReported marks a disk a linked host agent lists in its SMART // report, so the agent's CheckHost owns its temperature alert. Internal // poll evidence, set by the PVE disk poller's agent SMART merge. - AgentSMARTReported bool `json:"-"` - LastChecked time.Time `json:"lastChecked"` + AgentSMARTReported bool `json:"-"` + // AgentSMARTSplit marks a disk the operator split from the disk a linked + // host agent's SMART row describes, so the PVE disk poller pairs neither + // that row nor an earlier poll's record, which held its readings, with + // it. Internal poll evidence, set by the same merge. + AgentSMARTSplit bool `json:"-"` + LastChecked time.Time `json:"lastChecked"` } // PBSInstance represents a Proxmox Backup Server instance diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index 7d409f0fc0..f47fd60b1a 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -2268,7 +2268,7 @@ func TestProxmoxDiskAlertsRunOnMergedDiskState(t *testing.T) { { file: "monitor.go", snippets: []string{ - "func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.Node, hosts []models.Host) []models.PhysicalDisk {", + "func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.Node, hosts []models.Host, splits diskAgentSplitStore) []models.PhysicalDisk {", "deriveWearoutFromSMARTAttributes(matched.Attributes)", "storagehealth.RemainingLifeFromPercentageUsed(*attrs.PercentageUsed)", "shouldUseHostAgentPhysicalDiskHealth(updated[i].Health, matched.Health)", @@ -2278,7 +2278,7 @@ func TestProxmoxDiskAlertsRunOnMergedDiskState(t *testing.T) { { file: "monitor_pve.go", snippets: []string{ - "allDisks = mergeHostAgentSMARTIntoDisks(allDisks, nodesFromState, hosts)", + "allDisks = mergeHostAgentSMARTIntoDisks(allDisks, nodesFromState, hosts, splits)", "m.checkPhysicalDiskAlerts(inst, disk, diskExcludeByNode[disk.Node])", "m.alertManager.CheckDiskHealth(instance, disk.Node, proxmoxDiskFromPhysicalDisk(disk))", "func proxmoxDiskFromPhysicalDisk(disk models.PhysicalDisk) proxmox.Disk {", diff --git a/internal/monitoring/issue1595_collection_trust_test.go b/internal/monitoring/issue1595_collection_trust_test.go index e3de00161e..79f5366ca5 100644 --- a/internal/monitoring/issue1595_collection_trust_test.go +++ b/internal/monitoring/issue1595_collection_trust_test.go @@ -148,7 +148,7 @@ func TestIssue1595SASTopologySurvivesMergeRegistryAndReadState(t *testing.T) { } } - merged := mergeHostAgentSMARTIntoDisks(providerDisks, []models.Node{node}, []models.Host{host}) + merged := mergeHostAgentSMARTIntoDisks(providerDisks, []models.Node{node}, []models.Host{host}, nil) if len(merged) != 24 { t.Fatalf("merged disk count = %d, want 24", len(merged)) } @@ -286,6 +286,7 @@ func TestTrustedSMARTSerialPromotionDoesNotRewriteSATAOrNVMeIdentity(t *testing. disks, []models.Node{{Name: "node", LinkedAgentID: "agent"}}, hosts, + nil, )[0] if got.Serial != "PROVIDER-SERIAL" || got.Type != diskType { t.Fatalf("%s identity changed during SAS remediation: %+v", diskType, got) @@ -948,6 +949,7 @@ func TestHostAgentSMARTMergeRefusesPathMatchAcrossContradictingIdentity(t *testi []models.Host{{ID: "agent-1", Sensors: models.HostSensorSummary{ SMART: append([]models.HostDiskSMART{tc.smart}, tc.others...), }}}, + nil, )[0] if !tc.merged { if got.Serial != tc.disk.Serial || got.WWN != tc.disk.WWN || got.Type != tc.disk.Type || diff --git a/internal/monitoring/merge_temps_test.go b/internal/monitoring/merge_temps_test.go index a95ada4b46..87f5f884a0 100644 --- a/internal/monitoring/merge_temps_test.go +++ b/internal/monitoring/merge_temps_test.go @@ -394,7 +394,7 @@ func TestMergeHostAgentSMARTIntoDisks_EnrichesMissingIdentityAndZFSPool(t *testi }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if len(result) != 1 { t.Fatalf("got %d disks, want 1", len(result)) } @@ -465,7 +465,7 @@ func TestMergeHostAgentSMARTIntoDisks_PreservesExistingProxmoxIdentity(t *testin }, } - got := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts)[0] + got := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil)[0] if got.Model != "Proxmox Model" || got.Serial != "PVE-SERIAL" || got.WWN != "0x5000c500a1b2c3d4" || got.Type != "sas" { t.Fatalf("existing Proxmox identity was overwritten: %+v", got) } diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 0f732f9327..72aff4182a 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -390,7 +390,7 @@ func mergeNVMeTempsIntoDisks(disks []models.PhysicalDisk, nodes []models.Node) [ return diskinventory.IsUsableHardwareID(candidate.WWN) && strings.EqualFold(candidate.WWN, updated[i].WWN) }); temp != nil && temp.Temperature > 0 && !temp.StandbySkipped { - setPhysicalDiskTemperature(&updated[i], temp.Temperature, "proxmox_node_smart") + setPhysicalDiskTemperature(&updated[i], temp.Temperature, proxmoxNodeSMARTTemperatureSource) log.Debug(). Str("disk", updated[i].DevPath). Str("wwn", updated[i].WWN). @@ -405,7 +405,7 @@ func mergeNVMeTempsIntoDisks(disks []models.PhysicalDisk, nodes []models.Node) [ return diskinventory.IsUsableHardwareID(candidate.Serial) && strings.EqualFold(candidate.Serial, updated[i].Serial) }); temp != nil && temp.Temperature > 0 && !temp.StandbySkipped { - setPhysicalDiskTemperature(&updated[i], temp.Temperature, "proxmox_node_smart") + setPhysicalDiskTemperature(&updated[i], temp.Temperature, proxmoxNodeSMARTTemperatureSource) log.Debug(). Str("disk", updated[i].DevPath). Str("serial", updated[i].Serial). @@ -421,7 +421,7 @@ func mergeNVMeTempsIntoDisks(disks []models.PhysicalDisk, nodes []models.Node) [ normalizedTempDev := normalizeSMARTDeviceIdentifier(candidate.Device) return normalizedTempDev != "" && normalizedTempDev == normalizedDevPath }); temp != nil && temp.Temperature > 0 && !temp.StandbySkipped { - setPhysicalDiskTemperature(&updated[i], temp.Temperature, "proxmox_node_smart") + setPhysicalDiskTemperature(&updated[i], temp.Temperature, proxmoxNodeSMARTTemperatureSource) log.Debug(). Str("disk", updated[i].DevPath). Int("temp", temp.Temperature). @@ -458,7 +458,7 @@ func mergeNVMeTempsIntoDisks(disks []models.PhysicalDisk, nodes []models.Node) [ continue } - setPhysicalDiskTemperature(&updated[diskIdx], int(math.Round(tempVal)), "proxmox_node_nvme") + setPhysicalDiskTemperature(&updated[diskIdx], int(math.Round(tempVal)), proxmoxNodeNVMeTemperatureSource) log.Debug(). Str("disk", updated[diskIdx].DevPath). Int("temp", updated[diskIdx].Temperature). @@ -486,6 +486,55 @@ func uniqueDiskTemperatureMatch(temperatures []models.DiskTemp, predicate func(m return &temperatures[matchIndex] } +// Collection sources of a disk temperature mergeNVMeTempsIntoDisks took from +// the node's sensor lists. +const ( + proxmoxNodeSMARTTemperatureSource = "proxmox_node_smart" + proxmoxNodeNVMeTemperatureSource = "proxmox_node_nvme" +) + +// hostSuppliesNodeSMARTTemperatures reports whether the node's SMART sensor +// list is this host's report: the node poll takes the linked agent's list when +// it carries a usable temperature and the SSH collector's otherwise +// (mergeTemperatureData in host_agent_temps.go). An agent whose lease lapsed +// supplies nothing, so callers also require it to be reporting. +func hostSuppliesNodeSMARTTemperatures(host models.Host) bool { + for _, row := range host.Sensors.SMART { + if row.Temperature > 0 && !row.Standby { + return true + } + } + return false +} + +// dropNodeSensorTemperature removes a reading mergeNVMeTempsIntoDisks took +// from the node's sensor lists, for a disk the operator split from the +// linked agent's. The SMART list carries rows of the agent's disks when the +// agent supplies it (smartFromAgent), so a reading from it is the agent's row +// of the disk this one was split from; an SSH-supplied list is the node's own +// and stays. The legacy NVMe list names no disk, only an order, so its guess +// is dropped either way. A reading from another source, which Proxmox itself +// never supplies, is not touched. +func dropNodeSensorTemperature(disk *models.PhysicalDisk, smartFromAgent bool) { + if disk.Collection == nil { + return + } + switch disk.Collection.Temperature.Source { + case proxmoxNodeSMARTTemperatureSource: + if !smartFromAgent { + return + } + case proxmoxNodeNVMeTemperatureSource: + default: + return + } + disk.Temperature = 0 + // The copy shares its collection with the caller's slice. + collection := diskinventory.CloneStatus(disk.Collection) + collection.Temperature = diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature") + disk.Collection = collection +} + func setPhysicalDiskTemperature(disk *models.PhysicalDisk, temperature int, source string) { if disk == nil || temperature <= 0 { return @@ -497,10 +546,34 @@ func setPhysicalDiskTemperature(disk *models.PhysicalDisk, temperature int, sour disk.Collection.Temperature = diskinventory.Available(source) } +// diskAgentSplitStore is implemented by resource stores that hold the +// operator's merge exclusions (unifiedresources.MonitorAdapter). It reports +// whether the operator split a Proxmox disk from the disk a linked host +// agent reports in one SMART row. +type diskAgentSplitStore interface { + ProxmoxDiskAgentSMARTSplit(disk models.PhysicalDisk, host models.Host, smart models.HostDiskSMART) bool +} + +// proxmoxDiskAgentSplits returns the resource store's split decisions, or nil +// when the store holds none. +func (m *Monitor) proxmoxDiskAgentSplits() diskAgentSplitStore { + if m == nil { + return nil + } + m.mu.RLock() + store := m.resourceStore + m.mu.RUnlock() + splits, _ := store.(diskAgentSplitStore) + return splits +} + // mergeHostAgentSMARTIntoDisks merges SMART temperature data from linked host agents // into physical disks for Proxmox nodes. This allows disk temps collected by the -// pulse-agent running on a PVE node to populate the Physical Disks view. -func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.Node, hosts []models.Host) []models.PhysicalDisk { +// pulse-agent running on a PVE node to populate the Physical Disks view. A disk +// the operator split from the agent's disk its matching row describes (splits) +// takes nothing from that row, nor the node-sensor reading that stands in for +// it, and is marked AgentSMARTSplit instead. +func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.Node, hosts []models.Host, splits diskAgentSplitStore) []models.PhysicalDisk { if len(disks) == 0 || len(nodes) == 0 || len(hosts) == 0 { return disks } @@ -513,6 +586,7 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No // Build a map of node name to linked host's SMART data smartByNodeName := make(map[string][]models.HostDiskSMART) + hostByNodeName := make(map[string]*models.Host) reportingNodeNames := make(map[string]bool) for _, node := range nodes { if node.LinkedAgentID == "" { @@ -523,6 +597,7 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No continue } smartByNodeName[node.Name] = host.Sensors.SMART + hostByNodeName[node.Name] = host // Only an agent still reporting owns its disks' temperature alerts; // a silent one's retained rows still enrich the disk. if !strings.EqualFold(strings.TrimSpace(host.Status), "offline") { @@ -614,6 +689,16 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No if matched == nil { continue } + // The operator split this disk from the agent's (report-merge on the + // merged disk), so the row describes another disk: its readings, + // identity and temperature alert stay the agent's, and the PVE disk + // check judges what Proxmox reported. The refusal never pairs another + // row, as in the registry's linked disk join. + if host := hostByNodeName[updated[i].Node]; splits != nil && splits.ProxmoxDiskAgentSMARTSplit(updated[i], *host, *matched) { + updated[i].AgentSMARTSplit = true + dropNodeSensorTemperature(&updated[i], reportingNodeNames[updated[i].Node] && hostSuppliesNodeSMARTTemperatures(*host)) + continue + } // The linked agent reports this disk, so while it keeps reporting its // CheckHost owns the disk's temperature alert and the PVE disk check // stands aside. Once its lease lapses, CheckHost no longer alerts on diff --git a/internal/monitoring/monitor_integration_test.go b/internal/monitoring/monitor_integration_test.go index db2dd0d5ad..9fec703fc3 100644 --- a/internal/monitoring/monitor_integration_test.go +++ b/internal/monitoring/monitor_integration_test.go @@ -13,16 +13,16 @@ import ( func TestMergeHostAgentSMARTIntoDisks_EmptyInputs(t *testing.T) { // All combinations of empty inputs should return disks unchanged disks := []models.PhysicalDisk{{ID: "d1"}} - if got := mergeHostAgentSMARTIntoDisks(nil, nil, nil); got != nil { + if got := mergeHostAgentSMARTIntoDisks(nil, nil, nil, nil); got != nil { t.Error("nil disks should return nil") } - if got := mergeHostAgentSMARTIntoDisks(disks, nil, nil); len(got) != 1 { + if got := mergeHostAgentSMARTIntoDisks(disks, nil, nil, nil); len(got) != 1 { t.Error("nil nodes/hosts should return disks unchanged") } - if got := mergeHostAgentSMARTIntoDisks(disks, []models.Node{}, nil); len(got) != 1 { + if got := mergeHostAgentSMARTIntoDisks(disks, []models.Node{}, nil, nil); len(got) != 1 { t.Error("empty nodes should return disks unchanged") } - if got := mergeHostAgentSMARTIntoDisks(disks, []models.Node{{Name: "n1"}}, nil); len(got) != 1 { + if got := mergeHostAgentSMARTIntoDisks(disks, []models.Node{{Name: "n1"}}, nil, nil); len(got) != 1 { t.Error("nil hosts should return disks unchanged") } } @@ -44,7 +44,7 @@ func TestMergeHostAgentSMARTIntoDisks_MatchByWWN(t *testing.T) { }, }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Temperature != 42 { t.Errorf("expected temp 42 via WWN match, got %d", result[0].Temperature) } @@ -67,7 +67,7 @@ func TestMergeHostAgentSMARTIntoDisks_MatchBySerial(t *testing.T) { }, }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Temperature != 38 { t.Errorf("expected temp 38 via serial match (case-insensitive), got %d", result[0].Temperature) } @@ -90,7 +90,7 @@ func TestMergeHostAgentSMARTIntoDisks_MatchByDevPath(t *testing.T) { }, }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Temperature != 35 { t.Errorf("expected temp 35 via device path match (strip /dev/), got %d", result[0].Temperature) } @@ -113,7 +113,7 @@ func TestMergeHostAgentSMARTIntoDisks_SkipsStandby(t *testing.T) { }, }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Temperature != 0 { t.Error("standby disk should not have its temperature merged") } @@ -139,7 +139,7 @@ func TestMergeHostAgentSMARTIntoDisks_MergesSMARTAttributes(t *testing.T) { }, }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].SmartAttributes == nil { t.Fatal("SMART attributes should be merged") } @@ -174,7 +174,7 @@ func TestMergeHostAgentSMARTIntoDisks_DerivesWearoutFromSMARTAttributes(t *testi }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Wearout != 94 { t.Fatalf("expected wearout derived from PercentageUsed, got %d", result[0].Wearout) } @@ -208,7 +208,7 @@ func TestMergeHostAgentSMARTIntoDisks_SMARTFailureAndWearoutOverrideCoarsePVEVal }, }} - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Health != "FAILED" { t.Fatalf("SMART failure was suppressed by PVE health: %+v", result[0]) } @@ -227,7 +227,7 @@ func TestMergeHostAgentSMARTIntoDisks_DoesNotReplaceFailureWithPassed(t *testing }, }} - if got := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts)[0].Health; got != "FAILED" { + if got := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil)[0].Health; got != "FAILED" { t.Fatalf("PASSED replaced an existing failure: %q", got) } } @@ -266,7 +266,7 @@ func TestMergeHostAgentSMARTIntoDisks_DuplicateIdentityRequiresUniqueTopology(t }, }} - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Temperature != 47 || result[0].Health != "PASSED" { t.Fatalf("topology-specific SMART evidence was not selected: %+v", result[0]) } @@ -274,7 +274,7 @@ func TestMergeHostAgentSMARTIntoDisks_DuplicateIdentityRequiresUniqueTopology(t disks[0].Controller = "" disks[0].Target = "" disks[0].DevPath = "/dev/unknown" - result = mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result = mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Temperature != 0 || result[0].Health != "UNKNOWN" { t.Fatalf("ambiguous SMART identity was guessed: %+v", result[0]) } @@ -298,7 +298,7 @@ func TestMergeHostAgentSMARTIntoDisks_FillsMissingHealthFromHostSMART(t *testing }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Health != "FAILED" { t.Fatalf("expected host SMART health to fill unknown disk health, got %q", result[0].Health) } @@ -321,7 +321,7 @@ func TestMergeHostAgentSMARTIntoDisks_NoAgentLink(t *testing.T) { }, }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Temperature != 0 { t.Error("unlinked nodes should not get host agent temperatures") } @@ -344,7 +344,7 @@ func TestMergeHostAgentSMARTIntoDisks_PreservesExistingTemp(t *testing.T) { }, }, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Temperature != 50 { t.Errorf("existing temperature should be preserved, got %d", result[0].Temperature) } @@ -360,7 +360,7 @@ func TestMergeHostAgentSMARTIntoDisks_OriginalUnchanged(t *testing.T) { SMART: []models.HostDiskSMART{{Serial: "SER1", Temperature: 42}}, }}, } - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if disks[0].Temperature != 0 { t.Error("original disk slice should not be mutated") } diff --git a/internal/monitoring/monitor_pve.go b/internal/monitoring/monitor_pve.go index f563b5276d..25a6d5e1b0 100644 --- a/internal/monitoring/monitor_pve.go +++ b/internal/monitoring/monitor_pve.go @@ -911,7 +911,7 @@ func (m *Monitor) maybePollPhysicalDisksAsync( nodes := nodesForInstanceFromReadState(readState, instanceName) hosts := hostsFromReadState(readState) updated := mergeNVMeTempsIntoDisks(existing, nodes) - updated = mergeHostAgentSMARTIntoDisks(updated, nodes, hosts) + updated = mergeHostAgentSMARTIntoDisks(updated, nodes, hosts, m.proxmoxDiskAgentSplits()) m.state.UpdatePhysicalDisks(instanceName, updated) } return @@ -950,6 +950,7 @@ func (m *Monitor) maybePollPhysicalDisksAsync( nodesFromState := nodesForInstanceFromReadState(readState, inst) hosts := hostsFromReadState(readState) existingDisks := physicalDisksForInstanceFromReadState(readState, inst) + splits := m.proxmoxDiskAgentSplits() existingDisksMap := make(map[string]models.PhysicalDisk, len(existingDisks)) for _, disk := range existingDisks { if disk.Instance == inst { @@ -976,12 +977,14 @@ func (m *Monitor) maybePollPhysicalDisksAsync( // Proxmox disk query fails can still populate its physical disks from // the agent's smartctl view (#1516). smartByNode := make(map[string][]models.HostDiskSMART) + linkedHostByNode := make(map[string]models.Host) for _, n := range nodesFromState { if n.LinkedAgentID == "" || n.Instance != inst { continue } linkedNodes[n.Name] = true if linkedHost, ok := hostByID[n.LinkedAgentID]; ok { + linkedHostByNode[n.Name] = linkedHost if len(linkedHost.DiskExclude) > 0 { diskExcludeByNode[n.Name] = linkedHost.DiskExclude } @@ -1044,7 +1047,7 @@ func (m *Monitor) maybePollPhysicalDisksAsync( // exceed the API window PVE itself allows for disks/list, so // without this fallback their Physical Disks view empties // permanently (#1516). - if fallback := physicalDisksFromHostAgentSMART(inst, node.Node, smartByNode[node.Node]); len(fallback) > 0 { + if fallback := unsplitPhysicalDisksFromHostAgentSMART(inst, node.Node, linkedHostByNode[node.Node], smartByNode[node.Node], splits); len(fallback) > 0 { polledNodes[node.Node] = true allDisks = append(allDisks, fallback...) log.Info(). @@ -1145,8 +1148,14 @@ func (m *Monitor) maybePollPhysicalDisksAsync( } allDisks = mergeNVMeTempsIntoDisks(allDisks, nodesFromState) - allDisks = mergeHostAgentSMARTIntoDisks(allDisks, nodesFromState, hosts) + allDisks = mergeHostAgentSMARTIntoDisks(allDisks, nodesFromState, hosts, splits) for index := range allDisks { + // A split disk's earlier record held the agent's readings and + // identity until the split; retained, they would never leave it, + // and nothing records which retained value Proxmox collected. + if allDisks[index].AgentSMARTSplit { + continue + } if previous, ok := previousPhysicalDiskEvidence(allDisks[index], existingDisks); ok { allDisks[index] = preserveUnavailablePhysicalDiskEvidence(allDisks[index], previous) } @@ -1289,6 +1298,28 @@ func physicalDisksFromHostAgentSMART(inst, nodeName string, smartEntries []model return disks } +// unsplitPhysicalDisksFromHostAgentSMART is physicalDisksFromHostAgentSMART +// without the rows the operator split from the Proxmox disk in their slot. +// Such a row describes the agent's disk, not the slot's, and standing in for +// it would copy the agent's disk, readings and temperature alert included, +// into a second row that the PVE disk check alerts on as well. The stand-in +// is judged under its own ID, the key the registry holds it by once it is +// recorded. +func unsplitPhysicalDisksFromHostAgentSMART(inst, nodeName string, host models.Host, smartEntries []models.HostDiskSMART, splits diskAgentSplitStore) []models.PhysicalDisk { + if splits == nil { + return physicalDisksFromHostAgentSMART(inst, nodeName, smartEntries) + } + var disks []models.PhysicalDisk + for _, smart := range smartEntries { + for _, disk := range physicalDisksFromHostAgentSMART(inst, nodeName, []models.HostDiskSMART{smart}) { + if !splits.ProxmoxDiskAgentSMARTSplit(disk, host, smart) { + disks = append(disks, disk) + } + } + } + return disks +} + func cloneDiskIO(in *models.DiskIO) *models.DiskIO { if in == nil { return nil diff --git a/internal/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go index 41a48e0bf6..285dcd3eab 100644 --- a/internal/monitoring/physical_disk_roundtrip_test.go +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -3,6 +3,8 @@ package monitoring import ( "context" "encoding/json" + "errors" + "slices" "strconv" "sync" "testing" @@ -293,7 +295,7 @@ func TestMergeHostAgentSMARTIntoDisks_AgentWearoutDoesNotHideLowPVELife(t *testi }, }} - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Wearout != 0 { t.Fatalf("agent endurance reading raised the remaining life of a worn disk: wearout=%d", result[0].Wearout) } @@ -327,7 +329,7 @@ func TestMergeHostAgentSMARTIntoDisks_AgentWearoutFillsUnreportedPVELife(t *test }, }} - result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) if result[0].Wearout != 60 { t.Fatalf("agent endurance did not fill unreported PVE life: wearout=%d", result[0].Wearout) } @@ -535,7 +537,7 @@ func TestMergeHostAgentSMARTIntoDisks_LegacyAgentTemperatureFollowsLease(t *test Device: "/dev/sdb", Serial: "LEGACY1", Temperature: 38, }}}}) - reporting := mergeHostAgentSMARTIntoDisks([]models.PhysicalDisk{proxmoxDisk}, nodes, state.GetHosts())[0] + reporting := mergeHostAgentSMARTIntoDisks([]models.PhysicalDisk{proxmoxDisk}, nodes, state.GetHosts(), nil)[0] 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) } @@ -548,7 +550,7 @@ func TestMergeHostAgentSMARTIntoDisks_LegacyAgentTemperatureFollowsLease(t *test "full poll": proxmoxDisk, "skipped poll": reporting, } { - got := mergeHostAgentSMARTIntoDisks([]models.PhysicalDisk{disk}, nodes, state.GetHosts())[0] + got := mergeHostAgentSMARTIntoDisks([]models.PhysicalDisk{disk}, nodes, state.GetHosts(), nil)[0] if got.Temperature != 38 || got.Collection.Temperature.State != diskinventory.FieldUnavailable { t.Fatalf("%s: silent legacy agent temperature presented as collected: temp=%d collection=%+v", name, got.Temperature, got.Collection) } @@ -591,7 +593,7 @@ func TestSilentLegacyAgentCanonicalDiskFollowsItsWithdrawal(t *testing.T) { // 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())) + []models.PhysicalDisk{inventory}, state.GetSnapshot().Nodes, state.GetHosts(), nil)) canonical := func() (int, *diskinventory.CollectionStatus) { t.Helper() adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) @@ -791,6 +793,7 @@ type slotDiskPVEClient struct { fakeStorageClient mu sync.Mutex disk proxmox.Disk + err error } func (client *slotDiskPVEClient) setDisk(disk proxmox.Disk) { @@ -799,9 +802,20 @@ func (client *slotDiskPVEClient) setDisk(disk proxmox.Disk) { client.disk = disk } +// setError makes the disk query fail, as it does on a wide node that exceeds +// the API window (#1516), until it is cleared with nil. +func (client *slotDiskPVEClient) setError(err error) { + client.mu.Lock() + defer client.mu.Unlock() + client.err = err +} + func (client *slotDiskPVEClient) GetDisks(context.Context, string) ([]proxmox.Disk, error) { client.mu.Lock() defer client.mu.Unlock() + if client.err != nil { + return nil, client.err + } return []proxmox.Disk{client.disk}, nil } @@ -1146,7 +1160,7 @@ func TestMergeHostAgentSMARTIntoDisksMarksDisksTheAgentReports(t *testing.T) { {ID: "host-node3", Status: "offline", Sensors: models.HostSensorSummary{SMART: smart}}, } - merged := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + merged := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, nil) for i, want := range []bool{true, false, false, false} { if merged[i].AgentSMARTReported != want { t.Fatalf("%s/%s AgentSMARTReported = %v, want %v", merged[i].Node, merged[i].DevPath, merged[i].AgentSMARTReported, want) @@ -1159,7 +1173,7 @@ func TestMergeHostAgentSMARTIntoDisksMarksDisksTheAgentReports(t *testing.T) { t.Fatalf("a silent agent's retained row no longer enriches the disk: %+v", merged[3]) } - fallback := mergeHostAgentSMARTIntoDisks(physicalDisksFromHostAgentSMART("pve1", "node1", smart), nodes, hosts) + fallback := mergeHostAgentSMARTIntoDisks(physicalDisksFromHostAgentSMART("pve1", "node1", smart), nodes, hosts, nil) if len(fallback) != 1 || !fallback[0].AgentSMARTReported { t.Fatalf("a disk built from a reporting agent's SMART report is not the agent's: %+v", fallback) } @@ -1215,3 +1229,740 @@ func TestCheckPhysicalDiskAlertsRaisesProxmoxDiskTemperatureAlerts(t *testing.T) t.Fatalf("an excluded device raised a temperature alert") } } + +// An operator split of a Proxmox disk from the disk its linked agent reports +// (report-merge on the merged disk, the drawer's Split merged resource) holds +// for the PVE disk poller too. The poller paired the agent's SMART row with the +// Proxmox disk by WWN, serial or path before the registry saw either, and read +// no exclusions, so the registry kept two rows while the Proxmox record still +// carried the agent's attributes, I/O, health, serial, type and temperature, +// and the agent kept the disk's temperature alert (AgentSMARTReported). A split +// row carries what Proxmox reported, whatever an earlier poll merged into it, +// on a full poll and on the skipped polls between. That includes the +// temperature: a node with a linked agent takes its sensor lists from the +// agent, so the reading the node poll matches to the disk is the agent's row +// of the disk it was split from, and the disk neither shows it nor alerts on it +// a second time beside the agent's own alert. +func TestOperatorSplitKeepsAgentSMARTOffTheProxmoxDisk(t *testing.T) { + percentUsed, hours := 40, int64(1200) + agentSMART := func(device, model, serial, diskType string, temperature int) models.HostDiskSMART { + return models.HostDiskSMART{ + Device: device, Model: model, Serial: serial, Type: diskType, Health: "FAILED", Temperature: temperature, + IO: &models.DiskIO{Device: device, ReadBytes: 4096, WriteBytes: 8192}, + Attributes: &models.SMARTAttributes{PowerOnHours: &hours, PercentageUsed: &percentUsed}, + Collection: &diskinventory.CollectionStatus{ + Serial: diskinventory.Available("smartctl"), + Temperature: diskinventory.Available("smartctl"), + IO: diskinventory.Available("kernel_diskstats"), + }, + } + } + for _, shape := range []struct { + name string + agent models.HostDiskSMART + pve proxmox.Disk + // nodeTemp is the node sensor list's row for the disk. The node poll + // takes that list from the linked agent, so it is the agent's reading. + nodeTemp *models.DiskTemp + // What a paired record carries: the agent's serial and type, and the + // temperature it reports unless the node's sensors read the disk. + pairedSerial, pairedType string + pairedTemperature int + // The temperature a skipped poll reads back from the joined canonical + // disk, and what the split disk shows once the agent's reading is gone. + skippedTemperature, splitTemperature int + // Whether the PVE disk check alerts on the split disk's reading. + splitAlert bool + }{ + // Proxmox reports the SAS transport address as the serial (#1595), so + // only the device path pairs the rows, and the pairing promotes the + // agent's serial and transport over Proxmox's. + { + name: "sas-path", + agent: agentSMART("/dev/sda", "ST4000NM0023", "Z1Z0ABCD", "sas", 70), + pve: proxmox.Disk{DevPath: "/dev/sda", Model: "ST4000NM0023", Serial: "5000c500a1b2c3d4", Type: "hdd", + Health: "PASSED", Wearout: 100, Size: 4000787030016}, + pairedSerial: "Z1Z0ABCD", pairedType: "sas", pairedTemperature: 70, skippedTemperature: 70, + }, + // One serial on both sides, and the node's sensor list reads the disk, + // so it carries a reading and can raise a temperature alert. + { + name: "matching-serial", + agent: agentSMART("/dev/nvme0n1", "WD Black SN850X", "S6B0NL0W123456", "nvme", 40), + pve: proxmox.Disk{DevPath: "/dev/nvme0n1", Model: "WD Black SN850X", Serial: "S6B0NL0W123456", Type: "nvme", + Health: "PASSED", Wearout: 100, Size: 1000204886016}, + nodeTemp: &models.DiskTemp{Device: "/dev/nvme0n1", Serial: "S6B0NL0W123456", Type: "nvme", Temperature: 85}, + pairedSerial: "S6B0NL0W123456", pairedType: "nvme", pairedTemperature: 85, skippedTemperature: 40, + }, + // The agent's report carries no usable temperature, so the node's SMART + // list is the SSH collector's: the reading is the node's own, survives the + // split, and the PVE disk check, no longer deferring to the agent, alerts. + { + name: "ssh-sensors", + agent: agentSMART("/dev/nvme1n1", "WD Black SN770", "S7SSH0000001", "nvme", 0), + pve: proxmox.Disk{DevPath: "/dev/nvme1n1", Model: "WD Black SN770", Serial: "S7SSH0000001", Type: "nvme", + Health: "PASSED", Wearout: 100, Size: 1000204886016}, + nodeTemp: &models.DiskTemp{Device: "/dev/nvme1n1", Serial: "S7SSH0000001", Type: "nvme", Temperature: 85}, + pairedSerial: "S7SSH0000001", pairedType: "nvme", pairedTemperature: 85, skippedTemperature: 85, + splitTemperature: 85, splitAlert: true, + }, + } { + for _, request := range []struct { + name string + // exclusions are the pairs report-merge records for the merged disk. + exclusions func(merged, agentCandidate, proxmoxCandidate string) [][2]string + split bool + }{ + {"report-merge", func(merged, agent, proxmox string) [][2]string { + return [][2]string{{merged, agent}, {merged, proxmox}} + }, true}, + {"proxmox-candidate-only", func(merged, _, proxmox string) [][2]string { + return [][2]string{{merged, proxmox}} + }, true}, + // The agent's row holds the merged ID, so a pair naming only its + // candidate separates nothing, here as in the registry. + {"agent-candidate-only", func(merged, agent, _ string) [][2]string { + return [][2]string{{merged, agent}} + }, false}, + {"unrelated-exclusion", func(merged, _, _ string) [][2]string { + return [][2]string{{merged, "physical_disk-00000000deadbeef"}} + }, false}, + } { + t.Run(shape.name+"/"+request.name, func(t *testing.T) { + t.Setenv("PULSE_DATA_DIR", t.TempDir()) + now := time.Now() + const instance, nodeName = "pve1", "node1" + node := models.Node{ + ID: "pve1-node1", Name: nodeName, Instance: instance, Host: "https://10.0.0.5:8006", + Status: "online", LastSeen: now, LinkedAgentID: "agent-1", + } + if shape.nodeTemp != nil { + reading := *shape.nodeTemp + reading.LastUpdated = now + node.Temperature = &models.Temperature{Available: true, HasSMART: true, SMART: []models.DiskTemp{reading}, LastUpdate: now} + } + state := models.NewState() + state.UpdateNodesForInstance(instance, []models.Node{node}) + state.UpsertHost(models.Host{ + ID: "agent-1", Hostname: nodeName, MachineID: "0123456789abcdef", LinkedNodeID: node.ID, + Status: "online", IntervalSeconds: 30, LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{shape.agent}}, + }) + store, err := unifiedresources.NewSQLiteResourceStore(t.TempDir(), "default") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = store.Close() }) + alertManager := alerts.NewManager() + t.Cleanup(alertManager.Stop) + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(store)) + m := &Monitor{ + state: state, resourceStore: adapter, alertManager: alertManager, + metricsHistory: NewMetricsHistory(100, time.Hour), + startTime: now.Add(-time.Hour), lastPhysicalDiskPoll: make(map[string]time.Time), + } + client := &slotDiskPVEClient{} + client.setDisk(shape.pve) + + disk := func() models.PhysicalDisk { + t.Helper() + disks := state.GetSnapshot().PhysicalDisks + if len(disks) != 1 { + t.Fatalf("physical disks = %+v, want one", disks) + } + return disks[0] + } + fullPoll := func() models.PhysicalDisk { + t.Helper() + adapter.PopulateFromSnapshot(state.GetSnapshot()) + started := time.Now() + delete(m.lastPhysicalDiskPoll, instance) + m.maybePollPhysicalDisksAsync(context.Background(), instance, &config.PVEInstance{}, client, + []proxmox.Node{{Node: nodeName, Status: "online"}}, map[string]string{nodeName: "online"}, nil) + deadline := time.Now().Add(3 * time.Second) + for { + if disks := state.GetSnapshot().PhysicalDisks; len(disks) == 1 && !disks[0].LastChecked.Before(started) { + return disks[0] + } + if time.Now().After(deadline) { + t.Fatal("full physical disk poll did not land in state") + } + time.Sleep(10 * time.Millisecond) + } + } + skippedPoll := func() models.PhysicalDisk { + t.Helper() + adapter.PopulateFromSnapshot(state.GetSnapshot()) + m.lastPhysicalDiskPoll[instance] = time.Now() + m.maybePollPhysicalDisksAsync(context.Background(), instance, &config.PVEInstance{}, nil, nil, nil, nil) + return disk() + } + temperatureAlert := func() bool { + id := unifiedresources.ProxmoxPhysicalDiskAlertResourceID(instance, nodeName, shape.pve.DevPath) + "::metric-threshold:diskTemperature" + for _, alert := range alertManager.GetActiveAlerts() { + if alert.ID == id { + return true + } + } + return false + } + healthAlert := func() bool { + for _, alert := range alertManager.GetActiveAlerts() { + if alert.Type == "disk-health" { + return true + } + } + return false + } + // A poll whose Proxmox disk query fails carries the stored record + // (or the agent's rows) instead of a fresh one, so it is done when + // the state is written rather than when a disk is newly checked. + failedQueryPoll := func() models.PhysicalDisk { + t.Helper() + adapter.PopulateFromSnapshot(state.GetSnapshot()) + client.setError(errors.New("disks/list timed out")) + defer client.setError(nil) + started := time.Now() + delete(m.lastPhysicalDiskPoll, instance) + m.maybePollPhysicalDisksAsync(context.Background(), instance, &config.PVEInstance{}, client, + []proxmox.Node{{Node: nodeName, Status: "online"}}, map[string]string{nodeName: "online"}, nil) + deadline := time.Now().Add(3 * time.Second) + for { + if snapshot := state.GetSnapshot(); snapshot.LastUpdate.After(started) && len(snapshot.PhysicalDisks) == 1 { + return snapshot.PhysicalDisks[0] + } + if time.Now().After(deadline) { + t.Fatal("physical disk poll with a failing disk query did not land in state") + } + time.Sleep(10 * time.Millisecond) + } + } + // Whether the canonical disks are one joined row or two. + canonicalRows := func() (joined, agentOnly, proxmoxOnly []*unifiedresources.PhysicalDiskView) { + t.Helper() + adapter.PopulateFromSnapshot(state.GetSnapshot()) + for _, view := range adapter.PhysicalDisks() { + _, fromAgent := view.SourceStatus(unifiedresources.SourceAgent) + _, fromProxmox := view.SourceStatus(unifiedresources.SourceProxmox) + switch { + case fromAgent && fromProxmox: + joined = append(joined, view) + case fromAgent: + agentOnly = append(agentOnly, view) + case fromProxmox: + proxmoxOnly = append(proxmoxOnly, view) + } + } + return joined, agentOnly, proxmoxOnly + } + + assertPaired := func(step string, got models.PhysicalDisk, wantTemperature int) { + t.Helper() + if got.Serial != shape.pairedSerial || got.Type != shape.pairedType || got.Health != "FAILED" || + got.Temperature != wantTemperature || got.Wearout != 60 || + got.SmartAttributes == nil || got.IO == nil || !got.AgentSMARTReported || got.AgentSMARTSplit { + t.Fatalf("%s: the agent's row was not paired with the Proxmox disk: %+v", step, got) + } + } + assertProxmoxOnly := func(step string, got models.PhysicalDisk) { + t.Helper() + if got.Serial != shape.pve.Serial || got.Type != shape.pve.Type || got.Health != "PASSED" || + got.Temperature != shape.splitTemperature || got.Wearout != shape.pve.Wearout || + got.SmartAttributes != nil || got.IO != nil || got.AgentSMARTReported || !got.AgentSMARTSplit { + t.Fatalf("%s: the split disk carries the agent's data: %+v", step, got) + } + } + + // The disk's temperature alert is open before the agent's row + // is paired with it, as when the node's sensors read it alone. + // A shape whose alert should open after the split starts without + // one: the alert manager suppresses an alert it just resolved. + if shape.nodeTemp != nil && !shape.splitAlert { + m.checkPhysicalDiskAlerts(instance, models.PhysicalDisk{ + Instance: instance, Node: nodeName, DevPath: shape.pve.DevPath, Model: shape.pve.Model, + Type: shape.pve.Type, Health: "PASSED", Wearout: -1, Temperature: shape.nodeTemp.Temperature, + }, nil) + if !temperatureAlert() { + t.Fatal("fixture did not open the Proxmox disk's temperature alert") + } + } + assertPaired("before the request", fullPoll(), shape.pairedTemperature) + if !healthAlert() { + t.Fatal("the agent's failed health did not raise the disk health alert") + } + if joined, agentOnly, proxmoxOnly := canonicalRows(); len(joined) != 1 || len(agentOnly) != 0 || len(proxmoxOnly) != 0 { + t.Fatalf("fixture did not join the disks: joined %d, agent only %d, proxmox only %d", len(joined), len(agentOnly), len(proxmoxOnly)) + } + if temperatureAlert() { + t.Fatal("the PVE disk's temperature alert stayed open for a disk the agent reports") + } + + // Report-merge names the merged disk and each source's candidate. + ids := unifiedresources.NewRegistry(nil) + ids.IngestSnapshot(state.GetSnapshot()) + var merged, agentCandidate, proxmoxCandidate string + for _, resource := range ids.List() { + if resource.Type == unifiedresources.ResourceTypePhysicalDisk { + merged = resource.ID + } + } + for _, target := range ids.SourceTargets(merged) { + switch target.Source { + case unifiedresources.SourceAgent: + agentCandidate = target.CandidateID + case unifiedresources.SourceProxmox: + proxmoxCandidate = target.CandidateID + } + } + if merged == "" || agentCandidate == "" || proxmoxCandidate == "" || proxmoxCandidate == merged { + t.Fatalf("merged disk %q lists candidates agent=%q proxmox=%q", merged, agentCandidate, proxmoxCandidate) + } + for _, pair := range request.exclusions(merged, agentCandidate, proxmoxCandidate) { + if err := store.AddExclusion(unifiedresources.ResourceExclusion{ResourceA: pair[0], ResourceB: pair[1], CreatedAt: time.Now().UTC()}); err != nil { + t.Fatal(err) + } + } + + if !request.split { + assertPaired("after the request", fullPoll(), shape.pairedTemperature) + // A skipped poll merges the stored record, which the joined + // canonical disk reports at the agent's reading. + assertPaired("on a skipped poll", skippedPoll(), shape.skippedTemperature) + return + } + // A full poll builds the record from what Proxmox reported, + // and the poll after it must not retain what the one before + // held from the agent. The skipped polls between, which merge + // the stored record again, keep the agent's data off too. + assertProxmoxOnly("after the request", fullPoll()) + // The split disk is still evaluated, and Proxmox's own verdict + // of it closes the health alert the agent's failure raised. + if healthAlert() { + t.Fatal("the disk health alert raised by the agent's row stayed open on the split disk") + } + assertProxmoxOnly("on a skipped poll", skippedPoll()) + assertProxmoxOnly("on the next full poll", fullPoll()) + assertProxmoxOnly("on the next skipped poll", skippedPoll()) + // When Proxmox cannot list the node's disks, the agent's rows + // stand in for them (#1516), except the row split from the + // disk in its slot. + assertProxmoxOnly("when the disk query fails", failedQueryPoll()) + + joined, agentOnly, proxmoxOnly := canonicalRows() + if len(joined) != 0 || len(agentOnly) != 1 || len(proxmoxOnly) != 1 { + t.Fatalf("canonical disks: joined %d, agent only %d, proxmox only %d, want the two apart", len(joined), len(agentOnly), len(proxmoxOnly)) + } + if proxmoxOnly[0].Health() != "PASSED" || proxmoxOnly[0].Serial() != shape.pve.Serial { + t.Fatalf("canonical Proxmox disk took the agent's data: health %q, serial %q", proxmoxOnly[0].Health(), proxmoxOnly[0].Serial()) + } + if agentOnly[0].Health() != "FAILED" || agentOnly[0].Serial() != shape.agent.Serial { + t.Fatalf("canonical agent disk lost its own data: health %q, serial %q", agentOnly[0].Health(), agentOnly[0].Serial()) + } + // A reading the agent supplied is gone, so the PVE disk check has + // none to judge and the alert the agent took over does not come + // back; one the SSH collector supplied is judged by the PVE check. + if temperatureAlert() != shape.splitAlert { + t.Fatalf("PVE temperature alert active = %v, want %v once the disks were split", temperatureAlert(), shape.splitAlert) + } + }) + } + } +} + +// fakeDiskAgentSplits answers the PVE disk poller's split question from a +// set of Proxmox device paths and records what it was asked. +type fakeDiskAgentSplits struct { + splitDevPaths map[string]bool + splitIDs map[string]bool + asked []string + askedIDs []string +} + +func (f *fakeDiskAgentSplits) ProxmoxDiskAgentSMARTSplit(disk models.PhysicalDisk, host models.Host, smart models.HostDiskSMART) bool { + f.asked = append(f.asked, host.ID+":"+disk.DevPath+":"+smart.Serial) + f.askedIDs = append(f.askedIDs, disk.ID) + return f.splitDevPaths[disk.DevPath] || f.splitIDs[disk.ID] +} + +// The agent's SMART merge asks the split decider once per disk it matches a +// row to. A split disk takes nothing from the row, not even the alert +// ownership, and is marked so the poll does not retain the agent's data for it +// either; a disk no row matches is not asked about; the rows of the disks that +// are not split still merge, as they do without a decider. +func TestMergeHostAgentSMARTIntoDisksLeavesDisksTheOperatorSplitAlone(t *testing.T) { + percentUsed := 40 + smart := func(device, serial string) models.HostDiskSMART { + return models.HostDiskSMART{ + Device: device, Serial: serial, Type: "sata", Health: "FAILED", Temperature: 61, + WWN: "5-c50-" + serial, Controller: "ctrl", Target: "0:1", Pool: "tank", Model: "AgentModel", SizeBytes: 1000, + IO: &models.DiskIO{Device: device, ReadBytes: 1}, + Attributes: &models.SMARTAttributes{PercentageUsed: &percentUsed}, + } + } + disk := func(devPath, serial string) models.PhysicalDisk { + return models.PhysicalDisk{ID: unifiedresources.ProxmoxPhysicalDiskSourceID("pve1", "node1", devPath, "", ""), Node: "node1", Instance: "pve1", DevPath: devPath, Serial: serial, Type: "sata", Health: "PASSED", Wearout: 100} + } + nodes := []models.Node{{Name: "node1", LinkedAgentID: "agent-1"}} + hosts := []models.Host{{ID: "agent-1", Status: "online", Sensors: models.HostSensorSummary{ + SMART: []models.HostDiskSMART{smart("sda", "SERIAL-A"), smart("sdb", "SERIAL-B")}, + }}} + disks := []models.PhysicalDisk{disk("/dev/sda", "SERIAL-A"), disk("/dev/sdb", "SERIAL-B"), disk("/dev/sdc", "SERIAL-C")} + // The node's sensor list, which a linked agent supplies, was matched to the + // split disk before the agent merge ran. + disks[0].Temperature = 58 + disks[0].Collection = &diskinventory.CollectionStatus{Temperature: diskinventory.Available(proxmoxNodeSMARTTemperatureSource)} + // Proxmox's own verdict of the split disk, which the agent's PASSED/FAILED + // would otherwise replace or correct. + disks[0].Health, disks[0].Wearout = "FAILED", 30 + + splits := &fakeDiskAgentSplits{splitDevPaths: map[string]bool{"/dev/sda": true, "/dev/sdc": true}} + merged := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, splits) + + wantSplit := disk("/dev/sda", "SERIAL-A") + wantSplit.AgentSMARTSplit = true + if got := merged[0]; got.Temperature != 0 || got.Health != "FAILED" || got.Wearout != 30 || got.SmartAttributes != nil || got.IO != nil || + got.AgentSMARTReported || !got.AgentSMARTSplit || got.ID != wantSplit.ID || got.Serial != wantSplit.Serial || + got.WWN != "" || got.Controller != "" || got.Target != "" || got.StorageGroup != "" || got.Model != "" || got.Size != 0 { + t.Fatalf("a split disk took the agent's row: %+v", got) + } + if got := merged[1]; got.Temperature != 61 || got.Health != "FAILED" || got.Wearout != 60 || got.SmartAttributes == nil || got.IO == nil || + !got.AgentSMARTReported || got.AgentSMARTSplit { + t.Fatalf("a disk that is not split lost the agent's row: %+v", got) + } + if got := merged[2]; got.AgentSMARTSplit || got.AgentSMARTReported { + t.Fatalf("a disk no row matches was marked: %+v", got) + } + if want := []string{"agent-1:/dev/sda:SERIAL-A", "agent-1:/dev/sdb:SERIAL-B"}; !slices.Equal(splits.asked, want) { + t.Fatalf("decider asked %v, want one question per matched disk %v", splits.asked, want) + } + if disks[0].AgentSMARTSplit || disks[0].Temperature != 58 || disks[0].Collection.Temperature.State != diskinventory.FieldAvailable { + t.Fatalf("the merge modified the caller's slice: %+v / %+v", disks[0], disks[0].Collection) + } + if merged[0].Collection == nil || merged[0].Collection.Temperature.State == diskinventory.FieldAvailable { + t.Fatalf("the split disk still counts the node sensors' reading as collected: %+v", merged[0].Collection) + } + + // The decision is made under the observation's own ID, the key the registry + // holds it by: a controller member recorded by device alone is a different + // observation from the stand-in keyed by its target, so a split recorded + // under the other key is ignored, as it is for the registry. + memberRow := smart("sdd", "SERIAL-D") + memberRow.Controller, memberRow.Target = "megaraid", "megaraid,7" + memberHosts := []models.Host{{ID: "agent-1", Status: "online", Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{memberRow}}}} + slotID := unifiedresources.ProxmoxPhysicalDiskSourceID("pve1", "node1", "/dev/sdd", "", "") + memberID := unifiedresources.ProxmoxPhysicalDiskSourceID("pve1", "node1", "/dev/sdd", "megaraid", "megaraid,7") + if slotID == memberID { + t.Fatal("fixture: a controller member's key should differ from the slot's") + } + bySlot := disk("/dev/sdd", "SERIAL-D") + byMember := bySlot + byMember.ID = memberID + for name, tc := range map[string]struct { + disk models.PhysicalDisk + splitID string + wantSplit bool + wantAsked string + }{ + "slot-keyed record, split under its key": {bySlot, slotID, true, slotID}, + "slot-keyed record, split under member key": {bySlot, memberID, false, slotID}, + "member-keyed record, split under its key": {byMember, memberID, true, memberID}, + "member-keyed record, split under slot key": {byMember, slotID, false, memberID}, + } { + decider := &fakeDiskAgentSplits{splitIDs: map[string]bool{tc.splitID: true}} + got := mergeHostAgentSMARTIntoDisks([]models.PhysicalDisk{tc.disk}, nodes, memberHosts, decider)[0] + if got.AgentSMARTSplit != tc.wantSplit || got.AgentSMARTReported == tc.wantSplit { + t.Fatalf("%s: split = %v, reported = %v, want split %v", name, got.AgentSMARTSplit, got.AgentSMARTReported, tc.wantSplit) + } + if !slices.Equal(decider.askedIDs, []string{tc.wantAsked}) { + t.Fatalf("%s: decider asked about %v, want exactly the record's own ID %q", name, decider.askedIDs, tc.wantAsked) + } + } + + // The node-sensor reading already on the disk stays; the agent's fills only an empty one. + if unchanged := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts, &fakeDiskAgentSplits{}); !unchanged[0].AgentSMARTReported || unchanged[0].Temperature != 58 || unchanged[0].Health != "FAILED" { + t.Fatalf("a decider that splits nothing changed the merge: %+v", unchanged[0]) + } +} + +// When the Proxmox disk query fails, the linked agent's SMART rows stand in +// for the node's disks (#1516). A row the operator split from the Proxmox disk +// in its slot is not that disk's stand-in: it would put the agent's disk, with +// its readings and temperature alert, in a second row beside the agent's own. +// The stand-in is judged under its own ID, the key the registry holds it by +// once recorded: a controller member's is qualified by its target. +func TestHostAgentSMARTFallbackSkipsRowsTheOperatorSplit(t *testing.T) { + rows := []models.HostDiskSMART{ + {Device: "sda", Serial: "SERIAL-A", Type: "sata", Temperature: 61}, + {Device: "sdb", Serial: "SERIAL-B", Type: "sata", Temperature: 62}, + } + host := models.Host{ID: "agent-1"} + + if got := unsplitPhysicalDisksFromHostAgentSMART("pve1", "node1", host, rows, nil); len(got) != 2 { + t.Fatalf("without split decisions the fallback listed %d disks, want 2", len(got)) + } + if got := unsplitPhysicalDisksFromHostAgentSMART("pve1", "node1", host, rows, &fakeDiskAgentSplits{}); len(got) != 2 { + t.Fatalf("a decider that splits nothing left %d stand-ins, want 2", len(got)) + } + splits := &fakeDiskAgentSplits{splitDevPaths: map[string]bool{"/dev/sda": true}} + got := unsplitPhysicalDisksFromHostAgentSMART("pve1", "node1", host, rows, splits) + if len(got) != 1 || got[0].DevPath != "/dev/sdb" || got[0].Temperature != 62 { + t.Fatalf("fallback disks = %+v, want only /dev/sdb", got) + } + if want := []string{"agent-1:/dev/sda:SERIAL-A", "agent-1:/dev/sdb:SERIAL-B"}; !slices.Equal(splits.asked, want) { + t.Fatalf("decider asked %v, want one question per row, with the row %v", splits.asked, want) + } + + member := []models.HostDiskSMART{{Device: "sdc", Serial: "SERIAL-C", Type: "sat", Controller: "megaraid", Target: "megaraid,7"}} + stand := physicalDisksFromHostAgentSMART("pve1", "node1", member) + slot := unifiedresources.ProxmoxPhysicalDiskSourceID("pve1", "node1", "/dev/sdc", "", "") + if len(stand) != 1 || stand[0].ID == slot { + t.Fatalf("a controller member's stand-in should be keyed by its target: %+v", stand) + } + byOwnID := &fakeDiskAgentSplits{splitIDs: map[string]bool{stand[0].ID: true}} + if got := unsplitPhysicalDisksFromHostAgentSMART("pve1", "node1", host, member, byOwnID); len(got) != 0 { + t.Fatalf("a split recorded under the stand-in's key did not hold: %+v", got) + } + if want := []string{stand[0].ID}; !slices.Equal(byOwnID.askedIDs, want) { + t.Fatalf("decider asked about %v, want the stand-in's own ID %v", byOwnID.askedIDs, want) + } + // A split recorded under the slot's device-only key names a different + // observation, as for the registry: the stand-in is not that disk. + bySlot := &fakeDiskAgentSplits{splitIDs: map[string]bool{slot: true}} + if got := unsplitPhysicalDisksFromHostAgentSMART("pve1", "node1", host, member, bySlot); len(got) != 1 { + t.Fatalf("a split recorded under the slot's key applied to the member's stand-in: %+v", got) + } + if want := []string{stand[0].ID}; !slices.Equal(bySlot.askedIDs, want) { + t.Fatalf("decider asked about %v, want only the stand-in's own ID %v", bySlot.askedIDs, want) + } +} + +// When the Proxmox disk query fails and the operator split one of the node's +// disks from its agent row, only the rows that were not split stand in for the +// node's disks, as before; the split disk gets no stand-in, which would be the +// agent's disk again, so its Proxmox record is not listed while the query fails. +func TestFailedDiskQueryListsOnlyTheRowsTheOperatorDidNotSplit(t *testing.T) { + t.Setenv("PULSE_DATA_DIR", t.TempDir()) + now := time.Now() + state := models.NewState() + state.UpdateNodesForInstance("pve1", []models.Node{{ + ID: "pve1-node1", Name: "node1", Instance: "pve1", Status: "online", LastSeen: now, LinkedAgentID: "agent-1", + }}) + state.UpsertHost(models.Host{ + ID: "agent-1", Hostname: "node1", LinkedNodeID: "pve1-node1", Status: "online", IntervalSeconds: 30, LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{ + {Device: "/dev/sda", Serial: "AGENT-A", Type: "sata", Health: "FAILED", Temperature: 61}, + {Device: "/dev/sdb", Serial: "SERIAL-B", Type: "sata", Health: "PASSED", Temperature: 62}, + }}, + }) + record := func(device, serial string) models.PhysicalDisk { + return models.PhysicalDisk{ + ID: unifiedresources.ProxmoxPhysicalDiskSourceID("pve1", "node1", device, "", ""), Node: "node1", Instance: "pve1", + DevPath: device, Serial: serial, Type: "sata", Health: "PASSED", Wearout: 100, LastChecked: now, + Collection: &diskinventory.CollectionStatus{Temperature: diskinventory.Unsupported("proxmox_disks", "no temperature")}, + } + } + state.UpdatePhysicalDisks("pve1", []models.PhysicalDisk{record("/dev/sda", "PVE-A"), record("/dev/sdb", "SERIAL-B")}) + + alertManager := alerts.NewManager() + t.Cleanup(alertManager.Stop) + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + m := &Monitor{ + state: state, + resourceStore: splitDecidingStore{adapter, &fakeDiskAgentSplits{splitDevPaths: map[string]bool{"/dev/sda": true}}}, + alertManager: alertManager, metricsHistory: NewMetricsHistory(100, time.Hour), + startTime: now.Add(-time.Hour), lastPhysicalDiskPoll: make(map[string]time.Time), + } + adapter.PopulateFromSnapshot(state.GetSnapshot()) + client := &slotDiskPVEClient{} + client.setError(errors.New("disks/list timed out")) + started := time.Now() + m.maybePollPhysicalDisksAsync(context.Background(), "pve1", &config.PVEInstance{}, client, + []proxmox.Node{{Node: "node1", Status: "online"}}, map[string]string{"node1": "online"}, nil) + var disks []models.PhysicalDisk + for deadline := time.Now().Add(3 * time.Second); ; time.Sleep(10 * time.Millisecond) { + if snapshot := state.GetSnapshot(); snapshot.LastUpdate.After(started) { + disks = snapshot.PhysicalDisks + break + } + if time.Now().After(deadline) { + t.Fatal("physical disk poll with a failing disk query did not land in state") + } + } + byDevice := map[string]models.PhysicalDisk{} + for _, disk := range disks { + byDevice[disk.DevPath] = disk + } + if len(disks) != 1 { + t.Fatalf("disks = %+v, want only the row that was not split", disks) + } + if split, listed := byDevice["/dev/sda"]; listed { + t.Fatalf("the agent's row stood in for the disk the operator split from it: %+v", split) + } + if other := byDevice["/dev/sdb"]; other.Serial != "SERIAL-B" || other.Temperature != 62 || !other.AgentSMARTReported || + other.ID != unifiedresources.ProxmoxPhysicalDiskSourceID("pve1", "node1", "/dev/sdb", "", "") { + t.Fatalf("the other row did not stand in for its disk: %+v", other) + } +} + +// splitDecidingStore is the resource store a monitor test hands out when it +// wants to decide the operator's splits itself. +type splitDecidingStore struct { + *unifiedresources.MonitorAdapter + splits *fakeDiskAgentSplits +} + +func (s splitDecidingStore) ProxmoxDiskAgentSMARTSplit(disk models.PhysicalDisk, host models.Host, smart models.HostDiskSMART) bool { + return s.splits.ProxmoxDiskAgentSMARTSplit(disk, host, smart) +} + +// The node poll takes the node's SMART sensor list from a linked agent when +// the agent's report carries a usable temperature, and from the SSH collector +// otherwise, so only the first kind of reading is the agent's row of the disk +// a split Proxmox disk was split from. The legacy NVMe list names no disk, so +// a split disk drops its guess either way. A disk whose temperature has +// another source keeps it, and the caller's collection is never modified. +func TestDropNodeSensorTemperatureRemovesOnlyAgentSuppliedReadings(t *testing.T) { + for _, tc := range []struct { + source string + smartFromAgent bool + dropped bool + }{ + {proxmoxNodeSMARTTemperatureSource, true, true}, + {proxmoxNodeSMARTTemperatureSource, false, false}, + {proxmoxNodeNVMeTemperatureSource, true, true}, + {proxmoxNodeNVMeTemperatureSource, false, true}, + } { + shared := &diskinventory.CollectionStatus{ + Temperature: diskinventory.Available(tc.source), Serial: diskinventory.Available("proxmox_disks"), + IO: diskinventory.Unsupported("proxmox_disks", "no counters"), Controller: diskinventory.Missing("proxmox_disks", "none"), + Pool: diskinventory.Available("proxmox_zfs"), + } + disk := models.PhysicalDisk{Temperature: 60, Collection: shared} + dropNodeSensorTemperature(&disk, tc.smartFromAgent) + if tc.dropped { + other := *disk.Collection + other.Temperature = shared.Temperature + if disk.Temperature != 0 || disk.Collection.Temperature.State != diskinventory.FieldUnsupported || + disk.Collection.Temperature.Source != "proxmox_disks" || other != *shared { + t.Fatalf("%+v: reading not dropped cleanly: %+v / %+v", tc, disk, disk.Collection) + } + } else if disk.Temperature != 60 || disk.Collection != shared { + t.Fatalf("%+v: an SSH-supplied reading was dropped: %+v", tc, disk) + } + if shared.Temperature.State != diskinventory.FieldAvailable || shared.Temperature.Source != tc.source { + t.Fatalf("%+v: the caller's collection was modified: %+v", tc, shared) + } + } + for _, collection := range []*diskinventory.CollectionStatus{ + nil, + {Temperature: diskinventory.Available("smartctl")}, + {Temperature: diskinventory.Unsupported("proxmox_disks", "no temperature")}, + } { + disk := models.PhysicalDisk{Temperature: 60, Collection: collection} + dropNodeSensorTemperature(&disk, true) + if disk.Temperature != 60 || disk.Collection != collection { + t.Fatalf("a reading from another source was dropped: %+v", disk) + } + } +} + +// mergeNVMeTempsIntoDisks labels what it takes from the node's sensor lists +// with the sources dropNodeSensorTemperature recognises: the SMART list +// matched by identity or path, and the legacy NVMe list matched by order. +func TestMergeNVMeTempsLabelsReadingsWithTheNodeSensorSources(t *testing.T) { + node := func(temperature *models.Temperature) []models.Node { + return []models.Node{{Name: "node1", Temperature: temperature}} + } + disk := models.PhysicalDisk{Node: "node1", DevPath: "/dev/nvme0n1", Serial: "S6B0NL0W123456", Type: "nvme"} + + bySMART := mergeNVMeTempsIntoDisks([]models.PhysicalDisk{disk}, node(&models.Temperature{ + Available: true, HasSMART: true, + SMART: []models.DiskTemp{{Device: "/dev/nvme0n1", Serial: "S6B0NL0W123456", Temperature: 55}}, + }))[0] + if bySMART.Temperature != 55 || bySMART.Collection == nil || bySMART.Collection.Temperature.Source != proxmoxNodeSMARTTemperatureSource { + t.Fatalf("SMART list reading not labelled %q: %+v / %+v", proxmoxNodeSMARTTemperatureSource, bySMART, bySMART.Collection) + } + + for name, row := range map[string]models.DiskTemp{ + "WWN": {Device: "/dev/other", WWN: "0x5000c500a1b2c3d4", Temperature: 52}, + "device path": {Device: "/dev/nvme0n1", Temperature: 53}, + } { + wwnDisk := models.PhysicalDisk{Node: "node1", DevPath: "/dev/nvme0n1", WWN: "0x5000c500a1b2c3d4", Type: "nvme"} + if name == "device path" { + wwnDisk = models.PhysicalDisk{Node: "node1", DevPath: "/dev/nvme0n1", Type: "nvme"} + } + got := mergeNVMeTempsIntoDisks([]models.PhysicalDisk{wwnDisk}, node(&models.Temperature{ + Available: true, HasSMART: true, SMART: []models.DiskTemp{row}, + }))[0] + if got.Temperature != row.Temperature || got.Collection == nil || got.Collection.Temperature.Source != proxmoxNodeSMARTTemperatureSource { + t.Fatalf("%s match not labelled %q: %+v / %+v", name, proxmoxNodeSMARTTemperatureSource, got, got.Collection) + } + } + + byOrder := mergeNVMeTempsIntoDisks([]models.PhysicalDisk{disk}, node(&models.Temperature{ + Available: true, HasNVMe: true, NVMe: []models.NVMeTemp{{Device: "nvme0", Temp: 47}}, + }))[0] + if byOrder.Temperature != 47 || byOrder.Collection == nil || byOrder.Collection.Temperature.Source != proxmoxNodeNVMeTemperatureSource { + t.Fatalf("legacy NVMe reading not labelled %q: %+v / %+v", proxmoxNodeNVMeTemperatureSource, byOrder, byOrder.Collection) + } +} + +// Only a SMART report that carries a usable temperature makes the node's SMART +// list the agent's (mergeTemperatureData). +func TestHostSuppliesNodeSMARTTemperaturesFollowsUsableReadings(t *testing.T) { + host := func(rows ...models.HostDiskSMART) models.Host { + return models.Host{Sensors: models.HostSensorSummary{SMART: rows}} + } + for name, tc := range map[string]struct { + host models.Host + want bool + }{ + "a reading": {host(models.HostDiskSMART{Device: "sda", Temperature: 40}), true}, + "one reading of several": {host(models.HostDiskSMART{Device: "sda"}, models.HostDiskSMART{Device: "sdb", Temperature: 40}), true}, + "no temperature": {host(models.HostDiskSMART{Device: "sda"}), false}, + "standby row": {host(models.HostDiskSMART{Device: "sda", Temperature: 40, Standby: true}), false}, + "negative reading": {host(models.HostDiskSMART{Device: "sda", Temperature: -1}), false}, + "no rows": {host(), false}, + } { + if got := hostSuppliesNodeSMARTTemperatures(tc.host); got != tc.want { + t.Errorf("%s: got %v, want %v", name, got, tc.want) + } + } +} + +// When the linked agent's SMART report carries no usable temperature, the +// node's SMART sensor list comes from the SSH collector, so a reading of a +// split Proxmox disk taken from it is the node's own and stays, while the +// agent's row still takes nothing else from the disk. +func TestMergeHostAgentSMARTIntoDisksKeepsSSHNodeSensorReadingOfSplitDisk(t *testing.T) { + nodes := []models.Node{{Name: "node1", LinkedAgentID: "agent-1"}} + hosts := []models.Host{{ID: "agent-1", Status: "online", Sensors: models.HostSensorSummary{ + SMART: []models.HostDiskSMART{{Device: "sda", Serial: "SERIAL-A", Type: "sata", Health: "FAILED"}}, + }}} + disk := models.PhysicalDisk{ + ID: "pve1-node1-/dev/sda", Node: "node1", Instance: "pve1", DevPath: "/dev/sda", Serial: "SERIAL-A", Type: "sata", + Health: "PASSED", Wearout: 100, Temperature: 52, + Collection: &diskinventory.CollectionStatus{Temperature: diskinventory.Available(proxmoxNodeSMARTTemperatureSource)}, + } + splits := &fakeDiskAgentSplits{splitDevPaths: map[string]bool{"/dev/sda": true}} + got := mergeHostAgentSMARTIntoDisks([]models.PhysicalDisk{disk}, nodes, hosts, splits)[0] + if got.Temperature != 52 || got.Collection.Temperature.Source != proxmoxNodeSMARTTemperatureSource || + got.Health != "PASSED" || got.AgentSMARTReported || !got.AgentSMARTSplit { + t.Fatalf("split disk with an SSH-supplied reading: %+v / %+v", got, got.Collection) + } + + // An agent that stopped reporting keeps its rows, temperatures included, + // but the node poll ignores them once its lease lapsed, so the reading is + // the SSH collector's again. + hosts[0].Status = "offline" + hosts[0].Sensors.SMART[0].Temperature = 61 + lapsed := mergeHostAgentSMARTIntoDisks([]models.PhysicalDisk{disk}, nodes, hosts, splits)[0] + if lapsed.Temperature != 52 || lapsed.Collection.Temperature.Source != proxmoxNodeSMARTTemperatureSource || !lapsed.AgentSMARTSplit { + t.Fatalf("split disk with a lapsed agent dropped the node's own reading: %+v / %+v", lapsed, lapsed.Collection) + } + hosts[0].Status = "online" + live := mergeHostAgentSMARTIntoDisks([]models.PhysicalDisk{disk}, nodes, hosts, splits)[0] + if live.Temperature != 0 || live.Collection.Temperature.State != diskinventory.FieldUnsupported { + t.Fatalf("split disk with a reporting agent kept the agent-supplied reading: %+v / %+v", live, live.Collection) + } +} diff --git a/internal/unifiedresources/physical_disk_agent_split.go b/internal/unifiedresources/physical_disk_agent_split.go new file mode 100644 index 0000000000..3527f023ca --- /dev/null +++ b/internal/unifiedresources/physical_disk_agent_split.go @@ -0,0 +1,67 @@ +package unifiedresources + +import "github.com/rcourtman/pulse-go-rewrite/internal/models" + +// ProxmoxDiskAgentSMARTSplit reports whether the operator split a Proxmox +// disk observation from the disk a host agent reports in one SMART row: +// report-merge on the merged disk (the drawer's Split merged resource) +// recorded an exclusion that separates them by the rule the linked disk join +// applies (physicalDiskSplitLocked). The PVE disk poller asks before it pairs +// the agent's row with the Proxmox disk on the agent's node, so a split +// Proxmox row carries what Proxmox reported and the PVE disk check judges it. +// +// It reads the exclusions this generation loaded, and whether it holds the two +// observations as one disk, never what the poller's pairing would fill into +// the Proxmox row: the pairing re-keys a Proxmox disk whose serial it fills or +// promotes, so a decision read from those fields would flip with it. The +// generation holds them as one disk only through a manual link, which is the +// operator's later decision, so pairing resumes while it stands, and the link's +// removal or a new report-merge splits them again. A registry without a store +// carries no operator decisions. It takes no lock monitoring holds. +func (a *MonitorAdapter) ProxmoxDiskAgentSMARTSplit(disk models.PhysicalDisk, host models.Host, smart models.HostDiskSMART) bool { + registry := a.currentRegistry() + if registry == nil { + return false + } + return registry.proxmoxDiskAgentSMARTSplit(disk, host, smart) +} + +// proxmoxDiskAgentSMARTSplit judges the pair as the linked disk join would: +// the Proxmox observation by its source-specific candidate ID (its slot) and +// the identity Proxmox reported, the agent's disk by the ID this generation +// gave it, or, absent from it, by the identity its row reports. +func (rr *ResourceRegistry) proxmoxDiskAgentSMARTSplit(disk models.PhysicalDisk, host models.Host, smart models.HostDiskSMART) bool { + proxmoxSourceID := normalizeSourceID(disk.ID) + agentSourceID := normalizeSourceID(HostSMARTDiskSourceID(host, smart)) + if proxmoxSourceID == "" || agentSourceID == "" { + return false + } + + rr.mu.RLock() + defer rr.mu.RUnlock() + if len(rr.exclusions) == 0 { + return false + } + agentDiskID := rr.bySource[SourceAgent][agentSourceID] + if agentDiskID != "" && agentDiskID == rr.bySource[SourceProxmox][proxmoxSourceID] { + return false + } + agentDisk := rr.resources[agentDiskID] + if agentDisk == nil { + resource, identity := resourceFromHostSMARTDisk(host, smart) + resource.Identity = identity + // A row without hardware identity takes its candidate ID, which + // report-merge records for it, as the merged disk's ID. + resource.ID = rr.sourceSpecificID(ResourceTypePhysicalDisk, SourceAgent, agentSourceID) + if hostID := rr.bySource[SourceAgent][normalizeSourceID(host.ID)]; hostID != "" { + resource.ParentID = &hostID + } + agentDisk = &resource + } + observation, identity := resourceFromPhysicalDisk(disk) + observation.Identity = identity + if nodeID := rr.bySource[SourceProxmox][proxmoxNodeSourceID(disk.Instance, disk.Node)]; nodeID != "" { + observation.ParentID = &nodeID + } + return rr.physicalDiskSplitLocked(agentDisk, &observation, rr.sourceSpecificID(ResourceTypePhysicalDisk, SourceProxmox, proxmoxSourceID)) +} diff --git a/internal/unifiedresources/registry_merge_policy_test.go b/internal/unifiedresources/registry_merge_policy_test.go index d8bb80ed7c..b10e66d7d8 100644 --- a/internal/unifiedresources/registry_merge_policy_test.go +++ b/internal/unifiedresources/registry_merge_policy_test.go @@ -4243,7 +4243,9 @@ func TestPhysicalDiskExclusionFallbackStaysPerMachine(t *testing.T) { // on every rebuild. The registry ingests agent disks first, so the agent's // row holds the merged ID and keeps it; the Proxmox row takes the ID it holds // alone, or its source-specific ID where both share a hardware identity (the -// agent holds that ID), as findMatch's excluded branch does. +// agent holds that ID), as findMatch's excluded branch does. The PVE disk +// poller asks ProxmoxDiskAgentSMARTSplit before it pairs the agent's SMART +// row with the Proxmox disk, and it answers as the exclusions split the rows. func TestOperatorSplitSeparatesDisksJoinedInsideALinkedHost(t *testing.T) { now := time.Now().UTC() for _, shape := range []struct { @@ -4331,6 +4333,15 @@ func TestOperatorSplitSeparatesDisksJoinedInsideALinkedHost(t *testing.T) { t.Fatalf("fixture did not join the disks under the agent's ID: merged %q, alone: agent %q, proxmox %q", merged, agentAlone, pveAlone) } ids := diskSplitCandidates(t, adapter.currentRegistry(), merged) + // The PVE disk poller asks the adapter before it pairs the + // agent's SMART row with the Proxmox disk on the agent's node. + pollerSplit := func(step string, host models.Host, smart models.HostDiskSMART, want bool) { + t.Helper() + if got := adapter.ProxmoxDiskAgentSMARTSplit(shape.pve, host, smart); got != want { + t.Fatalf("%s: poller split = %v, want %v", step, got, want) + } + } + pollerSplit("before the request", host, shape.agent, false) addExclusions(t, store, request.exclusions(ids)) want := linkedDiskRows{joined: merged} @@ -4371,6 +4382,13 @@ func TestOperatorSplitSeparatesDisksJoinedInsideALinkedHost(t *testing.T) { check, describe := exactly(want) assertViews("after the request", snapshot, check, describe) + pollerSplit("after the request", host, shape.agent, request.split) + // A generation that has not ingested the agent's disk yet judges + // its row by the ID and identity the row would be keyed under. + unseen := NewMonitorAdapter(NewRegistry(store)) + if got := unseen.ProxmoxDiskAgentSMARTSplit(shape.pve, host, shape.agent); got != request.split { + t.Fatalf("a generation without the agent's disk judged the split = %v, want %v", got, request.split) + } if !request.split { return } @@ -4383,6 +4401,11 @@ func TestOperatorSplitSeparatesDisksJoinedInsideALinkedHost(t *testing.T) { } check, describe = exactly(linkedDiskRows{joined: merged}) assertViews("after relink", snapshot, check, describe) + // The link is the operator's later decision, whether it + // replaced an exclusion (the Proxmox row held its candidate + // ID) or left it standing: the generation holds the + // observations as one disk, so the poller pairs them again. + pollerSplit("after relink", host, shape.agent, false) relinked := adapter.currentRegistry() var again [][2]string for _, fold := range relinked.ManualLinkFolds(merged) { @@ -4393,6 +4416,22 @@ func TestOperatorSplitSeparatesDisksJoinedInsideALinkedHost(t *testing.T) { addExclusions(t, store, again) check, describe = exactly(want) assertViews("after splitting again", snapshot, check, describe) + pollerSplit("after splitting again", host, shape.agent, true) + + // The split names the IDs of the disks it separated, not the + // Proxmox slot, so a replacement in the slot (different + // hardware on both sides) is a different disk the poller pairs. + replacedPVE, replacedAgent := shape.pve, shape.agent + replacedPVE.Serial = "REPLACEMENT-0009" + replacedAgent.Serial = "Z9Z9REPLACED" + if shape.pve.Serial == shape.agent.Serial { + replacedAgent.Serial = replacedPVE.Serial + } + replacedHost := host + replacedHost.Sensors.SMART = []models.HostDiskSMART{replacedAgent} + if adapter.ProxmoxDiskAgentSMARTSplit(replacedPVE, replacedHost, replacedAgent) { + t.Fatalf("a replacement in the slot inherited the split recorded against the disk it replaced") + } // smartctl reads no serial from a disk in standby, so the // agent's row takes its device-keyed ID. A Proxmox @@ -4405,8 +4444,10 @@ func TestOperatorSplitSeparatesDisksJoinedInsideALinkedHost(t *testing.T) { assertViews("while the agent's disk is in standby", asleep, func(got linkedDiskRows) bool { return apart(got) && got.proxmox == merged }, "the disks apart, the Proxmox row on "+merged) + pollerSplit("while the agent's disk is in standby", asleep.Hosts[0], asleep.Hosts[0].Sensors.SMART[0], true) check, describe = exactly(want) assertViews("once the agent's disk wakes", snapshot, check, describe) + pollerSplit("once the agent's disk wakes", host, shape.agent, true) } // A same-serial disk on another machine re-keys the agent's @@ -4414,6 +4455,7 @@ func TestOperatorSplitSeparatesDisksJoinedInsideALinkedHost(t *testing.T) { // the unscoped ID, still holds. if shape.agent.Serial != "" { assertViews("after a same-serial disk appears elsewhere", withClone, apart, "the disks apart") + pollerSplit("after a same-serial disk appears elsewhere", host, shape.agent, true) } }) } @@ -4451,6 +4493,9 @@ func TestOperatorSplitSeparatesDisksJoinedInsideALinkedHost(t *testing.T) { t.Fatalf("%s rebuild %d %s: got %+v, want the disks apart", step.name, rebuild, view, got) } } + if !adapter.ProxmoxDiskAgentSMARTSplit(shape.pve, host, shape.agent) { + t.Fatalf("%s rebuild %d: the poller would pair the disks the rows split", step.name, rebuild) + } } } })