From 1b5e98143e9a614a4cb79004ca573383f684fba0 Mon Sep 17 00:00:00 2001 From: Junker der Provinz Date: Sun, 23 Aug 2026 05:44:35 +0200 Subject: [PATCH] worker: warn when a config store cannot supply a task's persisted config LoadConfigFromPersistence logged a single glog.V(1) "Using default X configuration" for every way of not loading anything, so the bug in issue #10874 - a store handed in that the type assertion rejects, leaving a task running on compiled-in defaults - looked exactly like the normal "no data directory configured" case. The reporter had to read the source to work out why their disabled task kept running, and asked for this specifically. Separate the cases. A non-nil store that does not provide the accessor is always a wiring bug and is now logged at warning level, naming the type and the missing method. A read error or a policy that will not apply is also a warning. No persistence configured, and a store with nothing saved yet, stay at V(1): those are normal. Refs #10874 --- weed/worker/tasks/balance/config.go | 17 ++++- weed/worker/tasks/ec_balance/config.go | 17 ++++- weed/worker/tasks/erasure_coding/config.go | 17 ++++- weed/worker/tasks/vacuum/config.go | 17 ++++- .../tasks/vacuum/config_persistence_test.go | 75 +++++++++++++++++++ 5 files changed, 135 insertions(+), 8 deletions(-) create mode 100644 weed/worker/tasks/vacuum/config_persistence_test.go diff --git a/weed/worker/tasks/balance/config.go b/weed/worker/tasks/balance/config.go index 1616788ee..f1372f1cc 100644 --- a/weed/worker/tasks/balance/config.go +++ b/weed/worker/tasks/balance/config.go @@ -178,12 +178,25 @@ func LoadConfigFromPersistence(configPersistence interface{}) *Config { if persistence, ok := configPersistence.(interface { LoadBalanceTaskPolicy() (*worker_pb.TaskPolicy, error) }); ok { - if policy, err := persistence.LoadBalanceTaskPolicy(); err == nil && policy != nil { - if err := config.FromTaskPolicy(policy); err == nil { + policy, err := persistence.LoadBalanceTaskPolicy() + switch { + case err != nil: + glog.Warningf("Could not read the persisted balance configuration, falling back to defaults: %v", err) + case policy == nil: + glog.V(1).Infof("No balance configuration persisted yet, using defaults") + default: + if err := config.FromTaskPolicy(policy); err != nil { + glog.Warningf("Could not apply the persisted balance configuration, falling back to defaults: %v", err) + } else { glog.V(1).Infof("Loaded balance configuration from persistence") return config } } + } else if configPersistence != nil { + // A store was handed in but does not expose the accessor, so the persisted + // settings are silently ignored - always a wiring bug, never a normal state. + glog.Warningf("%T cannot provide the persisted balance configuration: it has no LoadBalanceTaskPolicy() method, "+ + "so the compiled-in defaults are used and any saved balance settings are ignored", configPersistence) } glog.V(1).Infof("Using default balance configuration") diff --git a/weed/worker/tasks/ec_balance/config.go b/weed/worker/tasks/ec_balance/config.go index cc3397228..c9e410d6e 100644 --- a/weed/worker/tasks/ec_balance/config.go +++ b/weed/worker/tasks/ec_balance/config.go @@ -241,12 +241,25 @@ func LoadConfigFromPersistence(configPersistence interface{}) *Config { if persistence, ok := configPersistence.(interface { LoadEcBalanceTaskPolicy() (*worker_pb.TaskPolicy, error) }); ok { - if policy, err := persistence.LoadEcBalanceTaskPolicy(); err == nil && policy != nil { - if err := cfg.FromTaskPolicy(policy); err == nil { + policy, err := persistence.LoadEcBalanceTaskPolicy() + switch { + case err != nil: + glog.Warningf("Could not read the persisted EC balance configuration, falling back to defaults: %v", err) + case policy == nil: + glog.V(1).Infof("No EC balance configuration persisted yet, using defaults") + default: + if err := cfg.FromTaskPolicy(policy); err != nil { + glog.Warningf("Could not apply the persisted EC balance configuration, falling back to defaults: %v", err) + } else { glog.V(1).Infof("Loaded EC balance configuration from persistence") return cfg } } + } else if configPersistence != nil { + // A store was handed in but does not expose the accessor, so the persisted + // settings are silently ignored - always a wiring bug, never a normal state. + glog.Warningf("%T cannot provide the persisted EC balance configuration: it has no LoadEcBalanceTaskPolicy() method, "+ + "so the compiled-in defaults are used and any saved EC balance settings are ignored", configPersistence) } glog.V(1).Infof("Using default EC balance configuration") diff --git a/weed/worker/tasks/erasure_coding/config.go b/weed/worker/tasks/erasure_coding/config.go index 48371bb07..7079d1640 100644 --- a/weed/worker/tasks/erasure_coding/config.go +++ b/weed/worker/tasks/erasure_coding/config.go @@ -229,12 +229,25 @@ func LoadConfigFromPersistence(configPersistence interface{}) *Config { if persistence, ok := configPersistence.(interface { LoadErasureCodingTaskPolicy() (*worker_pb.TaskPolicy, error) }); ok { - if policy, err := persistence.LoadErasureCodingTaskPolicy(); err == nil && policy != nil { - if err := config.FromTaskPolicy(policy); err == nil { + policy, err := persistence.LoadErasureCodingTaskPolicy() + switch { + case err != nil: + glog.Warningf("Could not read the persisted erasure coding configuration, falling back to defaults: %v", err) + case policy == nil: + glog.V(1).Infof("No erasure coding configuration persisted yet, using defaults") + default: + if err := config.FromTaskPolicy(policy); err != nil { + glog.Warningf("Could not apply the persisted erasure coding configuration, falling back to defaults: %v", err) + } else { glog.V(1).Infof("Loaded erasure coding configuration from persistence") return config } } + } else if configPersistence != nil { + // A store was handed in but does not expose the accessor, so the persisted + // settings are silently ignored - always a wiring bug, never a normal state. + glog.Warningf("%T cannot provide the persisted erasure coding configuration: it has no LoadErasureCodingTaskPolicy() method, "+ + "so the compiled-in defaults are used and any saved erasure coding settings are ignored", configPersistence) } glog.V(1).Infof("Using default erasure coding configuration") diff --git a/weed/worker/tasks/vacuum/config.go b/weed/worker/tasks/vacuum/config.go index 5d4469707..e97970b65 100644 --- a/weed/worker/tasks/vacuum/config.go +++ b/weed/worker/tasks/vacuum/config.go @@ -73,12 +73,25 @@ func LoadConfigFromPersistence(configPersistence interface{}) *Config { if persistence, ok := configPersistence.(interface { LoadVacuumTaskPolicy() (*worker_pb.TaskPolicy, error) }); ok { - if policy, err := persistence.LoadVacuumTaskPolicy(); err == nil && policy != nil { - if err := config.FromTaskPolicy(policy); err == nil { + policy, err := persistence.LoadVacuumTaskPolicy() + switch { + case err != nil: + glog.Warningf("Could not read the persisted vacuum configuration, falling back to defaults: %v", err) + case policy == nil: + glog.V(1).Infof("No vacuum configuration persisted yet, using defaults") + default: + if err := config.FromTaskPolicy(policy); err != nil { + glog.Warningf("Could not apply the persisted vacuum configuration, falling back to defaults: %v", err) + } else { glog.V(1).Infof("Loaded vacuum configuration from persistence") return config } } + } else if configPersistence != nil { + // A store was handed in but does not expose the accessor, so the persisted + // settings are silently ignored - always a wiring bug, never a normal state. + glog.Warningf("%T cannot provide the persisted vacuum configuration: it has no LoadVacuumTaskPolicy() method, "+ + "so the compiled-in defaults are used and any saved vacuum settings are ignored", configPersistence) } glog.V(1).Infof("Using default vacuum configuration") diff --git a/weed/worker/tasks/vacuum/config_persistence_test.go b/weed/worker/tasks/vacuum/config_persistence_test.go new file mode 100644 index 000000000..c05021109 --- /dev/null +++ b/weed/worker/tasks/vacuum/config_persistence_test.go @@ -0,0 +1,75 @@ +package vacuum + +import ( + "errors" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/worker_pb" +) + +type stubVacuumStore struct { + policy *worker_pb.TaskPolicy + err error +} + +func (s *stubVacuumStore) LoadVacuumTaskPolicy() (*worker_pb.TaskPolicy, error) { + return s.policy, s.err +} + +// wrongShapedStore is what the maintenance manager used to be handed: a non-nil value that +// does not satisfy the accessor the loader asserts on. It has to keep falling back to the +// defaults, but no longer silently - the reporter of issue #10874 had to read the source to +// find out why their disabled task kept running. +type wrongShapedStore struct{} + +func (wrongShapedStore) SomethingElse() {} + +func TestLoadConfigFromPersistenceUsesPersistedPolicy(t *testing.T) { + persisted := NewDefaultConfig() + persisted.Enabled = false + persisted.GarbageThreshold = 0.75 + persisted.MaxConcurrent = 5 + + loaded := LoadConfigFromPersistence(&stubVacuumStore{policy: persisted.ToTaskPolicy()}) + if loaded == nil { + t.Fatal("LoadConfigFromPersistence returned nil") + } + if loaded.Enabled { + t.Error("enabled = true, want the persisted false") + } + if loaded.GarbageThreshold != 0.75 { + t.Errorf("garbage threshold = %v, want the persisted 0.75", loaded.GarbageThreshold) + } + if loaded.MaxConcurrent != 5 { + t.Errorf("max concurrent = %d, want the persisted 5", loaded.MaxConcurrent) + } +} + +func TestLoadConfigFromPersistenceFallsBackToDefaults(t *testing.T) { + defaults := NewDefaultConfig() + + cases := map[string]interface{}{ + "no store configured": nil, + "store without the value": &stubVacuumStore{}, + "store that errors": &stubVacuumStore{err: errors.New("disk on fire")}, + "store of the wrong type": wrongShapedStore{}, + } + + for name, store := range cases { + t.Run(name, func(t *testing.T) { + loaded := LoadConfigFromPersistence(store) + if loaded == nil { + t.Fatal("LoadConfigFromPersistence returned nil") + } + if !loaded.Enabled { + t.Error("enabled = false, want the compiled-in default true") + } + if loaded.GarbageThreshold != defaults.GarbageThreshold { + t.Errorf("garbage threshold = %v, want the default %v", loaded.GarbageThreshold, defaults.GarbageThreshold) + } + if loaded.ScanIntervalSeconds != defaults.ScanIntervalSeconds { + t.Errorf("scan interval = %d, want the default %d", loaded.ScanIntervalSeconds, defaults.ScanIntervalSeconds) + } + }) + } +}