diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 63c1a7dcd..c4c6b717d 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -189,6 +189,13 @@ forbidden. Per-platform defaults, per-resource overrides, global disables, active-alert reevaluation, history persistence, and notification delivery must use the same alert configuration shape rather than a platform-specific sidecar. +Notification cadence is part of that runtime contract. `Schedule.Cooldown` and +`Schedule.MaxAlertsHour` apply to already-active alert re-notifications as well +as first-fire creation, including canonical metric alerts, legacy metric paths, +and severity-change re-notifications. Accepted alert dispatch must record +`LastNotified` back onto the live active-alert state before persistence, even +when a restored or replayed alert is dispatched through a clone, so reloads do +not reopen the same alert's notification window. The browser thresholds surface is also platform-shaped: Proxmox, Docker, Kubernetes, TrueNAS, vSphere, PBS, PMG, and Systems. It must use the shared FilterBar chip and "+ Filter" pattern for resource filtering, and alert tables diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index 7d65c5796..6208dad94 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -17281,6 +17281,47 @@ func TestDispatchAlert(t *testing.T) { } }) + t.Run("records last notified on active alert when dispatched clone succeeds", func(t *testing.T) { + m := newTestManager(t) + + m.SetAlertCallback(func(a *Alert) {}) + + m.mu.Lock() + m.config.ActivationState = ActivationActive + active := &Alert{ + ID: "test-alert", + Type: "cpu", + ResourceName: "testvm", + } + m.setActiveAlertNoLock(active.ID, active) + m.mu.Unlock() + + dispatched := active.Clone() + if dispatched == nil { + t.Fatal("expected alert clone") + } + + if !m.dispatchAlert(dispatched, false) { + t.Fatal("expected dispatch to succeed") + } + if dispatched.LastNotified == nil { + t.Fatal("expected dispatched clone to record LastNotified") + } + + m.mu.RLock() + stored, exists := m.getActiveAlertNoLock(active.ID) + m.mu.RUnlock() + if !exists || stored == nil { + t.Fatalf("expected active alert %s to remain stored", active.ID) + } + if stored.LastNotified == nil { + t.Fatal("expected active alert to record LastNotified") + } + if !stored.LastNotified.Equal(*dispatched.LastNotified) { + t.Fatalf("active LastNotified = %v, want %v", stored.LastNotified, dispatched.LastNotified) + } + }) + t.Run("dispatches asynchronously when async is true", func(t *testing.T) { // t.Parallel() m := newTestManager(t) diff --git a/internal/alerts/canonical_metric.go b/internal/alerts/canonical_metric.go index 23b29d404..d1247c695 100644 --- a/internal/alerts/canonical_metric.go +++ b/internal/alerts/canonical_metric.go @@ -266,9 +266,9 @@ func (m *Manager) evaluateCanonicalMetricAlert(spec alertspecs.ResourceAlertSpec shouldRenotify := false if existingAlert.Acknowledged { } else if m.shouldNotifyAfterCooldown(existingAlert) { - shouldRenotify = true + shouldRenotify = m.allowNotificationByRateLimit(trackingKey, existingAlert, "cooldown") } else if oldLevel != existingAlert.Level && existingAlert.Level == AlertLevelCritical { - shouldRenotify = true + shouldRenotify = m.allowNotificationByRateLimit(trackingKey, existingAlert, "critical-escalation") } if shouldRenotify && m.getAlertCallback() != nil { diff --git a/internal/alerts/metric_runtime.go b/internal/alerts/metric_runtime.go index 5cd85dcdb..bb9f149ff 100644 --- a/internal/alerts/metric_runtime.go +++ b/internal/alerts/metric_runtime.go @@ -445,17 +445,21 @@ func (m *Manager) checkMetric(resourceID, resourceName, node, instance, resource Str("alertID", alertID). Msg("Alert is acknowledged, skipping re-notification") } else if m.shouldNotifyAfterCooldown(existingAlert) { - shouldRenotify = true - log.Debug(). - Str("alertID", alertID). - Dur("cooldown", time.Duration(m.config.Schedule.Cooldown)*time.Minute). - Msg("Cooldown period has passed, will re-notify") + shouldRenotify = m.allowNotificationByRateLimit(trackingKey, existingAlert, "cooldown") + if shouldRenotify { + log.Debug(). + Str("alertID", alertID). + Dur("cooldown", time.Duration(m.config.Schedule.Cooldown)*time.Minute). + Msg("Cooldown period has passed, will re-notify") + } } else if oldLevel != existingAlert.Level && existingAlert.Level == AlertLevelCritical { // Always re-notify if alert escalated to critical - shouldRenotify = true - log.Debug(). - Str("alertID", alertID). - Msg("Alert escalated to critical, will re-notify despite cooldown") + shouldRenotify = m.allowNotificationByRateLimit(trackingKey, existingAlert, "critical-escalation") + if shouldRenotify { + log.Debug(). + Str("alertID", alertID). + Msg("Alert escalated to critical, will re-notify despite cooldown") + } } // Send re-notification if appropriate (may be suppressed by quiet hours) diff --git a/internal/alerts/notification_policy.go b/internal/alerts/notification_policy.go index c5d8bbfd5..e97d8386c 100644 --- a/internal/alerts/notification_policy.go +++ b/internal/alerts/notification_policy.go @@ -174,7 +174,8 @@ func (m *Manager) dispatchAlert(alert *Alert, async bool) bool { } notifiedAt := time.Now() - alert.LastNotified = ¬ifiedAt + m.recordAlertNotifiedNoLock(alert, notifiedAt) + m.saveActiveAlertsAsync("alert-dispatch") alertCopy := cloneAlertForOutput(alert) if async { @@ -212,6 +213,34 @@ func (m *Manager) dispatchAlert(alert *Alert, async bool) bool { return true } +func (m *Manager) recordAlertNotifiedNoLock(alert *Alert, notifiedAt time.Time) { + if alert == nil { + return + } + + setAlertLastNotified(alert, notifiedAt) + + if trackingKey := canonicalTrackingKeyForAlert(alert); trackingKey != "" { + if active, exists := m.getActiveAlertNoLock(trackingKey); exists && active != nil { + setAlertLastNotified(active, notifiedAt) + return + } + } + if alert.ID != "" { + if active, exists := m.getActiveAlertNoLock(alert.ID); exists && active != nil { + setAlertLastNotified(active, notifiedAt) + } + } +} + +func setAlertLastNotified(alert *Alert, notifiedAt time.Time) { + if alert == nil { + return + } + t := notifiedAt + alert.LastNotified = &t +} + func isMonitorOnlyAlert(alert *Alert) bool { if alert == nil || alert.Metadata == nil { return false @@ -539,6 +568,33 @@ func (m *Manager) shouldNotifyAfterCooldown(alert *Alert) bool { return timeSinceLastNotification >= cooldownDuration } +func (m *Manager) allowNotificationByRateLimit(trackingKey string, alert *Alert, reason string) bool { + if trackingKey == "" && alert != nil { + trackingKey = canonicalTrackingKeyForAlert(alert) + } + if trackingKey == "" && alert != nil { + trackingKey = alert.ID + } + if m.checkRateLimit(trackingKey) { + return true + } + + log.Debug(). + Str("alertID", alertIDForLog(alert)). + Str("trackingKey", trackingKey). + Str("reason", reason). + Int("maxPerHour", m.config.Schedule.MaxAlertsHour). + Msg("Alert notification suppressed due to rate limit") + return false +} + +func alertIDForLog(alert *Alert) string { + if alert == nil { + return "" + } + return alert.ID +} + // checkRateLimit checks if an alert has exceeded rate limit func (m *Manager) checkRateLimit(alertID string) bool { if m.config.Schedule.MaxAlertsHour <= 0 { diff --git a/internal/alerts/unified_eval_test.go b/internal/alerts/unified_eval_test.go index 15b6f57a3..30f66deb0 100644 --- a/internal/alerts/unified_eval_test.go +++ b/internal/alerts/unified_eval_test.go @@ -126,6 +126,63 @@ func assertAlertMissing(t *testing.T, m *Manager, alertID string) { } } +func TestUnifiedGuestDiskRenotificationRespectsMaxAlertsHour(t *testing.T) { + m := newTestManager(t) + cfg := unifiedEvalBaseConfig() + cfg.GuestDefaults = ThresholdConfig{ + Disk: &HysteresisThreshold{Trigger: 80, Clear: 70}, + } + cfg.Schedule.Cooldown = 30 + cfg.Schedule.MaxAlertsHour = 1 + configureUnifiedEvalManager(t, m, cfg) + + dispatched := make(chan string, 4) + m.SetAlertCallback(func(alert *Alert) { + dispatched <- alert.ID + }) + + container := models.Container{ + ID: "ct101", + Name: "smr", + Node: "ryzen5800x", + Status: "running", + Disks: []models.Disk{{ + Mountpoint: "/", + Usage: 90.5, + Total: 100, + Used: 90, + Free: 10, + }}, + } + + m.CheckGuest(container, "pve1") + + select { + case <-dispatched: + case <-time.After(time.Second): + t.Fatal("expected initial guest disk alert notification") + } + + alertID := canonicalMetricStateID("ct101-disk-root", "disk") + lastNotified := time.Now().Add(-31 * time.Minute) + m.mu.Lock() + alert, exists := m.getActiveAlertNoLock(alertID) + if !exists { + m.mu.Unlock() + t.Fatalf("expected active guest disk alert %s", alertID) + } + alert.LastNotified = &lastNotified + m.mu.Unlock() + + m.CheckGuest(container, "pve1") + + select { + case id := <-dispatched: + t.Fatalf("expected max-alerts/hour to suppress repeated disk notification, got %s", id) + case <-time.After(250 * time.Millisecond): + } +} + func TestCheckUnifiedResourceMajorFamilies(t *testing.T) { m := newTestManager(t) configureUnifiedEvalManager(t, m, unifiedEvalBaseConfig())