From ac90859e428679d43901a5486fb99135a62c7fc1 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Thu, 20 Nov 2025 19:58:05 +0000 Subject: [PATCH] Related to #727: normalize persisted Proxmox hosts --- internal/config/persistence.go | 131 ++++++++++++---------------- internal/config/persistence_test.go | 57 ++++++++++++ pkg/proxmox/cluster_client.go | 5 +- 3 files changed, 117 insertions(+), 76 deletions(-) diff --git a/internal/config/persistence.go b/internal/config/persistence.go index 2715079..f00c221 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -958,7 +958,12 @@ func (c *ConfigPersistence) saveNodesConfig(pveInstances []PVEInstance, pbsInsta // LoadNodesConfig loads nodes configuration from file (decrypts if encrypted) func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { c.mu.RLock() - defer c.mu.RUnlock() + unlocked := false + defer func() { + if !unlocked { + c.mu.RUnlock() + } + }() data, err := os.ReadFile(c.nodesFile) if err != nil { @@ -1075,6 +1080,41 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { // Track if any migrations were applied migrationApplied := false + normalizeHostWithDefault := func(host, defaultPort string) (string, bool) { + normalized := normalizeHostPort(host, defaultPort) + if normalized == "" || normalized == host { + return host, false + } + return normalized, true + } + + // Normalize PVE hosts (and cluster endpoints) to restore default ports for configs + // saved during the 4.32.0 regression that skipped default port injection. + for i := range config.PVEInstances { + if normalized, changed := normalizeHostWithDefault(config.PVEInstances[i].Host, defaultPVEPort); changed { + log.Info(). + Str("instance", config.PVEInstances[i].Name). + Str("oldHost", config.PVEInstances[i].Host). + Str("newHost", normalized). + Msg("Normalized PVE host to include default port") + config.PVEInstances[i].Host = normalized + migrationApplied = true + } + + for j := range config.PVEInstances[i].ClusterEndpoints { + if normalized, changed := normalizeHostWithDefault(config.PVEInstances[i].ClusterEndpoints[j].Host, defaultPVEPort); changed { + log.Info(). + Str("instance", config.PVEInstances[i].Name). + Str("node", config.PVEInstances[i].ClusterEndpoints[j].NodeName). + Str("oldHost", config.PVEInstances[i].ClusterEndpoints[j].Host). + Str("newHost", normalized). + Msg("Normalized PVE cluster endpoint host to include default port") + config.PVEInstances[i].ClusterEndpoints[j].Host = normalized + migrationApplied = true + } + } + } + // Fix for bug where TokenName was incorrectly set when using password auth // If a PBS instance has both Password and TokenName, clear the TokenName for i := range config.PBSInstances { @@ -1084,49 +1124,17 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { Msg("Fixing PBS config: clearing TokenName since Password is set") config.PBSInstances[i].TokenName = "" config.PBSInstances[i].TokenValue = "" + migrationApplied = true } - // Fix for missing port in PBS host - host := config.PBSInstances[i].Host - if host != "" { - // Check if we need to add default port - protocolEnd := 0 - if strings.HasPrefix(host, "https://") { - protocolEnd = 8 - } else if strings.HasPrefix(host, "http://") { - protocolEnd = 7 - } else if !strings.Contains(host, "://") { - // No protocol specified, add https and check for port - if !strings.Contains(host, ":") { - // No port specified, add protocol and default port - config.PBSInstances[i].Host = "https://" + host + ":8007" - log.Info(). - Str("instance", config.PBSInstances[i].Name). - Str("oldHost", host). - Str("newHost", config.PBSInstances[i].Host). - Msg("Fixed PBS host by adding protocol and default port") - } else { - // Port specified, just add protocol - config.PBSInstances[i].Host = "https://" + host - log.Info(). - Str("instance", config.PBSInstances[i].Name). - Str("oldHost", host). - Str("newHost", config.PBSInstances[i].Host). - Msg("Fixed PBS host by adding protocol") - } - } else if protocolEnd > 0 { - // Has protocol, check if port is missing - hostAfterProtocol := host[protocolEnd:] - if !strings.Contains(hostAfterProtocol, ":") { - // No port specified, add default PBS port - config.PBSInstances[i].Host = host + ":8007" - log.Info(). - Str("instance", config.PBSInstances[i].Name). - Str("oldHost", host). - Str("newHost", config.PBSInstances[i].Host). - Msg("Fixed PBS host by adding default port 8007") - } - } + if normalized, changed := normalizeHostWithDefault(config.PBSInstances[i].Host, defaultPBSPort); changed { + log.Info(). + Str("instance", config.PBSInstances[i].Name). + Str("oldHost", config.PBSInstances[i].Host). + Str("newHost", normalized). + Msg("Normalized PBS host to include default port") + config.PBSInstances[i].Host = normalized + migrationApplied = true } // Migration: Ensure MonitorBackups is enabled for PBS instances @@ -1150,42 +1158,14 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { migrationApplied = true } - host := config.PMGInstances[i].Host - if host == "" { - continue - } - - protocolEnd := 0 - if strings.HasPrefix(host, "https://") { - protocolEnd = 8 - } else if strings.HasPrefix(host, "http://") { - protocolEnd = 7 - } else if !strings.Contains(host, "://") { - if !strings.Contains(host, ":") { - config.PMGInstances[i].Host = "https://" + host + ":8006" - } else { - config.PMGInstances[i].Host = "https://" + host - } + if normalized, changed := normalizeHostWithDefault(config.PMGInstances[i].Host, defaultPVEPort); changed { log.Info(). Str("instance", config.PMGInstances[i].Name). - Str("oldHost", host). - Str("newHost", config.PMGInstances[i].Host). - Msg("Fixed PMG host by adding protocol/port") + Str("oldHost", config.PMGInstances[i].Host). + Str("newHost", normalized). + Msg("Normalized PMG host to include default port") + config.PMGInstances[i].Host = normalized migrationApplied = true - continue - } - - if protocolEnd > 0 { - hostAfterProtocol := host[protocolEnd:] - if !strings.Contains(hostAfterProtocol, ":") { - config.PMGInstances[i].Host = host + ":8006" - log.Info(). - Str("instance", config.PMGInstances[i].Name). - Str("oldHost", host). - Str("newHost", config.PMGInstances[i].Host). - Msg("Fixed PMG host by adding default port 8006") - migrationApplied = true - } } } @@ -1209,6 +1189,7 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { copy(pmgCopy, config.PMGInstances) // Release read lock before saving (SaveNodesConfig acquires write lock) + unlocked = true c.mu.RUnlock() log.Info().Msg("Migrations applied, saving updated configuration") diff --git a/internal/config/persistence_test.go b/internal/config/persistence_test.go index 18ba54c..cb49de3 100644 --- a/internal/config/persistence_test.go +++ b/internal/config/persistence_test.go @@ -808,6 +808,63 @@ func TestImportAcceptsVersion40Bundle(t *testing.T) { assertJSONEqual(t, tokensAfter, baselineTokens, "api tokens unchanged for 4.0 import") } +func TestLoadNodesConfigNormalizesPVEHostsAndClusterEndpoints(t *testing.T) { + tempDir := t.TempDir() + cp := config.NewConfigPersistence(tempDir) + if err := cp.EnsureConfigDir(); err != nil { + t.Fatalf("EnsureConfigDir: %v", err) + } + + pveNodes := []config.PVEInstance{ + { + Name: "pve-cluster", + Host: "https://pve.local", + ClusterEndpoints: []config.ClusterEndpoint{ + {NodeName: "pve1", Host: "https://pve1.local"}, + {NodeName: "pve2", Host: "pve2.local"}, + }, + }, + } + + if err := cp.SaveNodesConfig(pveNodes, nil, nil); err != nil { + t.Fatalf("SaveNodesConfig: %v", err) + } + + loaded, err := cp.LoadNodesConfig() + if err != nil { + t.Fatalf("LoadNodesConfig: %v", err) + } + + if len(loaded.PVEInstances) != 1 { + t.Fatalf("expected 1 PVE instance, got %d", len(loaded.PVEInstances)) + } + + pve := loaded.PVEInstances[0] + if pve.Host != "https://pve.local:8006" { + t.Fatalf("expected primary host normalized with default port, got %q", pve.Host) + } + + if len(pve.ClusterEndpoints) != 2 { + t.Fatalf("expected 2 cluster endpoints, got %d", len(pve.ClusterEndpoints)) + } + + if pve.ClusterEndpoints[0].Host != "https://pve1.local:8006" { + t.Fatalf("expected endpoint host normalized, got %q", pve.ClusterEndpoints[0].Host) + } + if pve.ClusterEndpoints[1].Host != "https://pve2.local:8006" { + t.Fatalf("expected endpoint host normalized, got %q", pve.ClusterEndpoints[1].Host) + } + + // Second load should keep normalized values and not panic on migration. + loadedAgain, err := cp.LoadNodesConfig() + if err != nil { + t.Fatalf("LoadNodesConfig second read: %v", err) + } + if loadedAgain.PVEInstances[0].Host != "https://pve.local:8006" { + t.Fatalf("expected normalized host persisted, got %q", loadedAgain.PVEInstances[0].Host) + } +} + func mustDecodeExport(t *testing.T, payload, passphrase string) config.ExportData { t.Helper() diff --git a/pkg/proxmox/cluster_client.go b/pkg/proxmox/cluster_client.go index dc91072..aecc7a1 100644 --- a/pkg/proxmox/cluster_client.go +++ b/pkg/proxmox/cluster_client.go @@ -581,6 +581,7 @@ func (cc *ClusterClient) executeWithFailover(ctx context.Context, fn func(*Clien // Error 500 with any "guest agent" message means VM-specific issue, not node failure // Error 400 with "ds" parameter error means Proxmox 9.x doesn't support RRD data source filtering // JSON unmarshal errors are data format issues, not connectivity problems + // Context deadline/timeout errors on storage endpoints mean storage issues, not node unreachability if strings.Contains(errStr, "595") || (strings.Contains(errStr, "500") && strings.Contains(errStr, "hostname lookup")) || (strings.Contains(errStr, "500") && strings.Contains(errStr, "Name or service not known")) || @@ -593,7 +594,9 @@ func (cc *ClusterClient) executeWithFailover(ctx context.Context, fn func(*Clien (strings.Contains(errStr, "400") && strings.Contains(errStr, "\"ds\"") && strings.Contains(errStr, "property is not defined in schema")) || strings.Contains(errStr, "json: cannot unmarshal") || (strings.Contains(errStr, "storage '") && strings.Contains(errStr, "is not available on node")) || - strings.Contains(errStr, "unexpected response format") { + strings.Contains(errStr, "unexpected response format") || + (strings.Contains(errStr, "context deadline exceeded") && strings.Contains(errStr, "/storage/")) || + (strings.Contains(errStr, "Client.Timeout exceeded") && strings.Contains(errStr, "/storage/")) { // This is likely a node-specific failure, not an endpoint failure // Return the error but don't mark the endpoint as unhealthy log.Debug().