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)