From 190e02fbbcd299a6828c98acb23a79b2a6ce8e62 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sun, 7 Dec 2025 12:38:38 +0000 Subject: [PATCH] Add host alert deduplication with tests - Modified alerts.go for improved host alert handling - Added host_dedup_test.go for deduplication test coverage --- internal/alerts/alerts.go | 97 ++++++++++++++-- internal/alerts/host_dedup_test.go | 172 +++++++++++++++++++++++++++++ 2 files changed, 259 insertions(+), 10 deletions(-) create mode 100644 internal/alerts/host_dedup_test.go diff --git a/internal/alerts/alerts.go b/internal/alerts/alerts.go index 91d2c6d..8573117 100644 --- a/internal/alerts/alerts.go +++ b/internal/alerts/alerts.go @@ -516,6 +516,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) } type ackRecord struct { @@ -555,6 +559,7 @@ func NewManager() *Manager { flappingHistory: make(map[string][]time.Time), flappingActive: make(map[string]bool), cleanupStop: make(chan struct{}), + hostAgentHostnames: make(map[string]struct{}), config: AlertConfig{ Enabled: true, ActivationState: ActivationPending, @@ -2345,22 +2350,77 @@ func (m *Manager) CheckNode(node models.Node) { // Check each metric (only if node is online) - checkMetric will skip if threshold is nil or <= 0 if node.Status != "offline" { - m.checkMetric(node.ID, node.Name, node.Name, node.Instance, "Node", "cpu", node.CPU*100, thresholds.CPU, nil) - m.checkMetric(node.ID, node.Name, node.Name, node.Instance, "Node", "memory", node.Memory.Usage, thresholds.Memory, nil) - m.checkMetric(node.ID, node.Name, node.Name, node.Instance, "Node", "disk", node.Disk.Usage, thresholds.Disk, nil) + // 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. + if m.hasHostAgentForNode(node.Name) { + log.Debug(). + Str("node", node.Name). + Msg("Skipping node metric alerts - host agent is monitoring this machine") + } else { + m.checkMetric(node.ID, node.Name, node.Name, node.Instance, "Node", "cpu", node.CPU*100, thresholds.CPU, nil) + m.checkMetric(node.ID, node.Name, node.Name, node.Instance, "Node", "memory", node.Memory.Usage, thresholds.Memory, nil) + m.checkMetric(node.ID, node.Name, node.Name, node.Instance, "Node", "disk", node.Disk.Usage, thresholds.Disk, nil) - // Check temperature if available - if node.Temperature != nil && node.Temperature.Available && thresholds.Temperature != nil { - // Use CPU package temp if available, otherwise use max core temp - temp := node.Temperature.CPUPackage - if temp == 0 { - temp = node.Temperature.CPUMax + // Check temperature if available + if node.Temperature != nil && node.Temperature.Available && thresholds.Temperature != nil { + // Use CPU package temp if available, otherwise use max core temp + temp := node.Temperature.CPUPackage + if temp == 0 { + temp = node.Temperature.CPUMax + } + m.checkMetric(node.ID, node.Name, node.Name, node.Instance, "Node", "temperature", temp, thresholds.Temperature, nil) } - m.checkMetric(node.ID, node.Name, node.Name, node.Instance, "Node", "temperature", temp, thresholds.Temperature, 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. +func (m *Manager) RegisterHostAgentHostname(hostname string) { + normalized := strings.ToLower(strings.TrimSpace(hostname)) + if normalized == "" { + return + } + m.mu.Lock() + m.hostAgentHostnames[normalized] = struct{}{} + 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 + } + 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 == "" { + return false + } + m.mu.RLock() + _, exists := m.hostAgentHostnames[normalized] + m.mu.RUnlock() + return exists +} + + func hostResourceID(hostID string) string { trimmed := strings.TrimSpace(hostID) if trimmed == "" { @@ -2449,6 +2509,12 @@ 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) + } + // Fresh telemetry marks the host as online and clears offline tracking. m.HandleHostOnline(host) @@ -2697,6 +2763,11 @@ 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) + } + m.HandleHostOnline(host) m.clearHostMetricAlerts(host.ID) m.clearHostDiskAlerts(host.ID) @@ -2708,6 +2779,12 @@ func (m *Manager) HandleHostOffline(host models.Host) { 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) + } + m.mu.RLock() if !m.config.Enabled { m.mu.RUnlock() diff --git a/internal/alerts/host_dedup_test.go b/internal/alerts/host_dedup_test.go new file mode 100644 index 0000000..bdc6e17 --- /dev/null +++ b/internal/alerts/host_dedup_test.go @@ -0,0 +1,172 @@ +package alerts + +import ( + "testing" + + "github.com/rcourtman/pulse-go-rewrite/internal/models" +) + +func TestHostAgentDeduplicatesNodeAlerts(t *testing.T) { + // Test 1: Without a host agent registered, node metrics ARE checked + t.Run("node_metrics_checked_without_host_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") + } + + node := models.Node{ + ID: "node/pi", + Name: "pi", + Status: "online", + CPU: 0.95, // 95% CPU + } + + // This should attempt to check metrics (even if alert doesn't fire immediately due to time thresholds) + m.CheckNode(node) + + // The key test: pendingAlerts should have an entry because metrics WERE checked + m.mu.RLock() + _, hasPending := m.pendingAlerts["node/pi-cpu"] + m.mu.RUnlock() + + if !hasPending { + t.Error("Expected pending alert for node CPU when no host agent registered") + } + }) + + // Test 2: With host agent registered, node metrics are NOT checked + t.Run("node_metrics_skipped_with_host_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") + + // Verify host agent IS registered + if !m.hasHostAgentForNode("pi") { + t.Error("Expected host agent for 'pi' to be registered") + } + + node := models.Node{ + ID: "node/pi", + Name: "pi", + Status: "online", + CPU: 0.95, // 95% CPU + } + + m.CheckNode(node) + + // The key test: pendingAlerts should NOT have an entry because metrics were SKIPPED + m.mu.RLock() + _, hasPending := m.pendingAlerts["node/pi-cpu"] + m.mu.RUnlock() + + if hasPending { + t.Error("Expected NO pending alert for node CPU when host agent is registered") + } + }) + + // Test 3: After unregistering host agent, node metrics ARE checked again + t.Run("node_metrics_resume_after_host_agent_unregistered", func(t *testing.T) { + m := NewManager() + m.config.Enabled = true + m.config.NodeDefaults.CPU = &HysteresisThreshold{Trigger: 80, Clear: 75} + + // Register and then unregister + m.RegisterHostAgentHostname("pi") + m.UnregisterHostAgentHostname("pi") + + // Verify host agent is NOT registered + if m.hasHostAgentForNode("pi") { + t.Error("Expected host agent for 'pi' to be unregistered") + } + + node := models.Node{ + ID: "node/pi", + Name: "pi", + Status: "online", + CPU: 0.95, // 95% CPU + } + + m.CheckNode(node) + + // The key test: pendingAlerts should have an entry because metrics WERE checked + m.mu.RLock() + _, hasPending := m.pendingAlerts["node/pi-cpu"] + m.mu.RUnlock() + + if !hasPending { + t.Error("Expected pending alert for node CPU after host agent unregistration") + } + }) +} + +func TestHostAgentDeduplicationCaseInsensitive(t *testing.T) { + m := NewManager() + + // Register with lowercase + m.RegisterHostAgentHostname("myhost") + + // Check should match regardless of case + if !m.hasHostAgentForNode("MYHOST") { + t.Error("Expected hasHostAgentForNode to match uppercase hostname") + } + 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") + } +} + +func TestCheckHostRegistersHostname(t *testing.T) { + m := NewManager() + m.config.Enabled = true + + host := models.Host{ + ID: "host-test-123", + Hostname: "testhost", + CPUUsage: 50, + } + + // Initially no hostname registered + if m.hasHostAgentForNode("testhost") { + t.Error("Expected no host agent registered initially") + } + + // CheckHost should register the hostname + m.CheckHost(host) + + if !m.hasHostAgentForNode("testhost") { + t.Error("Expected host agent hostname to be registered after CheckHost") + } +} + +func TestHandleHostOfflineUnregistersHostname(t *testing.T) { + m := NewManager() + m.config.Enabled = true + + host := models.Host{ + ID: "host-test-456", + Hostname: "offlinehost", + } + + // Register the hostname + m.RegisterHostAgentHostname(host.Hostname) + + if !m.hasHostAgentForNode("offlinehost") { + t.Error("Expected host agent registered") + } + + // HandleHostOffline should unregister the hostname + m.HandleHostOffline(host) + + if m.hasHostAgentForNode("offlinehost") { + t.Error("Expected host agent to be unregistered after HandleHostOffline") + } +}