From 36c63bbf8beb74dc38890526b71bc40df175ec78 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:15:58 +0100 Subject: [PATCH] Accept PMG 9 cluster ids and stop guessing backup nodes Proxmox Mail Gateway 9 returns the cluster status cid as a string, so decoding it into an int failed the whole cluster read and the poller lost the node list (issue 2960). Pulse never reads cid, so the field is no longer decoded and any format works. With no nodes, the poller then requested backups from a node named after the connection's display name ("PMG-01"), which PMG rejected with a hostname-lookup error that pointed users at DNS. That fallback now runs only when PMG answered with no cluster entries; after a failed cluster read the request could never return backups, and the poll already records the cluster failure. --- .../v6/internal/subsystems/monitoring.md | 14 +++ internal/monitoring/monitor_pbs_pmg.go | 14 ++- internal/monitoring/monitor_pmg_test.go | 98 +++++++++++++++++++ pkg/pmg/client.go | 3 +- pkg/pmg/client_test.go | 33 +++++++ 5 files changed, 156 insertions(+), 6 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 01b410ac4e..a0d0c16dcf 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -322,6 +322,20 @@ successful zero objects remain observations. `TestPMGCollectionMissingObjectData and `TestPMGCollectionObservedZeroData` in `pkg/pmg/client_test.go` exercise the real request/decoder boundary using an in-memory HTTP transport, with no listener. +### PMG 9 cluster ids and the backup node fallback — issue #2960 + +PMG 9 returns the cluster status `cid` as a string. `pmg.ClusterStatusEntry` +no longer decodes the field, which Pulse never read, so no `cid` format can +fail the cluster read and hide the node list. The PMG poller falls back to +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. +`TestClientClusterStatusIgnoresClusterIDFormat` and the +`TestPollPMGInstance*` issue tests in `monitor_pmg_test.go` pin all three +cases. + ### 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 be87ec3fb5..cadb9bbf13 100644 --- a/internal/monitoring/monitor_pbs_pmg.go +++ b/internal/monitoring/monitor_pbs_pmg.go @@ -806,11 +806,11 @@ func (m *Monitor) pollPMGInstance(ctx context.Context, instanceName string, clie } } - cluster, err := client.GetClusterStatus(ctx, true) - recordReadError("pmg_cluster_status", err) - if err != nil { + cluster, clusterErr := client.GetClusterStatus(ctx, true) + recordReadError("pmg_cluster_status", clusterErr) + if clusterErr != nil { if debugEnabled { - log.Debug().Err(err).Str("instance", instanceName).Msg("failed to retrieve PMG cluster status") + log.Debug().Err(clusterErr).Str("instance", instanceName).Msg("failed to retrieve PMG cluster status") } } @@ -860,7 +860,11 @@ func (m *Monitor) pollPMGInstance(ctx context.Context, instanceName string, clie pmgInst.Nodes = nodes } - if len(backupNodes) == 0 { + // Fall back to the connection name only when PMG answered with no cluster + // entries. After a failed cluster read the name is a display label, not a + // node: requesting backups with it fails with a misleading hostname-lookup + // error on PMG (#2960) and cannot return backups. + if len(backupNodes) == 0 && clusterErr == nil { trimmed := strings.TrimSpace(instanceName) if trimmed != "" { backupNodes[trimmed] = struct{}{} diff --git a/internal/monitoring/monitor_pmg_test.go b/internal/monitoring/monitor_pmg_test.go index 6be3796c0a..c7680d5240 100644 --- a/internal/monitoring/monitor_pmg_test.go +++ b/internal/monitoring/monitor_pmg_test.go @@ -536,3 +536,101 @@ func TestPMGCollectionCancelledAndUnknown(t *testing.T) { t.Fatalf("cancelled/unknown requests=%v", f.calls) } } + +// pollPMGIssue2960 polls one PMG instance named like the reporter's +// connection ("PMG-01") against a server whose cluster status is served by +// clusterStatus, and records every backup path requested. +func pollPMGIssue2960(t *testing.T, clusterStatus func(w http.ResponseWriter)) (*Monitor, []string) { + t.Helper() + var backupPaths []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case r.URL.Path == "/api2/json/version": + fmt.Fprint(w, `{"data":{"version":"9.1.4","release":"1"}}`) + case r.URL.Path == "/api2/json/config/cluster/status": + clusterStatus(w) + case strings.HasSuffix(r.URL.Path, "/backup"): + backupPaths = append(backupPaths, r.URL.Path) + if r.URL.Path != "/api2/json/nodes/pmg-01/backup" { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"message":"hostname lookup failed","data":null}`) + return + } + timestamp := time.Now().Add(-24 * time.Hour).Unix() + fmt.Fprintf(w, `{"data":[{"filename":"pmg-backup.tgz","size":2048,"timestamp":%d}]}`, timestamp) + default: + t.Fatalf("unexpected path: %s", r.URL.Path) + } + })) + t.Cleanup(server.Close) + + client, err := pmg.NewClient(pmg.ClientConfig{ + Host: server.URL, + TokenName: "pulse@pmg!monitor", + TokenValue: "secret", + VerifySSL: false, + }) + if err != nil { + t.Fatalf("unexpected client error: %v", err) + } + mon, err := New(&config.Config{PMGInstances: []config.PMGInstance{{ + Name: "PMG-01", + Host: server.URL, + TokenName: "pulse@pmg!monitor", + TokenValue: "secret", + // Explicit scope with every collector off: only cluster status and + // backups are read. + MonitoringConfigured: true, + }}}) + if err != nil { + t.Fatalf("failed to create monitor: %v", err) + } + mon.pollPMGInstance(context.Background(), "PMG-01", client) + return mon, backupPaths +} + +// Issue #2960: PMG 9 sends the cluster id as a string. The cluster read must +// still succeed, so backups come from the node PMG names, not the connection. +func TestPollPMGInstanceAcceptsStringClusterID(t *testing.T) { + mon, backupPaths := pollPMGIssue2960(t, func(w http.ResponseWriter) { + fmt.Fprint(w, `{"data":[{"cid":"1","name":"pmg-01","type":"master","ip":"10.0.0.5"}]}`) + }) + + snapshot := mon.state.GetSnapshot() + if len(snapshot.PMGInstances) != 1 || len(snapshot.PMGInstances[0].Nodes) != 1 || + snapshot.PMGInstances[0].Nodes[0].Name != "pmg-01" { + t.Fatalf("expected the PMG node from cluster status, got %+v", snapshot.PMGInstances) + } + if !reflect.DeepEqual(backupPaths, []string{"/api2/json/nodes/pmg-01/backup"}) { + t.Fatalf("expected backups from node pmg-01 only, got %v", backupPaths) + } + if len(snapshot.PMGBackups) != 1 || snapshot.PMGBackups[0].Node != "pmg-01" { + t.Fatalf("expected one backup on pmg-01, got %+v", snapshot.PMGBackups) + } +} + +// Issue #2960: after a failed cluster read the connection's display name is +// not a node, so Pulse must not request backups with it. +func TestPollPMGInstanceDoesNotGuessNodeAfterFailedClusterRead(t *testing.T) { + _, backupPaths := pollPMGIssue2960(t, func(w http.ResponseWriter) { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"message":"cluster status unavailable","data":null}`) + }) + + if len(backupPaths) != 0 { + t.Fatalf("expected no backup request after a failed cluster read, got %v", backupPaths) + } +} + +// A PMG that answers with no cluster entries keeps the connection-name +// fallback some standalone installs rely on. +func TestPollPMGInstanceKeepsNameFallbackForEmptyCluster(t *testing.T) { + _, backupPaths := pollPMGIssue2960(t, func(w http.ResponseWriter) { + fmt.Fprint(w, `{"data":[]}`) + }) + + if !reflect.DeepEqual(backupPaths, []string{"/api2/json/nodes/PMG-01/backup"}) { + t.Fatalf("expected the connection-name fallback for an empty cluster, got %v", backupPaths) + } +} diff --git a/pkg/pmg/client.go b/pkg/pmg/client.go index 3df1edd827..d2d5f4d7b8 100644 --- a/pkg/pmg/client.go +++ b/pkg/pmg/client.go @@ -127,8 +127,9 @@ type SpamScore struct { Ratio flexibleFloat `json:"ratio"` } +// ClusterStatusEntry omits PMG's cluster id (cid): Pulse never reads it, and +// PMG 9 sends it as a string, which failed the whole cluster read (#2960). type ClusterStatusEntry struct { - CID int `json:"cid"` Name string `json:"name"` Type string `json:"type"` IP string `json:"ip"` diff --git a/pkg/pmg/client_test.go b/pkg/pmg/client_test.go index 391cb18e00..778ff30c3d 100644 --- a/pkg/pmg/client_test.go +++ b/pkg/pmg/client_test.go @@ -677,6 +677,39 @@ func TestClientClusterStatusListSingle(t *testing.T) { } } +// PMG 9 returns the cluster id as a string (#2960). Pulse does not use it, so +// no cid format may fail the cluster read. +func TestClientClusterStatusIgnoresClusterIDFormat(t *testing.T) { + t.Parallel() + + for _, cid := range []string{`1`, `"1"`, `"node-a"`, `null`} { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"data":[{"cid":%s,"name":"pmg-01","type":"master","ip":"10.0.0.5"}]}`, cid) + })) + + client, err := NewClient(ClientConfig{ + Host: server.URL, + TokenName: "apitoken", + TokenValue: "secret", + VerifySSL: false, + }) + if err != nil { + server.Close() + t.Fatalf("unexpected error creating client: %v", err) + } + + entries, err := client.GetClusterStatus(context.Background(), true) + server.Close() + if err != nil { + t.Fatalf("cid %s: GetClusterStatus failed: %v", cid, err) + } + if len(entries) != 1 || entries[0].Name != "pmg-01" || entries[0].Type != "master" { + t.Fatalf("cid %s: unexpected entries %+v", cid, entries) + } + } +} + func TestClientListBackupsEscapesNode(t *testing.T) { t.Parallel()