diff --git a/weed/server/master_server.go b/weed/server/master_server.go index 401e2ec37..d921203c3 100644 --- a/weed/server/master_server.go +++ b/weed/server/master_server.go @@ -473,6 +473,11 @@ func processEachCmd(reg *regexp.Regexp, line string, commandEnv *shell.CommandEn continue } glog.V(0).Infof("executing: %s %v", cmd, args) + // noLock belongs to the invocation that set it, not to the + // CommandEnv this loop reuses for every line of the script. Without + // this, a dry run earlier in the script leaves the mutations after + // it unlocked. + commandEnv.SetNoLock(false) if err := c.Do(args, commandEnv, os.Stdout); err != nil { glog.V(0).Infof("error: %v", err) } diff --git a/weed/shell/shell_liner.go b/weed/shell/shell_liner.go index 1ae4ce5c5..9ea8ca873 100644 --- a/weed/shell/shell_liner.go +++ b/weed/shell/shell_liner.go @@ -141,6 +141,12 @@ func processEachCmd(cmd string, commandEnv *CommandEnv) bool { foundCommand := false for _, c := range Commands { if c.Name() == cmd || c.Name() == "fs."+cmd { + // noLock says "this invocation changes nothing", which is a + // property of the invocation and not of the session. A + // command that set it for a dry run would otherwise leave + // every later command in the session unlocked, so a real + // mutation right after a simulation would skip its lock. + commandEnv.SetNoLock(false) if err := c.Do(args, commandEnv, os.Stdout); err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) } diff --git a/weed/shell/shell_nolock_scope_test.go b/weed/shell/shell_nolock_scope_test.go new file mode 100644 index 000000000..c8c0d85e5 --- /dev/null +++ b/weed/shell/shell_nolock_scope_test.go @@ -0,0 +1,48 @@ +package shell + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/cluster" + "github.com/seaweedfs/seaweedfs/weed/wdclient/exclusive_locks" +) + +// noLock says "this invocation changes nothing" -- volume.balance, +// volume.fix.replication and others set it for a dry run. It is a property of +// the invocation, not of the session: the CommandEnv is reused across every +// command, so without a reset a simulation leaves later commands unlocked and a +// real mutation run straight after one skips its lock silently. +func TestNoLockDoesNotOutliveTheCommandThatSetIt(t *testing.T) { + env := &CommandEnv{locker: exclusive_locks.NewExclusiveLocker(nil, cluster.AdminShellLockName)} + + env.SetNoLock(true) + if err := env.confirmIsLocked([]string{"volume.balance"}); err != nil { + t.Fatalf("a dry run needs no lock: %v", err) + } + + env.SetNoLock(false) + + if err := env.confirmIsLocked([]string{"volume.move"}); err == nil { + t.Error("the next command mutates and must require the lock again") + } +} + +// Both dispatchers reuse one CommandEnv: the interactive shell across the lines +// an operator types, and the master's maintenance script runner across the lines +// of a script. Resetting has to restore the lock requirement however many +// invocations share the environment. +func TestNoLockResetRestoresTheRequirementForEveryInvocation(t *testing.T) { + env := &CommandEnv{locker: exclusive_locks.NewExclusiveLocker(nil, cluster.AdminShellLockName)} + + for i := 0; i < 3; i++ { + env.SetNoLock(true) + if err := env.confirmIsLocked([]string{"dry run"}); err != nil { + t.Fatalf("run %d: a dry run needs no lock: %v", i, err) + } + + env.SetNoLock(false) + if err := env.confirmIsLocked([]string{"mutation"}); err == nil { + t.Fatalf("run %d: the mutation after it must require the lock again", i) + } + } +}