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) + } + }) + } +}