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

@@ -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")

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")

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")

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")

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