juju run should always use hook execution lock

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)

[thumper, 2014-01-15 01:06:48]

Please take a look.

[wallyworld, 2014-01-15 02:03:17]

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

[thumper, 2014-01-15 03:09:12]

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.