From dc1ddaa24ea2e0bd28bd5072fd36d5a34bc7fe93 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 26 Aug 2026 04:33:47 +0100 Subject: [PATCH] Fix stale backup alert attribution Change-source: pulse-maintainer --- .../v6/internal/subsystems/alerts.md | 13 ++++ internal/alerts/alerts_test.go | 73 +++++++++++++++++++ internal/alerts/backup_snapshot.go | 55 ++++++++++++-- 3 files changed, 133 insertions(+), 8 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index b31459c9a..79f37ecde 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -894,6 +894,19 @@ 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. + +### Backup-age attribution follows live identity + +Backup rollup subject refs are historical hints, not unconditional proof of a +guest's current placement. An exact `instance:node:vmid` ref is authoritative +only while it resolves to a live guest of the same type. Otherwise backup-age +evaluation retries typed VMID attribution and prefers live candidates carrying +a resource ID over last-known display metadata. A PVE-owned ref can still name +its authoritative orphan after the PVE inventory is ready; a stale PBS ref that +cannot be resolved uniquely remains unattributed instead of pinning an alert to +an old or wrong PVE node. `TestCheckBackupsRemapsStaleSubjectRefToUniqueLiveGuest` +and the PVE orphan/inventory-readiness tests in `internal/alerts/alerts_test.go` +pin both sides of this boundary. 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 diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index fe2c1925e..7dc94d012 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -1764,6 +1764,79 @@ func TestCheckBackupsDisambiguatesWithNamespace(t *testing.T) { } } +func TestCheckBackupsRemapsStaleSubjectRefToUniqueLiveGuest(t *testing.T) { + m := newTestManager(t) + m.ClearActiveAlerts() + + m.mu.Lock() + m.config.Enabled = true + m.config.BackupDefaults = BackupAlertConfig{ + Enabled: true, + WarningDays: 3, + CriticalDays: 5, + } + m.mu.Unlock() + + now := time.Now() + current := GuestLookup{ + ResourceID: "pve-current:node-a:100", + Name: "web-current", + Instance: "pve-current", + Node: "node-a", + Type: "qemu", + VMID: 100, + } + staleMetadata := GuestLookup{ + Name: "web-old", + Instance: "pve-retired", + Node: "node-b", + Type: "qemu", + VMID: 100, + } + + rollups := []recovery.ProtectionRollup{ + { + RollupID: "res:vm-web", + SubjectRef: &recovery.ExternalRef{ + Type: "proxmox-vm", + Namespace: "pve-retired", + Name: "web-old", + ID: "pve-retired:node-b:100", + Class: "node-b", + }, + LastSuccessAt: ptrTime(now.Add(-6 * 24 * time.Hour)), + LastOutcome: recovery.OutcomeSuccess, + Providers: []recovery.Provider{recovery.ProviderProxmoxPBS}, + }, + } + guestsByKey := map[string]GuestLookup{ + BuildGuestKey(current.Instance, current.Node, current.VMID): current, + } + guestsByVMID := map[string][]GuestLookup{ + "100": {current, staleMetadata}, + } + + m.CheckBackups(rollups, guestsByKey, guestsByVMID) + + m.mu.RLock() + defer m.mu.RUnlock() + expectedKey := "backup-age-" + sanitizeAlertKey(BuildGuestKey(current.Instance, current.Node, current.VMID)) + alert, exists := testLookupActiveAlert(t, m, expectedKey) + if !exists { + var keys []string + for storageKey, active := range m.activeAlerts { + keys = append(keys, effectiveAlertID(active, storageKey)) + } + t.Fatalf("expected alert remapped to current guest %q, found %v", expectedKey, keys) + } + if alert.Instance != current.Instance || alert.Node != current.Node { + t.Fatalf("alert location = %q/%q, want current guest %q/%q", alert.Instance, alert.Node, current.Instance, current.Node) + } + if alert.ResourceName != current.Name+" backup" { + t.Fatalf("alert resource name = %q, want %q", alert.ResourceName, current.Name+" backup") + } +} + // TestCheckBackupsVMIDCollisionNonMatchingNamespace verifies that when multiple guests // share a VMID and the PBS backup namespace matches none of them, the alert uses the // generic PBS key rather than falsely attributing to a specific guest. diff --git a/internal/alerts/backup_snapshot.go b/internal/alerts/backup_snapshot.go index fea347c27..3fcd264df 100644 --- a/internal/alerts/backup_snapshot.go +++ b/internal/alerts/backup_snapshot.go @@ -257,6 +257,23 @@ func filterGuestsByBackupType(guests []GuestLookup, backupType string) []GuestLo return filtered } +// preferLiveGuestCandidates prevents last-known display metadata from +// competing with current inventory during backup attribution. Persisted +// entries intentionally have no ResourceID; they remain useful when a backup +// is genuinely orphaned, but a live typed guest is stronger identity evidence. +func preferLiveGuestCandidates(guests []GuestLookup) []GuestLookup { + live := make([]GuestLookup, 0, len(guests)) + for _, guest := range guests { + if strings.TrimSpace(guest.ResourceID) != "" { + live = append(live, guest) + } + } + if len(live) > 0 { + return live + } + return guests +} + func backupOrphanInventoryReady(scope *BackupInventoryScope, record backupRecord) bool { if scope == nil || scope.PVEOrphanInventoryReady == nil { return true @@ -691,6 +708,8 @@ func (m *Manager) CheckBackupsWithInventory( node string vmID string subjectType string + refInstance string + refNode string ) ref := rollup.SubjectRef @@ -701,22 +720,28 @@ func (m *Manager) CheckBackupsWithInventory( // Primary: subjectRef.ID is the canonical proxmox guest source ID (instance:node:vmid) when linked. if ref != nil && strings.TrimSpace(ref.ID) != "" { if inst, nd, vmid, ok := parseGuestID(ref.ID); ok { - key = BuildGuestKey(inst, nd, vmid) - info = guestsByKey[key] + refInstance = inst + refNode = nd + candidateKey := BuildGuestKey(inst, nd, vmid) + candidate, live := guestsByKey[candidateKey] // A vm and a container can share instance:node:vmid; never // attribute the backup to a guest of the wrong kind. - if !guestMatchesBackupType(info, subjectType) { - info = GuestLookup{} + if live && guestMatchesBackupType(candidate, subjectType) { + key = candidateKey + info = candidate + instance = inst + node = nd } - instance = inst - node = nd vmID = strconv.Itoa(vmid) } } // Secondary: attempt to map by VMID for orphaned/ambiguous backups. if key == "" && ref != nil { - vmidStr := strings.TrimSpace(ref.ID) + vmidStr := strings.TrimSpace(vmID) + if vmidStr == "" { + vmidStr = strings.TrimSpace(ref.ID) + } if vmidStr == "" { vmidStr = strings.TrimSpace(ref.Name) } @@ -725,7 +750,7 @@ func (m *Manager) CheckBackupsWithInventory( vmID = vmidStr // Drop same-VMID guests of the wrong kind so a vm backup is // not matched to an lxc container (or vice-versa). - guests := filterGuestsByBackupType(guestsByVMID[vmidStr], subjectType) + guests := preferLiveGuestCandidates(filterGuestsByBackupType(guestsByVMID[vmidStr], subjectType)) if len(guests) == 1 { info = guests[0] } else if len(guests) > 1 && strings.TrimSpace(ref.Namespace) != "" { @@ -767,6 +792,20 @@ func (m *Manager) CheckBackupsWithInventory( } } + // A PVE rollup's source ref belongs to the authoritative PVE + // enumeration, so retain its location for a genuine orphan after the + // current-inventory fallback fails. PBS historical links are not given + // that authority: they can outlive or predate the guest's current PVE + // placement and must remain unattributed when the live candidates are + // ambiguous. + if key == "" && source == "PVE" && refInstance != "" && refNode != "" && vmID != "" { + if vmid, err := strconv.Atoi(vmID); err == nil && vmid > 0 { + key = BuildGuestKey(refInstance, refNode, vmid) + instance = refInstance + node = refNode + } + } + if key == "" { // Stable fallback for non-guest subjects and orphans. key = strings.TrimSpace(rollup.RollupID)