Allow dummy environs to use zookeeper state

Allow dummy environs to use zookeeper state

As in python, we don't want to have to bring up and tear down a zookeeper
server for every test we run against a dummy environment, so this CL
provides a mechanism for sharing a single zookeeper across multiple tests
against "different" environments.

This is done with a SetZookeeper func in the dummy package, which allows
the environ's StateInfo method to return a *state.Info pointing at that
zookeeper. Assuming the "zookeeper" config setting is set, and "isBroken"
is not, Bootstrap will initialize state and Reset will clear it; when
"zookeeper" is set to false, it should act entirely as before.

https://code.launchpad.net/~fwereade/juju/go-dummy-environ-state/+merge/107795

(do not edit description out of merge proposal)

[fwereade, 2012-05-29 14:20:45]

Please take a look.

[rog, 2012-05-29 15:35:40]

LGTM modulo the below, and a slight glimmer of a feeling that maybe
ResetZkServer isn't worth it.

https://codereview.appspot.com/6244060/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6244060/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:250: if state == nil {
i think we should guard against accidental zk sharing:

if cfg.zookeeper && len(p.state) != 0 {
    panic("cannot have more than one dummy environ using zookeeper at the same
time")
}

[niemeyer, 2012-05-29 18:16:17]

https://codereview.appspot.com/6244060/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6244060/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:40: func SetZookeeper(srv *zookeeper.Server) {
Dummy environments are now named (see environConfig.Open below). Should this
function also take a name and handle a map of name=>zk rather than a singleton?

https://codereview.appspot.com/6244060/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:250: if state == nil {
On 2012/05/29 15:35:40, rog wrote:
> i think we should guard against accidental zk sharing:

Agreed, but I'd prefer to do that by assigning zk's to specific environment
names rather than arbitrarily failing on the second try.

[fwereade, 2012-05-30 08:52:46]

Please take a look.

https://codereview.appspot.com/6244060/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6244060/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:40: func SetZookeeper(srv *zookeeper.Server) {
On 2012/05/29 18:16:17, niemeyer wrote:
> Dummy environments are now named (see environConfig.Open below). Should this
> function also take a name and handle a map of name=>zk rather than a
singleton?

Done.

[fwereade, 2012-05-30 10:18:22]

Please take a look.

[rog, 2012-05-30 10:40:39]

LGTM

https://codereview.appspot.com/6244060/diff/7001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6244060/diff/7001/environs/dummy/environs.go#n...
environs/dummy/environs.go:252: panic(errors.New("cannot share a zookeeper
between two dummy environs"))
no particular need for the errors.New here. a string is a legitimate panic
argument.

[niemeyer, 2012-05-30 22:49:44]

LGTM with some details.

https://codereview.appspot.com/6244060/diff/7001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6244060/diff/7001/environs/dummy/environs.go#n...
environs/dummy/environs.go:48: panic(errors.New("SetZookeeper not called"))
errors.New not needed here too.

https://codereview.appspot.com/6244060/diff/7001/environs/dummy/environs.go#n...
environs/dummy/environs.go:251: if cfg.zookeeper && len(p.state) != 0 {
This is great, thanks.

https://codereview.appspot.com/6244060/diff/7001/testing/zk.go
File testing/zk.go (right):

https://codereview.appspot.com/6244060/diff/7001/testing/zk.go#newcode63
testing/zk.go:63: }
defer zk.Close()?

[fwereade, 2012-05-31 07:37:12]

*** Submitted:

Allow dummy environs to use zookeeper state

As in python, we don't want to have to bring up and tear down a zookeeper
server for every test we run against a dummy environment, so this CL
provides a mechanism for sharing a single zookeeper across multiple tests
against "different" environments.

This is done with a SetZookeeper func in the dummy package, which allows
the environ's StateInfo method to return a *state.Info pointing at that
zookeeper. Assuming the "zookeeper" config setting is set, and "isBroken"
is not, Bootstrap will initialize state and Reset will clear it; when
"zookeeper" is set to false, it should act entirely as before.

R=rog, niemeyer
CC=
https://codereview.appspot.com/6244060

https://codereview.appspot.com/6244060/diff/7001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6244060/diff/7001/environs/dummy/environs.go#n...
environs/dummy/environs.go:48: panic(errors.New("SetZookeeper not called"))
On 2012/05/30 22:49:44, niemeyer wrote:
> errors.New not needed here too.

Done.

https://codereview.appspot.com/6244060/diff/7001/environs/dummy/environs.go#n...
environs/dummy/environs.go:252: panic(errors.New("cannot share a zookeeper
between two dummy environs"))
On 2012/05/30 10:40:39, rog wrote:
> no particular need for the errors.New here. a string is a legitimate panic
> argument.

Done.

https://codereview.appspot.com/6244060/diff/7001/testing/zk.go
File testing/zk.go (right):

https://codereview.appspot.com/6244060/diff/7001/testing/zk.go#newcode63
testing/zk.go:63: }
On 2012/05/30 22:49:44, niemeyer wrote:
> defer zk.Close()?

Done.