Keep last-known PMG backups when backup reads fail (#3028)

Since #2991 stopped guessing a PMG backup node from the connection name, a failed `/config/cluster/status` read leaves no node to ask for backups, and the poller still published that empty result. `UpdatePMGBackups` replaces the instance's whole list, so one transient cluster-status failure, or every node's backup read failing, wiped every known PMG backup 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 (every node after a failed cluster read, a node whose backup read failed, or one a cancelled poll never reached) keep their previous backups. This is the same per-scope carry the PVE storage backup poller applies to failed storages (`preserveFailedStorageBackups`). 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. #2991's rule against guessing node names is unchanged. Tests in `monitor_pmg_test.go`: the three retention tests fail on main and pass with this change. A complete enumeration that returns no backups still clears the list. The monitoring subsystem contract now records the carry (new section "PMG backups keep last-known entries for unread nodes"), and the #2960 note no longer says a failed cluster read publishes an empty backup list. Replaces #3005, rebuilt on current main with the contract note.
2026-10-26 21:47:20 +00:00 · 2026-10-17 10:10:03 +01:00
parent 5e91e03672 1169d36308
commit dff8ae12d5
3 changed files with 227 additions and 1 deletions
@@ -336,11 +336,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
@@ -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
@@ -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)
+	}
+}