From 33bcf698e036cd4c6a82b369da7b30e9415a1155 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 13:43:47 +0100 Subject: [PATCH] Keep node temperature alerts when a host agent runs on the node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Installing a Pulse agent on a Proxmox node silently ended CPU temperature alerting for it. CheckNode skipped every node metric, temperature included, once a host agent with the node's hostname registered, but CheckHost evaluates only CPU, memory and disk usage and SMART disk temperatures, never CPU temperature. On pulse-prod the last node temperature alert fired on 5 October; delly2 has since read 90°C and minipc 78-87°C with no alert. Temperature now stays with the node whether or not an agent is registered. The node poll already merges the agent's sensors into node.Temperature, so this gives one temperature alert per machine. The dedup skip path also never cleared node alerts it stopped evaluating, so a node alert open when the agent registered stayed frozen until the agent went offline. pulse-prod shows this today: the delly2 node memory alert, open since 29 September, beside the agent's own memory alert. CheckNode now releases node CPU, memory and disk alerts through the disabled-threshold path, which drops the pending run and resolves the open alert, leaving the agent as the only owner. A missing CPU reading also fed 0°C into the evaluator, which could resolve an open temperature alert without any evidence that the node had cooled. With a live trigger the check now skips the evaluation instead, while a disabled threshold still clears. --- .../v6/internal/subsystems/alerts.md | 14 ++ internal/alerts/node.go | 82 +++++++---- .../threshold_resolution_shared_test.go | 136 ++++++++++++++++++ 3 files changed, 206 insertions(+), 26 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index cb03c572e2..61495979e6 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -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 diff --git a/internal/alerts/node.go b/internal/alerts/node.go index 346b28ef1f..3cf82f3524 100644 --- a/internal/alerts/node.go +++ b/internal/alerts/node.go @@ -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. diff --git a/internal/alerts/threshold_resolution_shared_test.go b/internal/alerts/threshold_resolution_shared_test.go index 65e5ee2660..77436fee92 100644 --- a/internal/alerts/threshold_resolution_shared_test.go +++ b/internal/alerts/threshold_resolution_shared_test.go @@ -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)