From b8603b76b615ce5aff0267a19a7b4aa92ce5b8c2 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Tue, 25 Aug 2026 20:27:39 +0100 Subject: [PATCH] Preserve disabled backup collection scope Contract-Neutral: The release-note line records a persistence bug fix; it does not change release promotion or artifact dispatch contracts. --- docs/releases/RELEASE_NOTES_v6.4.0-rc.1.md | 2 + internal/config/persistence.go | 44 ++++++++++++++----- internal/config/persistence_test.go | 51 +++++++++++++++++----- 3 files changed, 75 insertions(+), 22 deletions(-) diff --git a/docs/releases/RELEASE_NOTES_v6.4.0-rc.1.md b/docs/releases/RELEASE_NOTES_v6.4.0-rc.1.md index 400d33189..054907e50 100644 --- a/docs/releases/RELEASE_NOTES_v6.4.0-rc.1.md +++ b/docs/releases/RELEASE_NOTES_v6.4.0-rc.1.md @@ -46,6 +46,8 @@ build path. ## Fixed - Fixed polling intervals are honored when adaptive scheduling is disabled. +- Disabled Proxmox backup collection now remains disabled after configuration + reloads instead of reverting to the default collection scope. - Re-enrollment clears host-removal blocks from every owning store so a valid returning agent is not held in a partially removed state. - Connection alerts can no longer bypass the configured offline-alert policy. diff --git a/internal/config/persistence.go b/internal/config/persistence.go index 943c97a9d..95ccd9e98 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -1477,6 +1477,18 @@ type NodesConfig struct { PMGInstances []PMGInstance `json:"pmgInstances"` } +func nodeConfigFieldPresent(instances []map[string]json.RawMessage, index int, field string) bool { + if index < 0 || index >= len(instances) { + return false + } + for key := range instances[index] { + if strings.EqualFold(key, field) { + return true + } + } + return false +} + // SystemSettings represents system configuration settings type SystemSettings struct { PVEPollingInterval int `json:"pvePollingInterval"` // PVE polling interval in seconds @@ -1827,6 +1839,18 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { if err := json.Unmarshal(data, &config); err != nil { return nil, fmt.Errorf("parse nodes config: %w", err) } + // Boolean collector settings predate an explicit schema version. Keep a + // parallel view of the saved object keys so a legacy missing field can + // still receive its default without treating an explicit false as missing. + // Decoding straight into bool loses that distinction and used to re-enable + // collectors every time the configuration was loaded. + var persistedFields struct { + PVEInstances []map[string]json.RawMessage `json:"pveInstances"` + PBSInstances []map[string]json.RawMessage `json:"pbsInstances"` + } + if err := json.Unmarshal(data, &persistedFields); err != nil { + return nil, fmt.Errorf("parse nodes config field presence: %w", err) + } if config.PVEInstances == nil { config.PVEInstances = []PVEInstance{} @@ -1875,12 +1899,12 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { } } - // Migration: Ensure MonitorBackups is enabled for PVE instances - // This fixes issue #1139 where PVE backups weren't showing - if !config.PVEInstances[i].MonitorBackups { + // Legacy records omitted MonitorBackups before backup collection became + // the default. Default only an absent field; false is an operator choice. + if !nodeConfigFieldPresent(persistedFields.PVEInstances, i, "MonitorBackups") { log.Info(). Str("instance", config.PVEInstances[i].Name). - Msg("Enabling MonitorBackups for PVE instance (was disabled)") + Msg("Enabling MonitorBackups for legacy PVE instance (field was absent)") config.PVEInstances[i].MonitorBackups = true migrationApplied = true } @@ -1916,12 +1940,12 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { migrationApplied = true } - // Migration: Ensure MonitorBackups is enabled for PBS instances - // This fixes issue #411 where PBS backups weren't showing - if !config.PBSInstances[i].MonitorBackups { + // As above, preserve explicit collection-scope choices while retaining + // the default for records written before these fields existed. + if !nodeConfigFieldPresent(persistedFields.PBSInstances, i, "MonitorBackups") { log.Info(). Str("instance", config.PBSInstances[i].Name). - Msg("Enabling MonitorBackups for PBS instance (was disabled)") + Msg("Enabling MonitorBackups for legacy PBS instance (field was absent)") config.PBSInstances[i].MonitorBackups = true migrationApplied = true } @@ -1930,10 +1954,10 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { // Without this, PBS datastores aren't polled, which means backup data // from the direct PBS connection is never fetched — and PVE-side backup // polling skips PBS storages when a direct connection exists. - if !config.PBSInstances[i].MonitorDatastores { + if !nodeConfigFieldPresent(persistedFields.PBSInstances, i, "MonitorDatastores") { log.Info(). Str("instance", config.PBSInstances[i].Name). - Msg("Enabling MonitorDatastores for PBS instance (was disabled)") + Msg("Enabling MonitorDatastores for legacy PBS instance (field was absent)") config.PBSInstances[i].MonitorDatastores = true migrationApplied = true } diff --git a/internal/config/persistence_test.go b/internal/config/persistence_test.go index 41285959e..5092ef6e6 100644 --- a/internal/config/persistence_test.go +++ b/internal/config/persistence_test.go @@ -2383,21 +2383,21 @@ func TestLoadSystemSettingsFileNotExist(t *testing.T) { } } -func TestLoadNodesConfig_PVEMonitorBackupsMigration(t *testing.T) { +func TestLoadNodesConfig_PreservesExplicitlyDisabledPVEBackups(t *testing.T) { tempDir := t.TempDir() cp := config.NewConfigPersistence(tempDir) if err := cp.EnsureConfigDir(); err != nil { t.Fatalf("EnsureConfigDir: %v", err) } - // Save PVE instance with MonitorBackups=false (simulating old config or missing field) + // Save through the canonical writer so the false field is explicitly present. pveInstances := []config.PVEInstance{ { Name: "pve-no-backups", Host: "https://pve.local:8006", User: "root@pam", Password: "secret", - MonitorBackups: false, // This should be migrated to true + MonitorBackups: false, }, } @@ -2415,28 +2415,55 @@ func TestLoadNodesConfig_PVEMonitorBackupsMigration(t *testing.T) { } pve := loaded.PVEInstances[0] - // MonitorBackups should be migrated to true - if !pve.MonitorBackups { - t.Errorf("expected MonitorBackups to be migrated to true, got false") + if pve.MonitorBackups { + t.Error("expected explicit MonitorBackups=false to survive reload") } } -func TestLoadNodesConfig_PBSMonitorDatastoresMigration(t *testing.T) { +func TestLoadNodesConfig_DefaultsMissingLegacyCollectionFields(t *testing.T) { tempDir := t.TempDir() cp := config.NewConfigPersistence(tempDir) if err := cp.EnsureConfigDir(); err != nil { t.Fatalf("EnsureConfigDir: %v", err) } - // Save PBS instance with MonitorDatastores=false (simulating old config) + legacy := []byte(`{ + "pveInstances": [{"Name":"legacy-pve","Host":"https://pve.local:8006"}], + "pbsInstances": [{"Name":"legacy-pbs","Host":"https://pbs.local:8007"}], + "pmgInstances": [] + }`) + if err := os.WriteFile(filepath.Join(tempDir, "nodes.enc"), legacy, 0600); err != nil { + t.Fatalf("write legacy nodes config: %v", err) + } + + loaded, err := cp.LoadNodesConfig() + if err != nil { + t.Fatalf("LoadNodesConfig: %v", err) + } + if len(loaded.PVEInstances) != 1 || !loaded.PVEInstances[0].MonitorBackups { + t.Fatalf("legacy PVE MonitorBackups was not defaulted: %+v", loaded.PVEInstances) + } + if len(loaded.PBSInstances) != 1 || !loaded.PBSInstances[0].MonitorBackups || !loaded.PBSInstances[0].MonitorDatastores { + t.Fatalf("legacy PBS collection fields were not defaulted: %+v", loaded.PBSInstances) + } +} + +func TestLoadNodesConfig_PreservesExplicitlyDisabledPBSCollection(t *testing.T) { + tempDir := t.TempDir() + cp := config.NewConfigPersistence(tempDir) + if err := cp.EnsureConfigDir(); err != nil { + t.Fatalf("EnsureConfigDir: %v", err) + } + + // Both false fields are explicit in the canonical saved representation. pbsInstances := []config.PBSInstance{ { Name: "pbs-no-datastores", Host: "https://pbs.local:8007", User: "admin@pbs", Password: "secret", - MonitorBackups: true, - MonitorDatastores: false, // This should be migrated to true + MonitorBackups: false, + MonitorDatastores: false, }, } @@ -2454,7 +2481,7 @@ func TestLoadNodesConfig_PBSMonitorDatastoresMigration(t *testing.T) { } pbs := loaded.PBSInstances[0] - if !pbs.MonitorDatastores { - t.Errorf("expected MonitorDatastores to be migrated to true, got false") + if pbs.MonitorBackups || pbs.MonitorDatastores { + t.Fatalf("expected explicit PBS collection flags to remain false: %+v", pbs) } }