From da5be2db15e8972058515fed513750ee7c830405 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 01:45:19 +0100 Subject: [PATCH] fix(alerts): preserve storage override identity during config reload Retain storage policy aliases in durable metric and forecast metadata and use them during active-alert re-evaluation. Recover exact legacy PBS aliases from the recorded instance and datastore identity. Prevent a configuration reload from fabricating recovery against global defaults after restart. Change-source: pulse-maintainer --- .../v6/internal/subsystems/alerts.md | 16 +++ internal/alerts/canonical_stateful_test.go | 114 ++++++++++++++++++ internal/alerts/capacity_forecast.go | 16 ++- internal/alerts/config_runtime.go | 2 +- internal/alerts/storage_override_identity.go | 29 +++++ internal/alerts/unified_eval.go | 15 +++ 6 files changed, 185 insertions(+), 7 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 37cb71d26..9bf5bc449 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -71,6 +71,22 @@ their updated thresholds and may apply explicit resource-disable policies, but it must not treat provider-owned incidents as missing thresholds. Unrelated configuration saves preserve those incidents and their acknowledgement state until their provider evaluator supplies recovery evidence. +Storage configuration re-evaluation must use the same ordered resource ID and +alias override lookup as polling. Static and forecast capacity alerts retain +storage policy aliases in durable metadata, including through JSON and SQLite +restore; a configuration reload must not fabricate recovery by substituting +global defaults for a still-applicable datastore override. Older PBS snapshots +without alias metadata may reconstruct the canonical datastore alias only when +the recorded PBS instance, datastore name and complete legacy resource ID agree. +Hyphenated names must not be split heuristically, and a same-named datastore on +another instance must not inherit the override. Explicit policy changes retain +normal resolution semantics. +`TestPBSDatastoreOverrideLifecycleAcrossRestart` and +`TestStoragePolicyAliasesLegacyIdentity` in +`internal/alerts/canonical_stateful_test.go` pin restored incident identity, +hysteresis, confirmed recovery, refiring, event counts and instance isolation +for both persisted-alias and legacy snapshots. + When a VM or container stops, guest evaluation resolves only metric-threshold alerts whose observations are no longer meaningful. Backup-age and snapshot posture remain owned by their posture evaluator and may stay active while the diff --git a/internal/alerts/canonical_stateful_test.go b/internal/alerts/canonical_stateful_test.go index 5587e38c0..7920c9563 100644 --- a/internal/alerts/canonical_stateful_test.go +++ b/internal/alerts/canonical_stateful_test.go @@ -4,7 +4,9 @@ import ( "testing" "time" + "github.com/rcourtman/pulse-go-rewrite/internal/alerts/eventlog" 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/storagehealth" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" ) @@ -165,3 +167,115 @@ func TestStatefulAlertReFireCooldown(t *testing.T) { } }) } + +// A UI-keyed datastore override must govern actual incidents, not just the +// threshold resolver. Exercise hysteresis and recurrence across SQLite reopen, +// including a same-named datastore on another PBS instance. +func TestPBSDatastoreOverrideLifecycleAcrossRestart(t *testing.T) { + for _, legacySnapshot := range []bool{false, true} { + name := "persisted aliases" + if legacySnapshot { + name = "legacy snapshot" + } + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + start := func() *Manager { + m := NewManagerWithDataDir(dir) + t.Cleanup(m.Stop) + m.EnableEventLog() + if !m.activeStateAuthoritative.Load() { + t.Fatal("SQLite active state is not authoritative") + } + m.UpdateConfig(AlertConfig{Enabled: true, ActivationState: ActivationActive, + StorageDefault: HysteresisThreshold{Trigger: 95, Clear: 90}, + Overrides: map[string]ThresholdConfig{"pbs-primary/backups": {Usage: &HysteresisThreshold{Trigger: 80, Clear: 70}}}, + }) + disableTestTimeThresholds(m) + return m + } + storage := func(instance string, usage float64) models.Storage { + return models.Storage{ID: instance + "-backups", AliasIDs: []string{instance + "/backups"}, Name: "backups", Instance: instance, Type: "pbs", Status: "online", Total: 1000, Used: int64(usage * 10), Free: int64(1000 - usage*10), Usage: usage} + } + observe := func(m *Manager, usage float64) { + t.Helper() + for range 5 { + m.CheckStorage(storage("pbs-primary", usage)) + m.CheckStorage(storage("pbs-secondary", usage)) + } + if testHasActiveAlert(t, m, canonicalMetricStateID("pbs-secondary-backups", "usage")) { + t.Fatal("override leaked to another PBS instance") + } + } + events := func(m *Manager, fired, resolved int) { + t.Helper() + for kind, want := range map[string]int{eventlog.TypeFired: fired, eventlog.TypeResolved: resolved} { + if got := len(queryAlertEvents(t, m, eventlog.Filter{Types: []string{kind}})); got != want { + t.Fatalf("%s events = %d, want %d", kind, got, want) + } + } + } + id := canonicalMetricStateID("pbs-primary-backups", "usage") + m := start() + observe(m, 85) + original := *testRequireActiveAlert(t, m, id) + events(m, 1, 0) + if legacySnapshot { + m.mu.Lock() + delete(m.activeAlerts[id].Metadata, storagePolicyAliasesKey) + m.mu.Unlock() + if err := m.SaveActiveAlerts(); err != nil { + t.Fatal(err) + } + } + m.Stop() + + m = start() + observe(m, 75) // Below trigger, but not below the override's clear threshold. + if got := testRequireActiveAlert(t, m, id); !got.StartTime.Equal(original.StartTime) { + t.Fatal("restart replaced the firing incident") + } + events(m, 1, 0) + observe(m, 65) + if testHasActiveAlert(t, m, id) { + t.Fatal("override recovery did not clear incident") + } + events(m, 1, 1) + m.Stop() + + m = start() + observe(m, 75) + if testHasActiveAlert(t, m, id) { + t.Fatal("resolved incident resurrected inside hysteresis band") + } + events(m, 1, 1) + observe(m, 85) + if got := testRequireActiveAlert(t, m, id); !got.StartTime.After(original.StartTime) { + t.Fatal("refire reused original incident start") + } + events(m, 2, 1) + }) + } +} + +func TestStoragePolicyAliasesLegacyIdentity(t *testing.T) { + for _, tc := range []struct { + name, instance, resource, datastore string + want bool + }{ + {"hyphenated names", "pbs-backup-east", "pbs-backup-east-daily-store", "daily-store", true}, + {"different instance", "pbs-backup-west", "pbs-backup-east-daily-store", "daily-store", false}, + {"not PBS", "backup-east", "backup-east-daily-store", "daily-store", false}, + {"missing datastore", "pbs-backup-east", "pbs-backup-east-", "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + got := storagePolicyAliases(&Alert{Instance: tc.instance, ResourceID: tc.resource, ResourceName: tc.datastore}) + if tc.want { + if len(got) != 1 || got[0] != tc.instance+"/"+tc.datastore { + t.Fatalf("aliases = %v", got) + } + } else if len(got) != 0 { + t.Fatalf("invented alias: %v", got) + } + }) + } +} diff --git a/internal/alerts/capacity_forecast.go b/internal/alerts/capacity_forecast.go index b435829b3..e7c2cc7d0 100644 --- a/internal/alerts/capacity_forecast.go +++ b/internal/alerts/capacity_forecast.go @@ -215,12 +215,13 @@ var capacityForecastMetadataKeys = []string{ func (m *Manager) evaluateStorageCapacity(storage models.Storage, thresholds ThresholdConfig, trend CapacityTrendObservation) { input := &UnifiedResourceInput{ - ID: storage.ID, - Type: "storage", - Name: storage.Name, - Node: storage.Node, - Instance: storage.Instance, - Disk: &UnifiedResourceMetric{Percent: storage.Usage}, + ID: storage.ID, + StorageAliases: storage.AliasIDs, + Type: "storage", + Name: storage.Name, + Node: storage.Node, + Instance: storage.Instance, + Disk: &UnifiedResourceMetric{Percent: storage.Usage}, } m.evaluateUnifiedCapacity(input, thresholds, trend, func() bool { if !m.config.Enabled { @@ -377,6 +378,9 @@ func (m *Manager) evaluateCapacityForecast(input *UnifiedResourceInput, threshol "forecastBucketCount": trend.BucketCount, "forecastCoverageSeconds": int64(trend.CoverageSpan / time.Second), } + if len(input.StorageAliases) > 0 { + metadata[storagePolicyAliasesKey] = append([]string(nil), input.StorageAliases...) + } _, _ = m.evaluateCanonicalLifecycleAlert(canonicalLifecycleAlertParams{ Spec: spec, Evidence: alertspecs.AlertEvidence{ diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go index efeabf819..27202d3d0 100644 --- a/internal/alerts/config_runtime.go +++ b/internal/alerts/config_runtime.go @@ -503,7 +503,7 @@ func (m *Manager) reevaluateActiveAlertsLocked() { alertsToResolve = append(alertsToResolve, alertID) continue } - thresholds := m.resolveResourceThresholds("storage", resourceID) + thresholds := m.effectiveAlertPolicyNoLock(alertPolicyQuery{TypeKey: "storage", ResourceID: resourceID, StorageAliases: storagePolicyAliases(alert)}).Thresholds if thresholds.Disabled { alertsToResolve = append(alertsToResolve, alertID) continue diff --git a/internal/alerts/storage_override_identity.go b/internal/alerts/storage_override_identity.go index a0c906114..00c19a86a 100644 --- a/internal/alerts/storage_override_identity.go +++ b/internal/alerts/storage_override_identity.go @@ -67,3 +67,32 @@ func (m *Manager) resolveStorageThresholdOverride(base ThresholdConfig, resource func (m *Manager) resolveStorageThresholdsNoLock(storage models.Storage) ThresholdConfig { return m.resolveStorageThresholdOverride(m.defaultThresholdsForResourceType("storage"), storage.ID, storage.AliasIDs) } + +// Keep the polling identity through JSON/SQLite restore so configuration saves +// cannot silently replace a datastore override with global storage defaults. +const storagePolicyAliasesKey = "storagePolicyAliases" + +func storagePolicyAliases(alert *Alert) []string { + if alert == nil { + return nil + } + switch aliases := alert.Metadata[storagePolicyAliasesKey].(type) { + case []string: + return append([]string(nil), aliases...) + case []interface{}: + result := make([]string, 0, len(aliases)) + for _, value := range aliases { + if alias, ok := value.(string); ok { + result = append(result, alias) + } + } + return result + } + // Older PBS snapshots predate alias metadata. The poller records the + // instance and datastore name separately: only reconstruct an alias when + // the complete legacy ID agrees, never split hyphenated names heuristically. + if strings.HasPrefix(alert.Instance, "pbs-") && alert.ResourceName != "" && alert.ResourceID == alert.Instance+"-"+alert.ResourceName { + return []string{alert.Instance + "/" + alert.ResourceName} + } + return nil +} diff --git a/internal/alerts/unified_eval.go b/internal/alerts/unified_eval.go index 7e065cb9b..bcffe8100 100644 --- a/internal/alerts/unified_eval.go +++ b/internal/alerts/unified_eval.go @@ -55,6 +55,8 @@ type UnifiedResourceInput struct { NetworkIn *UnifiedResourceMetric NetworkOut *UnifiedResourceMetric Temperature *UnifiedResourceMetric + + StorageAliases []string // Durable policy lookup identities for storage alerts. } type unifiedMetricCandidate struct { @@ -259,6 +261,19 @@ func (m *Manager) evaluateUnifiedMetrics(input *UnifiedResourceInput, thresholds } opts = metricOptionsWithTags(opts, input.Tags) + if len(input.StorageAliases) > 0 { + merged := metricOptions{} + if opts != nil { + merged = *opts + } + metadata := make(map[string]interface{}, len(merged.Metadata)+1) + for k, v := range merged.Metadata { + metadata[k] = v + } + metadata[storagePolicyAliasesKey] = append([]string(nil), input.StorageAliases...) + merged.Metadata = metadata + opts = &merged + } for _, candidate := range buildUnifiedMetricCandidates(input, thresholds) { m.checkMetricWithCanonicalSpec(candidate.Spec, input.Name, input.Node, input.Instance, unifiedAlertType(input.Type), candidate.Value, candidate.Threshold, opts) }