Flesh out the mmutex binary wrapper and test for existence of lockfile

Flesh out the mmutex binary wrapper and test for existence of lockfile

The logic in this CL is uninteresting, but I mostly want to make sure
that the wrapper, directory structure, and test harness are set up
correctly before getting too much further.

BUG=416072
Review-Url: https://codereview.chromium.org/2980603003
Committed: https://github.com/luci/luci-go/commit/6553a52aeef59501c0b106ff07113b83ad6d179a

[charliea (OOO until 10-5), 2017-07-11 20:02:09]

charliea@chromium.org changed reviewers:
+ iannucci@chromium.org

[iannucci, 2017-07-11 20:52:15]

lgtm as a step

https://codereview.chromium.org/2980603003/diff/20001/mmutex/cmd/mmutex/main.go
File mmutex/cmd/mmutex/main.go (right):

https://codereview.chromium.org/2980603003/diff/20001/mmutex/cmd/mmutex/main....
mmutex/cmd/mmutex/main.go:19: var isExclusive = flag.Bool("x", false, "Runs the
command after acquiring an exclusive lock")
we will probably want to convert this to use the luci-go cli library (like
https://chromium.googlesource.com/infra/infra/+/master/go/src/infra/tools/led...).

Then the syntax could look like:

  mmutex shared -- program args
  mmutex exclusive -- program args

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock.go
File mmutex/lib/flock.go (right):

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock.go#new...
mmutex/lib/flock.go:14: if _, err := os.Stat(path); os.IsNotExist(err) {
tbh, I would probably not stat, I'd just bail on ENOENT from the actual lock
invocation

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock.go#new...
mmutex/lib/flock.go:18: // TODO(charliea): Acquire the actual lock.
can also `panic("implement lock acquisition")`

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock_test.go
File mmutex/lib/flock_test.go (right):

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock_test.g...
mmutex/lib/flock_test.go:12: "testing"
imports should be separated basically like:

  <stdlib>

  <golang.org/x/... imports>

  <third-party>

  <luci-go>

  <app imports>

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock_test.g...
mmutex/lib/flock_test.go:24: So(AcquireExclusiveLock(path).Error(), ShouldEqual,
fmt.Sprintf("cannot acquire lock on %s because file does not exist", path))
add an import:

  . "github.com/luci/luci-go/common/testing/assertions"

Then:

  So(expression_resulting_in_error, ShouldErrLike, "some text in the error")

[charliea (OOO until 10-5), 2017-07-11 23:26:16]

PTAL

I figure there significant enough changes (where I'm not 100% sure what I'm
doing) that it's worth having another set of eyes on it.

https://codereview.chromium.org/2980603003/diff/20001/mmutex/cmd/mmutex/main.go
File mmutex/cmd/mmutex/main.go (right):

https://codereview.chromium.org/2980603003/diff/20001/mmutex/cmd/mmutex/main....
mmutex/cmd/mmutex/main.go:19: var isExclusive = flag.Bool("x", false, "Runs the
command after acquiring an exclusive lock")
On 2017/07/11 20:52:14, iannucci wrote:
> we will probably want to convert this to use the luci-go cli library (like
>
https://chromium.googlesource.com/infra/infra/+/master/go/src/infra/tools/led...).
> 
> Then the syntax could look like:
> 
>   mmutex shared -- program args
>   mmutex exclusive -- program args

Done. Started with just "exclusive" for now, given that it's the only one with a
stub of an implementation.

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock.go
File mmutex/lib/flock.go (right):

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock.go#new...
mmutex/lib/flock.go:14: if _, err := os.Stat(path); os.IsNotExist(err) {
On 2017/07/11 20:52:15, iannucci wrote:
> tbh, I would probably not stat, I'd just bail on ENOENT from the actual lock
> invocation

The actual lock implementation accepts an already-open file handle rather than a
path, so I instead bailed on the OpenFile failure (where the ENOENT first pops
up)

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock.go#new...
mmutex/lib/flock.go:18: // TODO(charliea): Acquire the actual lock.
On 2017/07/11 20:52:14, iannucci wrote:
> can also `panic("implement lock acquisition")`

Done.

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock_test.go
File mmutex/lib/flock_test.go (right):

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock_test.g...
mmutex/lib/flock_test.go:12: "testing"
On 2017/07/11 20:52:15, iannucci wrote:
> imports should be separated basically like:
> 
>   <stdlib>
> 
>   <golang.org/x/... imports>
> 
>   <third-party>
> 
>   <luci-go>
> 
>   <app imports>

Done.

https://codereview.chromium.org/2980603003/diff/20001/mmutex/lib/flock_test.g...
mmutex/lib/flock_test.go:24: So(AcquireExclusiveLock(path).Error(), ShouldEqual,
fmt.Sprintf("cannot acquire lock on %s because file does not exist", path))
On 2017/07/11 20:52:15, iannucci wrote:
> add an import:
> 
>   . "github.com/luci/luci-go/common/testing/assertions"
> 
> Then:
> 
>   So(expression_resulting_in_error, ShouldErrLike, "some text in the error")

Done.

[iannucci, 2017-07-12 01:34:59]

lgtm :)

https://codereview.chromium.org/2980603003/diff/40001/mmutex/cmd/mmutex/main.go
File mmutex/cmd/mmutex/main.go (right):

https://codereview.chromium.org/2980603003/diff/40001/mmutex/cmd/mmutex/main....
mmutex/cmd/mmutex/main.go:46: var cmdExclusiveLock = &subcommands.Command{
I would recommend putting each subcommand in its own file (it gets unruly
otherwise)

https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock.go
File mmutex/lib/flock.go (right):

https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock.go#new...
mmutex/lib/flock.go:16: return fmt.Errorf("cannot acquire lock on %s because
file does not exist", path)
recommend use of luci-go/common/errors (they collect stack traces and other
nifty things):

  errors.Reason("cannot acquire lock on %s because file does not exist",
path).Err()

https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock_test.go
File mmutex/lib/flock_test.go (right):

https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock_test.g...
mmutex/lib/flock_test.go:27: So(AcquireExclusiveLock(path), ShouldErrLike,
fmt.Sprintf("cannot acquire lock on %s because file does not exist", path))
tbh, "file does not exist" is enough of a substring for this test :)

[charliea (OOO until 10-5), 2017-07-12 02:15:30]

On 2017/07/12 01:34:59, iannucci wrote:
> lgtm :)
> 
>
https://codereview.chromium.org/2980603003/diff/40001/mmutex/cmd/mmutex/main.go
> File mmutex/cmd/mmutex/main.go (right):
> 
>
https://codereview.chromium.org/2980603003/diff/40001/mmutex/cmd/mmutex/main....
> mmutex/cmd/mmutex/main.go:46: var cmdExclusiveLock = &subcommands.Command{
> I would recommend putting each subcommand in its own file (it gets unruly
> otherwise)
> 
> https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock.go
> File mmutex/lib/flock.go (right):
> 
>
https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock.go#new...
> mmutex/lib/flock.go:16: return fmt.Errorf("cannot acquire lock on %s because
> file does not exist", path)
> recommend use of luci-go/common/errors (they collect stack traces and other
> nifty things):
> 
>   errors.Reason("cannot acquire lock on %s because file does not exist",
> path).Err()
> 
> https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock_test.go
> File mmutex/lib/flock_test.go (right):
> 
>
https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock_test.g...
> mmutex/lib/flock_test.go:27: So(AcquireExclusiveLock(path), ShouldErrLike,
> fmt.Sprintf("cannot acquire lock on %s because file does not exist", path))
> tbh, "file does not exist" is enough of a substring for this test :)

Great tips. Thanks Robbie!

[charliea (OOO until 10-5), 2017-07-12 02:17:17]

https://codereview.chromium.org/2980603003/diff/40001/mmutex/cmd/mmutex/main.go
File mmutex/cmd/mmutex/main.go (right):

https://codereview.chromium.org/2980603003/diff/40001/mmutex/cmd/mmutex/main....
mmutex/cmd/mmutex/main.go:46: var cmdExclusiveLock = &subcommands.Command{
On 2017/07/12 01:34:59, iannucci wrote:
> I would recommend putting each subcommand in its own file (it gets unruly
> otherwise)

Done.

https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock.go
File mmutex/lib/flock.go (right):

https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock.go#new...
mmutex/lib/flock.go:16: return fmt.Errorf("cannot acquire lock on %s because
file does not exist", path)
On 2017/07/12 01:34:59, iannucci wrote:
> recommend use of luci-go/common/errors (they collect stack traces and other
> nifty things):
> 
>   errors.Reason("cannot acquire lock on %s because file does not exist",
> path).Err()

Done.

https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock_test.go
File mmutex/lib/flock_test.go (right):

https://codereview.chromium.org/2980603003/diff/40001/mmutex/lib/flock_test.g...
mmutex/lib/flock_test.go:27: So(AcquireExclusiveLock(path), ShouldErrLike,
fmt.Sprintf("cannot acquire lock on %s because file does not exist", path))
On 2017/07/12 01:34:59, iannucci wrote:
> tbh, "file does not exist" is enough of a substring for this test :)

I went with "lock file does not exist"

[charliea (OOO until 10-5), 2017-07-12 02:17:58]

Patchset #4 (id:60001) has been deleted

[charliea (OOO until 10-5), 2017-07-12 02:18:55]

Patchset #4 (id:80001) has been deleted

[charliea (OOO until 10-5), 2017-07-12 02:19:39]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2017-07-12 02:19:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2980603003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2017-07-12 02:19:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-12 02:32:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-12 02:32:14]

Try jobs failed on following builders:
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/374d7016fbcffd10)

[charliea (OOO until 10-5), 2017-07-12 13:53:16]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2017-07-12 13:53:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2980603003/#ps120001
(title: "Change tempdir directory to work on Windows")

[commit-bot: I haz the power, 2017-07-12 13:53:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-12 13:59:14]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499867596921530,
"parent_rev": "3f5ae6c4a1c17663c8113708a912fea6d8b98781", "commit_rev":
"6553a52aeef59501c0b106ff07113b83ad6d179a"}

[commit-bot: I haz the power, 2017-07-12 13:59:19]

Description was changed from

==========
Flesh out the mmutex binary wrapper and test for existence of lockfile

The logic in this CL is uninteresting, but I mostly want to make sure
that the wrapper, directory structure, and test harness are set up
correctly before getting too much further.

BUG=416072
==========

to

==========
Flesh out the mmutex binary wrapper and test for existence of lockfile

The logic in this CL is uninteresting, but I mostly want to make sure
that the wrapper, directory structure, and test harness are set up
correctly before getting too much further.

BUG=416072

Review-Url: https://codereview.chromium.org/2980603003
Committed:
https://github.com/luci/luci-go/commit/6553a52aeef59501c0b106ff07113b83ad6d179a
==========

[commit-bot: I haz the power, 2017-07-12 13:59:20]

Committed patchset #5 (id:120001) as
https://github.com/luci/luci-go/commit/6553a52aeef59501c0b106ff07113b83ad6d179a