diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index bb6ec90d70..4631cb9f7b 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1854,8 +1854,8 @@ compatibility aliases and wrapper functions for the leaf config package, `model.go` owns alert runtime data structures and clone semantics, `constants.go` owns package-wide cleanup and storage constants, `metric_hooks.go` owns Prometheus integration callbacks, `manager.go` owns -Manager state and construction, `default_config.go` owns the default runtime -configuration literal, `lifecycle.go` owns shutdown, and `escalation.go` owns +Manager state and construction, `default_config.go` owns the factory runtime +configuration (`DefaultAlertConfig`), `lifecycle.go` owns shutdown, and `escalation.go` owns the escalation loop and escalation state mutation. Future changes must extend the owning file rather than reintroducing a central catch-all manager file. Alert notification policy now lives in `internal/alerts/notification_policy.go`. @@ -1865,6 +1865,34 @@ per-alert rate limiting; future notification-gating changes should extend that policy owner rather than burying new checks inside metric or resource-specific evaluators. +### Fresh installs load the factory alert configuration + +`DefaultAlertConfig` in `internal/alerts/default_config.go` is the only +declaration of factory alert settings. A new Manager starts from it, and +`LoadAlertConfig` in `internal/config/persistence.go` returns it while +`alerts.json` does not exist; persistence must not declare a second copy. The +copy it used to return left out `DockerDefaults`, a value struct, so a fresh +install loaded container CPU, memory and disk as trigger 0, `UpdateConfig` +kept that as a deliberate Off, the thresholds page called the container Global +Defaults Custom, and the startup identity migration wrote those zeros to +`alerts.json` within a second of first boot. The missing-file config carries +no activation state, because activation is a stored decision: node updates and +imports reapply `LoadAlertConfig`, and `UpdateConfig` then keeps a running +manager's state, while a new manager starts pending review. An existing +`alerts.json` is never rewritten to factory values, because a stored trigger 0 +is how a saved Off looks. The factory leaves `AutoAcknowledgeAfterHours` at 0: no settings +control shows it, and the 24 hours it used to declare never reached an +install. It leaves the quiet-hours timezone empty so the settings page offers +the viewer's browser zone. Flapping detection and alert TTL cleanup are on in +the factory and now reach fresh installs; a thresholds-page save still sends +neither field, so it turns them off. `TestLoadAlertConfigFreshInstallUsesFactoryConfig`, +`TestLoadAlertConfigMissingFileKeepsRunningActivation` and +`TestLoadAlertConfigKeepsStoredDockerDefaultsOff` in +`internal/config/persistence_alert_fresh_install_test.go`, plus +`TestDefaultAlertConfigLeavesFiringAlertsUnacknowledged` and +`TestDefaultAlertConfigLeavesQuietHoursTimezoneUnset` in +`internal/alerts/alerts_test.go`, pin this. + ### Agent disk temperature alerts clear when their threshold is off Host-agent SMART disk temperature alerts (`diskTemperature`, resource diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index 73287bfaa7..fb79279745 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -20538,8 +20538,8 @@ func TestSetAckRecordUsesCanonicalKeyWithoutMutatingLiveAlert(t *testing.T) { } func TestDefaultAlertConfigUsesIndependentBackupAlertOrphanedPointer(t *testing.T) { - first := defaultAlertConfig() - second := defaultAlertConfig() + first := DefaultAlertConfig() + second := DefaultAlertConfig() if first.BackupDefaults.AlertOrphaned == nil { t.Fatal("first default backup AlertOrphaned is nil") @@ -20558,6 +20558,41 @@ func TestDefaultAlertConfigUsesIndependentBackupAlertOrphanedPointer(t *testing. } } +// A fresh install runs the factory configuration, and the settings page has no +// auto-acknowledge control, so the factory must never acknowledge a firing +// alert on the user's behalf. +func TestDefaultAlertConfigLeavesFiringAlertsUnacknowledged(t *testing.T) { + m := newTestManager(t) + + now := time.Now() + m.mu.Lock() + m.activeAlerts["firing-alert"] = &Alert{ + ID: "firing-alert", + StartTime: now.Add(-72 * time.Hour), + LastSeen: now, + } + m.mu.Unlock() + + m.Cleanup(time.Hour) + + m.mu.RLock() + alert := testRequireActiveAlert(t, m, "firing-alert") + m.mu.RUnlock() + if alert.Acknowledged { + t.Fatalf("factory config acknowledged a firing alert as %q", alert.AckUser) + } +} + +// The settings page offers the viewer's browser zone only while the stored +// quiet-hours timezone is empty; a factory zone would replace it with one +// fixed city for every fresh install. +func TestDefaultAlertConfigLeavesQuietHoursTimezoneUnset(t *testing.T) { + cfg := DefaultAlertConfig() + if cfg.Schedule.QuietHours.Timezone != "" { + t.Fatalf("factory quiet-hours timezone = %q, want unset", cfg.Schedule.QuietHours.Timezone) + } +} + func TestUpdateConfigNormalizesNotificationDeliveryTargets(t *testing.T) { manager := NewManager() t.Cleanup(manager.Stop) diff --git a/internal/alerts/default_config.go b/internal/alerts/default_config.go index 516756aaa8..8fb7571fc7 100644 --- a/internal/alerts/default_config.go +++ b/internal/alerts/default_config.go @@ -1,6 +1,10 @@ package alerts -func defaultAlertConfig() AlertConfig { +// DefaultAlertConfig returns the factory alert configuration: the one a new +// Manager starts with and the one a fresh install loads before alerts.json +// exists. Callers that need factory values build them here instead of +// declaring their own copy. +func DefaultAlertConfig() AlertConfig { alertOrphaned := true return AlertConfig{ Enabled: true, @@ -165,7 +169,7 @@ func defaultAlertConfig() AlertConfig { Enabled: false, // OFF - users should opt-in to quiet hours Start: "22:00", End: "08:00", - Timezone: "America/New_York", + Timezone: "", // Unset: the server's local zone; the settings page offers the browser's zone Days: map[string]bool{ "monday": true, "tuesday": true, @@ -197,9 +201,9 @@ func defaultAlertConfig() AlertConfig { }, }, // Alert TTL defaults - MaxAlertAgeDays: 7, // Cleanup unacknowledged alerts inactive for 7 days - MaxAcknowledgedAgeDays: 1, // Auto-cleanup acknowledged alerts older than 1 day - AutoAcknowledgeAfterHours: 24, // Auto-acknowledge alerts after 24 hours + MaxAlertAgeDays: 7, // Cleanup unacknowledged alerts inactive for 7 days + MaxAcknowledgedAgeDays: 1, // Auto-cleanup acknowledged alerts older than 1 day + AutoAcknowledgeAfterHours: 0, // Off: a firing alert stays unacknowledged until someone acknowledges it // Flapping detection defaults FlappingEnabled: true, // Enable flapping detection FlappingWindowSeconds: 300, // 5 minute window diff --git a/internal/alerts/default_normalization_test.go b/internal/alerts/default_normalization_test.go index eb23e1fb1f..a225452bda 100644 --- a/internal/alerts/default_normalization_test.go +++ b/internal/alerts/default_normalization_test.go @@ -288,9 +288,9 @@ func TestNormalizeDiskFillByType(t *testing.T) { } func TestDefaultAlertConfigSeedsDiskFillByType(t *testing.T) { - cfg := defaultAlertConfig() + cfg := DefaultAlertConfig() if cfg.DiskFillByType == nil { - t.Fatal("expected defaultAlertConfig to seed DiskFillByType, got nil") + t.Fatal("expected DefaultAlertConfig to seed DiskFillByType, got nil") } if nvme, ok := cfg.DiskFillByType["nvme"]; !ok || nvme.Trigger != 92 || nvme.Clear != 87 { t.Fatalf("nvme = %+v ok=%v, want {Trigger:92 Clear:87}", nvme, ok) @@ -399,9 +399,9 @@ func TestNormalizeDiskTempByType(t *testing.T) { } func TestDefaultAlertConfigSeedsDiskTempByType(t *testing.T) { - cfg := defaultAlertConfig() + cfg := DefaultAlertConfig() if cfg.DiskTempByType == nil { - t.Fatal("expected defaultAlertConfig to seed DiskTempByType, got nil") + t.Fatal("expected DefaultAlertConfig to seed DiskTempByType, got nil") } if nvme, ok := cfg.DiskTempByType["nvme"]; !ok || nvme.Trigger != 70 || nvme.Clear != 65 { t.Fatalf("nvme = %+v ok=%v, want {Trigger:70 Clear:65}", nvme, ok) diff --git a/internal/alerts/host.go b/internal/alerts/host.go index be3d9aa092..5973049a27 100644 --- a/internal/alerts/host.go +++ b/internal/alerts/host.go @@ -1108,7 +1108,7 @@ func (m *Manager) HostDiskTemperatureThreshold(host DiskTemperatureHost, diskTyp // DefaultDiskTemperatureThreshold is DiskTemperatureThreshold under the // factory alert configuration, for callers with no alert manager. func DefaultDiskTemperatureThreshold(diskType string) *HysteresisThreshold { - config := defaultAlertConfig() + config := DefaultAlertConfig() return cloneThreshold(diskTemperatureThresholdForType(config.DiskTempByType, config.AgentDefaults.DiskTemperature, false, diskType)) } diff --git a/internal/alerts/manager.go b/internal/alerts/manager.go index 1447c03df2..b781bc58f1 100644 --- a/internal/alerts/manager.go +++ b/internal/alerts/manager.go @@ -251,7 +251,7 @@ func NewManagerWithDataDir(dataDir string, options ...ManagerOption) *Manager { nodeDisplayNames: make(map[string]string), instanceNodeDisplayNames: make(map[string]string), now: time.Now, - config: defaultAlertConfig(), + config: DefaultAlertConfig(), skipPersistedRestore: opts.skipPersistedAlertRestore, } intentClockEpoch := time.Now() diff --git a/internal/config/persistence.go b/internal/config/persistence.go index dff91ea307..1d222c88f5 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -979,70 +979,17 @@ func (c *ConfigPersistence) LoadAlertConfig() (*alerts.AlertConfig, error) { c.mu.RLock() defer c.mu.RUnlock() - alertOrphaned := true data, err := c.fs.ReadFile(c.alertFile) if err != nil { if os.IsNotExist(err) { - // Return default config if file doesn't exist - return &alerts.AlertConfig{ - Enabled: true, - GuestDefaults: alerts.ThresholdConfig{ - CPU: &alerts.HysteresisThreshold{Trigger: 80, Clear: 75}, - Memory: &alerts.HysteresisThreshold{Trigger: 85, Clear: 80}, - Disk: &alerts.HysteresisThreshold{Trigger: 90, Clear: 85}, - }, - NodeDefaults: alerts.ThresholdConfig{ - CPU: &alerts.HysteresisThreshold{Trigger: 80, Clear: 75}, - Memory: &alerts.HysteresisThreshold{Trigger: 85, Clear: 80}, - Disk: &alerts.HysteresisThreshold{Trigger: 90, Clear: 85}, - Temperature: &alerts.HysteresisThreshold{Trigger: 80, Clear: 75}, - }, - AgentDefaults: alerts.ThresholdConfig{ - CPU: &alerts.HysteresisThreshold{Trigger: 80, Clear: 75}, - Memory: &alerts.HysteresisThreshold{Trigger: 85, Clear: 80}, - Disk: &alerts.HysteresisThreshold{Trigger: 90, Clear: 85}, - }, - Schedule: alerts.ScheduleConfig{ - Cooldown: 5, - MaxAlertsHour: 10, - InitialNotify: "all", - NotifyOnResolve: true, - Grouping: alerts.GroupingConfig{ - Enabled: true, - Window: 30, - ByNode: true, - ByGuest: false, - }, - }, - StorageDefault: alerts.HysteresisThreshold{Trigger: 85, Clear: 80}, - TimeThresholds: map[string]int{ - "guest": 5, - "node": 5, - "agent": 5, - "storage": 5, - "pbs": 5, - }, - MinimumDelta: 2.0, - SuppressionWindow: 5, - HysteresisMargin: 5.0, - SnapshotDefaults: alerts.SnapshotAlertConfig{ - Enabled: false, - WarningDays: 30, - CriticalDays: 45, - WarningSizeGiB: 0, - CriticalSizeGiB: 0, - }, - BackupDefaults: alerts.BackupAlertConfig{ - Enabled: false, - WarningDays: 7, - CriticalDays: 14, - FreshHours: 24, - StaleHours: 72, - AlertOrphaned: &alertOrphaned, - IgnoreVMIDs: []string{}, - }, - Overrides: make(map[string]alerts.ThresholdConfig), - }, nil + // A fresh install has no alerts.json yet. It starts from the alert + // manager's factory configuration; a second literal here left out + // Docker container thresholds, so they loaded as Off. Activation + // is a stored decision, not a factory value: leaving it unset lets + // UpdateConfig keep a running manager's state. + config := alerts.DefaultAlertConfig() + config.ActivationState = "" + return &config, nil } return nil, err } diff --git a/internal/config/persistence_alert_fresh_install_test.go b/internal/config/persistence_alert_fresh_install_test.go new file mode 100644 index 0000000000..82fb15d34b --- /dev/null +++ b/internal/config/persistence_alert_fresh_install_test.go @@ -0,0 +1,82 @@ +package config + +import ( + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/alerts" + "github.com/stretchr/testify/require" +) + +// A fresh install has no alerts.json. It must load the alert manager's +// factory configuration rather than a copy declared in persistence: the copy +// left out Docker container thresholds, so container CPU, memory and disk +// alerts started Off and the settings page called them Custom. +func TestLoadAlertConfigFreshInstallUsesFactoryConfig(t *testing.T) { + persistence := NewConfigPersistence(t.TempDir()) + + loaded, err := persistence.LoadAlertConfig() + require.NoError(t, err) + factory := alerts.DefaultAlertConfig() + factory.ActivationState = "" + require.Equal(t, factory, *loaded) + + manager := alerts.NewManagerWithDataDir(t.TempDir()) + defer manager.Stop() + manager.UpdateConfig(*loaded) + + applied := manager.GetConfig() + require.Equal(t, alerts.ActivationPending, applied.ActivationState) + docker := applied.DockerDefaults + require.Equal(t, alerts.HysteresisThreshold{Trigger: 80, Clear: 75}, docker.CPU) + require.Equal(t, alerts.HysteresisThreshold{Trigger: 85, Clear: 80}, docker.Memory) + require.Equal(t, alerts.HysteresisThreshold{Trigger: 85, Clear: 80}, docker.Disk) +} + +// Node updates and imports reapply LoadAlertConfig to the running manager. A +// missing alerts.json holds no activation decision, so it must not send an +// active install back to pending review. +func TestLoadAlertConfigMissingFileKeepsRunningActivation(t *testing.T) { + persistence := NewConfigPersistence(t.TempDir()) + + manager := alerts.NewManagerWithDataDir(t.TempDir()) + defer manager.Stop() + active := manager.GetConfig() + activatedAt := time.Now().Add(-time.Hour) + active.ActivationState = alerts.ActivationActive + active.ActivationTime = &activatedAt + manager.UpdateConfig(active) + + loaded, err := persistence.LoadAlertConfig() + require.NoError(t, err) + manager.UpdateConfig(*loaded) + + applied := manager.GetConfig() + require.Equal(t, alerts.ActivationActive, applied.ActivationState) + require.NotNil(t, applied.ActivationTime) + require.True(t, applied.ActivationTime.Equal(activatedAt)) +} + +// Trigger 0 is how a saved Off is stored, so a stored all-zero Docker default +// stays Off; only the missing-file path takes factory values. +func TestLoadAlertConfigKeepsStoredDockerDefaultsOff(t *testing.T) { + persistence := NewConfigPersistence(t.TempDir()) + + stored := alerts.DefaultAlertConfig() + stored.DockerDefaults.CPU = alerts.HysteresisThreshold{} + stored.DockerDefaults.Memory = alerts.HysteresisThreshold{} + stored.DockerDefaults.Disk = alerts.HysteresisThreshold{} + require.NoError(t, persistence.SaveAlertConfig(stored)) + + loaded, err := persistence.LoadAlertConfig() + require.NoError(t, err) + + manager := alerts.NewManagerWithDataDir(t.TempDir()) + defer manager.Stop() + manager.UpdateConfig(*loaded) + + docker := manager.GetConfig().DockerDefaults + require.Equal(t, alerts.HysteresisThreshold{}, docker.CPU) + require.Equal(t, alerts.HysteresisThreshold{}, docker.Memory) + require.Equal(t, alerts.HysteresisThreshold{}, docker.Disk) +}