cmd/juju/machine: Enable GOMAXPROCS(NumCPU)

cmd/juju/machine: Enable GOMAXPROCS(NumCPU)

As part of my scale testing work, I noticed the API server is CPU bound
in many circumstances (logging in lots of machines at the same time).
However, we know that our test suite doesn't play nicely if we have
GOMAXPROCS>1. (Something about it running multiple tests concurrently
and tests that mutate global state.)

To avoid this affecting lots of code, I did it as a two phase approach.
We only will set GOMAXPROCS if

  1) GOMAXPROCS isn't already set in the environment. Since that would
     mean the go runtime would have already done its thing.
  2) We are running in response to the jujuDMain() (so it won't trigger
     in normal test suite operations.)
  3) We are running JobManageEnviron, so this will only affect the API
     server, and not all the other machines. (Even though it should be
     fine, if we were CPU bound there, we probably don't want to
     saturate machines that are running real workloads.)

I added tests that things get called properly when they are (and
aren't) enabled, but I didn't add a test that jujuDMain enables it
properly. I couldn't see an obvious way to hook into that code, so I
filed bug #1293383. Though if someone has a good way to test it, I'll
just do so.

This also has a change to how the MachineAgent works, in that it now
has a channel to let us know once it has called all of the StartWorker
steps and is going into the Wait() step. It made it possible to test
that we *didn't* call UseMultipleCPUs() for agents that aren't the API
server.

https://code.launchpad.net/~jameinel/juju-core/gomaxprocs-apiserver-1290841/+merge/211261

(do not edit description out of merge proposal)

[jameinel, 2014-03-17 08:34:48]

Please take a look.

[rog, 2014-03-17 10:02:15]

This does not LGTM - I think it could be considerably simpler - a suggestion
below.

https://codereview.appspot.com/76120044/diff/1/utils/gomaxprocs.go
File utils/gomaxprocs.go (right):

https://codereview.appspot.com/76120044/diff/1/utils/gomaxprocs.go#newcode12
utils/gomaxprocs.go:12: var mu = sync.Mutex{}
This does not fill me with joy.
If our tests really do not play nicely with GOMAXPROCS>1,
(they *really* should, but I understand that they
might not) there's a much easier way to override it - just set
the environment variable.

However, for the jujud tests specifically, the tests
pass fine for me when jujud sets GOMAXPROCS.

I suggest that we add just implement the following
function in cmd/jujud
(I don't think it's generally useful enough to deserve a place in utils,
but it can go here if you think it's worth it):

func UseMultipleCPUs() {
   if os.Getenv("GOMAXPROCS") == "" {
       runtime.GOMAXPROCS(runtime.NumCPU())
   }
}


and call it from the same place it's called currently.
To test it, you could declare it as a variable and
replace it to ensure it's actually called.

If we really find that it makes the tests flaky,
the relevant tests can patch $GOMAXPROCS.

[jameinel, 2014-03-19 09:43:02]

On 2014/03/17 10:02:15, rog wrote:
> This does not LGTM - I think it could be considerably simpler - a suggestion
> below.
> 
> https://codereview.appspot.com/76120044/diff/1/utils/gomaxprocs.go
> File utils/gomaxprocs.go (right):
> 
> https://codereview.appspot.com/76120044/diff/1/utils/gomaxprocs.go#newcode12
> utils/gomaxprocs.go:12: var mu = sync.Mutex{}
> This does not fill me with joy.
> If our tests really do not play nicely with GOMAXPROCS>1,
> (they *really* should, but I understand that they
> might not) there's a much easier way to override it - just set
> the environment variable.
> 
> However, for the jujud tests specifically, the tests
> pass fine for me when jujud sets GOMAXPROCS.
> 
> I suggest that we add just implement the following
> function in cmd/jujud
> (I don't think it's generally useful enough to deserve a place in utils,
> but it can go here if you think it's worth it):
> 
> func UseMultipleCPUs() {
>    if os.Getenv("GOMAXPROCS") == "" {
>        runtime.GOMAXPROCS(runtime.NumCPU())
>    }
> }
> 
> 
> and call it from the same place it's called currently.
> To test it, you could declare it as a variable and
> replace it to ensure it's actually called.
> 
> If we really find that it makes the tests flaky,
> the relevant tests can patch $GOMAXPROCS.

The new code is simplified to just always call GOMAXPROCS without the Enable
step.

[jameinel, 2014-03-19 09:45:27]

Please take a look.

https://codereview.appspot.com/76120044/diff/1/utils/gomaxprocs.go
File utils/gomaxprocs.go (right):

https://codereview.appspot.com/76120044/diff/1/utils/gomaxprocs.go#newcode12
utils/gomaxprocs.go:12: var mu = sync.Mutex{}
On 2014/03/17 10:02:16, rog wrote:
> This does not fill me with joy.
> If our tests really do not play nicely with GOMAXPROCS>1,
> (they *really* should, but I understand that they
> might not) there's a much easier way to override it - just set
> the environment variable.
> 
> However, for the jujud tests specifically, the tests
> pass fine for me when jujud sets GOMAXPROCS.
> 
> I suggest that we add just implement the following
> function in cmd/jujud
> (I don't think it's generally useful enough to deserve a place in utils,
> but it can go here if you think it's worth it):
> 
> func UseMultipleCPUs() {
>    if os.Getenv("GOMAXPROCS") == "" {
>        runtime.GOMAXPROCS(runtime.NumCPU())
>    }
> }
> 
> 
> and call it from the same place it's called currently.
> To test it, you could declare it as a variable and
> replace it to ensure it's actually called.
> 
> If we really find that it makes the tests flaky,
> the relevant tests can patch $GOMAXPROCS.

Done.

[jameinel, 2014-03-19 10:25:51]

Please take a look.

[rog, 2014-03-19 10:50:28]

I like that much better, thanks.
Some further simplification suggestions below.

https://codereview.appspot.com/76120044/diff/20001/cmd/jujud/main.go
File cmd/jujud/main.go (right):

https://codereview.appspot.com/76120044/diff/20001/cmd/jujud/main.go#newcode1
cmd/jujud/main.go:1: // Copyright 2012-2014 Canonical Ltd.
I think it's reasonable to leave the copyright dates alone. That's certainly the
explicit convention in Go core, and Juju de facto.

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:75: workersStarted  chan struct{}
Perhaps some documentation for this might be appropriate, given
that it's only here for tests.

Or a method:


// WorkersStarted returns a channel that's closed when
// the top level workers have been started. This is
// provided for testing purposes.
func (a *MachineAgent) WorkersStarted() <-chan struct{}
    return a.workersStarted
}

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:345: useMultipleCPUs()
If you took logging out of the utils package, you could add a log
message here instead:

   logger.Debugf("GOMAXPROCS set to %d", runtime.GOMAXPROCS(0))

That's probably sufficient for diagnosing gomaxprocs-related problems.

https://codereview.appspot.com/76120044/diff/40001/utils/export_test.go
File utils/export_test.go (right):

https://codereview.appspot.com/76120044/diff/40001/utils/export_test.go#newcode9
utils/export_test.go:9: func OverrideCPUFuncs(maxprocsfunc func(int) int,
numCPUFunc func() int) func() {
Assuming we go with this, I'd do:

var (
    GoMaxProcs = &gomaxprocs
    NumCPU = &numcpu
)

then we can use PatchValue in the tests.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go
File utils/gomaxprocs.go (right):

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode11
utils/gomaxprocs.go:11: var gomaxprocs = runtime.GOMAXPROCS
This seems a bit of overkill for such a function with such simple logic.
I'd just test that it does the right thing (even though it's not a full
test on a machine with a single cpu). It'll fail on any machine
with more than one CPU, which seems adequate to me. Is that overly
pragmatic?

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode16
utils/gomaxprocs.go:16: // variable if it is set.
// UseMultipleCPUs sets GOMAXPROCS to the number of CPU
// cores unless overridden by the GOMAXPROCS environment
// variable.

?

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode20
utils/gomaxprocs.go:20: logger.Debugf("GOMAXPROCS already set in environment to
%q, %d internally",
I'm not that keen on utils functions producing log messages (and since they're
only at Debug level, they can't really be that important). Honestly,
I'd just do:

func UseMultipleCPUs() {
     if os.Getenv("GOMAXPROCS") == "" {
         runtime.GOMAXPROCS(runtime.NumCPU())
     }
}

and be done with it. Alternatively, if it lived in cmd/jujud then logging might
be more appropriate, as it can be more location-specific there.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs_test.go
File utils/gomaxprocs_test.go (right):

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs_test.go#n...
utils/gomaxprocs_test.go:28: maxprocsfunc := func(n int) int {
or:

maxProcsFunc := func(n int) int {
    if n != 0 {
        s.setMaxProcs = n
    }
    return 1
}

then dispense with the channel and just
assert on s.setMaxProcs directly.

[jameinel, 2014-03-20 06:48:46]

Please take a look.

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:75: workersStarted  chan struct{}
On 2014/03/19 10:50:28, rog wrote:
> Perhaps some documentation for this might be appropriate, given
> that it's only here for tests.
> 
> Or a method:
> 
> 
> // WorkersStarted returns a channel that's closed when
> // the top level workers have been started. This is
> // provided for testing purposes.
> func (a *MachineAgent) WorkersStarted() <-chan struct{}
>     return a.workersStarted
> }
> 

Done.

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:345: useMultipleCPUs()
On 2014/03/19 10:50:28, rog wrote:
> If you took logging out of the utils package, you could add a log
> message here instead:
> 
>    logger.Debugf("GOMAXPROCS set to %d", runtime.GOMAXPROCS(0))
> 
> That's probably sufficient for diagnosing gomaxprocs-related problems.

That explicitly misses out on distinguishing between GOMAXPROCS was set in the
environment vs GOMAXPROCS was set from NumCPU(). Which (IMO) is actually a
useful thing to distinguish.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go
File utils/gomaxprocs.go (right):

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode11
utils/gomaxprocs.go:11: var gomaxprocs = runtime.GOMAXPROCS
On 2014/03/19 10:50:28, rog wrote:
> This seems a bit of overkill for such a function with such simple logic.
> I'd just test that it does the right thing (even though it's not a full
> test on a machine with a single cpu). It'll fail on any machine
> with more than one CPU, which seems adequate to me. Is that overly
> pragmatic?

Unless someone (we?) start setting GOMAXPROCS in the environment for regular
running of tests, etc.

I find it particularly useful to be able to write tests that are abstracted from
the runtime.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode16
utils/gomaxprocs.go:16: // variable if it is set.
On 2014/03/19 10:50:28, rog wrote:
> // UseMultipleCPUs sets GOMAXPROCS to the number of CPU
> // cores unless overridden by the GOMAXPROCS environment
> // variable.
> 
> ?

Done.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode20
utils/gomaxprocs.go:20: logger.Debugf("GOMAXPROCS already set in environment to
%q, %d internally",
On 2014/03/19 10:50:28, rog wrote:
> I'm not that keen on utils functions producing log messages (and since they're
> only at Debug level, they can't really be that important). Honestly,
> I'd just do:
> 
> func UseMultipleCPUs() {
>      if os.Getenv("GOMAXPROCS") == "" {
>          runtime.GOMAXPROCS(runtime.NumCPU())
>      }
> }
> 
> and be done with it. Alternatively, if it lived in cmd/jujud then logging
might
> be more appropriate, as it can be more location-specific there. 

We already have logging in utils (I didn't have to create 'logger'), so I'm
pretty sure its fine as a pattern.

I do think it is useful to be logged, and I think any code that does start
changing GOMAXPROCS at runtime should be logging that fact. So I think it is
perfectly appropriate to log here. 

Given we stopped doing the Enable vs set it, and set it always, we could have
just embedded it, but if we do grow more places that want to take advantage of
>1 CPU, I'd rather have that logic centralized.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs_test.go
File utils/gomaxprocs_test.go (right):

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs_test.go#n...
utils/gomaxprocs_test.go:28: maxprocsfunc := func(n int) int {
On 2014/03/19 10:50:28, rog wrote:
> or:
> 
> maxProcsFunc := func(n int) int {
>     if n != 0 {
>         s.setMaxProcs = n
>     }
>     return 1
> }
> 
> then dispense with the channel and just
> assert on s.setMaxProcs directly.

Done.

[rog, 2014-03-20 08:43:05]

LGTM, your call.

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:345: useMultipleCPUs()
On 2014/03/20 06:48:46, jameinel wrote:
> On 2014/03/19 10:50:28, rog wrote:
> > If you took logging out of the utils package, you could add a log
> > message here instead:
> > 
> >    logger.Debugf("GOMAXPROCS set to %d", runtime.GOMAXPROCS(0))
> > 
> > That's probably sufficient for diagnosing gomaxprocs-related problems.
> 
> That explicitly misses out on distinguishing between GOMAXPROCS was set in the
> environment vs GOMAXPROCS was set from NumCPU(). Which (IMO) is actually a
> useful thing to distinguish.

I'm struggling to think of a scenario where we'd really care, but fair enough.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go
File utils/gomaxprocs.go (right):

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode11
utils/gomaxprocs.go:11: var gomaxprocs = runtime.GOMAXPROCS
> Unless someone (we?) start setting GOMAXPROCS in the environment for
> regular running of tests, etc.

It's trivial to patch the GOMAXPROCS environment variable in the test.

> I find it particularly useful to be able to write tests that are
> abstracted from the runtime.

Fair enough. I like seeing code that's as straightforwardly obvious as possible,
but I appreciate that it's a trade-off.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode20
utils/gomaxprocs.go:20: logger.Debugf("GOMAXPROCS already set in environment to
%q, %d internally",
On 2014/03/20 06:48:46, jameinel wrote:
> On 2014/03/19 10:50:28, rog wrote:
> > I'm not that keen on utils functions producing log messages (and since
they're
> > only at Debug level, they can't really be that important). Honestly,
> > I'd just do:
> > 
> > func UseMultipleCPUs() {
> >      if os.Getenv("GOMAXPROCS") == "" {
> >          runtime.GOMAXPROCS(runtime.NumCPU())
> >      }
> > }
> > 
> > and be done with it. Alternatively, if it lived in cmd/jujud then logging
> might
> > be more appropriate, as it can be more location-specific there. 
> 
> We already have logging in utils (I didn't have to create 'logger'), so I'm
> pretty sure its fine as a pattern.
> 
> I do think it is useful to be logged, and I think any code that does start
> changing GOMAXPROCS at runtime should be logging that fact. So I think it is
> perfectly appropriate to log here. 

There is only one other place in utils that logs messages, and that
place (GetAddressForInterface) is a mistake (it should return an annotated
error rather than logging).

> Given we stopped doing the Enable vs set it, and set it always, we could have
> just embedded it, but if we do grow more places that want to take advantage of
> >1 CPU, I'd rather have that logic centralized.

FWIW, no non-main package should be changing GOMAXPROCS at all,
which is why IMHO it's a mistake to put it in utils at all. It might fit
reasonably inside cmd, I guess.

https://codereview.appspot.com/76120044/diff/60001/utils/gomaxprocs.go
File utils/gomaxprocs.go (right):

https://codereview.appspot.com/76120044/diff/60001/utils/gomaxprocs.go#newcode12
utils/gomaxprocs.go:12: var numcpu = runtime.NumCPU
Trivial point that I forgot to mention before:
we'd conventionally spell this numCPU.

I'd be tempted to use goMaxProcs too, but I see
the rationale for all lower case there too.