From 1169d36308bd609a433bea66046335a0d9ed5cdb Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Sat, 17 Oct 2026 09:02:36 +0100 Subject: [PATCH] Keep last-known PMG backups when backup reads fail Since PMG stopped guessing a backup node from the connection name, a failed cluster-status read leaves no node to ask, and the poller still published the empty result. UpdatePMGBackups replaces the instance's list, so one transient cluster failure (or every node's backup read failing) wiped all known PMG backups and the Backups view flapped empty until the next good poll. Only a node whose backup list was read is now authoritative for that cycle. Nodes that were not read, including every node after a failed cluster read, keep their previous backups, the same per-scope carry the PVE storage poller applies to failed storages. A complete read that returns no backups, or a cluster listing that drops a node, still clears them. The failed read stays recorded as a poll failure. --- .../v6/internal/subsystems/monitoring.md | 20 ++- internal/monitoring/monitor_pbs_pmg.go | 62 ++++++++ internal/monitoring/monitor_pmg_test.go | 146 ++++++++++++++++++ 3 files changed, 227 insertions(+), 1 deletion(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 5cc1ad375a..3473fdc275 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -331,11 +331,29 @@ the connection name as the backup node only when PMG answered with no cluster entries; after a failed cluster read that name is a display label, and requesting backups with it produced a misleading hostname-lookup error without ever returning backups. The poll still records the cluster read -failure, and the published backup list is empty exactly as before. +failure. It requests no backups, and the connection's last-known backups +are kept rather than replaced by an empty list (see the next section). `TestClientClusterStatusIgnoresClusterIDFormat` and the `TestPollPMGInstance*` issue tests in `monitor_pmg_test.go` pin all three cases. +### PMG backups keep last-known entries for unread nodes + +`UpdatePMGBackups` replaces a connection's whole backup list, so only a node +whose backup list was read in this poll is authoritative for it. A node that +was not read keeps its previous backups: every node after a failed cluster +read, a node whose backup read failed, and a node a cancelled poll never +reached. This is the same per-scope carry that `preserveFailedStorageBackups` +applies to failed PVE storages. A node that answered with no backups, or that +a successful cluster read no longer lists, still clears its entries. The +failed read is still recorded as a poll failure, and the connection-name +fallback above is unchanged. `TestPMGBackupsSurviveFailedClusterRead`, +`TestPMGBackupsSurviveEveryNodeBackupReadFailing`, +`TestPMGBackupsKeepFailedNodeOnPartialRead` and +`TestPMGBackupsClearAfterCompleteEnumeration` in `monitor_pmg_test.go` cover +the carry and the complete-enumeration clear. These are synthetic poller +controls, not native PMG acceptance. + ### Guest configuration reads preserve installation and node identity Resolving omitted placement for the existing guest-config read honours every diff --git a/internal/monitoring/monitor_pbs_pmg.go b/internal/monitoring/monitor_pbs_pmg.go index cadb9bbf13..1508a9217e 100644 --- a/internal/monitoring/monitor_pbs_pmg.go +++ b/internal/monitoring/monitor_pbs_pmg.go @@ -5,6 +5,7 @@ import ( stderrors "errors" "fmt" "math" + "sort" "strings" "time" @@ -873,6 +874,7 @@ func (m *Monitor) pollPMGInstance(ctx context.Context, instanceName string, clie pmgBackups := make([]models.PMGBackup, 0) seenBackupIDs := make(map[string]struct{}) + readBackupNodes := make(map[string]struct{}, len(backupNodes)) for nodeName := range backupNodes { if ctx.Err() != nil { @@ -890,6 +892,7 @@ func (m *Monitor) pollPMGInstance(ctx context.Context, instanceName string, clie } continue } + readBackupNodes[nodeName] = struct{}{} for _, b := range backups { timestamp := b.Timestamp.Int64() @@ -910,6 +913,24 @@ func (m *Monitor) pollPMGInstance(ctx context.Context, instanceName string, clie } } + // Only a node whose backup list was read is authoritative this cycle. A + // failed cluster read, a failed per-node read or a cancelled poll observed + // nothing, so those nodes keep their last-known backups instead of being + // published as having none; the failed read is already in pollErr. + if clusterErr != nil || len(readBackupNodes) < len(backupNodes) { + var preservedBackupNodes []string + pmgBackups, preservedBackupNodes = preserveUnreadPMGNodeBackups( + instanceName, m.state.GetSnapshot().PMGBackups, pmgBackups, + clusterErr != nil, backupNodes, readBackupNodes) + if len(preservedBackupNodes) > 0 { + log.Warn(). + Str("instance", instanceName). + Strs("nodes", preservedBackupNodes). + Bool("clusterReadFailed", clusterErr != nil). + Msg("PMG backup list unavailable for some nodes; keeping their previous backups") + } + } + if debugEnabled { log.Debug(). Str("instance", instanceName). @@ -1065,4 +1086,45 @@ func (m *Monitor) pollPMGInstance(ctx context.Context, instanceName string, clie } } +// preserveUnreadPMGNodeBackups carries forward previous backups for nodes +// whose backup list was not read this cycle, mirroring +// preserveFailedStorageBackups for PVE storages. When the cluster read failed +// no node was enumerated, so every previous node is unread. When the cluster +// read succeeded, a node it no longer lists is gone and its backups drop. +func preserveUnreadPMGNodeBackups(instanceName string, previous, current []models.PMGBackup, clusterUnread bool, enumeratedNodes, readNodes map[string]struct{}) ([]models.PMGBackup, []string) { + existing := make(map[string]struct{}, len(current)) + for _, backup := range current { + existing[backup.ID] = struct{}{} + } + + preserved := make(map[string]struct{}) + for _, backup := range previous { + if backup.Instance != instanceName { + continue + } + if _, read := readNodes[backup.Node]; read { + continue + } + if _, enumerated := enumeratedNodes[backup.Node]; !enumerated && !clusterUnread { + continue + } + if _, duplicate := existing[backup.ID]; duplicate { + continue + } + current = append(current, backup) + existing[backup.ID] = struct{}{} + preserved[backup.Node] = struct{}{} + } + + if len(preserved) == 0 { + return current, nil + } + nodes := make([]string, 0, len(preserved)) + for node := range preserved { + nodes = append(nodes, node) + } + sort.Strings(nodes) + return current, nodes +} + // GetState returns the current state diff --git a/internal/monitoring/monitor_pmg_test.go b/internal/monitoring/monitor_pmg_test.go index c7680d5240..46a9944207 100644 --- a/internal/monitoring/monitor_pmg_test.go +++ b/internal/monitoring/monitor_pmg_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "reflect" + "sort" "strings" "testing" "time" @@ -634,3 +635,148 @@ func TestPollPMGInstanceKeepsNameFallbackForEmptyCluster(t *testing.T) { t.Fatalf("expected the connection-name fallback for an empty cluster, got %v", backupPaths) } } + +// pmgBackupRetentionFixture drives the real PMG poll consumer with a cluster +// listing and per-node backup answers chosen by each test. +type pmgBackupRetentionFixture struct { + *pmgScopeFixture + clusterErr error + nodes []string + backupErr map[string]error + backups map[string][]pmg.BackupEntry +} + +func newPMGBackupRetentionFixture() *pmgBackupRetentionFixture { + return &pmgBackupRetentionFixture{ + pmgScopeFixture: &pmgScopeFixture{calls: map[string]int{}}, + nodes: []string{"one", "two"}, + backupErr: map[string]error{}, + backups: map[string][]pmg.BackupEntry{ + "one": {{Filename: "one.tgz", Timestamp: 1700000000, Size: 10}}, + "two": {{Filename: "two.tgz", Timestamp: 1700000100, Size: 20}}, + }, + } +} + +func (f *pmgBackupRetentionFixture) GetClusterStatus(context.Context, bool) ([]pmg.ClusterStatusEntry, error) { + f.calls["cluster"]++ + if f.clusterErr != nil { + return nil, f.clusterErr + } + entries := make([]pmg.ClusterStatusEntry, 0, len(f.nodes)) + for _, node := range f.nodes { + entries = append(entries, pmg.ClusterStatusEntry{Name: node, Type: "node"}) + } + return entries, nil +} + +func (f *pmgBackupRetentionFixture) ListBackups(_ context.Context, node string) ([]pmg.BackupEntry, error) { + f.calls["backup/"+node]++ + if err := f.backupErr[node]; err != nil { + return nil, err + } + return f.backups[node], nil +} + +func pmgBackupFilesByNode(t *testing.T, m *Monitor) map[string][]string { + t.Helper() + out := map[string][]string{} + for _, backup := range m.state.GetSnapshot().PMGBackups { + if backup.Instance != "gateway" { + t.Fatalf("unexpected instance on PMG backup: %+v", backup) + } + out[backup.Node] = append(out[backup.Node], backup.Filename) + } + for node := range out { + sort.Strings(out[node]) + } + return out +} + +func pollPMGBackupRetention(t *testing.T, m *Monitor, f *pmgBackupRetentionFixture) { + t.Helper() + m.pollPMGInstance(context.Background(), "gateway", f) +} + +func assertPMGPollFailed(t *testing.T, m *Monitor) { + t.Helper() + status := m.pollStatusMap["pmg::gateway"] + if status == nil || status.ConsecutiveFailures == 0 || status.LastErrorMessage == "" { + t.Fatalf("failed backup enumeration not recorded as a poll failure: %+v", status) + } +} + +// A failed cluster read cannot enumerate backups, so it must not publish an +// empty list over the backups the last good poll found. +func TestPMGBackupsSurviveFailedClusterRead(t *testing.T) { + m := pmgScopeMonitor(config.PMGInstance{MonitoringConfigured: true}) + f := newPMGBackupRetentionFixture() + pollPMGBackupRetention(t, m, f) + want := map[string][]string{"one": {"one.tgz"}, "two": {"two.tgz"}} + if got := pmgBackupFilesByNode(t, m); !reflect.DeepEqual(got, want) { + t.Fatalf("first poll backups=%v want=%v", got, want) + } + + f.clusterErr = errors.New("cluster status unavailable") + f.calls = map[string]int{} + pollPMGBackupRetention(t, m, f) + if f.calls["backup/one"] != 0 || f.calls["backup/two"] != 0 { + t.Fatalf("backups requested without a node list: %v", f.calls) + } + if got := pmgBackupFilesByNode(t, m); !reflect.DeepEqual(got, want) { + t.Fatalf("failed cluster read replaced backups: got=%v want=%v", got, want) + } + assertPMGPollFailed(t, m) +} + +// When every per-node backup read fails, nothing was observed this poll and +// the last-known list stays. +func TestPMGBackupsSurviveEveryNodeBackupReadFailing(t *testing.T) { + m := pmgScopeMonitor(config.PMGInstance{MonitoringConfigured: true}) + f := newPMGBackupRetentionFixture() + pollPMGBackupRetention(t, m, f) + + f.backupErr["one"] = errors.New("backup list timed out") + f.backupErr["two"] = errors.New("backup list timed out") + pollPMGBackupRetention(t, m, f) + want := map[string][]string{"one": {"one.tgz"}, "two": {"two.tgz"}} + if got := pmgBackupFilesByNode(t, m); !reflect.DeepEqual(got, want) { + t.Fatalf("failed backup reads replaced backups: got=%v want=%v", got, want) + } + assertPMGPollFailed(t, m) +} + +// A failed read on one node keeps that node's last-known backups while the +// nodes that answered publish what they returned. +func TestPMGBackupsKeepFailedNodeOnPartialRead(t *testing.T) { + m := pmgScopeMonitor(config.PMGInstance{MonitoringConfigured: true}) + f := newPMGBackupRetentionFixture() + pollPMGBackupRetention(t, m, f) + + f.backupErr["one"] = errors.New("backup list timed out") + f.backups["two"] = []pmg.BackupEntry{{Filename: "two-new.tgz", Timestamp: 1700000200, Size: 30}} + pollPMGBackupRetention(t, m, f) + want := map[string][]string{"one": {"one.tgz"}, "two": {"two-new.tgz"}} + if got := pmgBackupFilesByNode(t, m); !reflect.DeepEqual(got, want) { + t.Fatalf("partial read backups=%v want=%v", got, want) + } + assertPMGPollFailed(t, m) +} + +// A complete enumeration is authoritative: nodes that answered with no +// backups, and nodes no longer in the cluster, clear their entries. +func TestPMGBackupsClearAfterCompleteEnumeration(t *testing.T) { + m := pmgScopeMonitor(config.PMGInstance{MonitoringConfigured: true}) + f := newPMGBackupRetentionFixture() + pollPMGBackupRetention(t, m, f) + + f.nodes = []string{"one"} + f.backups["one"] = nil + pollPMGBackupRetention(t, m, f) + if got := pmgBackupFilesByNode(t, m); len(got) != 0 { + t.Fatalf("complete enumeration with no backups left entries: %v", got) + } + if status := m.pollStatusMap["pmg::gateway"]; status == nil || status.ConsecutiveFailures != 0 { + t.Fatalf("complete enumeration recorded as failed: %+v", status) + } +}