Keep node temperature alerts when a host agent runs on the node (#2569)

Installing a Pulse agent on a Proxmox node silently ended CPU temperature alerting for that node. `CheckNode` skipped every node metric once a host agent with the node's hostname registered, temperature included. `CheckHost` evaluates CPU, memory and disk usage plus SMART disk temperatures, but never CPU temperature. On pulse-prod the last node temperature alert fired on 5 October. Since then delly2 has read 90°C and minipc 78-87°C with no alert; all three nodes run the agent. ## Changes - **Temperature stays with the node.** `CheckNode` evaluates CPU temperature whether or not an agent is registered. The node poll already merges the agent's sensor readings into `node.Temperature`, so each machine still gets one temperature alert. - **Handover instead of freeze.** CPU, memory and disk usage alerts still belong to the agent when one is registered. Previously the skip path never cleared node alerts it stopped evaluating, so a node alert open when the agent registered stayed open until the agent went offline. pulse-prod shows this today: the node memory alert `homelab-delly2::metric-threshold:memory`, open since 29 September, sits beside the agent's own delly2 memory alert. `CheckNode` now releases those node metrics through the disabled-threshold path, which drops any pending run and resolves the open alert. - **A missing reading is not recovery.** When the node had no CPU temperature reading, 0°C was fed to the evaluator, which could resolve an open temperature alert with nothing showing the node had cooled. With a live trigger, the check now skips evaluation instead. A disabled threshold still clears. ## Notes At handover, the released node alert closes through the normal resolve path, so channels with recovery notifications enabled get one resolve for it. This is deliberate: webhook and incident integrations close their incident by alert ID, so suppressing the resolve would leave the node incident open on the receiving side. The resolved record carries no recovery evidence. This happens once per machine, when its agent first registers or on upgrade to this build. ## Tests In `internal/alerts/threshold_resolution_shared_test.go`. All three fail on the current `main` and pass with this change: - `TestCheckNodeKeepsTemperatureAlertWhenHostAgentMonitorsNode`: with the agent registered, exactly one temperature alert fires, and it recovers. - `TestCheckNodeReleasesOpenMetricAlertWhenHostAgentRegisters`: a node memory alert that was open when the agent registered is released, no node incident remains in the reducer, and the agent's alert is the only memory alert. - `TestCheckNodeMissingTemperatureDoesNotResolveOpenAlert`: a nil, unavailable or empty reading keeps the alert open, and a disabled threshold still clears it. `go test ./internal/alerts/...` passes, and so do the monitoring alert, temperature and host-agent tests.
2026-10-24 12:42:49 +00:00 · 2026-10-06 14:03:29 +01:00
parent d4315cef51 33bcf698e0
commit 13d604e1c7
3 changed files with 206 additions and 26 deletions
@@ -1802,6 +1802,20 @@ 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.
+`TestCheckNodeKeepsTemperatureAlertWhenHostAgentMonitorsNode`,
+`TestCheckNodeReleasesOpenMetricAlertWhenHostAgentRegisters` and
+`TestCheckNodeMissingTemperatureDoesNotResolveOpenAlert` in
+`internal/alerts/threshold_resolution_shared_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
@@ -94,11 +94,10 @@ func (m *Manager) CheckNode(node models.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,
-			// prefer the host agent alerts and skip node metric alerts to avoid duplicates.
+			// 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) {
-				log.Debug().
-					Str("node", node.Name).
-					Msg("Skipping node metric alerts - host agent is monitoring this machine")
+				m.releaseNodeMetricAlerts(node, "cpu", "memory", "disk")
 			} else {
 				var memoryMetric *UnifiedResourceMetric
 				if node.Memory.HasKnownUsage() {
@@ -114,33 +113,64 @@ func (m *Manager) CheckNode(node models.Node) {
 					Memory:   memoryMetric,
 					Disk:     &UnifiedResourceMetric{Percent: node.Disk.Usage},
 				}, thresholds, nil)
-
-				// Check temperature if available
-				// We pass the check unconditionally so that if the threshold triggers are disabled (set to 0),
-				// any existing alerts will be properly cleared.
-				var temp float64
-				if node.Temperature != nil && node.Temperature.Available {
-					// Use CPU package temp if available, otherwise use max core temp
-					temp = node.Temperature.CPUPackage
-					if temp == 0 {
-						temp = node.Temperature.CPUMax
-					}
-				}
-				spec, err := buildCanonicalMetricSpec(node.ID, node.Name, unifiedresources.ResourceType("node"), "temperature", thresholds.Temperature)
-				if err != nil {
-					log.Warn().
-						Err(err).
-						Str("resourceID", node.ID).
-						Str("node", node.Name).
-						Msg("Skipping invalid canonical node temperature metric spec")
-				} else {
-					m.checkMetricWithCanonicalSpec(spec, node.Name, node.Name, node.Instance, "node", temp, thresholds.Temperature, nil)
-				}
 			}
+
+			// CPU temperature stays with the node even when a host agent runs on it:
+			// CheckHost has no CPU temperature metric, and the node poll already
+			// merges the agent's sensor readings into node.Temperature.
+			m.checkNodeTemperature(node, thresholds.Temperature)
 		}
 	}
 }

+// 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.
+func (m *Manager) checkNodeTemperature(node models.Node, threshold *HysteresisThreshold) {
+	var temp float64
+	if node.Temperature != nil && node.Temperature.Available {
+		// Use CPU package temp if available, otherwise use max core temp
+		temp = node.Temperature.CPUPackage
+		if temp == 0 {
+			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().
+			Err(err).
+			Str("resourceID", node.ID).
+			Str("node", node.Name).
+			Msg("Skipping invalid canonical node temperature metric spec")
+		return
+	}
+	m.checkMetricWithCanonicalSpec(spec, node.Name, node.Name, node.Instance, "node", temp, threshold, nil)
+}
+
+// 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
+// agent registered would stay frozen until the agent went offline.
+func (m *Manager) releaseNodeMetricAlerts(node models.Node, metrics ...string) {
+	for _, metric := range metrics {
+		spec, err := buildCanonicalMetricSpec(node.ID, node.Name, unifiedresources.ResourceType("node"), metric, nil)
+		if err != nil {
+			log.Warn().
+				Err(err).
+				Str("resourceID", node.ID).
+				Str("node", node.Name).
+				Str("metric", metric).
+				Msg("Skipping invalid canonical node metric spec")
+			continue
+		}
+		m.checkMetricWithCanonicalSpec(spec, node.Name, node.Name, node.Instance, "node", 0, nil, nil)
+	}
+}
+
 // 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.
@@ -223,6 +223,142 @@ func TestCheckNodeDisabledOverrideClearsExistingAlerts(t *testing.T) {
 	}
 }

+func testActiveAlertIDsOfType(m *Manager, alertType string) []string {
+	var ids []string
+	for _, alert := range m.GetActiveAlerts() {
+		if alert.Type == alertType {
+			ids = append(ids, alert.ID)
+		}
+	}
+	return ids
+}
+
+func testNodeWithHostAgent() (models.Node, models.Host) {
+	node := models.Node{
+		ID:       "homelab-delly2",
+		Name:     "delly2",
+		Instance: "homelab",
+		Status:   "online",
+		CPU:      0.10,
+		Memory:   models.Memory{Total: 100, Used: 40, Free: 60, Usage: 40},
+		Disk:     models.Disk{Usage: 30},
+	}
+	host := models.Host{
+		ID:           "agent-delly2",
+		Hostname:     "delly2",
+		LinkedNodeID: node.ID,
+		CPUUsage:     10,
+		Memory:       models.Memory{Total: 100, Used: 40, Free: 60, Usage: 40},
+	}
+	return node, host
+}
+
+// A host agent on a Proxmox node takes over CPU, memory and disk alerts, but it
+// has no CPU temperature metric, so the node must keep raising temperature.
+func TestCheckNodeKeepsTemperatureAlertWhenHostAgentMonitorsNode(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, host := testNodeWithHostAgent()
+	m.CheckHost(host)
+	if !m.hasHostAgentForNode(node.Name) {
+		t.Fatalf("expected CheckHost to register %q for node deduplication", host.Hostname)
+	}
+
+	node.Temperature = &models.Temperature{Available: true, CPUPackage: 90}
+	m.CheckNode(node)
+	m.CheckHost(host)
+
+	tempAlertID := canonicalMetricStateID(node.ID, "temperature")
+	if got := testActiveAlertIDsOfType(m, "temperature"); len(got) != 1 || got[0] != tempAlertID {
+		t.Fatalf("expected exactly one temperature alert %q, got %v", tempAlertID, got)
+	}
+
+	node.Temperature = &models.Temperature{Available: true, CPUPackage: 60}
+	m.CheckNode(node)
+	if got := testActiveAlertIDsOfType(m, "temperature"); len(got) != 0 {
+		t.Fatalf("expected node temperature alert to recover while the agent is registered, got %v", got)
+	}
+}
+
+// A node alert that was open when the host agent registered must be handed
+// over, not frozen until the agent goes offline.
+func TestCheckNodeReleasesOpenMetricAlertWhenHostAgentRegisters(t *testing.T) {
+	m := newTestManager(t)
+	m.mu.Lock()
+	m.config.Enabled = true
+	m.config.TimeThresholds = map[string]int{}
+	m.config.NodeDefaults.Memory = &HysteresisThreshold{Trigger: 85, Clear: 80}
+	m.config.AgentDefaults.Memory = &HysteresisThreshold{Trigger: 85, Clear: 80}
+	m.mu.Unlock()
+
+	node, host := testNodeWithHostAgent()
+	node.Memory = models.Memory{Total: 100, Used: 95, Free: 5, Usage: 95}
+	host.Memory = node.Memory
+
+	m.CheckNode(node)
+	nodeAlertID := canonicalMetricStateID(node.ID, "memory")
+	if !testHasActiveAlert(t, m, nodeAlertID) {
+		t.Fatalf("expected node memory alert %q before the agent registers", nodeAlertID)
+	}
+
+	m.CheckHost(host)
+	m.CheckNode(node)
+
+	if testHasActiveAlert(t, m, nodeAlertID) {
+		t.Fatalf("expected node memory alert %q to be released once the host agent owns memory", nodeAlertID)
+	}
+	m.mu.RLock()
+	nodeIncident := testCoreHasIncident(m, node.ID, canonicalMetricSpecID(node.ID, "memory"))
+	m.mu.RUnlock()
+	if nodeIncident {
+		t.Fatalf("expected no node memory incident left in the reducer core after handover")
+	}
+	agentAlertID := canonicalMetricStateID(hostResourceID(host.ID), "memory")
+	if got := testActiveAlertIDsOfType(m, "memory"); len(got) != 1 || got[0] != agentAlertID {
+		t.Fatalf("expected exactly one memory alert %q, got %v", agentAlertID, got)
+	}
+}
+
+// Missing temperature telemetry is not a reading: it must not resolve an open
+// node temperature alert, while a disabled threshold still clears it.
+func TestCheckNodeMissingTemperatureDoesNotResolveOpenAlert(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)
+	}
+
+	for _, missing := range []*models.Temperature{nil, {Available: false}, {Available: true}} {
+		node.Temperature = missing
+		m.CheckNode(node)
+		if !testHasActiveAlert(t, m, tempAlertID) {
+			t.Fatalf("expected temperature alert to stay open without a reading (temperature %+v)", missing)
+		}
+	}
+
+	m.mu.Lock()
+	m.config.NodeDefaults.Temperature = &HysteresisThreshold{Trigger: 0, Clear: 0}
+	m.mu.Unlock()
+	m.CheckNode(node)
+	if testHasActiveAlert(t, m, tempAlertID) {
+		t.Fatalf("expected disabled temperature threshold to clear %q without a reading", tempAlertID)
+	}
+}
+
 func TestReevaluateActiveAlertsUsesSharedAgentOverrideResolution(t *testing.T) {
 	m := newTestManager(t)