Merge lp:~thumper/juju-core/juju-run-just-lock into lp:~go-bot/juju-core/trunk

[Tim Penhey]

juju run should always use hook execution lock

When juju run executes remotely, for services and
units it was grabbing the file system lock for the
hook execution to make sure things are serialized
appropriately.

This branch makes sure that the machine executions
also use the file system lock to make sure the
commands are serialized.

https://codereview.appspot.com/52470044/

juju run should always use hook execution lock

When juju run executes remotely, for services and
units it was grabbing the file system lock for the
hook execution to make sure things are serialized
appropriately.

This branch makes sure that the machine executions
also use the file system lock to make sure the
commands are serialized.

https://codereview.appspot.com/52470044/

[Tim Penhey (thumper)]

Reviewers: mp+201702_code.launchpad.net,

Message:
Please take a look.

Description:
juju run should always use hook execution lock

When juju run executes remotely, for services and
units it was grabbing the file system lock for the
hook execution to make sure things are serialized
appropriately.

This branch makes sure that the machine executions
also use the file system lock to make sure the
commands are serialized.

https://code.launchpad.net/~thumper/juju-core/juju-run-just-lock/+merge/201702

Requires:
https://code.launchpad.net/~thumper/juju-core/run-cmd-refactor/+merge/201680

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/52470044/

Affected files (+183, -66 lines):
   A [revision details]
   M cmd/jujud/run.go
   M cmd/jujud/run_test.go
   M state/apiserver/client/run.go
   M state/apiserver/client/run_test.go
   D utils/fslock/export_test.go
   M utils/fslock/fslock.go
   M utils/fslock/fslock_test.go
   A utils/fslock/package_test.go

[Ian Booth (wallyworld)]

LGTM with some consideration to my questions

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run.go
File cmd/jujud/run.go (right):

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run.go#newcode42
cmd/jujud/run.go:42: If --no-context is specified, the <unit-name>
positional
Not sure if --no-unit would be better here from a user perspective?

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run.go#newcode61
cmd/jujud/run.go:61: f.BoolVar(&c.noContext, "no-context", false, "do
not run the command in a unit context")
--no-unit???

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run_test.go
File cmd/jujud/run_test.go (right):

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run_test.go#newcode138
cmd/jujud/run_test.go:138: s.PatchValue(&fslock.LockWaitDelay,
10*time.Millisecond)
should this be ShortWait?

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run_test.go#newcode146
cmd/jujud/run_test.go:146: c.Assert(err, gc.ErrorMatches, "timeout")
What happens if this assert fails? The lock will still be held. Will
that adversely affect other tests? I think we should either add cleanup
or change the Assert to Check

https://codereview.appspot.com/52470044/

[Tim Penhey (thumper)]

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run.go
File cmd/jujud/run.go (right):

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run.go#newcode42
cmd/jujud/run.go:42: If --no-context is specified, the <unit-name>
positional
On 2014/01/15 02:03:18, wallyworld wrote:
> Not sure if --no-unit would be better here from a user perspective?

Users don't see this. This is only really used by the juju run
apiserver component.

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run_test.go
File cmd/jujud/run_test.go (right):

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run_test.go#newcode138
cmd/jujud/run_test.go:138: s.PatchValue(&fslock.LockWaitDelay,
10*time.Millisecond)
On 2014/01/15 02:03:18, wallyworld wrote:
> should this be ShortWait?

No, I wanted to make sure that it checked faster than short wait,
otherwise there was the potential that after unlocking, it didn't have
enough time to execute fully.

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run_test.go#newcode146
cmd/jujud/run_test.go:146: c.Assert(err, gc.ErrorMatches, "timeout")
On 2014/01/15 02:03:18, wallyworld wrote:
> What happens if this assert fails? The lock will still be held. Will
that
> adversely affect other tests? I think we should either add cleanup or
change the
> Assert to Check

No, the lock is in the LockDir, which is mocked out above in a temporary
test directory.

https://codereview.appspot.com/52470044/

=== modified file 'cmd/jujud/run.go'
--- cmd/jujud/run.go	2014-01-15 01:05:05 +0000
+++ cmd/jujud/run.go	2014-01-15 01:05:06 +0000
@@ -14,16 +14,21 @@
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/utils/exec"
+	"launchpad.net/juju-core/utils/fslock"
 	"launchpad.net/juju-core/worker/uniter"
 )
 
-var AgentDir = "/var/lib/juju/agents"
+var (
+	AgentDir = "/var/lib/juju/agents"
+	LockDir  = "/var/lib/juju/locks"
+)
 
 type RunCommand struct {
 	cmd.CommandBase
-	unit     string
-	commands string
-	showHelp bool
+	unit      string
+	commands  string
+	showHelp  bool
+	noContext bool
 }
 
 const runCommandDoc = `
@@ -34,6 +39,9 @@
 or the unit id:
  i.e.  ubuntu/0
 
+If --no-context is specified, the <unit-name> positional
+argument is not needed.
+
 The commands are executed with '/bin/bash -s', and the output returned.
 `
 
@@ -50,6 +58,7 @@
 func (c *RunCommand) SetFlags(f *gnuflag.FlagSet) {
 	f.BoolVar(&c.showHelp, "h", false, "show help on juju-run")
 	f.BoolVar(&c.showHelp, "help", false, "")
+	f.BoolVar(&c.noContext, "no-context", false, "do not run the command in a unit context")
 }
 
 func (c *RunCommand) Init(args []string) error {
@@ -57,19 +66,21 @@
 	if contextId, err := getenv("JUJU_CONTEXT_ID"); err == nil && contextId != "" {
 		return fmt.Errorf("juju-run cannot be called from within a hook, have context %q", contextId)
 	}
+	if !c.noContext {
+		if len(args) < 1 {
+			return fmt.Errorf("missing unit-name")
+		}
+		c.unit, args = args[0], args[1:]
+		// If the command line param is a unit id (like service/2) we need to
+		// change it to the unit tag as that is the format of the agent directory
+		// on disk (unit-service-2).
+		if names.IsUnit(c.unit) {
+			c.unit = names.UnitTag(c.unit)
+		}
+	}
 	if len(args) < 1 {
-		return fmt.Errorf("missing unit-name")
-	}
-	if len(args) < 2 {
 		return fmt.Errorf("missing commands")
 	}
-	c.unit, args = args[0], args[1:]
-	// If the command line param is a unit id (like service/2) we need to
-	// change it to the unit tag as that is the format of the agent directory
-	// on disk (unit-service-2).
-	if names.IsUnit(c.unit) {
-		c.unit = names.UnitTag(c.unit)
-	}
 	c.commands, args = args[0], args[1:]
 	return cmd.CheckEmpty(args)
 }
@@ -79,31 +90,62 @@
 		return gnuflag.ErrHelp
 	}
 
+	var result *exec.ExecResponse
+	var err error
+	if c.noContext {
+		result, err = c.executeNoContext()
+	} else {
+		result, err = c.executeInUnitContext()
+	}
+	if err != nil {
+		return err
+	}
+
+	ctx.Stdout.Write(result.Stdout)
+	ctx.Stderr.Write(result.Stderr)
+	return cmd.NewRcPassthroughError(result.Code)
+}
+
+func (c *RunCommand) executeInUnitContext() (*exec.ExecResponse, error) {
 	unitDir := filepath.Join(AgentDir, c.unit)
 	logger.Debugf("looking for unit dir %s", unitDir)
 	// make sure the unit exists
 	_, err := os.Stat(unitDir)
 	if os.IsNotExist(err) {
-		return fmt.Errorf("unit %q not found on this machine", c.unit)
+		return nil, fmt.Errorf("unit %q not found on this machine", c.unit)
 	} else if err != nil {
-		return err
+		return nil, err
 	}
 
 	socketPath := filepath.Join(unitDir, uniter.RunListenerFile)
 	// make sure the socket exists
 	client, err := rpc.Dial(uniter.RunListenerNetType, socketPath)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	defer client.Close()
 
 	var result exec.ExecResponse
 	err = client.Call(uniter.JujuRunEndpoint, c.commands, &result)
+	return &result, err
+}
+
+func getLock() (*fslock.Lock, error) {
+	return fslock.NewLock(LockDir, "uniter-hook-execution")
+}
+
+func (c *RunCommand) executeNoContext() (*exec.ExecResponse, error) {
+	// Acquire the uniter hook execution lock to make sure we don't
+	// stomp on each other.
+	lock, err := getLock()
 	if err != nil {
-		return err
+		return nil, err
 	}
+	lock.Lock("juju-run")
+	defer lock.Unlock()
 
-	ctx.Stdout.Write(result.Stdout)
-	ctx.Stderr.Write(result.Stderr)
-	return cmd.NewRcPassthroughError(result.Code)
+	return exec.RunCommands(
+		exec.RunParams{
+			Commands: c.commands,
+		})
 }

=== modified file 'cmd/jujud/run_test.go'
--- cmd/jujud/run_test.go	2014-01-15 01:05:05 +0000
+++ cmd/jujud/run_test.go	2014-01-15 01:05:06 +0000
@@ -7,6 +7,7 @@
 	"fmt"
 	"os"
 	"path/filepath"
+	"time"
 
 	gc "launchpad.net/gocheck"
 	"launchpad.net/loggo"
@@ -16,6 +17,7 @@
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/utils/exec"
+	"launchpad.net/juju-core/utils/fslock"
 	"launchpad.net/juju-core/worker/uniter"
 )
 
@@ -27,11 +29,12 @@
 
 func (*RunTestSuite) TestWrongArgs(c *gc.C) {
 	for i, test := range []struct {
-		title    string
-		args     []string
-		errMatch string
-		unit     string
-		commands string
+		title        string
+		args         []string
+		errMatch     string
+		unit         string
+		commands     string
+		avoidContext bool
 	}{{
 		title:    "no args",
 		errMatch: "missing unit-name",
@@ -53,15 +56,21 @@
 		args:     []string{"foo/1", "command"},
 		unit:     "unit-foo-1",
 		commands: "command",
+	}, {
+		title:        "execute not in a context",
+		args:         []string{"--no-context", "command"},
+		commands:     "command",
+		avoidContext: true,
 	},
 	} {
-		c.Log(fmt.Sprintf("\n%d: %s", i, test.title))
+		c.Logf("\n%d: %s", i, test.title)
 		runCommand := &RunCommand{}
-		err := runCommand.Init(test.args)
+		err := testing.InitCommand(runCommand, test.args)
 		if test.errMatch == "" {
 			c.Assert(err, gc.IsNil)
 			c.Assert(runCommand.unit, gc.Equals, test.unit)
 			c.Assert(runCommand.commands, gc.Equals, test.commands)
+			c.Assert(runCommand.noContext, gc.Equals, test.avoidContext)
 		} else {
 			c.Assert(err, gc.ErrorMatches, test.errMatch)
 		}
@@ -76,13 +85,73 @@
 }
 
 func (s *RunTestSuite) TestMissingAgent(c *gc.C) {
-	agentDir := c.MkDir()
-	s.PatchValue(&AgentDir, agentDir)
+	s.PatchValue(&AgentDir, c.MkDir())
 
 	_, err := testing.RunCommand(c, &RunCommand{}, []string{"foo", "bar"})
 	c.Assert(err, gc.ErrorMatches, `unit "foo" not found on this machine`)
 }
 
+func waitForResult(running <-chan *cmd.Context) (*cmd.Context, error) {
+	select {
+	case result := <-running:
+		return result, nil
+	case <-time.After(testing.ShortWait):
+		return nil, fmt.Errorf("timeout")
+	}
+}
+
+func startRunAsync(c *gc.C, params []string) <-chan *cmd.Context {
+	resultChannel := make(chan *cmd.Context)
+	go func() {
+		ctx, err := testing.RunCommand(c, &RunCommand{}, params)
+		c.Assert(err, jc.Satisfies, cmd.IsRcPassthroughError)
+		c.Assert(err, gc.ErrorMatches, "rc: 0")
+		resultChannel <- ctx
+		close(resultChannel)
+	}()
+	return resultChannel
+}
+
+func (s *RunTestSuite) TestNoContext(c *gc.C) {
+	s.PatchValue(&LockDir, c.MkDir())
+	s.PatchValue(&AgentDir, c.MkDir())
+
+	ctx, err := testing.RunCommand(c, &RunCommand{}, []string{"--no-context", "echo done"})
+	c.Assert(err, jc.Satisfies, cmd.IsRcPassthroughError)
+	c.Assert(err, gc.ErrorMatches, "rc: 0")
+	c.Assert(testing.Stdout(ctx), gc.Equals, "done\n")
+}
+
+func (s *RunTestSuite) TestNoContextAsync(c *gc.C) {
+	s.PatchValue(&LockDir, c.MkDir())
+	s.PatchValue(&AgentDir, c.MkDir())
+
+	channel := startRunAsync(c, []string{"--no-context", "echo done"})
+	ctx, err := waitForResult(channel)
+	c.Assert(err, gc.IsNil)
+	c.Assert(testing.Stdout(ctx), gc.Equals, "done\n")
+}
+
+func (s *RunTestSuite) TestNoContextWithLock(c *gc.C) {
+	s.PatchValue(&LockDir, c.MkDir())
+	s.PatchValue(&AgentDir, c.MkDir())
+	s.PatchValue(&fslock.LockWaitDelay, 10*time.Millisecond)
+
+	lock, err := getLock()
+	c.Assert(err, gc.IsNil)
+	lock.Lock("juju-run test")
+
+	channel := startRunAsync(c, []string{"--no-context", "echo done"})
+	ctx, err := waitForResult(channel)
+	c.Assert(err, gc.ErrorMatches, "timeout")
+
+	lock.Unlock()
+
+	ctx, err = waitForResult(channel)
+	c.Assert(err, gc.IsNil)
+	c.Assert(testing.Stdout(ctx), gc.Equals, "done\n")
+}
+
 func (s *RunTestSuite) TestMissingSocket(c *gc.C) {
 	s.PatchValue(&AgentDir, c.MkDir())
 	testAgentDir := filepath.Join(AgentDir, "foo")

=== modified file 'state/apiserver/client/run.go'
--- state/apiserver/client/run.go	2014-01-15 01:05:05 +0000
+++ state/apiserver/client/run.go	2014-01-15 01:05:06 +0000
@@ -106,7 +106,8 @@
 		if err != nil {
 			return results, err
 		}
-		execParam := remoteParamsForMachine(machine, run.Commands, run.Timeout)
+		command := fmt.Sprintf("juju-run --no-context %s", quotedCommands)
+		execParam := remoteParamsForMachine(machine, command, run.Timeout)
 		params = append(params, execParam)
 	}
 	return ParallelExecute(c.api.dataDir, params), nil
@@ -119,8 +120,10 @@
 		return params.RunResults{}, err
 	}
 	var params []*RemoteExec
+	quotedCommands := utils.ShQuote(run.Commands)
+	command := fmt.Sprintf("juju-run --no-context %s", quotedCommands)
 	for _, machine := range machines {
-		params = append(params, remoteParamsForMachine(machine, run.Commands, run.Timeout))
+		params = append(params, remoteParamsForMachine(machine, command, run.Timeout))
 	}
 	return ParallelExecute(c.api.dataDir, params), nil
 }

=== modified file 'state/apiserver/client/run_test.go'
--- state/apiserver/client/run_test.go	2014-01-15 01:05:05 +0000
+++ state/apiserver/client/run_test.go	2014-01-15 01:05:06 +0000
@@ -126,7 +126,7 @@
 		units:    []string{"magic/0"},
 		expected: []string{"magic/0", "magic/1"},
 	}} {
-		c.Log(fmt.Sprintf("%v: %s", i, test.message))
+		c.Logf("%v: %s", i, test.message)
 		result, err := client.GetAllUnitNames(s.State, test.units, test.services)
 		if test.error == "" {
 			c.Check(err, gc.IsNil)
@@ -230,7 +230,7 @@
 	for i := 0; i < 3; i++ {
 		expectedResults = append(expectedResults,
 			params.RunResult{
-				ExecResponse: exec.ExecResponse{Stdout: []byte("hostname\n")},
+				ExecResponse: exec.ExecResponse{Stdout: []byte("juju-run --no-context 'hostname'\n")},
 				MachineId:    fmt.Sprint(i),
 			})
 	}
@@ -264,7 +264,7 @@
 	c.Assert(results, gc.HasLen, 3)
 	expectedResults := []params.RunResult{
 		params.RunResult{
-			ExecResponse: exec.ExecResponse{Stdout: []byte("hostname\n")},
+			ExecResponse: exec.ExecResponse{Stdout: []byte("juju-run --no-context 'hostname'\n")},
 			MachineId:    "0",
 		},
 		params.RunResult{

=== removed file 'utils/fslock/export_test.go'
--- utils/fslock/export_test.go	2013-05-02 15:55:42 +0000
+++ utils/fslock/export_test.go	1970-01-01 00:00:00 +0000
@@ -1,15 +0,0 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
-
-package fslock
-
-import (
-	"time"
-)
-
-// SetLockWaitDelay updates the package lockWaitDelay for testing purposes.
-func SetLockWaitDelay(delay time.Duration) time.Duration {
-	oldValue := lockWaitDelay
-	lockWaitDelay = delay
-	return oldValue
-}

=== modified file 'utils/fslock/fslock.go'
--- utils/fslock/fslock.go	2013-10-10 20:58:54 +0000
+++ utils/fslock/fslock.go	2014-01-15 01:05:06 +0000
@@ -20,24 +20,26 @@
 	"regexp"
 	"time"
 
-	"launchpad.net/juju-core/log"
+	"launchpad.net/loggo"
+
 	"launchpad.net/juju-core/utils"
 )
 
 const (
-	// NameRegexp specifies the regular expression used to itentify valid lock names.
+	// NameRegexp specifies the regular expression used to identify valid lock names.
 	NameRegexp      = "^[a-z]+[a-z0-9.-]*$"
 	heldFilename    = "held"
 	messageFilename = "message"
 )
 
 var (
+	logger         = loggo.GetLogger("juju.utils.fslock")
 	ErrLockNotHeld = errors.New("lock not held")
 	ErrTimeout     = errors.New("lock timeout exceeded")
 
 	validName = regexp.MustCompile(NameRegexp)
 
-	lockWaitDelay = 1 * time.Second
+	LockWaitDelay = 1 * time.Second
 )
 
 type Lock struct {
@@ -141,10 +143,10 @@
 		}
 		currMessage := lock.Message()
 		if currMessage != heldMessage {
-			log.Infof("attempted lock failed %q, %s, currently held: %s", lock.name, message, currMessage)
+			logger.Infof("attempted lock failed %q, %s, currently held: %s", lock.name, message, currMessage)
 			heldMessage = currMessage
 		}
-		time.Sleep(lockWaitDelay)
+		time.Sleep(LockWaitDelay)
 	}
 }
 

=== modified file 'utils/fslock/fslock_test.go'
--- utils/fslock/fslock_test.go	2013-09-20 02:33:04 +0000
+++ utils/fslock/fslock_test.go	2014-01-15 01:05:06 +0000
@@ -10,7 +10,6 @@
 	"path"
 	"runtime"
 	"sync/atomic"
-	"testing"
 	"time"
 
 	gc "launchpad.net/gocheck"
@@ -21,10 +20,6 @@
 	"launchpad.net/juju-core/utils/fslock"
 )
 
-func Test(t *testing.T) {
-	gc.TestingT(t)
-}
-
 type fslockSuite struct {
 	testbase.LoggingSuite
 	lockDelay time.Duration
@@ -34,12 +29,7 @@
 
 func (s *fslockSuite) SetUpSuite(c *gc.C) {
 	s.LoggingSuite.SetUpSuite(c)
-	s.lockDelay = fslock.SetLockWaitDelay(1 * time.Millisecond)
-}
-
-func (s *fslockSuite) TearDownSuite(c *gc.C) {
-	fslock.SetLockWaitDelay(s.lockDelay)
-	s.LoggingSuite.TearDownSuite(c)
+	s.PatchValue(&fslock.LockWaitDelay, 1*time.Millisecond)
 }
 
 // This test also happens to test that locks can get created when the parent

=== added file 'utils/fslock/package_test.go'
--- utils/fslock/package_test.go	1970-01-01 00:00:00 +0000
+++ utils/fslock/package_test.go	2014-01-15 01:05:06 +0000
@@ -0,0 +1,24 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package fslock_test
+
+import (
+	"testing"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/testing/testbase"
+)
+
+func Test(t *testing.T) { gc.TestingT(t) }
+
+type Dependencies struct{}
+
+var _ = gc.Suite(&Dependencies{})
+
+func (*Dependencies) TestPackageDependencies(c *gc.C) {
+	// This test is to ensure we don't bring in dependencies without thinking.
+	c.Assert(testbase.FindJujuCoreImports(c, "launchpad.net/juju-core/utils/fslock"),
+		gc.DeepEquals, []string{"utils"})
+}