diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index a7cb638119..bc0caa053f 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -3121,6 +3121,28 @@ ownership is `internal/alerts/alert_identity_migration_test.go`, `internal/monitoring/monitor_alert_override_migration_test.go`, and `frontend-modern/src/features/alerts/__tests__/alertsConfigurationModel.snapshot.test.ts`. +### Alert config snapshots are owned copies + +`Manager.GetConfig()` returns `AlertConfig.Clone()`, a typed deep copy that +shares no map, slice or pointer with the live config (custom rule filter +values are copied as decoded JSON values). Callers encode, persist +and edit that snapshot outside `m.mu`: the GET config handler, alert +activation, the identity migration, backup evaluation and diagnostics. It used +to be a shallow copy, so `SaveAlertConfig` normalizing it in place, or +`UpdateConfig` re-normalizing the overrides of a re-applied snapshot, wrote the +live maps while evaluation or another encoder read them, and Go aborted Pulse +with a concurrent map write. `SaveAlertConfig` also clones before normalizing, +because a config just handed to `UpdateConfig` is the live config. Persistence +keeps an explicit `0` type-level delay as no delay on save and load, matching +`NormalizeTimeThresholds`. It used to store and load it as the 5-second factory +delay, which the noisy-gauge rule stretches to 300 seconds for memory and +temperature, and the shared map write was what made the running manager agree. +Pulse saves never wrote a `0`, so only a newly sent or hand-edited `0` changes +meaning. `internal/alerts/config/clone_test.go` +fills every field and fails when a new reference field is not copied. +Regression ownership is `internal/alerts/resolved_lock_discipline_test.go` and +`internal/config/persistence_alert_ownership_test.go`. + ### Versioned alert-intent policy The alerts runtime owns one versioned alert-intent document and its durable diff --git a/internal/alerts/config/clone.go b/internal/alerts/config/clone.go new file mode 100644 index 0000000000..388decc3fc --- /dev/null +++ b/internal/alerts/config/clone.go @@ -0,0 +1,164 @@ +package config + +import ( + "maps" + "slices" +) + +// Clone returns a deep copy of the configuration that shares no maps, slices +// or pointers with c, so the copy can be changed, normalized or encoded +// without touching c. Nil and empty maps and slices keep their distinction. +// Custom rule filter values are copied as the JSON values the API and +// alerts.json decode them to; other Go values stored there stay shared. +// +// The alert manager hands out its live configuration only through Clone; +// callers persist, encode and edit what it returns outside the manager lock. +func (c AlertConfig) Clone() AlertConfig { + clone := c + clone.ActivationTime = clonePointer(c.ActivationTime) + clone.GuestDefaults = c.GuestDefaults.Clone() + clone.NodeDefaults = c.NodeDefaults.Clone() + clone.AgentDefaults = c.AgentDefaults.Clone() + clone.DiskFillByType = maps.Clone(c.DiskFillByType) + clone.DiskTempByType = maps.Clone(c.DiskTempByType) + clone.DockerIgnoredContainerPrefixes = slices.Clone(c.DockerIgnoredContainerPrefixes) + clone.IgnoredGuestPrefixes = slices.Clone(c.IgnoredGuestPrefixes) + clone.GuestTagWhitelist = slices.Clone(c.GuestTagWhitelist) + clone.GuestTagBlacklist = slices.Clone(c.GuestTagBlacklist) + clone.PBSDefaults = c.PBSDefaults.Clone() + clone.KubernetesDefaults = c.KubernetesDefaults.Clone() + clone.TrueNASDefaults = c.TrueNASDefaults.Clone() + clone.TrueNASDiskDefaults = c.TrueNASDiskDefaults.Clone() + clone.VMwareDefaults = c.VMwareDefaults.Clone() + clone.BackupDefaults = c.BackupDefaults.clone() + if c.Overrides != nil { + clone.Overrides = make(map[string]ThresholdConfig, len(c.Overrides)) + for id, override := range c.Overrides { + clone.Overrides[id] = override.Clone() + } + } + if c.CustomRules != nil { + clone.CustomRules = make([]CustomAlertRule, len(c.CustomRules)) + for i, rule := range c.CustomRules { + clone.CustomRules[i] = rule.clone() + } + } + clone.Schedule.QuietHours.Days = maps.Clone(c.Schedule.QuietHours.Days) + if c.Schedule.Escalation.Levels != nil { + clone.Schedule.Escalation.Levels = make([]EscalationLevel, len(c.Schedule.Escalation.Levels)) + for i, level := range c.Schedule.Escalation.Levels { + level.DestinationIDs = slices.Clone(level.DestinationIDs) + clone.Schedule.Escalation.Levels[i] = level + } + } + clone.TimeThresholds = maps.Clone(c.TimeThresholds) + clone.MetricTimeThresholds = cloneNestedIntMap(c.MetricTimeThresholds) + clone.MetricEvaluationWindows = cloneNestedIntMap(c.MetricEvaluationWindows) + return clone +} + +// Clone returns a deep copy of the thresholds that shares no pointers with t. +func (t ThresholdConfig) Clone() ThresholdConfig { + clone := t + clone.CPU = clonePointer(t.CPU) + clone.Memory = clonePointer(t.Memory) + clone.Disk = clonePointer(t.Disk) + clone.DiskRead = clonePointer(t.DiskRead) + clone.DiskWrite = clonePointer(t.DiskWrite) + clone.NetworkIn = clonePointer(t.NetworkIn) + clone.NetworkOut = clonePointer(t.NetworkOut) + clone.Usage = clonePointer(t.Usage) + clone.Temperature = clonePointer(t.Temperature) + clone.DiskTemperature = clonePointer(t.DiskTemperature) + clone.SMARTHealthFailure = clonePointer(t.SMARTHealthFailure) + clone.SMARTReallocated = clonePointer(t.SMARTReallocated) + clone.SMARTPending = clonePointer(t.SMARTPending) + clone.SMARTUncorrectable = clonePointer(t.SMARTUncorrectable) + clone.SMARTMediaErrors = clonePointer(t.SMARTMediaErrors) + clone.SMARTCRCErrorDelta = clonePointer(t.SMARTCRCErrorDelta) + clone.SMARTLifeWarning = clonePointer(t.SMARTLifeWarning) + clone.SMARTLifeCritical = clonePointer(t.SMARTLifeCritical) + clone.SMARTSpareWarning = clonePointer(t.SMARTSpareWarning) + clone.SMARTSpareCritical = clonePointer(t.SMARTSpareCritical) + if t.Backup != nil { + backup := t.Backup.clone() + clone.Backup = &backup + } + clone.Snapshot = clonePointer(t.Snapshot) + clone.Note = clonePointer(t.Note) + return clone +} + +func (b BackupAlertConfig) clone() BackupAlertConfig { + clone := b + clone.AlertOrphaned = clonePointer(b.AlertOrphaned) + clone.IgnoreVMIDs = slices.Clone(b.IgnoreVMIDs) + return clone +} + +func (r CustomAlertRule) clone() CustomAlertRule { + clone := r + if r.FilterConditions.Filters != nil { + clone.FilterConditions.Filters = make([]FilterCondition, len(r.FilterConditions.Filters)) + for i, filter := range r.FilterConditions.Filters { + filter.Value = cloneJSONValue(filter.Value) + clone.FilterConditions.Filters[i] = filter + } + } + clone.Thresholds = r.Thresholds.Clone() + if r.Notifications.Email != nil { + email := *r.Notifications.Email + email.Recipients = slices.Clone(email.Recipients) + clone.Notifications.Email = &email + } + clone.Notifications.Webhook = clonePointer(r.Notifications.Webhook) + return clone +} + +// clonePointer copies the value p points at. Only use it for types that hold +// no maps, slices or pointers of their own. +func clonePointer[T any](p *T) *T { + if p == nil { + return nil + } + clone := *p + return &clone +} + +func cloneNestedIntMap(input map[string]map[string]int) map[string]map[string]int { + if input == nil { + return nil + } + clone := make(map[string]map[string]int, len(input)) + for key, inner := range input { + clone[key] = maps.Clone(inner) + } + return clone +} + +// cloneJSONValue copies a filter value as encoding/json decodes one: arrays +// and objects are copied, scalars are values already. +func cloneJSONValue(value interface{}) interface{} { + switch typed := value.(type) { + case []interface{}: + if typed == nil { + return typed + } + clone := make([]interface{}, len(typed)) + for i, item := range typed { + clone[i] = cloneJSONValue(item) + } + return clone + case map[string]interface{}: + if typed == nil { + return typed + } + clone := make(map[string]interface{}, len(typed)) + for key, item := range typed { + clone[key] = cloneJSONValue(item) + } + return clone + default: + return value + } +} diff --git a/internal/alerts/config/clone_test.go b/internal/alerts/config/clone_test.go new file mode 100644 index 0000000000..4afc629bf7 --- /dev/null +++ b/internal/alerts/config/clone_test.go @@ -0,0 +1,133 @@ +package config + +import ( + "reflect" + "testing" + "time" +) + +var timeType = reflect.TypeOf(time.Time{}) + +// Every field is filled, so a map, slice or pointer added to the config later +// fails here until Clone copies it. +func TestAlertConfigCloneSharesNothingWithTheOriginal(t *testing.T) { + var original AlertConfig + fillForClone(reflect.ValueOf(&original).Elem()) + + clone := original.Clone() + + if !reflect.DeepEqual(clone, original) { + t.Fatalf("clone differs from the original:\n got %+v\nwant %+v", clone, original) + } + assertNothingShared(t, "AlertConfig", reflect.ValueOf(original), reflect.ValueOf(clone)) +} + +func TestAlertConfigCloneKeepsNilAndEmptyApart(t *testing.T) { + original := AlertConfig{ + Overrides: map[string]ThresholdConfig{}, + CustomRules: []CustomAlertRule{{FilterConditions: FilterStack{Filters: []FilterCondition{{Value: []interface{}(nil)}}}}}, + } + + clone := original.Clone() + + if !reflect.DeepEqual(clone, original) { + t.Fatalf("clone differs from the original:\n got %#v\nwant %#v", clone, original) + } + if clone.Overrides == nil { + t.Fatal("clone turned an empty overrides map into nil") + } + if clone.TimeThresholds != nil { + t.Fatal("clone turned a nil delay map into an empty one") + } +} + +func fillForClone(value reflect.Value) { + switch value.Kind() { + case reflect.Bool: + value.SetBool(true) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + value.SetInt(7) + case reflect.Float32, reflect.Float64: + value.SetFloat(7.5) + case reflect.String: + value.SetString("filled") + case reflect.Pointer: + value.Set(reflect.New(value.Type().Elem())) + fillForClone(value.Elem()) + case reflect.Map: + value.Set(reflect.MakeMap(value.Type())) + key := reflect.New(value.Type().Key()).Elem() + fillForClone(key) + item := reflect.New(value.Type().Elem()).Elem() + fillForClone(item) + value.SetMapIndex(key, item) + case reflect.Slice: + value.Set(reflect.MakeSlice(value.Type(), 1, 1)) + fillForClone(value.Index(0)) + case reflect.Interface: + // Filter values arrive from encoding/json. + value.Set(reflect.ValueOf(map[string]interface{}{"filled": []interface{}{"filled", 7.5}})) + case reflect.Struct: + if value.Type() == timeType { + value.Set(reflect.ValueOf(time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC))) + return + } + for i := 0; i < value.NumField(); i++ { + if value.Field(i).CanSet() { + fillForClone(value.Field(i)) + } + } + } +} + +func assertNothingShared(t *testing.T, path string, original, clone reflect.Value) { + t.Helper() + switch original.Kind() { + case reflect.Pointer: + if original.IsNil() { + return + } + if original.Pointer() == clone.Pointer() { + t.Errorf("%s: clone shares the pointer", path) + return + } + assertNothingShared(t, path, original.Elem(), clone.Elem()) + case reflect.Map: + if original.IsNil() { + return + } + if original.Pointer() == clone.Pointer() { + t.Errorf("%s: clone shares the map", path) + return + } + iter := original.MapRange() + for iter.Next() { + assertNothingShared(t, path+"[key]", iter.Value(), clone.MapIndex(iter.Key())) + } + case reflect.Slice: + if original.Len() == 0 { + return + } + if original.Pointer() == clone.Pointer() { + t.Errorf("%s: clone shares the slice", path) + return + } + for i := 0; i < original.Len(); i++ { + assertNothingShared(t, path+"[i]", original.Index(i), clone.Index(i)) + } + case reflect.Interface: + if original.IsNil() { + return + } + assertNothingShared(t, path, original.Elem(), clone.Elem()) + case reflect.Struct: + if original.Type() == timeType { + return + } + for i := 0; i < original.NumField(); i++ { + if original.Type().Field(i).IsExported() { + assertNothingShared(t, path+"."+original.Type().Field(i).Name, original.Field(i), clone.Field(i)) + } + } + } +} diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go index 3b6259e71e..6b12269ca4 100644 --- a/internal/alerts/config_runtime.go +++ b/internal/alerts/config_runtime.go @@ -634,11 +634,15 @@ func (m *Manager) reevaluateActiveAlertsLocked() { } } -// GetConfig returns the current alert configuration. +// GetConfig returns a copy of the alert configuration that the caller owns. +// Callers encode, persist and edit it outside m.mu, and UpdateConfig +// normalizes the maps it is handed in place, so a copy that shared the live +// config's maps let a save or a re-apply write them while evaluation read +// them, which Go aborts as a concurrent map write. func (m *Manager) GetConfig() AlertConfig { m.mu.RLock() defer m.mu.RUnlock() - return m.config + return m.config.Clone() } func cloneThreshold(threshold *HysteresisThreshold) *HysteresisThreshold { diff --git a/internal/alerts/resolved_lock_discipline_test.go b/internal/alerts/resolved_lock_discipline_test.go index 311338a8e0..b746dafdee 100644 --- a/internal/alerts/resolved_lock_discipline_test.go +++ b/internal/alerts/resolved_lock_discipline_test.go @@ -1,6 +1,7 @@ package alerts import ( + "encoding/json" "fmt" "runtime" "sync" @@ -8,6 +9,7 @@ import ( "time" alertspecs "github.com/rcourtman/pulse-go-rewrite/internal/alerts/specs" + "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" "github.com/rs/zerolog" ) @@ -304,3 +306,80 @@ func waitForResolvedConcurrencyGroup(t *testing.T, wg *sync.WaitGroup) { t.Fatal("resolved alert concurrency paths deadlocked") } } + +// GetConfig snapshots are encoded, persisted and edited outside m.mu, and +// UpdateConfig normalizes the maps it is handed in place. While a snapshot +// shared the live config's maps, re-applying one rewrote the override map +// that the GET /api/alerts/config handler was encoding from another snapshot, +// and Go aborted the process with a concurrent map write. Run with -race. +func TestConfigSnapshotsShareNothingWithTheLiveConfig(t *testing.T) { + originalLogLevel := zerolog.GlobalLevel() + zerolog.SetGlobalLevel(zerolog.Disabled) + t.Cleanup(func() { + zerolog.SetGlobalLevel(originalLogLevel) + }) + + m := NewManagerWithDataDir(t.TempDir()) + t.Cleanup(m.Stop) + + seed := m.GetConfig() + seed.Enabled = true + seed.ActivationState = ActivationActive + seed.Overrides = make(map[string]ThresholdConfig) + for i := 0; i < 64; i++ { + seed.Overrides[fmt.Sprintf("agent:snapshot-%d", i)] = ThresholdConfig{ + CPU: &HysteresisThreshold{Trigger: 95, Clear: 90}, + } + } + m.UpdateConfig(seed) + + snapshot := m.GetConfig() + snapshot.Overrides["agent:snapshot-0"].CPU.Trigger = 1 + delete(snapshot.Overrides, "agent:snapshot-1") + snapshot.TimeThresholds["guest"] = 99 + snapshot.AgentDefaults.CPU.Trigger = 1 + + live := m.GetConfig() + if got := live.Overrides["agent:snapshot-0"].CPU.Trigger; got != 95 { + t.Fatalf("editing a snapshot changed the live override trigger to %v", got) + } + if _, ok := live.Overrides["agent:snapshot-1"]; !ok { + t.Fatal("deleting from a snapshot removed the live override") + } + if live.TimeThresholds["guest"] == 99 { + t.Fatal("editing a snapshot changed the live guest delay") + } + if live.AgentDefaults.CPU.Trigger == 1 { + t.Fatal("editing a snapshot changed the live agent CPU trigger") + } + + var wg sync.WaitGroup + wg.Add(3) + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + m.UpdateConfig(m.GetConfig()) + } + }() + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + if _, err := json.Marshal(m.GetConfig()); err != nil { + t.Errorf("encode snapshot: %v", err) + return + } + } + }() + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + id := fmt.Sprintf("snapshot-%d", i%64) + m.CheckHost(models.Host{ID: id, Hostname: id, Status: "online", CPUUsage: 50}) + } + }() + wg.Wait() + + if got := len(m.GetConfig().Overrides); got != 64 { + t.Fatalf("live overrides = %d after re-applying snapshots, want 64", got) + } +} diff --git a/internal/config/persistence.go b/internal/config/persistence.go index c1eefbf941..359e2b9af7 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -882,8 +882,11 @@ func normalizeAlertDefaults(config *alerts.AlertConfig) { config.TimeThresholds = make(map[string]int) } defaultDelay := 5 + // Zero means no delay, as the alert manager reads it + // (alerts.Manager.UpdateConfig keeps it); only a missing or negative + // delay takes the default. ensureDelay := func(key string) { - if delay, ok := config.TimeThresholds[key]; !ok || delay <= 0 { + if delay, ok := config.TimeThresholds[key]; !ok || delay < 0 { config.TimeThresholds[key] = defaultDelay } } @@ -892,7 +895,7 @@ func normalizeAlertDefaults(config *alerts.AlertConfig) { ensureDelay("agent") ensureDelay("storage") ensureDelay("pbs") - if delay, ok := config.TimeThresholds["all"]; ok && delay <= 0 { + if delay, ok := config.TimeThresholds["all"]; ok && delay < 0 { config.TimeThresholds["all"] = defaultDelay } @@ -962,6 +965,11 @@ func (c *ConfigPersistence) SaveAlertConfig(config alerts.AlertConfig) error { c.mu.Lock() defer c.mu.Unlock() + // Normalize a copy. Callers hand in alerts.Manager.GetConfig() snapshots + // and configs they have just applied to the manager, so normalizing their + // maps and threshold pointers in place wrote into the running config. + config = config.Clone() + // Save-specific: normalize override clear values before shared defaults run if config.MinimumDelta <= 0 { margin := config.HysteresisMargin diff --git a/internal/config/persistence_alert_ownership_test.go b/internal/config/persistence_alert_ownership_test.go new file mode 100644 index 0000000000..1b83aea3c0 --- /dev/null +++ b/internal/config/persistence_alert_ownership_test.go @@ -0,0 +1,171 @@ +package config_test + +import ( + "encoding/json" + "fmt" + "reflect" + "sync" + "testing" + + "github.com/rcourtman/pulse-go-rewrite/internal/alerts" + "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rs/zerolog" +) + +// SaveAlertConfig used to normalize the config it was handed in place. +// Callers hand it alerts.Manager.GetConfig() snapshots, whose maps and +// threshold pointers were the live manager config's own, so every save +// wrote into the running config outside the manager lock. Evaluation reads +// those maps under the lock and the GET handler encodes a snapshot outside +// it, so a save racing either one could abort Pulse with a concurrent map +// write. Run with -race: the rounds below take the same steps as alert +// activation, the identity migration and GET /api/alerts/config, while +// evaluation runs. +func TestSaveAlertConfigLeavesTheLiveManagerConfigAlone(t *testing.T) { + originalLevel := zerolog.GlobalLevel() + zerolog.SetGlobalLevel(zerolog.Disabled) + t.Cleanup(func() { zerolog.SetGlobalLevel(originalLevel) }) + + manager := alerts.NewManagerWithDataDir(t.TempDir()) + t.Cleanup(manager.Stop) + + seed := manager.GetConfig() + seed.Enabled = true + seed.ActivationState = alerts.ActivationActive + // Agent CPU alerts off: persistence rewrites the clear value of a zero + // trigger, so a shared threshold pointer is written on every save. + seed.AgentDefaults.CPU = &alerts.HysteresisThreshold{Trigger: 0, Clear: 0} + // No delay for guests: persistence used to replace an explicit zero with + // five seconds, writing the live delay map. + seed.TimeThresholds = map[string]int{"guest": 0} + seed.Overrides = make(map[string]alerts.ThresholdConfig) + for i := 0; i < 64; i++ { + seed.Overrides[fmt.Sprintf("agent:host-%d", i)] = alerts.ThresholdConfig{ + CPU: &alerts.HysteresisThreshold{Trigger: 95, Clear: 90}, + } + } + manager.UpdateConfig(seed) + want := manager.GetConfig() + wantJSON, err := json.Marshal(want) + if err != nil { + t.Fatalf("marshal seeded config: %v", err) + } + + persistence := config.NewConfigPersistence(t.TempDir()) + if err := persistence.EnsureConfigDir(); err != nil { + t.Fatalf("EnsureConfigDir: %v", err) + } + + const rounds = 200 + var wg sync.WaitGroup + run := func(step func(round int)) { + wg.Add(1) + go func() { + defer wg.Done() + for round := 0; round < rounds; round++ { + step(round) + } + }() + } + // POST /api/alerts/activate: apply a snapshot, then persist it. + run(func(int) { + cfg := manager.GetConfig() + manager.UpdateConfig(cfg) + if err := persistence.SaveAlertConfig(cfg); err != nil { + t.Errorf("save after update: %v", err) + } + }) + // The alert identity migration persists a snapshot. + run(func(int) { + if err := persistence.SaveAlertConfig(manager.GetConfig()); err != nil { + t.Errorf("save snapshot: %v", err) + } + }) + // GET /api/alerts/config encodes a snapshot outside the manager lock. + run(func(int) { + if _, err := json.Marshal(manager.GetConfig()); err != nil { + t.Errorf("encode snapshot: %v", err) + } + }) + run(func(round int) { + id := fmt.Sprintf("host-%d", round%64) + manager.CheckHost(models.Host{ID: id, Hostname: id, Status: "online", CPUUsage: 50}) + }) + wg.Wait() + + got := manager.GetConfig() + if got.TimeThresholds["guest"] != 0 { + t.Fatalf("live guest delay = %d after saves, want the configured 0", got.TimeThresholds["guest"]) + } + gotJSON, err := json.Marshal(got) + if err != nil { + t.Fatalf("marshal final config: %v", err) + } + if string(gotJSON) != string(wantJSON) { + t.Fatalf("saving changed the live config:\n got %s\nwant %s", gotJSON, wantJSON) + } + + loaded, err := persistence.LoadAlertConfig() + if err != nil { + t.Fatalf("LoadAlertConfig: %v", err) + } + if delay, ok := loaded.TimeThresholds["guest"]; !ok || delay != 0 { + t.Fatalf("persisted guest delay = %d (present %v), want the configured 0", delay, ok) + } +} + +func TestSaveAlertConfigDoesNotNormalizeTheCallersConfig(t *testing.T) { + persistence := config.NewConfigPersistence(t.TempDir()) + if err := persistence.EnsureConfigDir(); err != nil { + t.Fatalf("EnsureConfigDir: %v", err) + } + + cfg := alerts.AlertConfig{ + AgentDefaults: alerts.ThresholdConfig{ + CPU: &alerts.HysteresisThreshold{Trigger: 80, Clear: 0}, + }, + TimeThresholds: map[string]int{"guest": -1, "node": 0}, + Overrides: map[string]alerts.ThresholdConfig{ + "storage:local": {Usage: &alerts.HysteresisThreshold{Trigger: 90, Clear: 0}}, + }, + BackupDefaults: alerts.BackupAlertConfig{IgnoreVMIDs: []string{" 100 ", "100"}}, + } + before, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal: %v", err) + } + cpu, override := cfg.AgentDefaults.CPU, cfg.Overrides["storage:local"].Usage + + if err := persistence.SaveAlertConfig(cfg); err != nil { + t.Fatalf("SaveAlertConfig: %v", err) + } + + after, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(after) != string(before) { + t.Fatalf("SaveAlertConfig changed its argument:\n got %s\nwant %s", after, before) + } + if cfg.AgentDefaults.CPU != cpu || cfg.Overrides["storage:local"].Usage != override { + t.Fatal("SaveAlertConfig replaced the caller's threshold pointers") + } + + loaded, err := persistence.LoadAlertConfig() + if err != nil { + t.Fatalf("LoadAlertConfig: %v", err) + } + if got := loaded.TimeThresholds; got["guest"] != 5 || got["node"] != 0 { + t.Fatalf("persisted delays = %v, want guest 5 (unset) and node 0 (no delay)", got) + } + if got := *loaded.AgentDefaults.CPU; got != (alerts.HysteresisThreshold{Trigger: 80, Clear: 75}) { + t.Fatalf("persisted agent CPU = %+v, want clear derived below the trigger", got) + } + if got := *loaded.Overrides["storage:local"].Usage; got != (alerts.HysteresisThreshold{Trigger: 90, Clear: 85}) { + t.Fatalf("persisted override usage = %+v, want clear derived below the trigger", got) + } + if want := []string{"100"}; !reflect.DeepEqual(loaded.BackupDefaults.IgnoreVMIDs, want) { + t.Fatalf("persisted ignore list = %v, want %v", loaded.BackupDefaults.IgnoreVMIDs, want) + } +} diff --git a/internal/config/persistence_alerts_normalization_test.go b/internal/config/persistence_alerts_normalization_test.go index 33f768ea66..84eee6dc1f 100644 --- a/internal/config/persistence_alerts_normalization_test.go +++ b/internal/config/persistence_alerts_normalization_test.go @@ -213,7 +213,7 @@ func TestLoadAlertConfig_Normalization(t *testing.T) { }, }, { - name: "TimeThresholds defaults", + name: "TimeThresholds zero means no delay", input: map[string]interface{}{ "timeThreshold": 0, "timeThresholds": map[string]interface{}{ @@ -222,8 +222,9 @@ func TestLoadAlertConfig_Normalization(t *testing.T) { }, }, verify: func(t *testing.T, cfg *alerts.AlertConfig) { - assert.Equal(t, 5, cfg.TimeThresholds["guest"]) - assert.Equal(t, 5, cfg.TimeThresholds["all"]) + assert.Equal(t, 0, cfg.TimeThresholds["guest"]) + assert.Equal(t, 0, cfg.TimeThresholds["all"]) + assert.Equal(t, 5, cfg.TimeThresholds["node"]) }, }, { @@ -290,11 +291,13 @@ func TestLoadAlertConfig_Normalization(t *testing.T) { "timeThresholds": map[string]interface{}{ "guest": -1, "pbs": 0, + "all": -1, }, }, verify: func(t *testing.T, cfg *alerts.AlertConfig) { assert.Equal(t, 5, cfg.TimeThresholds["guest"]) - assert.Equal(t, 5, cfg.TimeThresholds["pbs"]) + assert.Equal(t, 0, cfg.TimeThresholds["pbs"]) + assert.Equal(t, 5, cfg.TimeThresholds["all"]) }, }, { diff --git a/internal/config/persistence_test.go b/internal/config/persistence_test.go index 21ef33a6b2..4da499c49e 100644 --- a/internal/config/persistence_test.go +++ b/internal/config/persistence_test.go @@ -397,7 +397,7 @@ func TestLoadAlertConfigAppliesDefaults(t *testing.T) { raw := alerts.AlertConfig{ Enabled: false, - TimeThresholds: map[string]int{"guest": 0, "node": 0}, + TimeThresholds: map[string]int{"guest": -1, "node": 0}, DockerIgnoredContainerPrefixes: []string{" Runner "}, SnapshotDefaults: alerts.SnapshotAlertConfig{ Enabled: true, @@ -432,10 +432,10 @@ func TestLoadAlertConfigAppliesDefaults(t *testing.T) { } if got := loaded.TimeThresholds["guest"]; got != 5 { - t.Fatalf("expected guest threshold default 5, got %d", got) + t.Fatalf("expected unset guest threshold default 5, got %d", got) } - if got := loaded.TimeThresholds["node"]; got != 5 { - t.Fatalf("expected node threshold default 5, got %d", got) + if got := loaded.TimeThresholds["node"]; got != 0 { + t.Fatalf("expected node threshold 0 (no delay) to be kept, got %d", got) } if loaded.NodeDefaults.Temperature == nil { t.Fatalf("expected node temperature defaults to be set")