shell: keep noLock to the command that set it (#11052)

noLock says "this invocation changes nothing" -- volume.balance, volume.move,
volume.copy, volume.merge and volume.fix.replication all set it for a dry run,
and none clears it. The CommandEnv is created once and reused by both
dispatchers, the interactive shell and the master's maintenance script runner,
so a simulation left every later command unlocked:

    volume.balance -noLock          # changes nothing
    volume.move ...                 # mutates, and skips its lock

Reset before dispatch in both, where the invocation begins. forceNoLock is
untouched: that is set once, deliberately, for a trusted path.
This commit is contained in:
Chris Lu
2026-08-31 11:36:23 -07:00
committed by GitHub
parent 87474c2f21
commit 0f4a7d0803
3 changed files with 59 additions and 0 deletions
+5
View File
@@ -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)
}
+6
View File
@@ -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)
}
+48
View File
@@ -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)
}
}
}