From 36b2e5fa0a2c689f1ff94d5952d40d77c0f882cf Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:46:11 +0100 Subject: [PATCH] Clear agent disk temperature alerts when their threshold is off Turning off the agent disk temperature threshold left every open disk temperature alert active indefinitely. CheckHost's disabled branch was an empty placeholder, the host-wide clear paths (all agent alerts disabled, a disabled host override, agent removal, confirmed offline) matched only the /disk: prefix and missed /disk_temp:, and a config save skipped these alerts because diskTemperature was not classed as a threshold metric. CheckHost now clears a host's disk temperature alerts when the effective threshold is off, and the host-wide clears include them. A config save resolves them against the same threshold CheckHost evaluates, including the per-type DiskTempByType refinement. That way an unrelated save does not resolve an alert above its disk type's trigger only for the next report to raise it again. The save path now also matches the evaluator when there is no recovery band. buildCanonicalMetricSpec drops a clear level at or above the trigger, and the reducer keeps firing at the trigger itself, but reevaluateActiveAlertsLocked resolved any reading at or below that clear level. A per-type entry such as 70/75 (normalization keeps it) resolved a 72C alert on save, and the next report raised it again. With no recovery band, a reading exactly at the trigger was resolved the same way. --- .../v6/internal/subsystems/alerts.md | 35 ++++ internal/alerts/config_runtime.go | 10 +- internal/alerts/host.go | 78 +++++++-- internal/alerts/host_unraid_lifecycle_test.go | 159 ++++++++++++++++++ internal/alerts/metric_runtime.go | 4 +- 5 files changed, 266 insertions(+), 20 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index a951911e5f..bbfbba386d 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1592,6 +1592,41 @@ per-alert rate limiting; future notification-gating changes should extend that policy owner rather than burying new checks inside metric or resource-specific evaluators. +### Agent disk temperature alerts clear when their threshold is off + +Host-agent SMART disk temperature alerts (`diskTemperature`, resource +`agent:/disk_temp:`) follow one effective threshold: the +resolved agent `DiskTemperature`, refined by the disk type's `DiskTempByType` +entry unless an explicit host or linked-resource override set it +(`hostDiskTemperatureThresholdNoLock` in `internal/alerts/host.go`). When that +threshold is off, `CheckHost` clears the host's disk temperature alerts. The +all-agents switch, a disabled host override, agent removal and confirmed +agent offline clear them with the host's other disk alerts. Before this, +nothing cleared them once the agent-level threshold was off. + +A config save judges an open disk temperature alert against the same +threshold (`resolveHostAlertThresholdsNoLock` refines it by the alert's +`diskType` metadata, and `metric_runtime.go` classifies `diskTemperature` as a +threshold metric), so turning the threshold off resolves the alert +immediately. Every firing evaluation refreshes `diskType`, so an alert at or +above its trigger always carries it. Only an alert held in its recovery band +since before that metadata existed (May 2026) falls back to the agent +threshold. Its reading is already below its trigger, so the fallback can +resolve it early or leave it to the evaluator, but it cannot resolve an alert +the next report raises again. + +For every threshold metric, a config save resolves an alert whose reading is +below the new trigger. That includes readings inside a valid recovery band, +which the evaluator would hold. A clear level at or above the trigger is no +recovery band (`buildCanonicalMetricSpec` drops it), and a reading at the +trigger stays firing, as it does in the reducer, so a save no longer resolves +an alert the next evaluation raises again. +`TestCheckHostClearsDiskTemperatureAlertsWhenThresholdTurnsOff`, +`TestHostDiskTemperatureAlertsClearWhenAgentLeaves`, +`TestConfigSaveResolvesDiskTemperatureAlertAgainstItsDiskTypeThreshold` and +`TestConfigSaveKeepsAlertTheEvaluatorKeepsWithoutRecoveryBand` in +`internal/alerts/host_unraid_lifecycle_test.go` pin these paths. + ### Configured flapping thresholds remain reachable Every accepted positive `FlappingThreshold`, including values above ten, must diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go index 97260eca4e..67c46b52c3 100644 --- a/internal/alerts/config_runtime.go +++ b/internal/alerts/config_runtime.go @@ -579,12 +579,14 @@ func (m *Manager) reevaluateActiveAlertsLocked() { continue } + // buildCanonicalMetricSpec keeps a clear level only below the trigger, + // and without one the evaluator keeps firing at the trigger itself. + // A clear level at or above the trigger must not resolve a reading + // the next evaluation raises again. clearThreshold := threshold.Clear - if clearThreshold <= 0 { - clearThreshold = threshold.Trigger - } + hasRecoveryBand := clearThreshold > 0 && clearThreshold < threshold.Trigger - if alert.Value <= clearThreshold { + if hasRecoveryBand && alert.Value <= clearThreshold { alertsToResolve = append(alertsToResolve, alertID) log.Info(). Str("alertID", alertID). diff --git a/internal/alerts/host.go b/internal/alerts/host.go index 8e0e8c5516..6084b5c158 100644 --- a/internal/alerts/host.go +++ b/internal/alerts/host.go @@ -136,7 +136,16 @@ func (m *Manager) resolveHostAlertThresholdsNoLock(alert *Alert, resourceID stri linkedContainerID = metadataStringValue(alert.Metadata, "linkedContainerId") } - return m.resolveHostThresholdsNoLock(hostID, linkedNodeID, linkedVMID, linkedContainerID) + thresholds := m.resolveHostThresholdsNoLock(hostID, linkedNodeID, linkedVMID, linkedContainerID) + // A disk temperature alert is judged against the per-type threshold + // CheckHost evaluates for that disk, so a config save does not resolve an + // alert the next report would raise again. + if alert != nil && alert.Type == "diskTemperature" { + override, exists := m.hostThresholdOverrideNoLock(hostID, linkedNodeID, linkedVMID, linkedContainerID) + overridden := exists && override.DiskTemperature != nil + thresholds.DiskTemperature = m.hostDiskTemperatureThresholdNoLock(thresholds.DiskTemperature, overridden, metadataStringValue(alert.Metadata, "diskType")) + } + return thresholds } func sanitizeHostComponent(value string) string { @@ -343,18 +352,12 @@ func (m *Manager) CheckHost(host models.Host) { if len(host.Sensors.SMART) > 0 { for _, disk := range host.Sensors.SMART { if disk.Temperature > 0 && !disk.Standby { - effectiveTempThreshold := thresholds.DiskTemperature - if diskType := strings.ToLower(strings.TrimSpace(disk.Type)); diskType != "" && !diskTempOverridden { - m.mu.RLock() - if th, ok := m.config.DiskTempByType[diskType]; ok { - t := th - effectiveTempThreshold = &t - } - m.mu.RUnlock() - } + m.mu.RLock() + effectiveTempThreshold := m.hostDiskTemperatureThresholdNoLock(thresholds.DiskTemperature, diskTempOverridden, disk.Type) + m.mu.RUnlock() // Use specific resource ID for the disk: hostID/disk-temp:device - tempResourceID := fmt.Sprintf("%s/disk_temp:%s", hostResourceID(host.ID), sanitizeHostComponent(disk.Device)) + tempResourceID := hostDiskTemperatureResourcePrefix(host.ID) + sanitizeHostComponent(disk.Device) tempResourceName := fmt.Sprintf("%s (%s Temp)", hostDisplayName(host), disk.Device) diskTempMetadata := cloneMetadata(baseMetadata) @@ -379,10 +382,9 @@ func (m *Manager) CheckHost(host models.Host) { } } } else { - // We can't easily clear all disk temp alerts without tracking them, - // but checkMetric logic handles auto-resolution if value drops. - // If feature is disabled, ideally we should clear existing alerts. - // For now simple implementation. + // Disk temperature alerting is off for this host, so no later reading + // will resolve an alert it raised while it was on. + m.clearHostDiskTemperatureAlerts(host.ID) } seenDisks := make(map[string]struct{}, len(host.Disks)) @@ -828,6 +830,7 @@ func (m *Manager) HandleHostOfflineWithCorrelation(host models.Host, correlation diskResourcePrefixes := []string{ fmt.Sprintf("%s/disk:", resourceKey), + hostDiskTemperatureResourcePrefix(host.ID), } raidAlertPrefix := fmt.Sprintf("host-%s-raid-", host.ID) var alertsToClear []string @@ -884,6 +887,7 @@ func (m *Manager) clearHostDiskAlerts(hostID string) { prefixes := []string{ fmt.Sprintf("%s/disk:", hostResourceID(hostID)), + hostDiskTemperatureResourcePrefix(hostID), } m.mu.Lock() @@ -908,6 +912,50 @@ func (m *Manager) clearHostDiskAlerts(hostID string) { } } +// hostDiskTemperatureResourcePrefix is the resource ID prefix of every SMART +// disk temperature alert CheckHost raises for a host. +func hostDiskTemperatureResourcePrefix(hostID string) string { + return hostResourceID(hostID) + "/disk_temp:" +} + +// hostDiskTemperatureThresholdNoLock returns the threshold for one SMART disk +// given the host's resolved disk temperature threshold. An enabled threshold +// gives way to the disk type's DiskTempByType entry unless an explicit host or +// linked-resource override set it. Callers must hold m.mu. +func (m *Manager) hostDiskTemperatureThresholdNoLock(hostThreshold *HysteresisThreshold, overridden bool, diskType string) *HysteresisThreshold { + if hostThreshold == nil || hostThreshold.Trigger <= 0 || overridden { + return hostThreshold + } + if diskType = strings.ToLower(strings.TrimSpace(diskType)); diskType != "" { + if th, ok := m.config.DiskTempByType[diskType]; ok { + return &th + } + } + return hostThreshold +} + +// clearHostDiskTemperatureAlerts resolves every SMART disk temperature alert +// for a host. +func (m *Manager) clearHostDiskTemperatureAlerts(hostID string) { + if hostID == "" { + return + } + + resourcePrefix := hostDiskTemperatureResourcePrefix(hostID) + + m.mu.Lock() + defer m.mu.Unlock() + + for storageKey, alert := range m.activeAlerts { + if alert == nil || alert.Type != "diskTemperature" { + continue + } + if strings.HasPrefix(alert.ResourceID, resourcePrefix) { + m.clearAlertNoLock(storageKey) + } + } +} + var customSensorAssessmentCodes = []string{ "custom_sensor_warning", "custom_sensor_critical", diff --git a/internal/alerts/host_unraid_lifecycle_test.go b/internal/alerts/host_unraid_lifecycle_test.go index 3388516e35..300f35b26f 100644 --- a/internal/alerts/host_unraid_lifecycle_test.go +++ b/internal/alerts/host_unraid_lifecycle_test.go @@ -1,6 +1,7 @@ package alerts import ( + "fmt" "strings" "testing" "time" @@ -199,6 +200,164 @@ func TestHandleHostTelemetryExpiredPreservesStaticStorageRisk(t *testing.T) { }) } +func TestCheckHostClearsDiskTemperatureAlertsWhenThresholdTurnsOff(t *testing.T) { + cases := []struct { + name string + turnOff func(m *Manager, hostID string) + }{ + { + name: "agent default disabled", + turnOff: func(m *Manager, hostID string) { + m.config.AgentDefaults.DiskTemperature = &HysteresisThreshold{Trigger: 0, Clear: 0} + }, + }, + { + name: "host override disables disk temperature", + turnOff: func(m *Manager, hostID string) { + m.config.Overrides[hostID] = ThresholdConfig{DiskTemperature: &HysteresisThreshold{Trigger: 0, Clear: 0}} + }, + }, + { + name: "host override disables the agent", + turnOff: func(m *Manager, hostID string) { + m.config.Overrides[hostID] = ThresholdConfig{Disabled: true} + }, + }, + { + name: "all agent alerts disabled", + turnOff: func(m *Manager, hostID string) { + m.config.DisableAllAgents = true + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := configureDiskTempTypeHostManager(t) + host := hostWithSMARTDiskTemp("host-temp-off", "sata", 60) + alertID := hostDiskTempAlertID(host) + + m.CheckHost(host) + alert, exists := testLookupActiveAlert(t, m, alertID) + if !exists { + t.Fatalf("expected disk temperature alert at 60C (sata trigger 55), active: %v", alertKeys(m)) + } + if alert.Type != "diskTemperature" || !strings.HasPrefix(alert.ResourceID, hostDiskTemperatureResourcePrefix(host.ID)) { + t.Fatalf("disk temperature alert type/resource = %q/%q", alert.Type, alert.ResourceID) + } + + // Change config directly so only CheckHost can resolve the alert. + m.mu.Lock() + tc.turnOff(m, host.ID) + m.mu.Unlock() + + m.CheckHost(host) + if _, exists := testLookupActiveAlert(t, m, alertID); exists { + t.Fatalf("disk temperature alert stayed active after its threshold was turned off, active: %v", alertKeys(m)) + } + }) + } +} + +func TestHostDiskTemperatureAlertsClearWhenAgentLeaves(t *testing.T) { + t.Run("agent removed", func(t *testing.T) { + m := configureDiskTempTypeHostManager(t) + host := hostWithSMARTDiskTemp("host-temp-removed", "sata", 60) + m.CheckHost(host) + if _, exists := testLookupActiveAlert(t, m, hostDiskTempAlertID(host)); !exists { + t.Fatalf("expected disk temperature alert, active: %v", alertKeys(m)) + } + + m.HandleHostRemoved(host) + if _, exists := testLookupActiveAlert(t, m, hostDiskTempAlertID(host)); exists { + t.Fatalf("disk temperature alert outlived its removed agent, active: %v", alertKeys(m)) + } + }) + + t.Run("agent confirmed offline", func(t *testing.T) { + m := configureDiskTempTypeHostManager(t) + host := hostWithSMARTDiskTemp("host-temp-offline", "sata", 60) + m.CheckHost(host) + if _, exists := testLookupActiveAlert(t, m, hostDiskTempAlertID(host)); !exists { + t.Fatalf("expected disk temperature alert, active: %v", alertKeys(m)) + } + + for i := 0; i < 3; i++ { + m.HandleHostOffline(host) + } + if _, exists := testLookupActiveAlert(t, m, canonicalConnectivityStateID(hostResourceID(host.ID))); !exists { + t.Fatalf("expected confirmed offline alert, active: %v", alertKeys(m)) + } + if _, exists := testLookupActiveAlert(t, m, hostDiskTempAlertID(host)); exists { + t.Fatalf("disk temperature alert stayed beside the offline alert, active: %v", alertKeys(m)) + } + }) +} + +func TestConfigSaveResolvesDiskTemperatureAlertAgainstItsDiskTypeThreshold(t *testing.T) { + m := configureDiskTempTypeHostManager(t) + + m.mu.Lock() + m.config.AgentDefaults.DiskTemperature = &HysteresisThreshold{Trigger: 75, Clear: 70} + m.mu.Unlock() + + // 72C is above the nvme trigger (70) but below the agent default (75). + host := hostWithSMARTDiskTemp("host-temp-save", "nvme", 72) + alertID := hostDiskTempAlertID(host) + m.CheckHost(host) + if _, exists := testLookupActiveAlert(t, m, alertID); !exists { + t.Fatalf("expected nvme disk temperature alert at 72C, active: %v", alertKeys(m)) + } + + // An unrelated save keeps the alert CheckHost would raise again. + cfg := m.GetConfig() + cfg.AgentDefaults.CPU = &HysteresisThreshold{Trigger: 85, Clear: 80} + m.UpdateConfig(cfg) + if _, exists := testLookupActiveAlert(t, m, alertID); !exists { + t.Fatalf("unrelated config save resolved a disk temperature alert above its nvme trigger, active: %v", alertKeys(m)) + } + + // Turning disk temperature off resolves it on save, before any report. + cfg = m.GetConfig() + cfg.AgentDefaults.DiskTemperature = &HysteresisThreshold{Trigger: 0, Clear: 0} + m.UpdateConfig(cfg) + if _, exists := testLookupActiveAlert(t, m, alertID); exists { + t.Fatalf("disk temperature alert stayed active after the threshold was turned off, active: %v", alertKeys(m)) + } +} + +func TestConfigSaveKeepsAlertTheEvaluatorKeepsWithoutRecoveryBand(t *testing.T) { + // Normalization keeps a 70/75 entry, and the evaluator treats a clear + // level at or above the trigger as no recovery band, firing from 70C. + for _, reading := range []int{72, 70} { + t.Run(fmt.Sprintf("%dC", reading), func(t *testing.T) { + m := configureDiskTempTypeHostManager(t) + m.mu.Lock() + m.config.DiskTempByType["nvme"] = HysteresisThreshold{Trigger: 70, Clear: 75} + m.mu.Unlock() + + host := hostWithSMARTDiskTemp("host-temp-no-band", "nvme", reading) + alertID := hostDiskTempAlertID(host) + m.CheckHost(host) + if _, exists := testLookupActiveAlert(t, m, alertID); !exists { + t.Fatalf("expected nvme disk temperature alert at %dC, active: %v", reading, alertKeys(m)) + } + + cfg := m.GetConfig() + cfg.AgentDefaults.CPU = &HysteresisThreshold{Trigger: 85, Clear: 80} + m.UpdateConfig(cfg) + if _, exists := testLookupActiveAlert(t, m, alertID); !exists { + t.Fatalf("config save resolved an alert the evaluator keeps firing at %dC, active: %v", reading, alertKeys(m)) + } + + m.CheckHost(host) + if _, exists := testLookupActiveAlert(t, m, alertID); !exists { + t.Fatalf("expected the next report to keep the alert, active: %v", alertKeys(m)) + } + }) + } +} + func hasAlertType(alerts []Alert, alertType string) bool { for _, alert := range alerts { if alert.Type == alertType { diff --git a/internal/alerts/metric_runtime.go b/internal/alerts/metric_runtime.go index 6d1ad3cba2..bfe670920d 100644 --- a/internal/alerts/metric_runtime.go +++ b/internal/alerts/metric_runtime.go @@ -51,7 +51,7 @@ func metricStabilityFor(metricType string, delaySeconds int) metricStabilityPoli func isMetricThresholdAlertType(metricType string) bool { switch metricType { - case "cpu", "memory", "disk", "diskRead", "diskWrite", "networkIn", "networkOut", "temperature", "usage": + case "cpu", "memory", "disk", "diskRead", "diskWrite", "networkIn", "networkOut", "temperature", "diskTemperature", "usage": return true default: return false @@ -77,6 +77,8 @@ func getThresholdForMetric(config ThresholdConfig, metricType string) *Hysteresi return config.NetworkOut case "temperature": return config.Temperature + case "diskTemperature": + return config.DiskTemperature case "usage": return config.Usage default: