diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index aca39a8060..d954c272c6 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1808,10 +1808,15 @@ CPU, memory or disk usage metric it actually evaluates, and `CheckNode` releases its own copy of those metrics 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. `CheckHost` registers -the link only after evaluation, recording which metrics had a live threshold -(memory also needs a known reading, disk at least one evaluated filesystem), and -removes it while agent alerts are disabled; a metric the agent does not evaluate -stays with the node, so deduplication never leaves a machine unmonitored. +the link only after evaluation, recording which metrics the agent is configured +to evaluate (a live CPU or memory threshold; for disk, a live threshold on the +summary filesystem `models.SummaryDisk` picks, which is also what the linked +node's disk metric reports), and removes it while agent alerts are disabled. +Ownership follows configuration, not one report's data, so a missing agent +reading keeps the agent's alert open instead of handing the metric back to the +node for a cycle. With several agents linked to one node, a metric is owned when +any of them evaluates it. A metric no agent evaluates stays with the node, so +deduplication never leaves a machine unmonitored. 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 @@ -1832,7 +1837,8 @@ instance name in `Instance`. `TestCheckNodeMissingTemperatureDoesNotResolveOpenAlert`, `TestCheckNodeMissingTemperatureInterruptsTimingRuns` and `TestConfigSaveKeepsNodeTemperatureAlertOverTrigger` and -`TestCheckNodeKeepsUsageMetricsTheAgentDoesNotEvaluate` in +`TestCheckNodeKeepsUsageMetricsTheAgentDoesNotEvaluate`, +`TestCheckNodeUsageOwnershipFollowsWhatAgentsEvaluate` in `internal/alerts/threshold_resolution_shared_test.go`, and `TestHostAgentDeduplicationFollowsNodeLink` in `internal/alerts/host_dedup_test.go`, pin these rules. diff --git a/internal/alerts/host.go b/internal/alerts/host.go index 718cc75f9a..8cdd3e89dd 100644 --- a/internal/alerts/host.go +++ b/internal/alerts/host.go @@ -387,7 +387,13 @@ func (m *Manager) CheckHost(host models.Host) { } seenDisks := make(map[string]struct{}, len(host.Disks)) - evaluatesDiskUsage := false + // A linked node's disk metric is this agent's summary filesystem (root when + // reported), so the agent owns it only while it evaluates that filesystem. + summaryDiskResourceID := "" + if summary, ok := models.SummaryDisk(host.Disks); ok { + summaryDiskResourceID, _ = hostDiskResourceID(host, summary) + } + evaluatesSummaryDisk := false if len(host.Sensors.SMART) > 0 { for _, disk := range host.Sensors.SMART { diskResourceID, diskName := hostSMARTDiskResourceID(host, disk) @@ -445,7 +451,7 @@ func (m *Manager) CheckHost(host models.Host) { if effectiveDiskThreshold == nil { continue } - evaluatesDiskUsage = evaluatesDiskUsage || effectiveDiskThreshold.Trigger > 0 + evaluatesSummaryDisk = evaluatesSummaryDisk || (diskResourceID == summaryDiskResourceID && effectiveDiskThreshold.Trigger > 0) diskMetadata := cloneMetadata(baseMetadata) diskMetadata["metric"] = "disk" @@ -493,15 +499,18 @@ func (m *Manager) CheckHost(host models.Host) { m.cleanupHostDiskAlerts(host, seenDisks) - // The linked node releases exactly the usage metrics evaluated above and - // keeps the rest, so deduplication never leaves the machine unmonitored. + // The linked node releases exactly the usage metrics this agent is set up + // to evaluate and keeps the rest, so deduplication never leaves the machine + // unmonitored. Ownership follows configuration, not one report's data: a + // missing memory reading keeps the agent's alert open rather than handing + // memory back to the node for a cycle. m.registerHostAgentNodeLink(hostAgentNodeLink{ agentID: host.ID, agentName: resourceName, nodeID: host.LinkedNodeID, cpu: thresholds.CPU != nil && thresholds.CPU.Trigger > 0, - memory: thresholds.Memory != nil && thresholds.Memory.Trigger > 0 && host.Memory.HasKnownUsage(), - disk: evaluatesDiskUsage, + memory: thresholds.Memory != nil && thresholds.Memory.Trigger > 0, + disk: evaluatesSummaryDisk, }) if host.Unraid != nil { diff --git a/internal/alerts/node.go b/internal/alerts/node.go index 2c310e3d83..861e615ee8 100644 --- a/internal/alerts/node.go +++ b/internal/alerts/node.go @@ -263,9 +263,11 @@ func (m *Manager) unregisterHostAgentNodeLink(hostID string) { } } -// linkedHostAgentForNode returns the host agent link that owns usage metrics -// for the given Proxmox node. If several agents are linked to one node, the -// lowest agent ID wins so the choice is stable. +// linkedHostAgentForNode returns the host agent coverage for the given Proxmox +// node. A usage metric counts as owned when any agent linked to the node +// evaluates it, so a second linked agent covering a metric the first does not +// still keeps the node from alerting twice. The agent identity reported is the +// lowest agent ID among the linked agents, so the choice is stable. func (m *Manager) linkedHostAgentForNode(nodeID string) (hostAgentNodeLink, bool) { nodeID = strings.TrimSpace(nodeID) if nodeID == "" { @@ -280,9 +282,14 @@ func (m *Manager) linkedHostAgentForNode(nodeID string) (hostAgentNodeLink, bool continue } if !ok || link.agentID < found.agentID { - found = link - ok = true + found.agentID = link.agentID + found.agentName = link.agentName } + found.nodeID = nodeID + found.cpu = found.cpu || link.cpu + found.memory = found.memory || link.memory + found.disk = found.disk || link.disk + ok = true } return found, ok } diff --git a/internal/alerts/threshold_resolution_shared_test.go b/internal/alerts/threshold_resolution_shared_test.go index 3b3b3f1b44..bdffd75d22 100644 --- a/internal/alerts/threshold_resolution_shared_test.go +++ b/internal/alerts/threshold_resolution_shared_test.go @@ -420,6 +420,97 @@ func TestCheckNodeKeepsUsageMetricsTheAgentDoesNotEvaluate(t *testing.T) { }) } +// Ownership follows what the linked agents are configured to evaluate, not one +// report's data: a missing agent memory reading must not hand memory back to +// the node, a second linked agent's metrics count, and the node's disk metric +// belongs to the agent only when the agent evaluates that same filesystem. +func TestCheckNodeUsageOwnershipFollowsWhatAgentsEvaluate(t *testing.T) { + setup := func(t *testing.T) (*Manager, models.Node, models.Host) { + m := newTestManager(t) + m.mu.Lock() + m.config.Enabled = true + m.config.TimeThresholds = map[string]int{} + m.config.NodeDefaults.CPU = &HysteresisThreshold{Trigger: 80, Clear: 75} + m.config.NodeDefaults.Memory = &HysteresisThreshold{Trigger: 85, Clear: 80} + m.config.NodeDefaults.Disk = &HysteresisThreshold{Trigger: 90, Clear: 85} + m.config.AgentDefaults.CPU = &HysteresisThreshold{Trigger: 80, Clear: 75} + m.config.AgentDefaults.Memory = &HysteresisThreshold{Trigger: 85, Clear: 80} + m.config.AgentDefaults.Disk = &HysteresisThreshold{Trigger: 90, Clear: 85} + m.mu.Unlock() + node, host := testNodeWithHostAgent() + return m, node, host + } + + t.Run("missing_agent_memory_reading", func(t *testing.T) { + m, node, host := setup(t) + node.Memory = models.Memory{Total: 100, Used: 95, Free: 5, Usage: 95} + host.Memory = node.Memory + m.CheckHost(host) + m.CheckNode(node) + + host.Memory = models.Memory{Total: 100, UsageUnavailable: true} + m.CheckHost(host) + m.CheckNode(node) + agentAlertID := canonicalMetricStateID(hostResourceID(host.ID), "memory") + if got := testActiveAlertIDsOfType(m, "memory"); len(got) != 1 || got[0] != agentAlertID { + t.Fatalf("expected only the agent memory alert while its reading is missing, got %v", got) + } + }) + + t.Run("second_linked_agent", func(t *testing.T) { + m, node, host := setup(t) + node.CPU = 0.95 + m.mu.Lock() + m.config.Overrides = map[string]ThresholdConfig{ + "agent-a": {CPU: &HysteresisThreshold{Trigger: 0, Clear: 0}}, + } + m.mu.Unlock() + agentA := host + agentA.ID = "agent-a" + agentB := host + agentB.ID = "agent-b" + agentB.CPUUsage = 95 + m.CheckHost(agentA) + m.CheckHost(agentB) + m.CheckNode(node) + agentBAlertID := canonicalMetricStateID(hostResourceID(agentB.ID), "cpu") + if got := testActiveAlertIDsOfType(m, "cpu"); len(got) != 1 || got[0] != agentBAlertID { + t.Fatalf("expected only agent-b's CPU alert when it covers CPU for the node, got %v", got) + } + }) + + t.Run("summary_disk", func(t *testing.T) { + m, node, host := setup(t) + root := models.Disk{Mountpoint: "/", Device: "/dev/sda1", Total: 100, Used: 95, Free: 5, Usage: 95} + data := models.Disk{Mountpoint: "/data", Device: "/dev/sdb1", Total: 100, Used: 10, Free: 90, Usage: 10} + host.Disks = []models.Disk{root, data} + node.Disk = models.Disk{Total: 100, Used: 95, Free: 5, Usage: 95} + rootResourceID, _ := hostDiskResourceID(host, root) + nodeDiskAlertID := canonicalMetricStateID(node.ID, "disk") + + m.mu.Lock() + m.config.Overrides = map[string]ThresholdConfig{rootResourceID: {Disabled: true}} + m.mu.Unlock() + m.CheckHost(host) + m.CheckNode(node) + if !testHasActiveAlert(t, m, nodeDiskAlertID) { + t.Fatalf("expected the node to keep its root disk alert while the agent skips that filesystem") + } + + m.mu.Lock() + m.config.Overrides = nil + m.mu.Unlock() + m.CheckHost(host) + m.CheckNode(node) + if testHasActiveAlert(t, m, nodeDiskAlertID) { + t.Fatalf("expected the node to release its disk alert once the agent evaluates the same filesystem") + } + if !testHasActiveAlert(t, m, canonicalMetricStateID(rootResourceID, "disk")) { + t.Fatalf("expected the agent's root filesystem alert") + } + }) +} + // 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) {