Implement the 'shared' subcommand for mmutex

Implement the 'shared' subcommand for mmutex

After this is done, I'll move onto incorporating the drain file so that
the lock scheduling is fairer for our purposes.

BUG=416072
Review-Url: https://codereview.chromium.org/2990863002
Committed: https://github.com/luci/luci-go/commit/b6f37562e3b7780198095124623d6e40fce9971d

[charliea (OOO until 10-5), 2017-07-27 14:08:48]

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

[charliea (OOO until 10-5), 2017-07-27 14:08:48]

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/exclusive...
File mmutex/cmd/mmutex/exclusive_test.go (right):

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/exclusive...
mmutex/cmd/mmutex/exclusive_test.go:93: RunExclusive([]string{"echo",
"should_succeed"}, 5*time.Millisecond, 0)
This is unrelated to this CL, but is hopefully a small enough correction that
you don't mind.

[iannucci, 2017-07-27 14:17:39]

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/exclusive...
File mmutex/cmd/mmutex/exclusive_test.go (right):

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/exclusive...
mmutex/cmd/mmutex/exclusive_test.go:71: Convey("RunExclusive times out if shared
lock isn't released", t, func(c C) {
fyi, you can nest these Convey blocks like this:

  Convey(`Make sure that X`, t, func() { // only need `c C` if you're doing So
or Convey statements in a goroutine
    // some common set up
    
    Convey(`when Y is set`, func() { 
      // can use common set up
    })
   
    Convey(`when Z is set`, func() {
      // can use common set up
    })
  })

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/exclusive...
mmutex/cmd/mmutex/exclusive_test.go:93: RunExclusive([]string{"echo",
"should_succeed"}, 5*time.Millisecond, 0)
On 2017/07/27 14:08:48, charliea wrote:
> This is unrelated to this CL, but is hopefully a small enough correction that
> you don't mind.

yep, don't mind

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go
File mmutex/cmd/mmutex/shared.go (right):

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go...
mmutex/cmd/mmutex/shared.go:30: UsageLine: "shared -- <command>",
"shared [options] -- <command>"

Doesn't matter too much, but is good to hint that the flags below should happen
before the `--`

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go...
mmutex/cmd/mmutex/shared.go:34: c.Flags.DurationVar(&c.fslockTimeout,
"fslock-timeout", 0, "Lock acquisition timeout")
what happens when the lock times out; can the caller tell?

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go...
mmutex/cmd/mmutex/shared.go:35: c.Flags.DurationVar(&c.fslockPollingInterval,
"fslock-polling-interval", 0, "Lock acquisition polling interval")
not sure this needs to be adjustable. I would also set the defaults for both of
these to be reasonable values.

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go...
mmutex/cmd/mmutex/shared.go:61: return cmd.Run()
hm... does this mean that we're masking the return value of the original
command?

[charliea (OOO until 10-5), 2017-07-27 21:00:15]

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/exclusive...
File mmutex/cmd/mmutex/exclusive_test.go (right):

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/exclusive...
mmutex/cmd/mmutex/exclusive_test.go:71: Convey("RunExclusive times out if shared
lock isn't released", t, func(c C) {
On 2017/07/27 14:17:38, iannucci wrote:
> fyi, you can nest these Convey blocks like this:
> 
>   Convey(`Make sure that X`, t, func() { // only need `c C` if you're doing So
> or Convey statements in a goroutine
>     // some common set up
>     
>     Convey(`when Y is set`, func() { 
>       // can use common set up
>     })
>    
>     Convey(`when Z is set`, func() {
>       // can use common set up
>     })
>   })

Good to know! For this specific case, I realized that the previous two cases
that share common set up code (hold an exclusive lock) should really be
condensed down into a single test case. In this test case, the setup is actually
slightly different (hold a SHARED lock), so I kept them all separate for now.

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go
File mmutex/cmd/mmutex/shared.go (right):

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go...
mmutex/cmd/mmutex/shared.go:30: UsageLine: "shared -- <command>",
On 2017/07/27 14:17:39, iannucci wrote:
> "shared [options] -- <command>"
> 
> Doesn't matter too much, but is good to hint that the flags below should
happen
> before the `--`

Done. (here and for exclusive)

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go...
mmutex/cmd/mmutex/shared.go:34: c.Flags.DurationVar(&c.fslockTimeout,
"fslock-timeout", 0, "Lock acquisition timeout")
On 2017/07/27 14:17:39, iannucci wrote:
> what happens when the lock times out; can the caller tell?

yep, they get the error: "fslock: lock is held"

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go...
mmutex/cmd/mmutex/shared.go:35: c.Flags.DurationVar(&c.fslockPollingInterval,
"fslock-polling-interval", 0, "Lock acquisition polling interval")
On 2017/07/27 14:17:38, iannucci wrote:
> not sure this needs to be adjustable. I would also set the defaults for both
of
> these to be reasonable values.

What I wasn't sure about is that it's very likely the polling interval will be
different for a command that's waiting for 1 second versus 1 minute versus 1
hour. However, doing something like setting the polling interval to timeout/1000
could actually cause a performance/power impact on the machine, which defeats
the purpose of us trying to minimize the perf impact on the machine. I figured,
by allowing these both to be adjustable, I'd allow people to responsibly make
the tradeoff between urgency of acquiring the lock and the potential performance
impact on the machine.

I did go ahead and set them to what seemed like reasonable values for our
purposes (2h timeout, 5s polling interval).

https://codereview.chromium.org/2990863002/diff/1/mmutex/cmd/mmutex/shared.go...
mmutex/cmd/mmutex/shared.go:61: return cmd.Run()
On 2017/07/27 14:17:38, iannucci wrote:
> hm... does this mean that we're masking the return value of the original
> command?

Yea, or at least not surfacing the return value. Return values are apparently
kind of a pain in the butt to get from go.

Went ahead and implemented it, though (for both and exclusive and shared)

[iannucci, 2017-07-28 12:45:10]

lgtm

https://codereview.chromium.org/2990863002/diff/20001/mmutex/cmd/mmutex/exclu...
File mmutex/cmd/mmutex/exclusive_test.go (right):

https://codereview.chromium.org/2990863002/diff/20001/mmutex/cmd/mmutex/exclu...
mmutex/cmd/mmutex/exclusive_test.go:59: Convey("RunExclusive times out if
exclusive lock isn't released", t, func(c C) {
you don't need the `c C` bit, unless you use the `c` variable directly

[charliea (OOO until 10-5), 2017-07-28 13:14:52]

The CQ bit was checked by charliea@chromium.org

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

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/2990863002/#ps40001
(title: "Remove (c C) from tests")

[commit-bot: I haz the power, 2017-07-28 13:15:00]

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-28 13:26:29]

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

[commit-bot: I haz the power, 2017-07-28 13:26:30]

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

[charliea (OOO until 10-5), 2017-07-31 16:34:01]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2017-07-31 16:34:02]

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/2990863002/#ps60001
(title: "Made echo cross-platform")

[commit-bot: I haz the power, 2017-07-31 16:34:09]

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-31 16:47:35]

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

[commit-bot: I haz the power, 2017-07-31 16:47:36]

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

[charliea (OOO until 10-5), 2017-07-31 16:55:20]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2017-07-31 16:55:21]

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/2990863002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2017-07-31 16:55:26]

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-31 17:12:56]

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

[commit-bot: I haz the power, 2017-07-31 17:12:57]

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

[charliea (OOO until 10-5), 2017-07-31 17:14:11]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2017-07-31 17:14:11]

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/2990863002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2017-07-31 17:14:18]

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-31 17:30:46]

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

[commit-bot: I haz the power, 2017-07-31 17:30:47]

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

[charliea (OOO until 10-5), 2017-07-31 17:43:36]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2017-07-31 17:43:36]

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/2990863002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2017-07-31 17:43:51]

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-31 17:53:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1501523016136330,
"parent_rev": "7875602af13ed009f3e609f698fb7aba3b1787df", "commit_rev":
"b6f37562e3b7780198095124623d6e40fce9971d"}

[commit-bot: I haz the power, 2017-07-31 17:53:39]

Description was changed from

==========
Implement the 'shared' subcommand for mmutex

After this is done, I'll move onto incorporating the drain file so that
the lock scheduling is fairer for our purposes.

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

to

==========
Implement the 'shared' subcommand for mmutex

After this is done, I'll move onto incorporating the drain file so that
the lock scheduling is fairer for our purposes.

BUG=416072

Review-Url: https://codereview.chromium.org/2990863002
Committed:
https://github.com/luci/luci-go/commit/b6f37562e3b7780198095124623d6e40fce9971d
==========

[commit-bot: I haz the power, 2017-07-31 17:53:40]

Committed patchset #7 (id:120001) as
https://github.com/luci/luci-go/commit/b6f37562e3b7780198095124623d6e40fce9971d