diff --git a/weed/admin/dash/maintenance_startup_test.go b/weed/admin/dash/maintenance_startup_test.go new file mode 100644 index 000000000..7bd9502c5 --- /dev/null +++ b/weed/admin/dash/maintenance_startup_test.go @@ -0,0 +1,123 @@ +package dash + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/admin/maintenance" + "github.com/seaweedfs/seaweedfs/weed/worker/tasks" + "github.com/seaweedfs/seaweedfs/weed/worker/tasks/balance" + "github.com/seaweedfs/seaweedfs/weed/worker/tasks/vacuum" + "github.com/seaweedfs/seaweedfs/weed/worker/types" +) + +// The task definitions these tests configure are process-global, so put them back the way a +// fresh process would have them. Passing no config store makes every task fall back to its +// own NewDefaultConfig, which is exactly the state package init left them in. +func restoreGlobalTaskState(t *testing.T) { + t.Helper() + + t.Cleanup(func() { + tasks.GetGlobalConfigUpdateRegistry().UpdateAllConfigs(nil) + }) +} + +// TestDisabledTaskIsNotScannedAfterStartup walks the admin server's startup sequence over a +// data directory that has a disabled balance task saved in it, and checks the end state that +// actually matters: the balance detector reports disabled, so ScanWithTaskDetectors skips it. +// +// This is the whole of issue #10874 in one test. The reporter disabled balance, and the +// scanner kept detecting balance tasks, cancelling them and re-detecting them. Two separate +// defects had to line up for the disabled flag to survive to here: the policy had to be built +// from the persisted configs rather than from a nil store, and the policy had to reach +// detector.IsEnabled() rather than dying in a failed type assertion. +func TestDisabledTaskIsNotScannedAfterStartup(t *testing.T) { + restoreGlobalTaskState(t) + + dir := t.TempDir() + cp := NewConfigPersistence(dir) + + // What the admin writes when a user turns balance off, and leaves vacuum on. + disabledBalance := balance.NewDefaultConfig() + disabledBalance.Enabled = false + if err := cp.SaveBalanceTaskPolicy(disabledBalance.ToTaskPolicy()); err != nil { + t.Fatalf("save balance policy: %v", err) + } + + enabledVacuum := vacuum.NewDefaultConfig() + enabledVacuum.Enabled = true + if err := cp.SaveVacuumTaskPolicy(enabledVacuum.ToTaskPolicy()); err != nil { + t.Fatalf("save vacuum policy: %v", err) + } + + // The admin server's startup sequence, in order: + // loadTaskConfigurationsFromPersistence, then InitMaintenanceManager. + tasks.GetGlobalConfigUpdateRegistry().UpdateAllConfigs(cp) + + maintenanceConfig, err := cp.LoadMaintenanceConfig() + if err != nil { + t.Fatalf("load maintenance config: %v", err) + } + manager := maintenance.NewMaintenanceManager(nil, maintenanceConfig, cp) + if manager == nil { + t.Fatal("NewMaintenanceManager returned nil") + } + + registry := tasks.GetGlobalTypesRegistry() + + balanceDetector := registry.GetDetector(types.TaskTypeBalance) + if balanceDetector == nil { + t.Fatal("no balance detector registered") + } + if balanceDetector.IsEnabled() { + t.Error("balance detector reports enabled after startup over a data directory where " + + "balance is saved as disabled; the scanner will keep detecting and cancelling balance tasks") + } + + vacuumDetector := registry.GetDetector(types.TaskTypeVacuum) + if vacuumDetector == nil { + t.Fatal("no vacuum detector registered") + } + if !vacuumDetector.IsEnabled() { + t.Error("vacuum detector reports disabled although vacuum is saved as enabled; " + + "the fix must not switch off tasks the user left on") + } + + // Tasks the user never touched keep their compiled-in default of enabled rather than + // being switched off by a policy entry built from a config that was never saved. + for _, taskType := range []types.TaskType{types.TaskTypeErasureCoding, types.TaskTypeECBalance} { + detector := registry.GetDetector(taskType) + if detector == nil { + t.Fatalf("no %s detector registered", taskType) + } + if !detector.IsEnabled() { + t.Errorf("%s detector reports disabled although its config was never saved", taskType) + } + } +} + +// TestPolicyMirrorsWhatTheDetectorsReport checks that the maintenance policy the queue and +// the scanner run on agrees with the detectors. A disagreement means one of the two paths +// into the task configs has gone stale again. +func TestPolicyMirrorsWhatTheDetectorsReport(t *testing.T) { + restoreGlobalTaskState(t) + + dir := t.TempDir() + cp := NewConfigPersistence(dir) + + disabledBalance := balance.NewDefaultConfig() + disabledBalance.Enabled = false + if err := cp.SaveBalanceTaskPolicy(disabledBalance.ToTaskPolicy()); err != nil { + t.Fatalf("save balance policy: %v", err) + } + + tasks.GetGlobalConfigUpdateRegistry().UpdateAllConfigs(cp) + policy := cp.buildPolicyFromTaskConfigs() + + for taskType, detector := range tasks.GetGlobalTypesRegistry().GetAllDetectors() { + policyEnabled := maintenance.IsTaskEnabled(policy, maintenance.MaintenanceTaskType(taskType)) + if policyEnabled != detector.IsEnabled() { + t.Errorf("%s: policy says enabled=%v but the detector says enabled=%v", + taskType, policyEnabled, detector.IsEnabled()) + } + } +} diff --git a/weed/admin/maintenance/maintenance_policy_wiring_test.go b/weed/admin/maintenance/maintenance_policy_wiring_test.go index f72ae3795..c80744b18 100644 --- a/weed/admin/maintenance/maintenance_policy_wiring_test.go +++ b/weed/admin/maintenance/maintenance_policy_wiring_test.go @@ -177,3 +177,33 @@ func policyWithAllTasks(t *testing.T, enabled bool) *MaintenancePolicy { } return policy } + +// TestPolicyHelpersTolerateNilPolicy pins the nil handling in the exported policy helpers. +// MaintenanceConfig.Policy is nil until something builds one, and UpdateConfig accepts a +// config that has none, so these are reachable with a nil policy - GetTaskPolicy used to +// dereference it and take the admin process down. +func TestPolicyHelpersTolerateNilPolicy(t *testing.T) { + const taskType = MaintenanceTaskType("balance") + + if got := GetTaskPolicy(nil, taskType); got != nil { + t.Errorf("GetTaskPolicy(nil) = %v, want nil", got) + } + if IsTaskEnabled(nil, taskType) { + t.Error("IsTaskEnabled(nil) = true, want false") + } + if got := GetMaxConcurrent(nil, taskType); got != 1 { + t.Errorf("GetMaxConcurrent(nil) = %d, want the safe default 1", got) + } + if got := GetRepeatInterval(nil, taskType); got != 0 { + t.Errorf("GetRepeatInterval(nil) = %d, want 0 so callers fall back to their own default", got) + } + + // A policy that exists but lists nothing must behave the same way. + empty := &MaintenancePolicy{} + if got := GetTaskPolicy(empty, taskType); got != nil { + t.Errorf("GetTaskPolicy(empty) = %v, want nil", got) + } + if IsTaskEnabled(empty, taskType) { + t.Error("IsTaskEnabled(empty) = true, want false") + } +} diff --git a/weed/admin/maintenance/maintenance_types.go b/weed/admin/maintenance/maintenance_types.go index 811893953..b6bc773f2 100644 --- a/weed/admin/maintenance/maintenance_types.go +++ b/weed/admin/maintenance/maintenance_types.go @@ -144,9 +144,14 @@ func DefaultMaintenanceConfig() *MaintenanceConfig { // Policy helper functions (since we can't add methods to type aliases) -// GetTaskPolicy returns the policy for a specific task type +// GetTaskPolicy returns the policy for a specific task type, or nil when the maintenance +// policy has no entry for it. +// +// A nil maintenance policy is a legitimate state, not a programming error: +// MaintenanceConfig.Policy is unset until something builds one, and UpdateConfig accepts a +// config that carries none. Dereferencing it here panicked the whole admin process. func GetTaskPolicy(mp *MaintenancePolicy, taskType MaintenanceTaskType) *TaskPolicy { - if mp.TaskPolicies == nil { + if mp == nil || mp.TaskPolicies == nil { return nil } return mp.TaskPolicies[string(taskType)] @@ -174,6 +179,9 @@ func GetMaxConcurrent(mp *MaintenancePolicy, taskType MaintenanceTaskType) int { func GetRepeatInterval(mp *MaintenancePolicy, taskType MaintenanceTaskType) int { policy := GetTaskPolicy(mp, taskType) if policy == nil { + if mp == nil { + return 0 + } return int(mp.DefaultRepeatIntervalSeconds) } return int(policy.RepeatIntervalSeconds)