|
|
|
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)
Patch Set 1 #
Total comments: 2
Patch Set 2 : cmd/juju/machine: Enable GOMAXPROCS(NumCPU) #
Total comments: 1
Patch Set 3 : cmd/juju/machine: Enable GOMAXPROCS(NumCPU) #
Total comments: 16
Patch Set 4 : cmd/juju/machine: Enable GOMAXPROCS(NumCPU) #
Total comments: 1
Total messages: 8
|
jameinel
Please take a look.
|
11 years, 10 months ago (2014年03月17日 08:34:48 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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.
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.
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.
Please take a look.
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.
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.
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.