all: use MgoSuite rather than ZkSuite

all: use MgoSuite rather than ZkSuite

Also fix a test in worker/machiner.

https://code.launchpad.net/~rogpeppe/juju-core/078-use-mgo/+merge/125671

(do not edit description out of merge proposal)

[rog, 2012-09-21 12:09:35]

Please take a look.

[TheMue, 2012-09-21 13:11:57]

Marked some ZK references.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap.go#newco...
cmd/jujud/bootstrap.go:25: stateInfoVar(f, &c.StateInfo, "state-servers",
[]string{"127.0.0.1:2181"}, "address of zookeeper to initialize")
"address of state server to initialize"

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:37: args = append(args, "--state-servers",
"zk1:2181,zk2:2181")
zk1, zk2?

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:40: c.Assert(cmd.StateInfo.Addrs, DeepEquals,
[]string{"zk1:2181", "zk2:2181"})
Ditto.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go
File cmd/jujud/main_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go#newco...
cmd/jujud/main_test.go:50: msgz := `invalid value "localhost:2181,zk" for flag
--state-servers: "zk" is not a valid zookeeper address`
zk and "zk" is not a valid zookeeper address should be changed too.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go#newco...
cmd/jujud/main_test.go:53: checkMessage(c, msgz, cmd, "--state-servers",
"localhost:2181,zk")
Again zk.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go#newco...
cmd/jujud/main_test.go:59: "--state-servers", "zk:2181",
Ditto. Also below.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/util_test.go
File cmd/jujud/util_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/util_test.go#newco...
cmd/jujud/util_test.go:26: args = append(args, "--state-servers",
"zk1:2181,zk2:2181")
Again zk.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/util_test.go#newco...
cmd/jujud/util_test.go:45: "--state-servers", "zk:2181",
Ditto.

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:133: " --state-servers localhost"+zkPortSuffix+
zkPortSuffix should be sPortSuffix or ssPortSuffix.

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit_test.go:96: t.checkScripts(c, "jujud provisioning
--state-servers 'localhost"+cloudinit.ZkPortSuffix+"'")
More ZK references (ZkPortSuffix).

[dave_cheney.net, 2012-09-21 13:12:54]

Thank you. A few comments, non of them major.

https://codereview.appspot.com/6553049/diff/6041/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/6553049/diff/6041/charm/url.go#newcode207
charm/url.go:207: // in a file path.  ASCII letters, ASCII digits, dot and dash
stay the
One space following the period.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/agent.go#newcode23
cmd/jujud/agent.go:23: // Set splits the comma-separated list of server
addresses and stores
nit: state server

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/agent.go#newcode29
cmd/jujud/agent.go:29: return fmt.Errorf("%q is not a valid server address",
addr)
I'm sure niemeyer will suggest state server here.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap.go#newco...
cmd/jujud/bootstrap.go:25: stateInfoVar(f, &c.StateInfo, "state-servers",
[]string{"127.0.0.1:2181"}, "address of zookeeper to initialize")
s/zookeeper/state server

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go
File cmd/jujud/main_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go#newco...
cmd/jujud/main_test.go:50: msgz := `invalid value "localhost:2181,zk" for flag
--state-servers: "zk" is not a valid zookeeper address`
,zk ?

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go#newco...
cmd/jujud/main_test.go:59: "--state-servers", "zk:2181",
lots of zk:2181 turds here

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/util_test.go
File cmd/jujud/util_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/util_test.go#newco...
cmd/jujud/util_test.go:26: args = append(args, "--state-servers",
"zk1:2181,zk2:2181")
ditto

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:133: " --state-servers localhost"+zkPortSuffix+
zkPortSuffix should be removed.

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit_test.go:100: t.checkScripts(c, "jujud machine
--state-servers 'localhost"+cloudinit.ZkPortSuffix+"' .* --machine-id [0-9]+")
kill ZkPortSuffix with fire.

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/ec2.go#newcode22
environs/ec2/ec2.go:22: const mgoPort = 2181
I don't think this is the mgo port

https://codereview.appspot.com/6553049/diff/6041/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6553049/diff/6041/juju/testing/conn.go#newcode141
juju/testing/conn.go:141: func (s *JujuConnSuite) StateInfo(c *C) *state.Info {
Delete this function, it shadows an identical function from ConnSuite.

[niemeyer, 2012-09-21 13:17:52]

LGTM with the trivials, but please wait for David.

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/ec2.go#newcode22
environs/ec2/ec2.go:22: const mgoPort = 2181
s/2181/37017/

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/ec2.go#newcode241
environs/ec2/ec2.go:241: MgoInstances: []string{inst.Id()},
s/Mgo/State/

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/state.go
File environs/ec2/state.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/state.go#newcode13
environs/ec2/state.go:13: MgoInstances []string `yaml:"mgo-instances"`
s/mgo-/state-/

[rog, 2012-09-21 13:38:10]

Please take a look.

https://codereview.appspot.com/6553049/diff/6041/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/6553049/diff/6041/charm/url.go#newcode207
charm/url.go:207: // in a file path.  ASCII letters, ASCII digits, dot and dash
stay the
On 2012/09/21 13:12:54, dfc wrote:
> One space following the period.

not done, as discussed in person.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/agent.go#newcode23
cmd/jujud/agent.go:23: // Set splits the comma-separated list of server
addresses and stores
On 2012/09/21 13:12:54, dfc wrote:
> nit: state server

Done.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/agent.go#newcode29
cmd/jujud/agent.go:29: return fmt.Errorf("%q is not a valid server address",
addr)
On 2012/09/21 13:12:54, dfc wrote:
> I'm sure niemeyer will suggest state server here.

Done.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap.go#newco...
cmd/jujud/bootstrap.go:25: stateInfoVar(f, &c.StateInfo, "state-servers",
[]string{"127.0.0.1:2181"}, "address of zookeeper to initialize")
On 2012/09/21 13:12:54, dfc wrote:
> s/zookeeper/state server

Done.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:37: args = append(args, "--state-servers",
"zk1:2181,zk2:2181")
On 2012/09/21 13:11:57, TheMue wrote:
> zk1, zk2?

Done.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:40: c.Assert(cmd.StateInfo.Addrs, DeepEquals,
[]string{"zk1:2181", "zk2:2181"})
On 2012/09/21 13:11:57, TheMue wrote:
> Ditto.

Done.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go
File cmd/jujud/main_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go#newco...
cmd/jujud/main_test.go:50: msgz := `invalid value "localhost:2181,zk" for flag
--state-servers: "zk" is not a valid zookeeper address`
On 2012/09/21 13:12:54, dfc wrote:
> ,zk ?

Done.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go#newco...
cmd/jujud/main_test.go:53: checkMessage(c, msgz, cmd, "--state-servers",
"localhost:2181,zk")
On 2012/09/21 13:11:57, TheMue wrote:
> Again zk.

Done.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/main_test.go#newco...
cmd/jujud/main_test.go:59: "--state-servers", "zk:2181",
On 2012/09/21 13:12:54, dfc wrote:
> lots of zk:2181 turds here

Done.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/util_test.go
File cmd/jujud/util_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/util_test.go#newco...
cmd/jujud/util_test.go:26: args = append(args, "--state-servers",
"zk1:2181,zk2:2181")
On 2012/09/21 13:12:54, dfc wrote:
> ditto

Done.

https://codereview.appspot.com/6553049/diff/6041/cmd/jujud/util_test.go#newco...
cmd/jujud/util_test.go:45: "--state-servers", "zk:2181",
On 2012/09/21 13:11:57, TheMue wrote:
> Ditto.

Done.

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:133: " --state-servers localhost"+zkPortSuffix+
On 2012/09/21 13:12:54, dfc wrote:
> zkPortSuffix should be removed.

i've deliberately left lots of zk->mgo changes undone in this package, because
it's a significant job and one that is best done in its own CL IMHO. i did start
down that road, then backed out.

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit_test.go:100: t.checkScripts(c, "jujud machine
--state-servers 'localhost"+cloudinit.ZkPortSuffix+"' .* --machine-id [0-9]+")
On 2012/09/21 13:12:54, dfc wrote:
> kill ZkPortSuffix with fire.

gustavo's lighting that one.

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/ec2.go#newcode22
environs/ec2/ec2.go:22: const mgoPort = 2181
On 2012/09/21 13:17:53, niemeyer wrote:
> s/2181/37017/

Done.

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/ec2.go#newcode241
environs/ec2/ec2.go:241: MgoInstances: []string{inst.Id()},
On 2012/09/21 13:17:53, niemeyer wrote:
> s/Mgo/State/

Done.

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/state.go
File environs/ec2/state.go (right):

https://codereview.appspot.com/6553049/diff/6041/environs/ec2/state.go#newcode13
environs/ec2/state.go:13: MgoInstances []string `yaml:"mgo-instances"`
On 2012/09/21 13:17:53, niemeyer wrote:
> s/mgo-/state-/

Done.

https://codereview.appspot.com/6553049/diff/6041/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6553049/diff/6041/juju/testing/conn.go#newcode141
juju/testing/conn.go:141: func (s *JujuConnSuite) StateInfo(c *C) *state.Info {
On 2012/09/21 13:12:54, dfc wrote:
> Delete this function, it shadows an identical function from ConnSuite.

good catch. done.

[dave_cheney.net, 2012-09-21 13:53:02]

Thank you, getting very close.

https://codereview.appspot.com/6553049/diff/11002/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (left):

https://codereview.appspot.com/6553049/diff/11002/cmd/juju/cmd_test.go#oldcode28
cmd/juju/cmd_test.go:28: type: dummy
Please audit status_test.go and deploy_test.go for ZK references.

https://codereview.appspot.com/6553049/diff/11002/cmd/jujud/main_test.go
File cmd/jujud/main_test.go (right):

https://codereview.appspot.com/6553049/diff/11002/cmd/jujud/main_test.go#newc...
cmd/jujud/main_test.go:50: msgz := `invalid value "localhost:37017,srv" for flag
--state-servers: "srv" is not a valid zookeeper address`
s/zookeeper/mongo/g

https://codereview.appspot.com/6553049/diff/11002/state/confignode_test.go
File state/confignode_test.go (right):

https://codereview.appspot.com/6553049/diff/11002/state/confignode_test.go#ne...
state/confignode_test.go:196: // Now get another state instance and change
ZooKeeper state.
s/ZooKeeper//

https://codereview.appspot.com/6553049/diff/11002/worker/firewaller/firewalle...
File worker/firewaller/firewaller_test.go (right):

https://codereview.appspot.com/6553049/diff/11002/worker/firewaller/firewalle...
worker/firewaller/firewaller_test.go:277: c.Check(fw.Wait(), ErrorMatches, ".*
zookeeper is closing")
Can you please update these error checks.

https://codereview.appspot.com/6553049/diff/11002/worker/provisioner/provisio...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/6553049/diff/11002/worker/provisioner/provisio...
worker/provisioner/provisioner_test.go:52: zkConn := coretesting.ZkConnect()
Does this compile ?

https://codereview.appspot.com/6553049/diff/11002/worker/provisioner/provisio...
worker/provisioner/provisioner_test.go:169: c.Check(p.Wait(), ErrorMatches, ".*
zookeeper is closing")
Can you fix these up please.

[rog, 2012-09-21 14:10:07]

Please take a look.

https://codereview.appspot.com/6553049/diff/11002/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (left):

https://codereview.appspot.com/6553049/diff/11002/cmd/juju/cmd_test.go#oldcode28
cmd/juju/cmd_test.go:28: type: dummy
On 2012/09/21 13:53:02, dfc wrote:
> Please audit status_test.go and deploy_test.go for ZK references.

Done.

https://codereview.appspot.com/6553049/diff/11002/cmd/jujud/main_test.go
File cmd/jujud/main_test.go (right):

https://codereview.appspot.com/6553049/diff/11002/cmd/jujud/main_test.go#newc...
cmd/jujud/main_test.go:50: msgz := `invalid value "localhost:37017,srv" for flag
--state-servers: "srv" is not a valid zookeeper address`
On 2012/09/21 13:53:02, dfc wrote:
> s/zookeeper/mongo/g

"state server" done. pity it doesn't build yet.

https://codereview.appspot.com/6553049/diff/11002/state/confignode_test.go
File state/confignode_test.go (right):

https://codereview.appspot.com/6553049/diff/11002/state/confignode_test.go#ne...
state/confignode_test.go:196: // Now get another state instance and change
ZooKeeper state.
On 2012/09/21 13:53:02, dfc wrote:
> s/ZooKeeper//

Done.

https://codereview.appspot.com/6553049/diff/11002/worker/firewaller/firewalle...
File worker/firewaller/firewaller_test.go (right):

https://codereview.appspot.com/6553049/diff/11002/worker/firewaller/firewalle...
worker/firewaller/firewaller_test.go:277: c.Check(fw.Wait(), ErrorMatches, ".*
zookeeper is closing")
On 2012/09/21 13:53:02, dfc wrote:
> Can you please update these error checks.

i'd prefer to leave them, as the test is broken anyway. it's not clear what the
best way to fix it is.

https://codereview.appspot.com/6553049/diff/11002/worker/provisioner/provisio...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/6553049/diff/11002/worker/provisioner/provisio...
worker/provisioner/provisioner_test.go:52: zkConn := coretesting.ZkConnect()
On 2012/09/21 13:53:02, dfc wrote:
> Does this compile ?

nope. but that's a matter for another CL - it's not a trivial fix.

https://codereview.appspot.com/6553049/diff/11002/worker/provisioner/provisio...
worker/provisioner/provisioner_test.go:169: c.Check(p.Wait(), ErrorMatches, ".*
zookeeper is closing")
On 2012/09/21 13:53:02, dfc wrote:
> Can you fix these up please.

as above.

[dave_cheney.net, 2012-09-21 14:11:25]

LGTM. Drop it like third period french.

[rog, 2012-09-21 14:37:09]

*** Submitted:

all: use MgoSuite rather than ZkSuite

Also fix a test in worker/machiner.

R=TheMue, dfc, niemeyer
CC=
https://codereview.appspot.com/6553049