diff --git a/weed/admin/dash/admin_server.go b/weed/admin/dash/admin_server.go index 52c518cca2..afdb28b39e 100644 --- a/weed/admin/dash/admin_server.go +++ b/weed/admin/dash/admin_server.go @@ -380,29 +380,29 @@ func (m *masterVacuumToggler) enableVacuum() error { }) } -// syncVacuumState performs a single sync step: checks if a vacuum-capable worker -// is present and calls disable/enable accordingly. Returns the updated state -// and whether the call failed (for log dedup on retries). +// syncVacuumState tells the master to disable its automatic vacuum while a +// vacuum-capable worker is present and to re-enable it otherwise. It re-sends +// on every tick because the master keeps the flag in memory only, so a leader +// change or master restart drops it. Only transitions are logged. Returns the +// state the master was last set to and whether this call failed. func syncVacuumState(hasWorker bool, previouslyActive bool, toggler vacuumToggler, retrying bool) (active bool, failed bool) { - if hasWorker == previouslyActive { - return previouslyActive, false - } + changed := hasWorker != previouslyActive if hasWorker { - if !retrying { + if changed && !retrying { glog.V(0).Infof("Vacuum plugin worker connected, disabling master automatic vacuum") } if err := toggler.disableVacuum(); err != nil { glog.Warningf("Failed to disable vacuum on master: %v", err) - return false, true // retry next tick + return previouslyActive, true // retry next tick } return true, false } - if !retrying { + if changed && !retrying { glog.V(0).Infof("Vacuum plugin worker disconnected, re-enabling master automatic vacuum") } if err := toggler.enableVacuum(); err != nil { glog.Warningf("Failed to enable vacuum on master: %v", err) - return true, true // retry next tick + return previouslyActive, true // retry next tick } return false, false } diff --git a/weed/admin/dash/vacuum_monitor_test.go b/weed/admin/dash/vacuum_monitor_test.go index 69b97f14c2..03adecf6cf 100644 --- a/weed/admin/dash/vacuum_monitor_test.go +++ b/weed/admin/dash/vacuum_monitor_test.go @@ -1,6 +1,7 @@ package dash import ( + "errors" "testing" "time" @@ -56,36 +57,22 @@ func TestVacuumMonitorStateTransitions(t *testing.T) { type fakeToggler struct { disableCalls int enableCalls int + err error } func (f *fakeToggler) disableVacuum() error { f.disableCalls++ - return nil + return f.err } func (f *fakeToggler) enableVacuum() error { f.enableCalls++ - return nil + return f.err } func TestSyncVacuumState(t *testing.T) { t.Parallel() - t.Run("no change when state matches", func(t *testing.T) { - tog := &fakeToggler{} - result, failed := syncVacuumState(false, false, tog, false) - if result != false || failed { - t.Error("expected false, no failure") - } - result, failed = syncVacuumState(true, true, tog, false) - if result != true || failed { - t.Error("expected true, no failure") - } - if tog.disableCalls != 0 || tog.enableCalls != 0 { - t.Errorf("expected no calls, got disable=%d enable=%d", tog.disableCalls, tog.enableCalls) - } - }) - t.Run("worker connects triggers disable", func(t *testing.T) { tog := &fakeToggler{} result, failed := syncVacuumState(true, false, tog, false) @@ -113,4 +100,61 @@ func TestSyncVacuumState(t *testing.T) { t.Errorf("expected 0 disable calls, got %d", tog.disableCalls) } }) + + // The master keeps the flag in memory only, so a new leader or a restarted + // master has lost it. The disable has to be re-sent while the worker stays + // connected, not only when it first connects. + t.Run("disable is re-sent every tick while the worker stays connected", func(t *testing.T) { + tog := &fakeToggler{} + active := false + for i := 0; i < 3; i++ { + var failed bool + active, failed = syncVacuumState(true, active, tog, false) + if !active || failed { + t.Fatalf("tick %d: expected active, no failure", i) + } + } + if tog.disableCalls != 3 { + t.Errorf("expected 3 disable calls, got %d", tog.disableCalls) + } + if tog.enableCalls != 0 { + t.Errorf("expected 0 enable calls, got %d", tog.enableCalls) + } + }) + + // Same for the enable side: a master that still holds an old plugin disable + // and becomes leader would otherwise never vacuum again. + t.Run("enable is re-sent every tick while no worker is connected", func(t *testing.T) { + tog := &fakeToggler{} + active := false + for i := 0; i < 3; i++ { + var failed bool + active, failed = syncVacuumState(false, active, tog, false) + if active || failed { + t.Fatalf("tick %d: expected inactive, no failure", i) + } + } + if tog.enableCalls != 3 { + t.Errorf("expected 3 enable calls, got %d", tog.enableCalls) + } + if tog.disableCalls != 0 { + t.Errorf("expected 0 disable calls, got %d", tog.disableCalls) + } + }) + + t.Run("failed call keeps the previous state and reports failure", func(t *testing.T) { + tog := &fakeToggler{err: errors.New("master unavailable")} + result, failed := syncVacuumState(true, false, tog, false) + if result != false || !failed { + t.Error("expected previous state false and a failure") + } + result, failed = syncVacuumState(true, true, tog, false) + if result != true || !failed { + t.Error("expected previous state true and a failure") + } + result, failed = syncVacuumState(false, true, tog, false) + if result != true || !failed { + t.Error("expected previous state true and a failure") + } + }) } diff --git a/weed/topology/topology.go b/weed/topology/topology.go index 5559240385..0d393bdfd9 100644 --- a/weed/topology/topology.go +++ b/weed/topology/topology.go @@ -1543,17 +1543,21 @@ func (t *Topology) EnableVacuum() { } // DisableVacuumByPlugin is called by the admin server's vacuum monitor -// when a vacuum plugin worker connects. Only sets the plugin flag. +// while a vacuum plugin worker is connected. Only sets the plugin flag. +// The monitor repeats the call on every poll, so only a change is logged. func (t *Topology) DisableVacuumByPlugin() { - glog.V(0).Infof("DisableVacuum (by plugin worker)") - t.vacuumDisabledByPlugin.Store(true) + if !t.vacuumDisabledByPlugin.Swap(true) { + glog.V(0).Infof("DisableVacuum (by plugin worker)") + } } // EnableVacuumByPlugin is called by the admin server's vacuum monitor -// when a vacuum plugin worker disconnects. Only clears the plugin flag. +// while no vacuum plugin worker is connected. Only clears the plugin flag. +// The monitor repeats the call on every poll, so only a change is logged. func (t *Topology) EnableVacuumByPlugin() { - glog.V(0).Infof("EnableVacuum (by plugin worker)") - t.vacuumDisabledByPlugin.Store(false) + if t.vacuumDisabledByPlugin.Swap(false) { + glog.V(0).Infof("EnableVacuum (by plugin worker)") + } } // IsVacuumDisabledByPlugin returns whether the plugin monitor has disabled vacuum.