From 7ee8c0d1e2458e94f67417f49923c1d5aa5572dc Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 14:34:09 +0100 Subject: [PATCH] Dedup node alerts by agent link and keep node thresholds on save A review of the node temperature fix found three more ways node alerts went wrong. Node deduplication matched a host agent's hostname against node names across every PVE instance. With two instances that each have a node called pve and an agent on only one of them, both nodes released their CPU, memory and disk alerts, so the second machine went unmonitored. An agent reporting an FQDN never matched its node, so that machine alerted twice, and an operator unlinking an agent did not hand the alerts back. Dedup now keys on the agent's LinkedNodeID, which the monitor already sets automatically or from an operator link. On pulse-prod the delly2 agent is linked to homelab-delly2, the node alert ID it should dedup. Saving the alert config resolved node temperature alerts that were still over their trigger. Reevaluation only treated an alert as a node alert when its Instance equalled its node name, but current node alerts keep the PVE instance name there, so they fell through to guest thresholds, which have no temperature threshold. Metadata resourceType "node" now decides, and the Instance heuristic only covers legacy alerts. A missing temperature reading no longer resolves an open alert, but a pending run still spanned the gap: one hot sample after ten minutes without readings fired at once, and a recovery run could complete the same way. The reducer gains InterruptMetricRun, which drops a pending run or restarts a recovery run without resolving anything, and the node temperature check calls it when the reading is missing. --- .../v6/internal/subsystems/alerts.md | 40 ++++-- internal/alerts/config_runtime.go | 11 +- internal/alerts/host.go | 22 ++-- internal/alerts/host_dedup_test.go | 99 ++++++++------- internal/alerts/manager.go | 10 +- internal/alerts/node.go | 117 ++++++++++++------ internal/alerts/reducer/reducer.go | 23 ++++ .../threshold_resolution_shared_test.go | 102 ++++++++++++++- 8 files changed, 305 insertions(+), 119 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 61495979e6..c385b63579 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1802,20 +1802,34 @@ owns node metric and temperature projection, node offline lifecycle handling, host-agent deduplication bookkeeping, and instance-scoped node display-name cache updates; future Proxmox node alert behavior should extend that resource checker owner rather than expanding the central Manager file. -When a host agent with the node's hostname is registered, the agent resource -owns the machine's CPU, memory and disk usage alerts, and `CheckNode` releases -its own copies through the disabled-threshold path every cycle: the pending run -is dropped and any node alert still open from before the agent registered is -resolved, never left frozen until the agent goes offline. CPU temperature has no -host-agent metric, so the node keeps evaluating it whether or not an agent is -registered; the node poll already merges agent sensor readings into -`node.Temperature`, giving one temperature alert per machine. A missing CPU -reading is not recovery evidence: with a live trigger, `checkNodeTemperature` -skips evaluation instead of feeding 0°C, while a disabled threshold still clears. +When a reporting host agent is linked to a node (`LinkedNodeID`, the monitor's +automatic or operator-set identity decision), the agent resource owns that +machine's CPU, memory and disk usage alerts, and `CheckNode` releases its own +copies through the disabled-threshold path every cycle: the pending run is +dropped and any node alert still open from before the link is resolved, never +left frozen until the agent goes offline. Deduplication keys on that link, +never on a hostname match, so a same-named node in another instance keeps its +alerts, an agent reporting an FQDN still dedups its node, and an operator unlink +hands the alerts back. CPU temperature has no host-agent metric, so the node +keeps evaluating it whether or not an agent is linked; the node poll already +merges agent sensor readings into `node.Temperature`, giving one temperature +alert per machine. A missing CPU reading is not evidence: with a live trigger, +`checkNodeTemperature` neither feeds 0°C nor resolves the open alert, but +`interruptMetricRun` (reducer `InterruptMetricRun`) drops a pending activation +run and restarts a recovery run, so no sustained-for or recovery delay spans a +gap in readings; a disabled threshold still clears. Config-save reevaluation +classifies alerts whose metadata `resourceType` is `node` as node alerts and +judges them against node thresholds; the `Instance` heuristic only covers legacy +alerts without a resource type, because current node alerts keep the PVE +instance name in `Instance`. `TestCheckNodeKeepsTemperatureAlertWhenHostAgentMonitorsNode`, -`TestCheckNodeReleasesOpenMetricAlertWhenHostAgentRegisters` and -`TestCheckNodeMissingTemperatureDoesNotResolveOpenAlert` in -`internal/alerts/threshold_resolution_shared_test.go` pin these rules. +`TestCheckNodeReleasesOpenMetricAlertWhenHostAgentRegisters`, +`TestCheckNodeMissingTemperatureDoesNotResolveOpenAlert`, +`TestCheckNodeMissingTemperatureInterruptsTimingRuns` and +`TestConfigSaveKeepsNodeTemperatureAlertOverTrigger` in +`internal/alerts/threshold_resolution_shared_test.go`, and +`TestHostAgentDeduplicationFollowsNodeLink` in +`internal/alerts/host_dedup_test.go`, pin these rules. Host-agent alert evaluation now lives in `internal/alerts/host.go`. That file owns host identity, host-agent metric projection, host disk/SMART/RAID/Unraid health handling, host cleanup, and host offline lifecycle handling; future host diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go index 17d0ecf11e..97260eca4e 100644 --- a/internal/alerts/config_runtime.go +++ b/internal/alerts/config_runtime.go @@ -487,9 +487,16 @@ func (m *Manager) reevaluateActiveAlertsLocked() { threshold = getThresholdForMetric(thresholds, metricType) } - isNodeResource := primaryResourceType == "" || primaryResourceType == "node" + // Node alerts carry resourceType "node" in metadata and keep the PVE + // instance name in Instance, so the metadata decides. The Instance + // heuristic only classifies legacy alerts without a resource type; + // applied to current node alerts it fell through to guest thresholds, + // which have no temperature threshold, and a config save resolved + // temperature alerts that were still over their trigger. + isNodeResource := primaryResourceType == "node" || + (primaryResourceType == "" && !strings.Contains(resourceID, ":") && (alert.Instance == "Node" || alert.Instance == alert.Node)) isStorageResource := alertResourceTypeKeysContain(resourceTypeKeys, "storage") - if threshold == nil && !handledModernPlatformType && isNodeResource && !strings.Contains(resourceID, ":") && (alert.Instance == "Node" || alert.Instance == alert.Node) { + if threshold == nil && !handledModernPlatformType && isNodeResource { if allDisabled, _ := m.alertPolicyTypeSwitchesNoLock("node"); allDisabled { alertsToResolve = append(alertsToResolve, alertID) continue diff --git a/internal/alerts/host.go b/internal/alerts/host.go index cc2969046f..8e0e8c5516 100644 --- a/internal/alerts/host.go +++ b/internal/alerts/host.go @@ -220,11 +220,9 @@ func (m *Manager) CheckHost(host models.Host) { return } - // Register this host agent hostname for deduplication with Proxmox nodes. - // This prevents duplicate alerts when both a Node and Host agent monitor the same machine. - if host.Hostname != "" { - m.RegisterHostAgentHostname(host.Hostname) - } + // Record the Proxmox node this agent is linked to, so the node releases the + // usage alerts the agent now owns instead of alerting twice for one machine. + m.registerHostAgentNodeLink(host) // Cache display name so host alerts show the user-configured name. m.UpdateNodeDisplayName("", host.Hostname, host.DisplayName) @@ -619,10 +617,8 @@ func (m *Manager) HandleHostRemoved(host models.Host) { return } - // Unregister the host agent hostname since it's being removed. - if host.Hostname != "" { - m.UnregisterHostAgentHostname(host.Hostname) - } + // The removed agent no longer owns its linked node's usage alerts. + m.unregisterHostAgentNodeLink(host.ID) m.HandleHostOnline(host) m.clearHostMetricAlerts(host.ID) @@ -729,11 +725,9 @@ func (m *Manager) HandleHostOfflineWithCorrelation(host models.Host, correlation return } - // Unregister the host agent hostname since it's no longer actively monitoring. - // This allows node alerts to resume if a Proxmox node with the same hostname exists. - if host.Hostname != "" { - m.UnregisterHostAgentHostname(host.Hostname) - } + // The agent is no longer actively monitoring, so its linked Proxmox node + // resumes evaluating its own usage alerts. + m.unregisterHostAgentNodeLink(host.ID) m.HandleHostTelemetryExpired(host) m.mu.RLock() diff --git a/internal/alerts/host_dedup_test.go b/internal/alerts/host_dedup_test.go index 66075acc32..a0b4ac3751 100644 --- a/internal/alerts/host_dedup_test.go +++ b/internal/alerts/host_dedup_test.go @@ -7,15 +7,17 @@ import ( ) func TestHostAgentDeduplicatesNodeAlerts(t *testing.T) { - // Test 1: Without a host agent registered, node metrics ARE checked + agent := models.Host{ID: "host-pi", Hostname: "pi", LinkedNodeID: "node/pi"} + + // Test 1: Without a linked host agent, node metrics ARE checked t.Run("node_metrics_checked_without_agent", func(t *testing.T) { m := NewManager() m.config.Enabled = true m.config.NodeDefaults.CPU = &HysteresisThreshold{Trigger: 80, Clear: 75} - // Verify no host agent is registered - if m.hasHostAgentForNode("pi") { - t.Error("Expected no host agent for 'pi' initially") + // Verify no host agent is linked + if m.hasHostAgentForNode("node/pi") { + t.Error("Expected no host agent for 'node/pi' initially") } node := models.Node{ @@ -38,18 +40,18 @@ func TestHostAgentDeduplicatesNodeAlerts(t *testing.T) { } }) - // Test 2: With host agent registered, node metrics are NOT checked + // Test 2: With a host agent linked to the node, node metrics are NOT checked t.Run("node_metrics_skipped_with_agent", func(t *testing.T) { m := NewManager() m.config.Enabled = true m.config.NodeDefaults.CPU = &HysteresisThreshold{Trigger: 80, Clear: 75} - // Register a host agent with the same hostname BEFORE checking the node - m.RegisterHostAgentHostname("pi") + // Link a host agent to the node BEFORE checking the node + m.registerHostAgentNodeLink(agent) - // Verify host agent IS registered - if !m.hasHostAgentForNode("pi") { - t.Error("Expected host agent for 'pi' to be registered") + // Verify host agent IS linked + if !m.hasHostAgentForNode("node/pi") { + t.Error("Expected host agent for 'node/pi' to be linked") } node := models.Node{ @@ -67,7 +69,7 @@ func TestHostAgentDeduplicatesNodeAlerts(t *testing.T) { m.mu.RUnlock() if hasPending { - t.Error("Expected NO pending alert for node CPU when host agent is registered") + t.Error("Expected NO pending alert for node CPU when host agent is linked") } }) @@ -78,12 +80,12 @@ func TestHostAgentDeduplicatesNodeAlerts(t *testing.T) { m.config.NodeDefaults.CPU = &HysteresisThreshold{Trigger: 80, Clear: 75} // Register and then unregister - m.RegisterHostAgentHostname("pi") - m.UnregisterHostAgentHostname("pi") + m.registerHostAgentNodeLink(agent) + m.unregisterHostAgentNodeLink(agent.ID) - // Verify host agent is NOT registered - if m.hasHostAgentForNode("pi") { - t.Error("Expected host agent for 'pi' to be unregistered") + // Verify host agent is NOT linked + if m.hasHostAgentForNode("node/pi") { + t.Error("Expected host agent for 'node/pi' to be unregistered") } node := models.Node{ @@ -106,67 +108,76 @@ func TestHostAgentDeduplicatesNodeAlerts(t *testing.T) { }) } -func TestHostAgentDeduplicationCaseInsensitive(t *testing.T) { +// Deduplication follows the agent's node link, never a hostname match: two +// instances can both have a node named "pve", and an agent can report an FQDN. +func TestHostAgentDeduplicationFollowsNodeLink(t *testing.T) { m := NewManager() - // Register with lowercase - m.RegisterHostAgentHostname("myhost") + m.registerHostAgentNodeLink(models.Host{ID: "host-a", Hostname: "pve.example.com", LinkedNodeID: "site-a-pve"}) + if !m.hasHostAgentForNode("site-a-pve") { + t.Error("Expected the linked node to dedup even though the agent hostname is an FQDN") + } + if m.hasHostAgentForNode("site-b-pve") { + t.Error("Expected a same-named node in another instance to keep its own alerts") + } - // Check should match regardless of case - if !m.hasHostAgentForNode("MYHOST") { - t.Error("Expected hasHostAgentForNode to match uppercase hostname") + m.registerHostAgentNodeLink(models.Host{ID: "host-b", Hostname: "pve"}) + if m.hasHostAgentForNode("site-b-pve") { + t.Error("Expected an unlinked agent with a matching hostname not to suppress node alerts") } - if !m.hasHostAgentForNode("MyHost") { - t.Error("Expected hasHostAgentForNode to match mixed-case hostname") - } - if !m.hasHostAgentForNode("myhost") { - t.Error("Expected hasHostAgentForNode to match lowercase hostname") + + // An operator unlinking the agent hands the node's alerts back. + m.registerHostAgentNodeLink(models.Host{ID: "host-a", Hostname: "pve.example.com"}) + if m.hasHostAgentForNode("site-a-pve") { + t.Error("Expected an unlinked agent to stop deduplicating its former node") } } -func TestCheckHostRegistersHostname(t *testing.T) { +func TestCheckHostRegistersNodeLink(t *testing.T) { m := NewManager() m.config.Enabled = true host := models.Host{ - ID: "host-test-123", - Hostname: "testhost", - CPUUsage: 50, + ID: "host-test-123", + Hostname: "testhost", + LinkedNodeID: "pve-testhost", + CPUUsage: 50, } - // Initially no hostname registered - if m.hasHostAgentForNode("testhost") { + // Initially no node link registered + if m.hasHostAgentForNode("pve-testhost") { t.Error("Expected no host agent registered initially") } - // CheckHost should register the hostname + // CheckHost should register the node link m.CheckHost(host) - if !m.hasHostAgentForNode("testhost") { - t.Error("Expected host agent hostname to be registered after CheckHost") + if !m.hasHostAgentForNode("pve-testhost") { + t.Error("Expected host agent node link to be registered after CheckHost") } } -func TestHandleHostOfflineUnregistersHostname(t *testing.T) { +func TestHandleHostOfflineUnregistersNodeLink(t *testing.T) { m := NewManager() m.config.Enabled = true host := models.Host{ - ID: "host-test-456", - Hostname: "offlinehost", + ID: "host-test-456", + Hostname: "offlinehost", + LinkedNodeID: "pve-offlinehost", } - // Register the hostname - m.RegisterHostAgentHostname(host.Hostname) + // Register the node link + m.registerHostAgentNodeLink(host) - if !m.hasHostAgentForNode("offlinehost") { + if !m.hasHostAgentForNode("pve-offlinehost") { t.Error("Expected host agent registered") } - // HandleHostOffline should unregister the hostname + // HandleHostOffline should unregister the node link m.HandleHostOffline(host) - if m.hasHostAgentForNode("offlinehost") { + if m.hasHostAgentForNode("pve-offlinehost") { t.Error("Expected host agent to be unregistered after HandleHostOffline") } } diff --git a/internal/alerts/manager.go b/internal/alerts/manager.go index 79aa6dc464..a511bc3691 100644 --- a/internal/alerts/manager.go +++ b/internal/alerts/manager.go @@ -101,10 +101,10 @@ type Manager struct { flappingActive map[string]bool // Track which alerts are currently in flapping state // Cleanup control cleanupStop chan struct{} // Signal to stop cleanup goroutine - // Host agent deduplication: track hostnames of active host agents - // When a host agent is running on a Proxmox node, we prefer the host agent - // alerts and suppress the node alerts to avoid duplicate monitoring. - hostAgentHostnames map[string]struct{} // Normalized hostnames (lowercase) + // Host agent deduplication: the Proxmox node each reporting host agent is + // linked to. While a link holds, the agent resource owns that machine's + // CPU, memory and disk usage alerts and the node releases its own copies. + hostAgentNodeLinks map[string]string // Host agent ID -> linked node ID // Node display name caches. Proxmox nodes can share the same raw node name // across multiple configured instances, so keep instance-scoped entries in // addition to the legacy raw-name cache used by instance-less resources. @@ -242,7 +242,7 @@ func NewManagerWithDataDir(dataDir string, options ...ManagerOption) *Manager { flappingHistory: make(map[string][]time.Time), flappingActive: make(map[string]bool), cleanupStop: make(chan struct{}), - hostAgentHostnames: make(map[string]struct{}), + hostAgentNodeLinks: make(map[string]string), nodeDisplayNames: make(map[string]string), instanceNodeDisplayNames: make(map[string]string), now: time.Now, diff --git a/internal/alerts/node.go b/internal/alerts/node.go index 3cf82f3524..5f9e0500b2 100644 --- a/internal/alerts/node.go +++ b/internal/alerts/node.go @@ -93,10 +93,10 @@ func (m *Manager) CheckNode(node models.Node) { m.clearNodeOfflineAlert(node) // Check each metric (only if node is online and reachable) - // Check for host agent deduplication: if a host agent is running on this node, + // Check for host agent deduplication: if a host agent is linked to this node, // CPU, memory and disk usage alerts belong to the agent resource (CheckHost // evaluates them), so the node releases its own copies instead of alerting twice. - if m.hasHostAgentForNode(node.Name) { + if m.hasHostAgentForNode(node.ID) { m.releaseNodeMetricAlerts(node, "cpu", "memory", "disk") } else { var memoryMetric *UnifiedResourceMetric @@ -126,7 +126,8 @@ func (m *Manager) CheckNode(node models.Node) { // checkNodeTemperature evaluates the node CPU temperature alert. // A disabled threshold (nil or trigger 0) is always passed through so any // existing alert is cleared. Otherwise a missing CPU reading is not evidence: -// feeding 0°C would resolve an open alert without proving the node cooled down. +// feeding 0°C would resolve an open alert without proving the node cooled down, +// so the open alert is kept and only its timing runs are interrupted. func (m *Manager) checkNodeTemperature(node models.Node, threshold *HysteresisThreshold) { var temp float64 if node.Temperature != nil && node.Temperature.Available { @@ -136,9 +137,6 @@ func (m *Manager) checkNodeTemperature(node models.Node, threshold *HysteresisTh temp = node.Temperature.CPUMax } } - if temp <= 0 && threshold != nil && threshold.Trigger > 0 { - return - } spec, err := buildCanonicalMetricSpec(node.ID, node.Name, unifiedresources.ResourceType("node"), "temperature", threshold) if err != nil { log.Warn(). @@ -148,9 +146,29 @@ func (m *Manager) checkNodeTemperature(node models.Node, threshold *HysteresisTh Msg("Skipping invalid canonical node temperature metric spec") return } + if temp <= 0 && !spec.Disabled { + m.interruptMetricRun(spec) + return + } m.checkMetricWithCanonicalSpec(spec, node.Name, node.Name, node.Instance, "node", temp, threshold, nil) } +// interruptMetricRun records a missing observation for a metric spec. The +// incident and any open alert are kept, but a pending activation run is +// dropped and a recovery run restarts, so a sustained-for or recovery delay +// never completes across a gap in evidence. +func (m *Manager) interruptMetricRun(spec alertspecs.ResourceAlertSpec) { + m.mu.Lock() + for _, state := range m.mirrorStatesNoLock() { + state.InterruptMetricRun(spec.ResourceID, spec.ID) + } + intentChanged := m.clearIntentPendingNoLock(canonicalTrackingKeyForSpec(spec, spec.ID)) + m.mu.Unlock() + if intentChanged { + m.saveActiveAlertsAsync("canonical metric intent pending state") + } +} + // releaseNodeMetricAlerts stops node-side evaluation of the given metrics the // same way a disabled threshold does: any pending run is dropped and any open // node alert is resolved. Without this, a node alert that was open when a host @@ -171,50 +189,71 @@ func (m *Manager) releaseNodeMetricAlerts(node models.Node, metrics ...string) { } } -// RegisterHostAgentHostname registers a host agent hostname for deduplication. -// When a host agent is actively monitoring a machine, we prefer its alerts -// over Proxmox node alerts to avoid duplicate monitoring of the same machine. -func (m *Manager) RegisterHostAgentHostname(hostname string) { - normalized := strings.ToLower(strings.TrimSpace(hostname)) - if normalized == "" { +// registerHostAgentNodeLink records which Proxmox node a reporting host agent +// is linked to. While the link holds, the agent resource owns that machine's +// CPU, memory and disk usage alerts. The link is the monitor's identity decision +// (automatic or operator-set), so an agent whose hostname merely matches a node +// name in another instance, or one an operator unlinked, never makes a node +// release its alerts, and an agent reporting an FQDN still dedups its node. +func (m *Manager) registerHostAgentNodeLink(host models.Host) { + hostID := strings.TrimSpace(host.ID) + if hostID == "" { + return + } + linkedNodeID := strings.TrimSpace(host.LinkedNodeID) + m.mu.Lock() + previous, existed := m.hostAgentNodeLinks[hostID] + if linkedNodeID == "" { + delete(m.hostAgentNodeLinks, hostID) + } else { + m.hostAgentNodeLinks[hostID] = linkedNodeID + } + m.mu.Unlock() + + if !existed || previous != linkedNodeID { + log.Debug(). + Str("hostID", hostID). + Str("hostname", host.Hostname). + Str("linkedNodeID", linkedNodeID). + Msg("Updated host agent node link for deduplication") + } +} + +// unregisterHostAgentNodeLink removes a host agent from deduplication tracking, +// so its linked node evaluates its own usage alerts again. +func (m *Manager) unregisterHostAgentNodeLink(hostID string) { + hostID = strings.TrimSpace(hostID) + if hostID == "" { return } m.mu.Lock() - m.hostAgentHostnames[normalized] = struct{}{} + linkedNodeID, existed := m.hostAgentNodeLinks[hostID] + delete(m.hostAgentNodeLinks, hostID) m.mu.Unlock() - log.Debug(). - Str("hostname", hostname). - Msg("Registered host agent hostname for deduplication") -} - -// UnregisterHostAgentHostname removes a host agent hostname from deduplication tracking. -func (m *Manager) UnregisterHostAgentHostname(hostname string) { - normalized := strings.ToLower(strings.TrimSpace(hostname)) - if normalized == "" { - return + if existed { + log.Debug(). + Str("hostID", hostID). + Str("linkedNodeID", linkedNodeID). + Msg("Removed host agent node link from deduplication") } - m.mu.Lock() - delete(m.hostAgentHostnames, normalized) - m.mu.Unlock() - - log.Debug(). - Str("hostname", hostname). - Msg("Unregistered host agent hostname from deduplication") } -// hasHostAgentForNode checks if a host agent is monitoring a machine with the same -// hostname as the given Proxmox node. If so, we should suppress node alerts to -// avoid duplicate alerting. -func (m *Manager) hasHostAgentForNode(nodeName string) bool { - normalized := strings.ToLower(strings.TrimSpace(nodeName)) - if normalized == "" { +// hasHostAgentForNode reports whether a reporting host agent is linked to the +// given Proxmox node, in which case the node's usage alerts belong to the agent. +func (m *Manager) hasHostAgentForNode(nodeID string) bool { + nodeID = strings.TrimSpace(nodeID) + if nodeID == "" { return false } m.mu.RLock() - _, exists := m.hostAgentHostnames[normalized] - m.mu.RUnlock() - return exists + defer m.mu.RUnlock() + for _, linkedNodeID := range m.hostAgentNodeLinks { + if linkedNodeID == nodeID { + return true + } + } + return false } // UpdateNodeDisplayName caches the display name for a node/host so alerts diff --git a/internal/alerts/reducer/reducer.go b/internal/alerts/reducer/reducer.go index 35cb37059f..4f8e98aa6c 100644 --- a/internal/alerts/reducer/reducer.go +++ b/internal/alerts/reducer/reducer.go @@ -583,6 +583,29 @@ func (s *State) Forget(resourceID, subKey string, at time.Time) { } } +// InterruptMetricRun ends the timing evidence for one metric key without +// resolving anything: a pending activation run is dropped and a firing +// incident's recovery run restarts. Evaluators call it when an observation is +// missing, so neither a sustained-for delay nor a recovery delay spans a gap +// in evidence. It reports whether a run was interrupted. +func (s *State) InterruptMetricRun(resourceID, subKey string) bool { + key := incidentKey(resourceID, subKey) + incident, ok := s.incidents[key] + if !ok { + return false + } + switch incident.State { + case StatePending: + delete(s.incidents, key) + return true + case StateFiring: + interrupted := !incident.RecoverySince.IsZero() + resetMetricRecoveryRun(incident) + return interrupted + } + return false +} + // ShiftResolved moves every resolved-occurrence timestamp by delta // (negative = older). It exists for tests and simulations that age // resolved records past RefireRetention; production code has no reason to diff --git a/internal/alerts/threshold_resolution_shared_test.go b/internal/alerts/threshold_resolution_shared_test.go index 77436fee92..540ab6aedd 100644 --- a/internal/alerts/threshold_resolution_shared_test.go +++ b/internal/alerts/threshold_resolution_shared_test.go @@ -265,8 +265,8 @@ func TestCheckNodeKeepsTemperatureAlertWhenHostAgentMonitorsNode(t *testing.T) { node, host := testNodeWithHostAgent() m.CheckHost(host) - if !m.hasHostAgentForNode(node.Name) { - t.Fatalf("expected CheckHost to register %q for node deduplication", host.Hostname) + if !m.hasHostAgentForNode(node.ID) { + t.Fatalf("expected CheckHost to link %q to node %q for deduplication", host.ID, node.ID) } node.Temperature = &models.Temperature{Available: true, CPUPackage: 90} @@ -359,6 +359,104 @@ func TestCheckNodeMissingTemperatureDoesNotResolveOpenAlert(t *testing.T) { } } +// A missing reading keeps the incident but must not let a sustained-for or +// recovery delay complete across the gap in evidence. +func TestCheckNodeMissingTemperatureInterruptsTimingRuns(t *testing.T) { + m := newTestManager(t) + m.mu.Lock() + m.config.Enabled = true + m.config.TimeThresholds = map[string]int{} + m.config.MetricTimeThresholds = map[string]map[string]int{"node": {"temperature": 300}} + m.config.NodeDefaults.Temperature = &HysteresisThreshold{Trigger: 80, Clear: 75} + m.mu.Unlock() + + node, _ := testNodeWithHostAgent() + tempAlertID := canonicalMetricStateID(node.ID, "temperature") + specID := canonicalMetricSpecID(node.ID, "temperature") + hot := &models.Temperature{Available: true, CPUPackage: 85} + + node.Temperature = hot + m.CheckNode(node) + m.mu.Lock() + pending := testCoreIsPending(m, node.ID, specID) + m.core.ShiftPending(-10 * time.Minute) + m.mu.Unlock() + if !pending { + t.Fatalf("expected a pending temperature run before the reading went missing") + } + + node.Temperature = nil + m.CheckNode(node) + node.Temperature = hot + m.CheckNode(node) + if testHasActiveAlert(t, m, tempAlertID) { + t.Fatalf("expected the sustained-for delay to restart after a missing reading, not fire on the first sample back") + } + + m.mu.Lock() + m.core.ShiftPending(-10 * time.Minute) + m.mu.Unlock() + m.CheckNode(node) + if !testHasActiveAlert(t, m, tempAlertID) { + t.Fatalf("expected the temperature alert to fire after a sustained run") + } + + node.Temperature = &models.Temperature{Available: true, CPUPackage: 60} + m.CheckNode(node) + m.mu.RLock() + incident, _ := m.core.Incident(node.ID, specID) + m.mu.RUnlock() + if incident.RecoverySince.IsZero() { + t.Fatalf("expected a recovery run to start below the clear threshold") + } + + node.Temperature = nil + m.CheckNode(node) + m.mu.RLock() + incident, _ = m.core.Incident(node.ID, specID) + m.mu.RUnlock() + if !incident.RecoverySince.IsZero() { + t.Fatalf("expected a missing reading to restart the recovery run") + } + if !testHasActiveAlert(t, m, tempAlertID) { + t.Fatalf("expected the temperature alert to stay open without a reading") + } +} + +// Node alerts keep the PVE instance name in Instance. A config save must judge +// them against node thresholds, not fall through to guest thresholds, which +// have no temperature threshold and used to resolve a live temperature alert. +func TestConfigSaveKeepsNodeTemperatureAlertOverTrigger(t *testing.T) { + m := newTestManager(t) + m.mu.Lock() + m.config.Enabled = true + m.config.TimeThresholds = map[string]int{} + m.config.NodeDefaults.Temperature = &HysteresisThreshold{Trigger: 80, Clear: 75} + m.mu.Unlock() + + node, _ := testNodeWithHostAgent() + node.Temperature = &models.Temperature{Available: true, CPUPackage: 90} + m.CheckNode(node) + tempAlertID := canonicalMetricStateID(node.ID, "temperature") + if !testHasActiveAlert(t, m, tempAlertID) { + t.Fatalf("expected node temperature alert %q", tempAlertID) + } + + m.UpdateConfig(m.GetConfig()) + if !testHasActiveAlert(t, m, tempAlertID) { + t.Fatalf("expected a config save to keep a node temperature alert still over its trigger") + } + + config := m.GetConfig() + config.Overrides = map[string]ThresholdConfig{ + node.ID: {Temperature: &HysteresisThreshold{Trigger: 95, Clear: 92}}, + } + m.UpdateConfig(config) + if testHasActiveAlert(t, m, tempAlertID) { + t.Fatalf("expected a node override raising the trigger above the reading to resolve the alert") + } +} + func TestReevaluateActiveAlertsUsesSharedAgentOverrideResolution(t *testing.T) { m := newTestManager(t)