From 344f1f85310199ec7880c722bd2f5a65718e5244 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 23:38:25 +0100 Subject: [PATCH] Raise disk health alerts for mock physical disks checkMockAlerts evaluated guests, agents, nodes, storage, PBS, PMG and Docker against the mock estate but never physical disks. The fixture keeps a FAILED cohort and worn SSDs stable on purpose, so mock and demo stacks showed failing and "Replace Now" disks with no disk-health or disk-wearout alert, and the disk alert lifecycle could not be exercised without real hardware. The pass applies the physical disk poller's boundary: only disks on online nodes, matched by instance and node name, with the linked agent's --disk-exclude patterns forcing a matched device healthy. The per-disk evaluation moves into checkPhysicalDiskAlerts so the poller and the mock pass share one rule. The poller's agent-link settle deferral is not applied, because fixture links and exclusions are complete from the first pass. Like the other loops in checkMockAlerts, the disk loop runs after the pass's single mode check, so the existing race with leaving mock mode now covers disk alerts too. That race also involves other fixture evaluation paths and is left for a monitor-wide fix. On the public demo's estate (8 nodes) this adds one warning-level disk-wearout alert and no disk-health alert: that estate has no FAILED Proxmox disk. --- .../v6/internal/subsystems/monitoring.md | 56 ++++++++ .../monitoring/canonical_guardrails_test.go | 3 +- internal/monitoring/monitor_alerts.go | 43 +++++++ .../monitoring/monitor_mock_alerts_test.go | 121 ++++++++++++++++++ internal/monitoring/monitor_pve.go | 25 ++-- 5 files changed, 238 insertions(+), 10 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index a912bfce62..0c751ad9be 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1679,6 +1679,62 @@ boundaries: `TestMockKubernetesLoneHeavyPodStaysUnderNodeCeiling` in `internal/mock/generator_test.go`. +Mock alert evaluation covers physical disks as well. Live Proxmox disks are +evaluated by the physical disk poller in `maybePollPhysicalDisksAsync`, which +only polls configured Proxmox instances, so fixture disks had no evaluation +path. The fixture deliberately keeps a FAILED health cohort and worn SSDs +stable across restarts, so the estate showed failing and "Replace Now" disks +with no `disk-health` or `disk-wearout` alert. `checkMockAlerts` now evaluates +fixture disks through `checkMockPhysicalDiskAlerts`. Both paths share the +per-disk rule in `checkPhysicalDiskAlerts` (a device matched by the linked +agent's `--disk-exclude` patterns is evaluated as healthy, otherwise +`CheckDiskHealth` judges the disk), and the mock pass applies the poller's +boundary around it: + +- Only disks on online nodes are evaluated, as the poller only evaluates nodes + it reached. A disk on an offline node keeps whatever alert it had. The mock + pass treats every disk on an online fixture node as collected; the poller + also skips a node whose disk query and SMART fallback both failed. Nodes are + matched by instance and name, as the poller only pairs a disk with its own + instance's nodes. +- Excluding a disk resolves the alerts it already raised, because the excluded + device is evaluated as healthy rather than skipped. +- The poller's wait for host-agent links to settle after a restart (#1674) is + not applied: fixture links and exclusions are complete from the first pass. +- A disk whose node name leaves the estate loses its alerts through + `CleanupAlertsForNodes`, which the same pass already runs. That cleanup + matches node names only, so it relies on fixture node names being unique + across instances, which they are. + +The disk loop relies on the pass's single up-front mode check, like every +loop in `checkMockAlerts` except the host-agent step, which re-checks under its +own lock. A pass already past that check when mock mode is switched off can +reopen its fixture alerts after `SetMockMode(false)` clears them, and +other paths that evaluate fixture data (frontend refresh into unified alert +evaluation, backup rollups) have the same window. That mode-exit race predates +the disk loop and is not closed here; a lock around `checkMockAlerts` alone +would leave the other paths open. + +The disk evaluation runs on the mock alert tick, not the poller's disk +interval (five minutes by default, `PhysicalDiskPollingMinutes` per instance). +`CheckDiskHealth` logs at error level whenever it evaluates a FAILED disk +(models with a known firmware bug skip the health check) and at warn level for +a worn SSD, so an evaluated fixture disk in either state logs on every tick. On +2026-10-06, private stacks with the public demo's estate (8 nodes) carried 12 +Proxmox disks, none FAILED. Baseline against patched, the only new disk alert +was one warning-level `disk-wearout` (a 7% SSD on `pve5`), whose resource ID is +one of the disk resource's canonical aliases, and neither stack had a +`disk-health` alert. `CheckDiskHealth` raises only those two alert types, so +the other differences between the two boots (ZFS and Docker service alerts) are +not from the disk pass; those fixtures draw random state each boot. The default +test estate raises five `disk-health` and one `disk-wearout` alert. The tests +pin the boundary: `TestCheckMockAlertsEvaluatesPhysicalDisks` and +`TestCheckMockPhysicalDiskAlertsFollowsPollerBoundary` in +`internal/monitoring/monitor_mock_alerts_test.go`, with +`TestProxmoxDiskAlertsRunOnMergedDiskState` in +`internal/monitoring/canonical_guardrails_test.go` keeping the poller on the +shared helper. + Host and container-runtime disk collection supports an explicit include list for filesystems hidden by Pulse's automatic virtual/container filtering. The include list is bounded to that automatic filter; explicit disk exclusions diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index 3c398a8cbf..6234aff22a 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -2234,7 +2234,8 @@ func TestProxmoxDiskAlertsRunOnMergedDiskState(t *testing.T) { file: "monitor_pve.go", snippets: []string{ "allDisks = mergeHostAgentSMARTIntoDisks(allDisks, nodesFromState, hosts)", - "m.alertManager.CheckDiskHealth(inst, disk.Node, proxmoxDiskFromPhysicalDisk(disk))", + "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/monitor_alerts.go b/internal/monitoring/monitor_alerts.go index 89e870836f..a75e7dd557 100644 --- a/internal/monitoring/monitor_alerts.go +++ b/internal/monitoring/monitor_alerts.go @@ -716,6 +716,14 @@ func (m *Monitor) checkMockAlerts() { m.alertManager.CheckStorageWithCapacityTrend(storage, m.storageCapacityTrend(storage, time.Now())) } + // Check alerts for physical disks. Live disks are evaluated by the + // physical disk poller, which only polls configured Proxmox instances, and + // the fixture deliberately keeps a FAILED cohort and worn SSDs, so without + // this pass the estate shows failing disks with no disk-health or + // disk-wearout alert. + log.Debug().Int("diskCount", len(state.PhysicalDisks)).Msg("checking physical disk alerts") + m.checkMockPhysicalDiskAlerts(state.PhysicalDisks, state.Nodes, state.Hosts) + // Check alerts for PBS instances log.Debug().Int("pbsCount", len(state.PBSInstances)).Msg("checking PBS alerts") for _, pbsInst := range state.PBSInstances { @@ -815,3 +823,38 @@ func (m *Monitor) checkMockDockerHostAlerts(host models.DockerHost) { } m.alertManager.CheckDockerHost(host) } + +// checkMockPhysicalDiskAlerts applies the physical disk poller's alert +// boundary to fixture disks. The poller only evaluates disks on nodes it +// reached, so a disk on a node that is not online keeps whatever alert it +// had, and a device matched by the linked agent's --disk-exclude patterns is +// evaluated as healthy. The poller's wait for host-agent links to settle after +// a restart does not apply: fixture links and exclusions are complete from the +// first pass. +func (m *Monitor) checkMockPhysicalDiskAlerts(disks []models.PhysicalDisk, nodes []models.Node, hosts []models.Host) { + type nodeKey struct{ instance, name string } + + excludeByHost := make(map[string][]string, len(hosts)) + for _, host := range hosts { + if len(host.DiskExclude) > 0 { + excludeByHost[host.ID] = host.DiskExclude + } + } + onlineNodes := make(map[nodeKey]bool, len(nodes)) + excludeByNode := make(map[nodeKey][]string) + for _, node := range nodes { + key := nodeKey{instance: node.Instance, name: node.Name} + onlineNodes[key] = node.Status == "online" + if patterns := excludeByHost[node.LinkedAgentID]; node.LinkedAgentID != "" && len(patterns) > 0 { + excludeByNode[key] = patterns + } + } + + for _, disk := range disks { + key := nodeKey{instance: disk.Instance, name: disk.Node} + if !onlineNodes[key] { + continue + } + m.checkPhysicalDiskAlerts(disk.Instance, disk, excludeByNode[key]) + } +} diff --git a/internal/monitoring/monitor_mock_alerts_test.go b/internal/monitoring/monitor_mock_alerts_test.go index 66e69d616a..3c5c933339 100644 --- a/internal/monitoring/monitor_mock_alerts_test.go +++ b/internal/monitoring/monitor_mock_alerts_test.go @@ -239,3 +239,124 @@ func TestMockHostAgentPassAfterLeavingMockModeRegistersNothing(t *testing.T) { } t.Fatal("stale mock pass registered fixture hostname pve1 and suppressed the real node's memory alert") } + +// diskAlertKeys lists the active disk-health and disk-wearout alerts as +// "type instance/node device" so assertions name the disk, not the alert ID. +func diskAlertKeys(manager *alerts.Manager) map[string]bool { + keys := make(map[string]bool) + for _, alert := range manager.GetActiveAlerts() { + if alert.Type != "disk-health" && alert.Type != "disk-wearout" { + continue + } + devPath, _ := alert.Metadata["disk_path"].(string) + keys[alert.Type+" "+alert.Instance+"/"+alert.Node+" "+devPath] = true + } + return keys +} + +func TestCheckMockAlertsEvaluatesPhysicalDisks(t *testing.T) { + // Pin the default estate: a PULSE_MOCK_* environment can select one with + // no FAILED disk (the public demo's eight nodes have none). + previousEnabled, previousConfig := mock.IsMockEnabled(), mock.GetConfig() + t.Cleanup(func() { + mustSetMockEnabled(t, false) + mock.SetMockConfig(previousConfig) + mustSetMockEnabled(t, previousEnabled) + }) + mustSetMockEnabled(t, false) + cfg := mock.DefaultConfig + cfg.UpdateInterval = 5 * time.Minute + mock.SetMockConfig(cfg) + mustSetMockEnabled(t, true) + manager := newMockHostAlertTestManager(t) + + monitor := &Monitor{alertManager: manager} + monitor.checkMockAlerts() + + // The fixture keeps a FAILED cohort and worn SSDs stable across restarts + // so the disk alert lifecycle has evidence to act on. Each must raise the + // alert the physical disk poller raises for a live disk, and a disk on a + // node the poller would not reach raises nothing. + state := mock.CurrentFixtureGraph().State + onlineNodes := make(map[string]bool, len(state.Nodes)) + for _, node := range state.Nodes { + onlineNodes[node.Instance+"/"+node.Name] = node.Status == "online" + } + want := make(map[string]bool) + failed, worn := 0, 0 + for _, disk := range state.PhysicalDisks { + if !onlineNodes[disk.Instance+"/"+disk.Node] { + continue + } + if strings.EqualFold(disk.Health, "FAILED") { + want["disk-health "+disk.Instance+"/"+disk.Node+" "+disk.DevPath] = true + failed++ + } + if disk.Wearout >= 0 && disk.Wearout < 10 { + want["disk-wearout "+disk.Instance+"/"+disk.Node+" "+disk.DevPath] = true + worn++ + } + } + if failed == 0 || worn == 0 { + t.Fatalf("mock fixture has %d FAILED and %d worn disks on online nodes, want at least one of each", failed, worn) + } + + got := diskAlertKeys(manager) + for key := range got { + if !want[key] { + t.Errorf("mock pass raised unexpected disk alert %q", key) + } + } + for key := range want { + if !got[key] { + t.Errorf("mock pass raised no alert %q", key) + } + } +} + +func TestCheckMockPhysicalDiskAlertsFollowsPollerBoundary(t *testing.T) { + manager := newMockHostAlertTestManager(t) + monitor := &Monitor{alertManager: manager} + + nodes := []models.Node{ + {ID: "west-pve1", Name: "pve1", Instance: "west", Status: "online", LinkedAgentID: "host-west-pve1"}, + {ID: "east-pve1", Name: "pve1", Instance: "east", Status: "offline"}, + } + hosts := []models.Host{{ID: "host-west-pve1", Hostname: "pve1", Status: "online"}} + disks := []models.PhysicalDisk{ + {Instance: "west", Node: "pve1", DevPath: "/dev/sda", Model: "Crucial MX500 2TB", Type: "sata", Health: "FAILED", Wearout: -1}, + {Instance: "west", Node: "pve1", DevPath: "/dev/nvme0n1", Model: "Samsung 980 PRO 2TB", Type: "nvme", Health: "PASSED", Wearout: 6}, + // Same node name on another instance: that node is offline, so the + // poller would not reach it and its disk must not alert. + {Instance: "east", Node: "pve1", DevPath: "/dev/sdb", Model: "WD Red Pro 8TB", Type: "sata", Health: "FAILED", Wearout: -1}, + } + + monitor.checkMockPhysicalDiskAlerts(disks, nodes, hosts) + got := diskAlertKeys(manager) + if len(got) != 2 || !got["disk-health west/pve1 /dev/sda"] || !got["disk-wearout west/pve1 /dev/nvme0n1"] { + t.Fatalf("disk alerts = %v, want only FAILED health on west/pve1 /dev/sda and wearout on west/pve1 /dev/nvme0n1", got) + } + + // The linked agent's --disk-exclude forces matched devices healthy, so + // excluding a disk resolves the alerts it already raised. Endurance + // recovery needs three healthy-looking passes. + hosts[0].DiskExclude = []string{"/dev/sda", "nvme0n1"} + for i := 0; i < 3; i++ { + monitor.checkMockPhysicalDiskAlerts(disks, nodes, hosts) + } + if got := diskAlertKeys(manager); len(got) != 0 { + t.Fatalf("excluded devices kept disk alerts %v", got) + } + + // A node that leaves the estate takes its disk alerts with it through the + // node cleanup the mock pass runs first. + hosts[0].DiskExclude = nil + monitor.checkMockPhysicalDiskAlerts(disks, nodes, hosts) + if got := diskAlertKeys(manager); len(got) != 2 { + t.Fatalf("disk alerts after removing the exclusion = %v, want both west/pve1 alerts back", got) + } + manager.CleanupAlertsForNodes(map[string]bool{"pve2": true}) + if got := diskAlertKeys(manager); len(got) != 0 { + t.Fatalf("disk alerts outlived their departed node: %v", got) + } +} diff --git a/internal/monitoring/monitor_pve.go b/internal/monitoring/monitor_pve.go index 8b03fac7e0..581307f263 100644 --- a/internal/monitoring/monitor_pve.go +++ b/internal/monitoring/monitor_pve.go @@ -1175,15 +1175,7 @@ func (m *Monitor) maybePollPhysicalDisksAsync( Int("wearout", disk.Wearout). Msg("Checking disk health") - if excludePatterns, ok := diskExcludeByNode[disk.Node]; ok && fsfilters.MatchesDeviceExclude(disk.DevPath, excludePatterns) { - healthyDisk := proxmoxDiskFromPhysicalDisk(disk) - healthyDisk.Health = "PASSED" - healthyDisk.Wearout = 100 - m.alertManager.CheckDiskHealth(inst, disk.Node, healthyDisk) - continue - } - - m.alertManager.CheckDiskHealth(inst, disk.Node, proxmoxDiskFromPhysicalDisk(disk)) + m.checkPhysicalDiskAlerts(inst, disk, diskExcludeByNode[disk.Node]) } // Write SMART metrics to persistent store @@ -1380,6 +1372,21 @@ func preserveUnavailablePhysicalDiskEvidence(current, previous models.PhysicalDi return current } +// checkPhysicalDiskAlerts evaluates one Proxmox physical disk's health and +// wearout alerts. A device matched by the linked host agent's --disk-exclude +// patterns is evaluated as healthy, so excluding a disk also resolves the +// alerts it already raised. +func (m *Monitor) checkPhysicalDiskAlerts(instance string, disk models.PhysicalDisk, excludePatterns []string) { + if len(excludePatterns) > 0 && fsfilters.MatchesDeviceExclude(disk.DevPath, excludePatterns) { + healthyDisk := proxmoxDiskFromPhysicalDisk(disk) + healthyDisk.Health = "PASSED" + healthyDisk.Wearout = 100 + m.alertManager.CheckDiskHealth(instance, disk.Node, healthyDisk) + return + } + m.alertManager.CheckDiskHealth(instance, disk.Node, proxmoxDiskFromPhysicalDisk(disk)) +} + func proxmoxDiskFromPhysicalDisk(disk models.PhysicalDisk) proxmox.Disk { return proxmox.Disk{ DevPath: disk.DevPath,