From 92fd1682abb354c094b77b8b4768fde0400c47d8 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:44:24 +0100 Subject: [PATCH] Start fresh installs with factory Docker alert thresholds A fresh install has no alerts.json, and LoadAlertConfig answered that with its own AlertConfig literal. The literal never set DockerDefaults, a value struct, so container CPU, memory and disk loaded as trigger 0. UpdateConfig keeps trigger 0 as a deliberate Off, so container metric alerts never fired, the thresholds page showed the container Global Defaults as Custom with every metric Off, and the startup identity migration wrote those zeros to alerts.json a second after first boot. The literal had also drifted from the manager's factory config on flapping detection, alert TTL cleanup and the quiet-hours and escalation templates. The missing-file path now returns alerts.DefaultAlertConfig, the factory constructor (now exported), so persistence keeps no second copy. It leaves the activation state unset, as the literal did: node updates and imports reapply LoadAlertConfig, and an explicit pending_review there would send an active install back to pending review. Stored configs are untouched, because trigger 0 is how a saved Off looks. Two factory values would have reached installs for the first time through this path, so they change with it. AutoAcknowledgeAfterHours drops from 24 to 0: no settings control shows it, no install ever ran it, and it would acknowledge still-firing alerts on the user's behalf. The quiet-hours timezone drops from America/New_York to empty, which keeps the settings page offering the viewer's browser zone. --- .../v6/internal/subsystems/alerts.md | 32 +++++++- internal/alerts/alerts_test.go | 39 ++++++++- internal/alerts/default_config.go | 14 ++-- internal/alerts/default_normalization_test.go | 8 +- internal/alerts/host.go | 2 +- internal/alerts/manager.go | 2 +- internal/config/persistence.go | 69 ++-------------- .../persistence_alert_fresh_install_test.go | 82 +++++++++++++++++++ 8 files changed, 172 insertions(+), 76 deletions(-) create mode 100644 internal/config/persistence_alert_fresh_install_test.go diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 9d7946179e..a8ef4df884 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1746,8 +1746,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`. @@ -1757,6 +1757,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 f4c4463c5e..a0db6dd20c 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -20161,8 +20161,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") @@ -20181,6 +20181,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 74f522f5d7..0ca0ba0550 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, @@ -162,7 +166,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, @@ -194,9 +198,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 852db656db..f34ba4c8cc 100644 --- a/internal/alerts/default_normalization_test.go +++ b/internal/alerts/default_normalization_test.go @@ -287,9 +287,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) @@ -397,9 +397,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 65ae1cbd40..01b8a5c8c0 100644 --- a/internal/alerts/host.go +++ b/internal/alerts/host.go @@ -1057,7 +1057,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 c1eefbf941..adedf2d081 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -1005,70 +1005,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) +}