From 359a372cdfb667bd20b559ecd3f25e2ac0cf3b58 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 12 Aug 2026 06:38:25 +0100 Subject: [PATCH] Make per-guest backup/snapshot toggles inherit global thresholds The per-guest Backup/Snapshot toggle persisted a full copy of the current global defaults just to flip enabled, freezing the threshold values into the override. Later global edits (a 32-day backup warning) then silently never applied to toggled guests, which kept firing at the frozen factory 7-day warning. Reported twice in discussion #1126. - Guest overrides now resolve against the globals at evaluation time: zero-valued fields inherit the global value, explicit values still win, and hand-written sparse overrides stop decoding as accidental zeros. - The toggles write enabled-only overrides instead of freezing a copy. - Normalization rewrites stored overrides whose thresholds exactly match the current globals into sparse form, which is behavior-preserving at migration time and un-freezes existing installs. - The Backups/Snapshots global editors reconcile warning/critical pairs by adjusting the untouched field, so typing a 32-day warning no longer silently snaps back to the 14-day critical default. Refs #1126 --- .../v6/internal/subsystems/alerts.md | 37 ++++ .../subsystems/frontend-primitives.md | 7 + frontend-modern/browser-verification.json | 46 ++-- .../ThresholdsTableProxmoxBackupsSection.tsx | 31 ++- ...ThresholdsTableProxmoxSnapshotsSection.tsx | 57 +++-- .../thresholds/__tests__/helpers.test.ts | 27 +++ .../src/features/alerts/thresholds/helpers.ts | 19 ++ .../useThresholdsOverrideMutations.test.tsx | 104 +++++++++ .../hooks/useThresholdsOverrideMutations.ts | 18 +- internal/alerts/alerts_test.go | 208 ++++++++++++++++++ internal/alerts/backup_snapshot.go | 51 ++++- internal/alerts/config/normalize.go | 36 +++ internal/alerts/config_facade.go | 7 + internal/alerts/config_runtime.go | 1 + internal/config/persistence.go | 2 + 15 files changed, 598 insertions(+), 53 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 3c1441844..e036d2081 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -816,6 +816,28 @@ evaluation, backup rollup age evaluation, backup inventory readiness, PVE template subject matching, namespace disambiguation, and snapshot/backup active alert cleanup; future backup or snapshot alert behavior should extend that owner rather than expanding the central Manager file. +Per-guest backup and snapshot overrides are sparse, not frozen copies. A +zero-valued threshold field in an override (`warningDays`, `criticalDays`, +`freshHours`, `staleHours`, and the snapshot size pair) inherits the current +global default at evaluation time through the merge helpers in +`internal/alerts/backup_snapshot.go`; explicit non-zero values win, and +`alertOrphaned` plus `ignoreVMIDs` always resolve from the globals because they +are instance-wide filters. `NormalizeRecoveryOverrides` in +`internal/alerts/config/normalize.go` (called from `UpdateConfig` in +`internal/alerts/config_runtime.go` and from the shared persistence +normalization in `internal/config/persistence.go`) rewrites stored overrides +whose threshold tuple exactly matches the current globals into sparse +enabled-only form. Those full copies were artifacts of the legacy per-guest +toggle, and the equality condition makes the rewrite behavior-preserving at +the moment it runs while letting the guest track later global edits (#1126). +Overrides whose thresholds differ from the current globals are deliberate +per-guest values and must never be rewritten. +`TestGuestBackupOverrideInheritsGlobalThresholds`, +`TestFrozenBackupOverrideMigratesToSparse`, +`TestDeliberateBackupOverridePreserved`, and +`TestMergeSnapshotOverrideInheritsZeroFields` in +`internal/alerts/alerts_test.go` pin the merge, migration, and preservation +behaviors. Proxmox disk health alert evaluation now lives in `internal/alerts/disk_health.go`. That file owns Proxmox disk canonical identity, disk health assessment alerts, known-firmware health suppression, and @@ -1107,6 +1129,21 @@ override toggles now route through while powered-off/connectivity state transitions plus alert-removal side effects now route through `frontend-modern/src/features/alerts/thresholds/hooks/useThresholdsAvailabilityMutations.ts`. +The backup/snapshot override toggles in that mutations owner write sparse +enabled-only overrides (zero-valued threshold fields) rather than copying the +current global defaults into the override, because zero-valued fields inherit +the globals at evaluation time and a copied value freezes the guest against +later global edits (#1126). Toggling a guest that already carries explicit +override thresholds must preserve those values while flipping enabled. +The Backups and Snapshots global-defaults editors in +`frontend-modern/src/components/Alerts/ThresholdsTableProxmoxBackupsSection.tsx` +and +`frontend-modern/src/components/Alerts/ThresholdsTableProxmoxSnapshotsSection.tsx` +reconcile warning/critical pairs through `reconcileWarningCriticalEdit` in +`frontend-modern/src/features/alerts/thresholds/helpers.ts`: a single-field +edit that creates a conflict adjusts the field the user did not touch, so a +32-day warning edit raises critical alongside it instead of being silently +clamped back down by the sanitizers before save. That same thresholds host-data boundary now treats top-level TrueNAS appliances as canonical `agent` resources with `platformType: 'truenas'`. System-disk group headers must use agent-owned header metadata instead of guest/node- diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 9b6be07ff..204b395c5 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -319,6 +319,13 @@ tabs. The thresholds platform IA is platform-shaped: Proxmox, Docker, Kubernetes, TrueNAS, vSphere, PBS, PMG, and Systems. Frontend primitives own the chip, reset, "+ Filter", and route-backed shell pattern; alerts must not replace that with page-local search/tab chrome or legacy neutral buckets. +Threshold edit semantics under `frontend-modern/src/features/alerts/thresholds/` +also stay alerts-owned: the override mutation hooks write sparse enabled-only +backup/snapshot overrides that inherit global thresholds at evaluation time, +and the warning/critical pair reconciliation in the thresholds helpers adjusts +the untouched field on a conflicting single-field edit. Primitives must not +absorb those persistence or validation rules into shared form/table chrome +(#1126). PlatformTableToolbar may accept compact consumer-owned context through its shared leading-control rail when the context is actionable for the inventory immediately below it. Consumers must use that extension point for local diff --git a/frontend-modern/browser-verification.json b/frontend-modern/browser-verification.json index fed067ea8..c8ca79745 100644 --- a/frontend-modern/browser-verification.json +++ b/frontend-modern/browser-verification.json @@ -1,22 +1,22 @@ { "version": 1, - "base_sha": "cea33b8ee29255035d7d248704b80428761085c6", - "verified_at": "2026-08-11T19:40:09Z", + "base_sha": "1243cde4971a70007ac5f867593da76f19698e22", + "verified_at": "2026-08-12T05:34:19Z", "result": "passed", "changed_paths": [ - "frontend-modern/src/components/Workloads/diskListModel.ts", - "frontend-modern/src/components/Workloads/stackedDiskBarModel.ts", - "frontend-modern/src/features/standalone/agentMachineTableModel.ts", - "frontend-modern/src/utils/format.ts" + "frontend-modern/src/components/Alerts/ThresholdsTableProxmoxBackupsSection.tsx", + "frontend-modern/src/components/Alerts/ThresholdsTableProxmoxSnapshotsSection.tsx", + "frontend-modern/src/features/alerts/thresholds/helpers.ts", + "frontend-modern/src/features/alerts/thresholds/hooks/useThresholdsOverrideMutations.ts" ], "content_sha256": { - "frontend-modern/src/components/Workloads/diskListModel.ts": "82dd17c7d9ba7c5ee103db375d11748df245ecf843ab56505f9e4fa6d21f0b81", - "frontend-modern/src/components/Workloads/stackedDiskBarModel.ts": "a72196fdc5c75bdd40ad5c45f1934e06d05ebdd4a89f68f6e932a12f97383b78", - "frontend-modern/src/features/standalone/agentMachineTableModel.ts": "fe5a0b14cc9324ac9d6197295a6f201fe59c6d22fd66437b22aeb4b567b316cf", - "frontend-modern/src/utils/format.ts": "8c09d25938796573232752d5e326055712c784dc906731f110188804a09b2114" + "frontend-modern/src/components/Alerts/ThresholdsTableProxmoxBackupsSection.tsx": "cebf62372258700510ff8f9a912bbf51df2c7fafbbc263de12e52353aaf0d335", + "frontend-modern/src/components/Alerts/ThresholdsTableProxmoxSnapshotsSection.tsx": "705c40d5b57090e551d56eeef5a9e04fa007a43f5d6a78c34033e5d6472a5ffc", + "frontend-modern/src/features/alerts/thresholds/helpers.ts": "b3da861fb86315e87beeade0376871bf4cf9dbeb14bd53d13055584a98866abb", + "frontend-modern/src/features/alerts/thresholds/hooks/useThresholdsOverrideMutations.ts": "1e917cd27216674aa58c038b9715f7621668bcc157a97e1b4f0e6ac1b606e0f8" }, "routes": [ - "/proxmox" + "/alerts/thresholds/proxmox" ], "viewports": [ { @@ -24,20 +24,24 @@ "height": 800 }, { - "width": 768, - "height": 900 + "width": 375, + "height": 812 } ], "states": [ - "mock-mode LXC edge-proxy-01 with a measured rootfs plus a config-only mount (/srv/archive, usage -1, total 20 GiB) in the shared workloads table", - "workloads row disk cell: single measured bar (37% target), no fabricated 0% mini-slot for the config-only mount, at both viewports", - "row disk tooltip: Disk Breakdown lists / at 37% (21.9 GB/59.0 GB) and /srv/archive as — with ?/20.0 GB", - "guest drawer Filesystems block: rootfs card 21.9 GB/59.0 GB 37% ROOTFS and /srv/archive card ?/20.0 GB — MP0 at both viewports" + "Backups section global defaults row at factory 7/14 warning/critical days", + "Backups global defaults row after warning-days edit to 32 (critical auto-raised to 32, warning preserved)", + "Backups global defaults row after critical-days edit to 45 (consistent 32/45 pair untouched)", + "Backups global defaults row after critical-days edit to 10 below warning (warning lowered to 10/10)", + "Narrow 375px card layout of the same Backups global defaults row with the warning-days edit applied", + "All edits discarded via reload before saving; live dev alert config unchanged" ], "interactions": [ - "searched the Proxmox workloads table for edge-proxy-01 at 1280x800", - "hovered the row disk cell to open the Disk Breakdown tooltip and read both entries", - "clicked the row to expand the guest drawer and read the Filesystems block", - "resized to 768x900, reloaded /proxmox, re-filtered to edge-proxy-01, re-opened the drawer and re-read the disk cell and Filesystems block" + "Logged in as admin and opened Alerts > Thresholds > Proxmox", + "Edited Backups global Warning Days from 7 to 32 and confirmed the value stayed 32 with Critical Days raised to 32 instead of the old silent 14 clamp", + "Edited Critical Days to 45 and confirmed the 32/45 pair was left untouched", + "Edited Critical Days to 10 and confirmed Warning Days lowered with it to 10/10", + "Repeated the Warning Days 32 edit at 375px mobile viewport and confirmed identical 32/32 behavior in the card layout", + "Reloaded without saving after each pass so no threshold change persisted to the dev backend" ] } diff --git a/frontend-modern/src/components/Alerts/ThresholdsTableProxmoxBackupsSection.tsx b/frontend-modern/src/components/Alerts/ThresholdsTableProxmoxBackupsSection.tsx index 744354a75..283ad504d 100644 --- a/frontend-modern/src/components/Alerts/ThresholdsTableProxmoxBackupsSection.tsx +++ b/frontend-modern/src/components/Alerts/ThresholdsTableProxmoxBackupsSection.tsx @@ -10,7 +10,10 @@ import { DEFAULT_BACKUP_FRESH_HOURS, DEFAULT_BACKUP_STALE_HOURS, } from '@/features/alerts/thresholds/constants'; -import { formatMetricValue } from '@/features/alerts/thresholds/helpers'; +import { + formatMetricValue, + reconcileWarningCriticalEdit, +} from '@/features/alerts/thresholds/helpers'; import type { ThresholdsTableSectionProps } from '@/features/alerts/thresholds/thresholdsTableSectionProps'; export function ThresholdsTableProxmoxBackupsSection(props: ThresholdsTableSectionProps) { @@ -56,6 +59,22 @@ export function ThresholdsTableProxmoxBackupsSection(props: ThresholdsTableSecti typeof value === 'function' ? value(currentRecord) : { ...currentRecord, ...value }; + const days = reconcileWarningCriticalEdit( + { + warning: currentRecord['warning days'], + critical: currentRecord['critical days'], + }, + { + warning: + typeof nextRecord['warning days'] === 'number' + ? nextRecord['warning days'] + : currentRecord['warning days'], + critical: + typeof nextRecord['critical days'] === 'number' + ? nextRecord['critical days'] + : currentRecord['critical days'], + }, + ); return { ...prev, freshHours: @@ -66,14 +85,8 @@ export function ThresholdsTableProxmoxBackupsSection(props: ThresholdsTableSecti typeof nextRecord['stale hours'] === 'number' ? nextRecord['stale hours'] : prev.staleHours, - warningDays: - typeof nextRecord['warning days'] === 'number' - ? nextRecord['warning days'] - : prev.warningDays, - criticalDays: - typeof nextRecord['critical days'] === 'number' - ? nextRecord['critical days'] - : prev.criticalDays, + warningDays: days.warning, + criticalDays: days.critical, }; }); }} diff --git a/frontend-modern/src/components/Alerts/ThresholdsTableProxmoxSnapshotsSection.tsx b/frontend-modern/src/components/Alerts/ThresholdsTableProxmoxSnapshotsSection.tsx index fb25be7d1..7337186da 100644 --- a/frontend-modern/src/components/Alerts/ThresholdsTableProxmoxSnapshotsSection.tsx +++ b/frontend-modern/src/components/Alerts/ThresholdsTableProxmoxSnapshotsSection.tsx @@ -3,7 +3,10 @@ import Camera from 'lucide-solid/icons/camera'; import { ResourceTable } from './ResourceTable'; import { CollapsibleSection } from './Thresholds/sections/CollapsibleSection'; -import { formatMetricValue } from '@/features/alerts/thresholds/helpers'; +import { + formatMetricValue, + reconcileWarningCriticalEdit, +} from '@/features/alerts/thresholds/helpers'; import type { ThresholdsTableSectionProps } from '@/features/alerts/thresholds/thresholdsTableSectionProps'; export function ThresholdsTableProxmoxSnapshotsSection(props: ThresholdsTableSectionProps) { @@ -49,24 +52,44 @@ export function ThresholdsTableProxmoxSnapshotsSection(props: ThresholdsTableSec typeof value === 'function' ? value(currentRecord) : { ...currentRecord, ...value }; + const days = reconcileWarningCriticalEdit( + { + warning: currentRecord['warning days'], + critical: currentRecord['critical days'], + }, + { + warning: + typeof nextRecord['warning days'] === 'number' + ? nextRecord['warning days'] + : currentRecord['warning days'], + critical: + typeof nextRecord['critical days'] === 'number' + ? nextRecord['critical days'] + : currentRecord['critical days'], + }, + ); + const sizes = reconcileWarningCriticalEdit( + { + warning: currentRecord.warningSizeGiB, + critical: currentRecord.criticalSizeGiB, + }, + { + warning: + typeof nextRecord.warningSizeGiB === 'number' + ? nextRecord.warningSizeGiB + : currentRecord.warningSizeGiB, + critical: + typeof nextRecord.criticalSizeGiB === 'number' + ? nextRecord.criticalSizeGiB + : currentRecord.criticalSizeGiB, + }, + ); return { ...prev, - warningDays: - typeof nextRecord['warning days'] === 'number' - ? nextRecord['warning days'] - : prev.warningDays, - criticalDays: - typeof nextRecord['critical days'] === 'number' - ? nextRecord['critical days'] - : prev.criticalDays, - warningSizeGiB: - typeof nextRecord.warningSizeGiB === 'number' - ? nextRecord.warningSizeGiB - : prev.warningSizeGiB, - criticalSizeGiB: - typeof nextRecord.criticalSizeGiB === 'number' - ? nextRecord.criticalSizeGiB - : prev.criticalSizeGiB, + warningDays: days.warning, + criticalDays: days.critical, + warningSizeGiB: sizes.warning, + criticalSizeGiB: sizes.critical, }; }); }} diff --git a/frontend-modern/src/features/alerts/thresholds/__tests__/helpers.test.ts b/frontend-modern/src/features/alerts/thresholds/__tests__/helpers.test.ts index 533db6c5f..ba43ef586 100644 --- a/frontend-modern/src/features/alerts/thresholds/__tests__/helpers.test.ts +++ b/frontend-modern/src/features/alerts/thresholds/__tests__/helpers.test.ts @@ -3,6 +3,7 @@ import { normalizeThresholdLabel, normalizeDockerIgnoredInput, formatMetricValue, + reconcileWarningCriticalEdit, } from '@/features/alerts/thresholds/helpers'; describe('alerts thresholds helpers', () => { @@ -132,4 +133,30 @@ describe('alerts thresholds helpers', () => { expect(formatMetricValue('unknown', 123)).toBe('123'); }); }); + + describe('reconcileWarningCriticalEdit', () => { + it('raises critical when the warning edit exceeds it', () => { + expect( + reconcileWarningCriticalEdit({ warning: 7, critical: 14 }, { warning: 32, critical: 14 }), + ).toEqual({ warning: 32, critical: 32 }); + }); + + it('lowers warning when the critical edit drops below it', () => { + expect( + reconcileWarningCriticalEdit({ warning: 32, critical: 45 }, { warning: 32, critical: 10 }), + ).toEqual({ warning: 10, critical: 10 }); + }); + + it('leaves consistent pairs untouched', () => { + expect( + reconcileWarningCriticalEdit({ warning: 7, critical: 14 }, { warning: 10, critical: 14 }), + ).toEqual({ warning: 10, critical: 14 }); + }); + + it('allows warning-only configurations when critical is unset', () => { + expect( + reconcileWarningCriticalEdit({ warning: 7, critical: 0 }, { warning: 32, critical: 0 }), + ).toEqual({ warning: 32, critical: 0 }); + }); + }); }); diff --git a/frontend-modern/src/features/alerts/thresholds/helpers.ts b/frontend-modern/src/features/alerts/thresholds/helpers.ts index 22617c1ac..edb543ee8 100644 --- a/frontend-modern/src/features/alerts/thresholds/helpers.ts +++ b/frontend-modern/src/features/alerts/thresholds/helpers.ts @@ -20,6 +20,25 @@ export const pmgColumn = (key: keyof PMGThresholdDefaults, label: string) => ({ normalized: normalizeThresholdLabel(label), }); +// Keeps a warning/critical pair consistent after a single-field edit by +// adjusting the field the user did NOT touch. The sanitizers clamp the warning +// value down when it exceeds critical, which silently rewrote the number the +// user just typed (a 32-day backup warning collapsed back to the 14-day +// critical default, #1126). Raising the counterpart instead preserves the +// typed value and stays visible in the form before saving. +export const reconcileWarningCriticalEdit = ( + prev: { warning: number; critical: number }, + next: { warning: number; critical: number }, +): { warning: number; critical: number } => { + const warningChanged = next.warning !== prev.warning; + const criticalChanged = next.critical !== prev.critical; + if (next.critical <= 0 || next.warning <= next.critical) return next; + if (warningChanged && !criticalChanged) return { warning: next.warning, critical: next.warning }; + if (criticalChanged && !warningChanged) + return { warning: next.critical, critical: next.critical }; + return next; +}; + export const normalizeDockerIgnoredInput = (value: string): string[] => value .split('\n') diff --git a/frontend-modern/src/features/alerts/thresholds/hooks/__tests__/useThresholdsOverrideMutations.test.tsx b/frontend-modern/src/features/alerts/thresholds/hooks/__tests__/useThresholdsOverrideMutations.test.tsx index 7555b77ce..c67ad5258 100644 --- a/frontend-modern/src/features/alerts/thresholds/hooks/__tests__/useThresholdsOverrideMutations.test.tsx +++ b/frontend-modern/src/features/alerts/thresholds/hooks/__tests__/useThresholdsOverrideMutations.test.tsx @@ -169,4 +169,108 @@ describe('useThresholdsOverrideMutations', () => { }, }); }); + + it('toggleBackup writes a sparse enabled-only override instead of freezing the globals', () => { + const overrideSignal = createSignal([]); + const { props, rawOverridesConfig } = buildTableProps(overrideSignal); + const guestResource: TableResource = { + id: 'cluster-a:node-2:100', + name: 'db-01', + type: 'guest', + resourceType: 'VM', + vmid: 100, + node: 'node-2', + instance: 'cluster-a', + defaults: { cpu: 80 }, + thresholds: { cpu: 80 }, + }; + + const { result } = renderHook(() => + useThresholdsOverrideMutations({ + props, + resources: { + nodesWithOverrides: () => [], + agentsWithOverrides: () => [], + agentDisksWithOverrides: () => [], + dockerHostsWithOverrides: () => [], + guestsFlat: () => [guestResource], + dockerContainersFlat: () => [], + pbsServersWithOverrides: () => [], + pmgServersWithOverrides: () => [], + storageWithOverrides: () => [], + }, + editingThresholds: () => ({}), + editingNote: () => '', + bulkEditIds: () => [], + cancelEdit: vi.fn(), + }), + ); + + result.toggleBackup(guestResource.id, true); + + // Zero-valued thresholds inherit the globals at evaluation time. A copy of + // the global 7/14 here would freeze those values into the override (#1126). + const rawEntries = Object.values(rawOverridesConfig()); + expect(rawEntries).toHaveLength(1); + expect(rawEntries[0].backup).toEqual({ + enabled: true, + warningDays: 0, + criticalDays: 0, + }); + }); + + it('toggleSnapshot preserves explicit override thresholds while flipping enabled', () => { + const overrideSignal = createSignal([ + { + id: 'cluster-a:node-2:100', + name: 'db-01', + type: 'guest', + thresholds: {}, + snapshot: { enabled: false, warningDays: 5, criticalDays: 9 }, + }, + ]); + const { props, rawOverridesConfig } = buildTableProps(overrideSignal); + const guestResource: TableResource = { + id: 'cluster-a:node-2:100', + name: 'db-01', + type: 'guest', + resourceType: 'VM', + vmid: 100, + node: 'node-2', + instance: 'cluster-a', + defaults: { cpu: 80 }, + thresholds: { cpu: 80 }, + }; + + const { result } = renderHook(() => + useThresholdsOverrideMutations({ + props, + resources: { + nodesWithOverrides: () => [], + agentsWithOverrides: () => [], + agentDisksWithOverrides: () => [], + dockerHostsWithOverrides: () => [], + guestsFlat: () => [guestResource], + dockerContainersFlat: () => [], + pbsServersWithOverrides: () => [], + pmgServersWithOverrides: () => [], + storageWithOverrides: () => [], + }, + editingThresholds: () => ({}), + editingNote: () => '', + bulkEditIds: () => [], + cancelEdit: vi.fn(), + }), + ); + + result.toggleSnapshot(guestResource.id); + + const rawEntries = Object.values(rawOverridesConfig()); + expect(rawEntries).toHaveLength(1); + expect(rawEntries[0].snapshot).toEqual({ + enabled: true, + warningDays: 5, + criticalDays: 9, + }); + }); }); diff --git a/frontend-modern/src/features/alerts/thresholds/hooks/useThresholdsOverrideMutations.ts b/frontend-modern/src/features/alerts/thresholds/hooks/useThresholdsOverrideMutations.ts index 27d2d34c0..13b88b739 100644 --- a/frontend-modern/src/features/alerts/thresholds/hooks/useThresholdsOverrideMutations.ts +++ b/frontend-modern/src/features/alerts/thresholds/hooks/useThresholdsOverrideMutations.ts @@ -322,7 +322,15 @@ export function useThresholdsOverrideMutations({ const { storageId } = getOverridePersistenceIdentity(resource); const existingOverride = findOverrideForResource(props.overrides(), resource); const previousRaw = findRawOverrideConfigForResource(props.rawOverridesConfig(), resource); - const baseConfig = existingOverride?.backup || props.backupDefaults(); + // Zero-valued thresholds inherit the global defaults at evaluation time, + // so a toggle-created override stores only the enabled flag. Copying the + // globals here would freeze their current values into the override and + // silently detach the guest from later global edits (#1126). + const baseConfig = existingOverride?.backup ?? { + enabled: props.backupDefaults().enabled, + warningDays: 0, + criticalDays: 0, + }; const newBackup = { ...baseConfig, enabled: forceState !== undefined ? forceState : !baseConfig.enabled, @@ -361,7 +369,13 @@ export function useThresholdsOverrideMutations({ const { storageId } = getOverridePersistenceIdentity(resource); const existingOverride = findOverrideForResource(props.overrides(), resource); const previousRaw = findRawOverrideConfigForResource(props.rawOverridesConfig(), resource); - const baseConfig = existingOverride?.snapshot || props.snapshotDefaults(); + // Sparse for the same reason as toggleBackup: zero thresholds inherit the + // globals at evaluation time instead of freezing a copy (#1126). + const baseConfig = existingOverride?.snapshot ?? { + enabled: props.snapshotDefaults().enabled, + warningDays: 0, + criticalDays: 0, + }; const newSnapshot = { ...baseConfig, enabled: forceState !== undefined ? forceState : !baseConfig.enabled, diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index 263b82ac6..aad23d6a9 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -19954,3 +19954,211 @@ func TestDockerContainerOverrideLegacyIDKeyStillHonoured(t *testing.T) { t.Fatalf("expected legacy ID-keyed override to still disable container alerts") } } + +func backupTestFixtures(now time.Time, ageDays int) ( + []recovery.ProtectionRollup, + map[string]GuestLookup, + map[string][]GuestLookup, + string, +) { + rollups := []recovery.ProtectionRollup{ + { + RollupID: "res:vm:proxmox:inst:node:100", + SubjectRef: &recovery.ExternalRef{ + Type: "proxmox-vm", + Namespace: "inst", + Name: "app-server", + ID: "inst:node:100", + Class: "node", + }, + LastSuccessAt: ptrTime(now.Add(-time.Duration(ageDays) * 24 * time.Hour)), + LastOutcome: recovery.OutcomeSuccess, + Providers: []recovery.Provider{recovery.ProviderProxmoxPVE}, + }, + } + key := BuildGuestKey("inst", "node", 100) + guestsByKey := map[string]GuestLookup{ + key: { + ResourceID: "inst:node:100", + Name: "app-server", + Instance: "inst", + Node: "node", + Type: "qemu", + VMID: 100, + }, + } + guestsByVMID := map[string][]GuestLookup{"100": {guestsByKey[key]}} + return rollups, guestsByKey, guestsByVMID, key +} + +// A sparse enabled-only override must inherit the global thresholds instead of +// zeroing them out (#1126). +func TestGuestBackupOverrideInheritsGlobalThresholds(t *testing.T) { + m := newTestManager(t) + m.ClearActiveAlerts() + + cfg := m.GetConfig() + cfg.Enabled = true + cfg.BackupDefaults = BackupAlertConfig{ + Enabled: true, + WarningDays: 32, + CriticalDays: 45, + } + cfg.Overrides = map[string]ThresholdConfig{ + "inst:node:100": {Backup: &BackupAlertConfig{Enabled: true}}, + } + m.UpdateConfig(cfg) + + now := time.Now() + rollups, guestsByKey, guestsByVMID, key := backupTestFixtures(now, 10) + + m.CheckBackups(rollups, guestsByKey, guestsByVMID) + m.mu.RLock() + _, exists := m.activeAlerts["backup-age-"+sanitizeAlertKey(key)] + m.mu.RUnlock() + if exists { + t.Fatalf("10-day-old backup alerted despite inherited 32-day warning threshold") + } + + rollups, guestsByKey, guestsByVMID, key = backupTestFixtures(now, 33) + m.CheckBackups(rollups, guestsByKey, guestsByVMID) + m.mu.RLock() + alert, exists := testLookupActiveAlert(t, m, "backup-age-"+sanitizeAlertKey(key)) + m.mu.RUnlock() + if !exists { + t.Fatalf("expected warning alert at inherited 32-day threshold") + } + if alert.Level != AlertLevelWarning { + t.Fatalf("expected warning alert, got %s", alert.Level) + } +} + +// findBackupOverride returns the single backup override in the config, +// regardless of the storage key normalizeOverrides settled on. +func findBackupOverride(t *testing.T, m *Manager) BackupAlertConfig { + t.Helper() + for _, override := range m.GetConfig().Overrides { + if override.Backup != nil { + return *override.Backup + } + } + t.Fatalf("no backup override found in config") + return BackupAlertConfig{} +} + +// An override that is an exact threshold copy of the current globals is a +// frozen artifact of the legacy toggle. It must be rewritten to sparse so a +// later global threshold change applies to the guest (#1126). +func TestFrozenBackupOverrideMigratesToSparse(t *testing.T) { + m := newTestManager(t) + m.ClearActiveAlerts() + + cfg := m.GetConfig() + cfg.Enabled = true + cfg.BackupDefaults = BackupAlertConfig{ + Enabled: true, + WarningDays: 7, + CriticalDays: 14, + FreshHours: 24, + StaleHours: 72, + } + cfg.Overrides = map[string]ThresholdConfig{ + "inst:node:100": { + Backup: &BackupAlertConfig{ + Enabled: true, + WarningDays: 7, + CriticalDays: 14, + FreshHours: 24, + StaleHours: 72, + }, + }, + } + m.UpdateConfig(cfg) + + migrated := findBackupOverride(t, m) + if !migrated.Enabled { + t.Fatalf("expected enabled sparse backup override, got %+v", migrated) + } + if migrated.WarningDays != 0 || migrated.CriticalDays != 0 { + t.Fatalf("frozen copy not migrated to sparse: %+v", migrated) + } + + // Raising the global warning threshold must now take effect for the guest. + cfg = m.GetConfig() + cfg.BackupDefaults.WarningDays = 32 + cfg.BackupDefaults.CriticalDays = 45 + m.UpdateConfig(cfg) + + rollups, guestsByKey, guestsByVMID, key := backupTestFixtures(time.Now(), 10) + m.CheckBackups(rollups, guestsByKey, guestsByVMID) + m.mu.RLock() + _, exists := m.activeAlerts["backup-age-"+sanitizeAlertKey(key)] + m.mu.RUnlock() + if exists { + t.Fatalf("10-day-old backup alerted at stale frozen 7-day threshold after global raised to 32") + } +} + +// Overrides whose thresholds differ from the globals are deliberate and must +// survive migration and win at evaluation. +func TestDeliberateBackupOverridePreserved(t *testing.T) { + m := newTestManager(t) + m.ClearActiveAlerts() + + cfg := m.GetConfig() + cfg.Enabled = true + cfg.BackupDefaults = BackupAlertConfig{ + Enabled: true, + WarningDays: 32, + CriticalDays: 45, + } + cfg.Overrides = map[string]ThresholdConfig{ + "inst:node:100": { + Backup: &BackupAlertConfig{ + Enabled: true, + WarningDays: 7, + CriticalDays: 10, + }, + }, + } + m.UpdateConfig(cfg) + + kept := findBackupOverride(t, m) + if kept.WarningDays != 7 || kept.CriticalDays != 10 { + t.Fatalf("deliberate override rewritten: %+v", kept) + } + + rollups, guestsByKey, guestsByVMID, key := backupTestFixtures(time.Now(), 11) + m.CheckBackups(rollups, guestsByKey, guestsByVMID) + m.mu.RLock() + alert, exists := testLookupActiveAlert(t, m, "backup-age-"+sanitizeAlertKey(key)) + m.mu.RUnlock() + if !exists { + t.Fatalf("expected critical alert at deliberate 10-day override threshold") + } + if alert.Level != AlertLevelCritical { + t.Fatalf("expected critical alert, got %s", alert.Level) + } +} + +func TestMergeSnapshotOverrideInheritsZeroFields(t *testing.T) { + defaults := SnapshotAlertConfig{ + Enabled: true, + WarningDays: 20, + CriticalDays: 30, + WarningSizeGiB: 50, + CriticalSizeGiB: 100, + } + merged := mergeSnapshotOverride(defaults, SnapshotAlertConfig{Enabled: true}) + if merged.WarningDays != 20 || merged.CriticalDays != 30 { + t.Fatalf("sparse snapshot override did not inherit day thresholds: %+v", merged) + } + if merged.WarningSizeGiB != 50 || merged.CriticalSizeGiB != 100 { + t.Fatalf("sparse snapshot override did not inherit size thresholds: %+v", merged) + } + + explicit := mergeSnapshotOverride(defaults, SnapshotAlertConfig{Enabled: true, WarningDays: 5}) + if explicit.WarningDays != 5 || explicit.CriticalDays != 30 { + t.Fatalf("explicit snapshot override field lost: %+v", explicit) + } +} diff --git a/internal/alerts/backup_snapshot.go b/internal/alerts/backup_snapshot.go index 2b6ba072b..fea347c27 100644 --- a/internal/alerts/backup_snapshot.go +++ b/internal/alerts/backup_snapshot.go @@ -38,10 +38,53 @@ func backupIgnoreVMID(vmID string, ignoreList []string) bool { return false } +// mergeSnapshotOverride resolves a per-guest snapshot override against the +// global defaults. Overrides written by the per-guest toggle carry only the +// enabled flag, so zero-valued thresholds inherit the global value; explicit +// non-zero values win (#1126). +func mergeSnapshotOverride(defaults SnapshotAlertConfig, override SnapshotAlertConfig) SnapshotAlertConfig { + merged := override + if merged.WarningDays <= 0 { + merged.WarningDays = defaults.WarningDays + } + if merged.CriticalDays <= 0 { + merged.CriticalDays = defaults.CriticalDays + } + if merged.WarningSizeGiB <= 0 { + merged.WarningSizeGiB = defaults.WarningSizeGiB + } + if merged.CriticalSizeGiB <= 0 { + merged.CriticalSizeGiB = defaults.CriticalSizeGiB + } + return merged +} + +// mergeBackupOverride is the backup counterpart of mergeSnapshotOverride. +// AlertOrphaned and IgnoreVMIDs are instance-wide filters, so the globals +// always win for those regardless of what a stale override carries. +func mergeBackupOverride(defaults BackupAlertConfig, override BackupAlertConfig) BackupAlertConfig { + merged := override + if merged.WarningDays <= 0 { + merged.WarningDays = defaults.WarningDays + } + if merged.CriticalDays <= 0 { + merged.CriticalDays = defaults.CriticalDays + } + if merged.FreshHours <= 0 { + merged.FreshHours = defaults.FreshHours + } + if merged.StaleHours <= 0 { + merged.StaleHours = defaults.StaleHours + } + merged.AlertOrphaned = defaults.AlertOrphaned + merged.IgnoreVMIDs = defaults.IgnoreVMIDs + return merged +} + func (m *Manager) resolvedSnapshotAlertConfigNoLock(thresholds ThresholdConfig) SnapshotAlertConfig { cfg := m.config.SnapshotDefaults if thresholds.Snapshot != nil { - cfg = *thresholds.Snapshot + cfg = mergeSnapshotOverride(cfg, *thresholds.Snapshot) } return cfg } @@ -49,7 +92,7 @@ func (m *Manager) resolvedSnapshotAlertConfigNoLock(thresholds ThresholdConfig) func (m *Manager) resolvedBackupAlertConfigNoLock(thresholds ThresholdConfig) BackupAlertConfig { cfg := m.config.BackupDefaults if thresholds.Backup != nil { - cfg = *thresholds.Backup + cfg = mergeBackupOverride(cfg, *thresholds.Backup) } if cfg.AlertOrphaned == nil { alertOrphaned := true @@ -355,7 +398,7 @@ func (m *Manager) CheckSnapshotsForInstance(instanceName string, snapshots []mod currentSnapshotCfg := snapshotCfg if gh.Snapshot != nil { - currentSnapshotCfg = *gh.Snapshot + currentSnapshotCfg = mergeSnapshotOverride(snapshotCfg, *gh.Snapshot) } if !currentSnapshotCfg.Enabled { @@ -799,7 +842,7 @@ func (m *Manager) CheckBackupsWithInventory( continue } if gh.Backup != nil { - currentBackupCfg = *gh.Backup + currentBackupCfg = mergeBackupOverride(backupCfg, *gh.Backup) } } diff --git a/internal/alerts/config/normalize.go b/internal/alerts/config/normalize.go index af158c35c..d66130ed6 100644 --- a/internal/alerts/config/normalize.go +++ b/internal/alerts/config/normalize.go @@ -211,6 +211,42 @@ func NormalizeSnapshotDefaults(config *AlertConfig) { } } +// NormalizeRecoveryOverrides rewrites per-guest backup/snapshot overrides whose +// threshold values are exact copies of the current global defaults into sparse +// overrides that carry only the enabled flag. The legacy per-guest toggle +// persisted a full copy of the globals just to flip enabled, freezing the +// threshold values so later global edits silently stopped applying to those +// guests (#1126). Zero-valued fields inherit the global default at evaluation +// time, so the rewrite is behavior-preserving at the moment it runs and lets +// the override track future global changes. Overrides whose thresholds differ +// from the current globals are deliberate per-guest values and are left alone. +func NormalizeRecoveryOverrides(config *AlertConfig) { + for id, override := range config.Overrides { + changed := false + if b := override.Backup; b != nil { + if b.WarningDays == config.BackupDefaults.WarningDays && + b.CriticalDays == config.BackupDefaults.CriticalDays && + b.FreshHours == config.BackupDefaults.FreshHours && + b.StaleHours == config.BackupDefaults.StaleHours { + override.Backup = &BackupAlertConfig{Enabled: b.Enabled} + changed = true + } + } + if s := override.Snapshot; s != nil { + if s.WarningDays == config.SnapshotDefaults.WarningDays && + s.CriticalDays == config.SnapshotDefaults.CriticalDays && + s.WarningSizeGiB == config.SnapshotDefaults.WarningSizeGiB && + s.CriticalSizeGiB == config.SnapshotDefaults.CriticalSizeGiB { + override.Snapshot = &SnapshotAlertConfig{Enabled: s.Enabled} + changed = true + } + } + if changed { + config.Overrides[id] = override + } + } +} + func NormalizeBackupDefaults(config *AlertConfig) { if config.BackupDefaults.WarningDays < 0 { config.BackupDefaults.WarningDays = 0 diff --git a/internal/alerts/config_facade.go b/internal/alerts/config_facade.go index 0fb0c0692..83960a0f4 100644 --- a/internal/alerts/config_facade.go +++ b/internal/alerts/config_facade.go @@ -97,4 +97,11 @@ func ensureValidHysteresis(threshold *HysteresisThreshold, metricName string) { func normalizeSnapshotDefaults(config *AlertConfig) { alertconfig.NormalizeSnapshotDefaults(config) } func normalizeBackupDefaults(config *AlertConfig) { alertconfig.NormalizeBackupDefaults(config) } +// NormalizeRecoveryOverrides converts per-guest backup/snapshot overrides that +// are exact threshold copies of the current global defaults into sparse +// enabled-only overrides so they track future global edits (#1126). +func NormalizeRecoveryOverrides(config *AlertConfig) { + alertconfig.NormalizeRecoveryOverrides(config) +} + func validateQuietHoursTimezone(config *AlertConfig) { alertconfig.ValidateQuietHoursTimezone(config) } diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go index b7dd8bce8..0fc01a0cd 100644 --- a/internal/alerts/config_runtime.go +++ b/internal/alerts/config_runtime.go @@ -31,6 +31,7 @@ func (m *Manager) UpdateConfig(config AlertConfig) { alertconfig.NormalizePBSDefaults(&config) alertconfig.NormalizeSnapshotDefaults(&config) alertconfig.NormalizeBackupDefaults(&config) + alertconfig.NormalizeRecoveryOverrides(&config) alertconfig.NormalizeNodeDefaults(&config) alertconfig.NormalizeAgentDefaults(&config) alertconfig.NormalizeKubernetesDefaults(&config) diff --git a/internal/config/persistence.go b/internal/config/persistence.go index a712a1e45..377757e13 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -912,6 +912,8 @@ func normalizeAlertDefaults(config *alerts.AlertConfig) { } config.DockerIgnoredContainerPrefixes = alerts.NormalizeDockerIgnoredPrefixes(config.DockerIgnoredContainerPrefixes) + + alerts.NormalizeRecoveryOverrides(config) } // SaveAlertConfig saves alert configuration to file