MachineAgent using EnsureMongoServer

MachineAgent using EnsureMongoServer



https://code.launchpad.net/~natefinch/juju-core/030-MA-HA/+merge/209954

(do not edit description out of merge proposal)

[natefinch, 2014-03-07 15:57:12]

Please take a look.

[natefinch, 2014-03-07 16:11:52]

Please take a look.

[natefinch, 2014-03-26 11:25:22]

Please take a look.

[natefinch, 2014-03-26 20:53:52]

Please take a look.

[jameinel, 2014-03-27 18:17:49]

A bunch of comments, hopefully it is constructive.
I also see a bunch of stuff that seems to get rid of the upstart job for
starting mongodb, but I don't see the corresponding "and this is how we now
start mongodb". I would have expected that we would still use upstart, just
change *when* the file is getting written. (It would be written by the jujud
bootstrap-state operation, rather than by cloud-init/the juju bootstrap client
code.)

https://codereview.appspot.com/72500043/diff/60001/agent/bootstrap.go
File agent/bootstrap.go (right):

https://codereview.appspot.com/72500043/diff/60001/agent/bootstrap.go#newcode59
agent/bootstrap.go:59: func (c *configInternal) StateInfo() *state.Info {
As this is just a blob (of data), it seems like it would be fine to return it as
the content, rather than as a pointer. Not that I really care.

https://codereview.appspot.com/72500043/diff/60001/agent/format-1.18_whitebox...
File agent/format-1.18_whitebox_test.go (left):

https://codereview.appspot.com/72500043/diff/60001/agent/format-1.18_whitebox...
agent/format-1.18_whitebox_test.go:104: MONGO_SERVICE_NAME: juju-db-user-local
As this is landing after 1.18, I don't think we can change this value.

Won't this be a compatibility break?

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:65: // This is a variable so it can be overridden in tests
It doesn't look like a variable to me.

initiateReplicaSet is, should this have been moved there?

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:78: }
it feels like this would be usefully split into a small helper func:

func validateMongoParams(p EnsureMongoParams) error {

}

That way the logic about what EnsureMongoServer is really doing is a bit
clearer.

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:104: logger.Infof("Mongod service %q started.", name)
Should we actually be uninstalling the service if we are failing to start it?

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:121: // TODO remove me
?

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:213: 
It seems odd to have this bare function here.

https://codereview.appspot.com/72500043/diff/60001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/72500043/diff/60001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:83: 
this would appear to be a typo.

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

https://codereview.appspot.com/72500043/diff/60001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:86: insts, err :=
env.Instances([]instance.Id{instance.Id(c.InstanceId)})
Wow, that is a lot of the words Instance and Id put together in different
combinations.

https://codereview.appspot.com/72500043/diff/60001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:146: // state server addresses earlier.
I can get this information just from the name. Can you clarify what that order
is? It makes it easier to ensure that the rest of the code is actually
implementing it.

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

https://codereview.appspot.com/72500043/diff/60001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:165: }
It feels like all this shouldn't be in cmd/jujud/machine.go, but should actually
live in worker/state_starter.go or somewhere like that.

We should try to have only glue code in a main package (IMO).

https://codereview.appspot.com/72500043/diff/60001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:247: // })
Commented out code should probably just be removed?

https://codereview.appspot.com/72500043/diff/60001/environs/cloudinit/cloudin...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/72500043/diff/60001/environs/cloudinit/cloudin...
environs/cloudinit/cloudinit.go:350: `[ -f /etc/default/mongodb ] ||
We don't need to do this if we are using the juju-mongodb package, since that
*doesn't* start a mongodb-server itself.
I'm a bit surprised you took out the bit that disabled it in that case.
Mostly because if you just "apt-get install mongodb-server" you *won't* have an
/etc/default/mongodb, and we only do it because in the Cloud case we don't want
to run 2 mongod's when one isn't ever doing anything. (It is just the local case
where someone *might* want to play with juju and still run mongodb for their
other work.)

https://codereview.appspot.com/72500043/diff/60001/environs/cloudinit/cloudin...
environs/cloudinit/cloudinit.go:362:
c.AddPackageFromTargetRelease("mongodb-server", "precise-updates/cloud-tools")
I thought Dimiter & Wayne had done a change where all things are defaulted to
the cloud-tools pocket.
Did that end up getting reverted?

https://codereview.appspot.com/72500043/diff/60001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/72500043/diff/60001/state/open.go#newcode88
state/open.go:88: logger.Infof("dialing mongo")
Was this just done to make it easier to debug? It doesn't look like any of the
logic actually changed.

Maybe you just needed DialInfo to be shared code with something else?

https://codereview.appspot.com/72500043/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/72500043/diff/60001/state/state.go#newcode1365
state/state.go:1365: }
Is this being done for all users, or just in the for State clients?

[wwitzel3, 2014-03-27 19:54:24]

https://codereview.appspot.com/72500043/diff/60001/environs/cloudinit/cloudin...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/72500043/diff/60001/environs/cloudinit/cloudin...
environs/cloudinit/cloudinit.go:362:
c.AddPackageFromTargetRelease("mongodb-server", "precise-updates/cloud-tools")
On 2014/03/27 18:17:49, jameinel wrote:
> I thought Dimiter & Wayne had done a change where all things are defaulted to
> the cloud-tools pocket.
> Did that end up getting reverted?

Yes, that was reverted as not all things (like git) are in cloud.archive.
https://bugs.launchpad.net/juju-core/+bug/1290890

We ended up creating an AptGetPreparePackages in utils/apt.go that handles if a
package should come from cloud.archive or not.

[natefinch, 2014-03-27 21:04:37]

Please take a look.

https://codereview.appspot.com/72500043/diff/60001/agent/bootstrap.go
File agent/bootstrap.go (right):

https://codereview.appspot.com/72500043/diff/60001/agent/bootstrap.go#newcode59
agent/bootstrap.go:59: func (c *configInternal) StateInfo() *state.Info {
On 2014/03/27 18:17:49, jameinel wrote:
> As this is just a blob (of data), it seems like it would be fine to return it
as
> the content, rather than as a pointer. Not that I really care.

It's a little bit less copying of data to copy the pointer. No, it probably
doesn't make a measurable difference.

https://codereview.appspot.com/72500043/diff/60001/agent/format-1.18_whitebox...
File agent/format-1.18_whitebox_test.go (left):

https://codereview.appspot.com/72500043/diff/60001/agent/format-1.18_whitebox...
agent/format-1.18_whitebox_test.go:104: MONGO_SERVICE_NAME: juju-db-user-local
On 2014/03/27 18:17:49, jameinel wrote:
> As this is landing after 1.18, I don't think we can change this value.
> 
> Won't this be a compatibility break?

Yes, but I'd hoped that we could avoid having to maintain a special service name
for local, since there seems to be no reason to actually have a different name.

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:65: // This is a variable so it can be overridden in tests
On 2014/03/27 18:17:49, jameinel wrote:
> It doesn't look like a variable to me.
> 
> initiateReplicaSet is, should this have been moved there?

Old comment, fixed.

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:78: }
On 2014/03/27 18:17:49, jameinel wrote:
> it feels like this would be usefully split into a small helper func:
> 
> func validateMongoParams(p EnsureMongoParams) error {
> 
> }
> 
> That way the logic about what EnsureMongoServer is really doing is a bit
> clearer.

Done.

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:104: logger.Infof("Mongod service %q started.", name)
On 2014/03/27 18:17:49, jameinel wrote:
> Should we actually be uninstalling the service if we are failing to start it?

Good question. I'm not sure. We could remove it and try to reinstall it.

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:121: // TODO remove me
On 2014/03/27 18:17:49, jameinel wrote:
> ?

removed

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:213: 
On 2014/03/27 18:17:49, jameinel wrote:
> It seems odd to have this bare function here.

Not really.  We have mongo.EnsureMongoServer() and we have mongo.RemoveService()
 I don't really know what type we'd want to put it on, it doesn't really have
any data associated with it.

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

https://codereview.appspot.com/72500043/diff/60001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:86: insts, err :=
env.Instances([]instance.Id{instance.Id(c.InstanceId)})
On 2014/03/27 18:17:49, jameinel wrote:
> Wow, that is a lot of the words Instance and Id put together in different
> combinations.

Yeah.  Static typing can be a pain sometimes.

https://codereview.appspot.com/72500043/diff/60001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:146: // state server addresses earlier.
On 2014/03/27 18:17:49, jameinel wrote:
> I can get this information just from the name. Can you clarify what that order
> is? It makes it easier to ensure that the rest of the code is actually
> implementing it.

Done.

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

https://codereview.appspot.com/72500043/diff/60001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:247: // })
On 2014/03/27 18:17:49, jameinel wrote:
> Commented out code should probably just be removed?

Yep, totally right.

https://codereview.appspot.com/72500043/diff/60001/environs/cloudinit/cloudin...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/72500043/diff/60001/environs/cloudinit/cloudin...
environs/cloudinit/cloudinit.go:350: `[ -f /etc/default/mongodb ] ||
I actually have no idea what this change is about. It's possible it was just a
bad merge, unless this is something Roger specifically wanted to remove.

https://codereview.appspot.com/72500043/diff/60001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/72500043/diff/60001/state/open.go#newcode88
state/open.go:88: logger.Infof("dialing mongo")
On 2014/03/27 18:17:49, jameinel wrote:
> Was this just done to make it easier to debug? It doesn't look like any of the
> logic actually changed.
> 
> Maybe you just needed DialInfo to be shared code with something else?

Yep, we use the DialInfo in several spots now, where we call EnsureMongoServer.

https://codereview.appspot.com/72500043/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/72500043/diff/60001/state/state.go#newcode1365
state/state.go:1365: }
On 2014/03/27 18:17:49, jameinel wrote:
> Is this being done for all users, or just in the for State clients?

It looks like only the state machine calls this, but I might be missing
something.

[axw, 2014-03-28 01:26:54]

Just some preliminary notes, I haven't looked too deeply.

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:205: return fmt.Sprintf("juju-db-v%d", version)
The local provider has a different name for its Mongo service, as multiple users
may bootstrap the local provider on the same machine. This function should take
that into account, I think.

FYI we propagate a "Namespace" attribute through agent.Config that can be used
for this purpose.

https://codereview.appspot.com/72500043/diff/80001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/72500043/diff/80001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:66: DestroyOnError bool
Leftover from debugging?

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

https://codereview.appspot.com/72500043/diff/80001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:99: st, _, err := c.Conf.config.InitializeState(
What where is the bootstrap machine's mongo installed?

https://codereview.appspot.com/72500043/diff/80001/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/72500043/diff/80001/provider/local/environ.go#...
provider/local/environ.go:441: if err := mongo.RemoveService(); err != nil {
Yeah, this is a bit broken. We need to be able to customise the mongo service
name for the local provider.

[natefinch, 2014-04-03 15:21:06]

On 2014/03/28 01:26:54, axw wrote:
> Just some preliminary notes, I haven't looked too deeply.
> 
> https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go
> File agent/mongo/mongo.go (right):
> 
>
https://codereview.appspot.com/72500043/diff/60001/agent/mongo/mongo.go#newco...
> agent/mongo/mongo.go:205: return fmt.Sprintf("juju-db-v%d", version)
> The local provider has a different name for its Mongo service, as multiple
users
> may bootstrap the local provider on the same machine. This function should
take
> that into account, I think.
> 
> FYI we propagate a "Namespace" attribute through agent.Config that can be used
> for this purpose.
> 
> https://codereview.appspot.com/72500043/diff/80001/cmd/juju/bootstrap.go
> File cmd/juju/bootstrap.go (right):
> 
>
https://codereview.appspot.com/72500043/diff/80001/cmd/juju/bootstrap.go#newc...
> cmd/juju/bootstrap.go:66: DestroyOnError bool
> Leftover from debugging?
> 
> https://codereview.appspot.com/72500043/diff/80001/cmd/jujud/bootstrap.go
> File cmd/jujud/bootstrap.go (right):
> 
>
https://codereview.appspot.com/72500043/diff/80001/cmd/jujud/bootstrap.go#new...
> cmd/jujud/bootstrap.go:99: st, _, err := c.Conf.config.InitializeState(
> What where is the bootstrap machine's mongo installed?
> 
> https://codereview.appspot.com/72500043/diff/80001/provider/local/environ.go
> File provider/local/environ.go (right):
> 
>
https://codereview.appspot.com/72500043/diff/80001/provider/local/environ.go#...
> provider/local/environ.go:441: if err := mongo.RemoveService(); err != nil {
> Yeah, this is a bit broken. We need to be able to customise the mongo service
> name for the local provider.

Yeah, the namespace stuff is totally broken in trunk.  I have a different CL to
fix it.

[natefinch, 2014-04-03 15:36:12]

Please take a look.

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

https://codereview.appspot.com/72500043/diff/80001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:99: st, _, err := c.Conf.config.InitializeState(
On 2014/03/28 01:26:55, axw wrote:
> What where is the bootstrap machine's mongo installed?
It's done inside initialize state.

[natefinch, 2014-04-03 19:08:43]

Please take a look.

[rog, 2014-04-03 19:50:01]

Getting there! Some comments and suggestions below.

https://codereview.appspot.com/72500043/diff/40001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/72500043/diff/40001/agent/agent.go#newcode508
agent/agent.go:508: copy(c2.caCert, c.caCert)
this doesn't do what you think it does.

the slice needs needs to be reallocated.


c2.caCert = append([]byte(nil), c.caCert...)

https://codereview.appspot.com/72500043/diff/40001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/72500043/diff/40001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:66: DestroyOnError bool
d

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

https://codereview.appspot.com/72500043/diff/40001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:85: func NewMachineAgent() *MachineAgent {
Since the zero voyeur.Value is ok to use, I think
we can still live without this function.
(and we've got Init anyway if we need it)

https://codereview.appspot.com/72500043/diff/40001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:296: //  watch API addresses
i think the only thing we actually need to watch is the API addresses.
Let's leave the others out of this for the moment.

https://codereview.appspot.com/72500043/diff/120001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/72500043/diff/120001/agent/agent.go#newcode103
agent/agent.go:103: // StateServer() bool
This can go, presumably.

https://codereview.appspot.com/72500043/diff/120001/agent/agent.go#newcode349
agent/agent.go:349:
logger.Debugf("config:\n-------------\n%s\n-----------------\n", configData)
I think we should leave this out (or have it at trace level) it includes
secrets.

https://codereview.appspot.com/72500043/diff/120001/agent/agent.go#newcode556
agent/agent.go:556: // func (c *configInternal) StateServer() bool {
as can this.

https://codereview.appspot.com/72500043/diff/120001/agent/bootstrap.go
File agent/bootstrap.go (right):

https://codereview.appspot.com/72500043/diff/120001/agent/bootstrap.go#newcode20
agent/bootstrap.go:20: // configuration. It uses that information to create the
state server, dial the
dial and initialize it.
?

https://codereview.appspot.com/72500043/diff/120001/agent/bootstrap.go#newcode22
agent/bootstrap.go:22: // bootstrap machine and calls Write to save the the
configuration.
and sets it on the configuration by calling SetPassword.

https://codereview.appspot.com/72500043/diff/120001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/72500043/diff/120001/agent/mongo/mongo.go#newc...
agent/mongo/mongo.go:164: func EnsureMongoServer(dataDir string, port int) error
{
I think this needs to take a StateServingInfo struct as an argument, and write
out the keyfile and shared secret to the files that mongo will read them from.

https://codereview.appspot.com/72500043/diff/120001/agent/mongo/mongo.go#newc...
agent/mongo/mongo.go:295: " --keyFile " + utils.ShQuote(keyFile),
I don't see where this is written.

https://codereview.appspot.com/72500043/diff/120001/agent/mongo/mongo_test.go
File agent/mongo/mongo_test.go (right):

https://codereview.appspot.com/72500043/diff/120001/agent/mongo/mongo_test.go...
agent/mongo/mongo_test.go:170: // TODO(natefinch) add a test that
InitiateMongoServer works when
// TODO(natefinch) When we support upgrading of existing
// environments: ...

?

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

https://codereview.appspot.com/72500043/diff/120001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:139: preferredAddr, err :=
selectPreferredStateServerAddress(addrs)
i think this should just use mongo.SelectPeerAddress. we can delete all the
selectPreferredStateServerAddress stuff now.

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

https://codereview.appspot.com/72500043/diff/120001/cmd/jujud/machine.go#newc...
cmd/jujud/machine.go:217: //			info, err := st.StateServingInfo()
Why is this deleted? We need it to get the state serving info details from the
state?

https://codereview.appspot.com/72500043/diff/120001/cmd/jujud/machine.go#newc...
cmd/jujud/machine.go:374: //agentConfig.StateServingInfo())
delete?

https://codereview.appspot.com/72500043/diff/120001/environs/cloudinit/cloudi...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/72500043/diff/120001/environs/cloudinit/cloudi...
environs/cloudinit/cloudinit.go:366: c.AddFile(cfg.dataFile("server.pem"),
certKey, 0600)
This needs to go, I believe.

[natefinch, 2014-04-03 21:01:01]

Please take a look.

[axw, 2014-04-04 02:34:06]

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

https://codereview.appspot.com/72500043/diff/80001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:99: st, _, err := c.Conf.config.InitializeState(
On 2014/04/03 15:36:13, nate.finch wrote:
> On 2014/03/28 01:26:55, axw wrote:
> > What where is the bootstrap machine's mongo installed?
> It's done inside initialize state.

Sorry if I'm being slow, but I still don't see how this can work. I just merged
it into trunk and it doesn't work for the local provider.

It appears that the ensureMongoServer call below is meant to install the upstart
service. The agent.InitializeState method requires mongo to be available
(otherwise what's it intialising?), so that's a bit back-to-front I think.

I moved InitializeState down to the bottom of this function, but bootstrap still
gets stuck trying to dial in MaybeInitiateMongoServer. I don't quite know what
I'm doing with mongo, but I'll keep poking around a bit longer.

[axw, 2014-04-04 07:51:24]

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

https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:153: if err := ensureMongoServer(agentConfig.DataDir(),
envCfg.StatePort()); err != nil {
I think this has to come before the InitializeState call above.

https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:157: return
maybeInitiateMongoServer(mongo.InitiateMongoParams{
It seems a bit wasteful to make another connection to mongo when just did that
to for InitializeState. I suggest defer st.Close() after coming out of the
ChangeConfig above, and then use st.MongoSession().

There's another reason to prefer this: you can only use the localhost exception
up until the admin database has users populated. Reusing the session simplifies
authentication.

https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:157: return
maybeInitiateMongoServer(mongo.InitiateMongoParams{
I found that I had to set dialInfo.Direct=true when initiating the replicaset,
otherwise mgo just sits in a loop and never progresses. Not sure if this is a
bug or expected behaviour.

[axw, 2014-04-04 08:10:14]

On 2014/04/04 07:51:24, axw wrote:
> https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/bootstrap.go
> File cmd/jujud/bootstrap.go (right):
> 
>
https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/bootstrap.go#ne...
> cmd/jujud/bootstrap.go:153: if err := ensureMongoServer(agentConfig.DataDir(),
> envCfg.StatePort()); err != nil {
> I think this has to come before the InitializeState call above.
> 
>
https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/bootstrap.go#ne...
> cmd/jujud/bootstrap.go:157: return
> maybeInitiateMongoServer(mongo.InitiateMongoParams{
> It seems a bit wasteful to make another connection to mongo when just did that
> to for InitializeState. I suggest defer st.Close() after coming out of the
> ChangeConfig above, and then use st.MongoSession().

Scratch that: seems that the replicaset config must be configured before all
else.

> There's another reason to prefer this: you can only use the localhost
exception
> up until the admin database has users populated. Reusing the session
simplifies
> authentication.
> 
>
https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/bootstrap.go#ne...
> cmd/jujud/bootstrap.go:157: return
> maybeInitiateMongoServer(mongo.InitiateMongoParams{
> I found that I had to set dialInfo.Direct=true when initiating the replicaset,
> otherwise mgo just sits in a loop and never progresses. Not sure if this is a
> bug or expected behaviour.

Also, you can't just change Addrs[0] to the preferredHost, because then the
localhost exception fails.

I think you probably want a separate field for the preferred address in
InitiateMongoParams.

[axw, 2014-04-04 08:22:55]

Sorry about the spam... last one (I hope). I managed to get local to bootstrap
now, without any mongo CLI hacking.

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

https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:151: net.JoinHostPort(preferredAddr.String(),
fmt.Sprint(envCfg.StatePort())),
This is wrong, and generates something like "cloud-local:10.0.0.1:37017". See
instance/address.go.

Instead, I suggest:
    instance.HostPort{preferredAddr, envCfg.StatePort()}.NetAddr()

[wwitzel3, 2014-04-04 15:07:51]

https://codereview.appspot.com/72500043/diff/140001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/72500043/diff/140001/agent/agent.go#newcode103
agent/agent.go:103: // StateServer() bool
d?

https://codereview.appspot.com/72500043/diff/140001/agent/agent.go#newcode343
agent/agent.go:343:
logger.Debugf("config:\n-------------\n%s\n-----------------\n", configData)
d?

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

https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:157: return
maybeInitiateMongoServer(mongo.InitiateMongoParams{
On 2014/04/04 07:51:24, axw wrote:
> I found that I had to set dialInfo.Direct=true when initiating the replicaset,
> otherwise mgo just sits in a loop and never progresses. Not sure if this is a
> bug or expected behaviour.

Well, it is known behavior, but we should be setting Direct to true during the
initialization of the replicaset for sure.

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

https://codereview.appspot.com/72500043/diff/140001/cmd/jujud/machine.go#newc...
cmd/jujud/machine.go:217: //			info, err := st.StateServingInfo()
d?

[natefinch, 2014-04-08 15:05:59]

Please take a look.

[rog, 2014-04-08 15:39:52]

LGTM modulo a bunch of trivials and assuming it passes against the local
provider and in the cloud.

https://codereview.appspot.com/72500043/diff/160001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/72500043/diff/160001/agent/agent.go#newcode338
agent/agent.go:338:
logger.Debugf("config:\n-------------\n%s\n-----------------\n", configData)
let's lose this - it'll really clutter the logs.

https://codereview.appspot.com/72500043/diff/160001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/72500043/diff/160001/agent/mongo/mongo.go#newc...
agent/mongo/mongo.go:179: func EnsureMongoServer(dir string, port int, namespace
string) error {
could we name the directory dataDir to fit with most other existing usage?
Otherwise it's not clear what it's used for.

https://codereview.appspot.com/72500043/diff/160001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/72500043/diff/160001/cmd/juju/bootstrap.go#new...
cmd/juju/bootstrap.go:83: 
d

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

https://codereview.appspot.com/72500043/diff/160001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:86: logger.Infof("creating new Environ")
this is surely Debugf at the most.

https://codereview.appspot.com/72500043/diff/160001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:114: 
d

https://codereview.appspot.com/72500043/diff/160001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:165: logger.Infof("calling ensureMongoServer")
again, Debugf or delete.

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

https://codereview.appspot.com/72500043/diff/160001/cmd/jujud/bootstrap_test....
cmd/jujud/bootstrap_test.go:58: initiateParams mongo.InitiateMongoParams
shouldn't some of the above fields be asserted on somewhere?

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

https://codereview.appspot.com/72500043/diff/160001/cmd/jujud/machine.go#newc...
cmd/jujud/machine.go:376: return nil, fmt.Errorf("No state info on agent
config")
s/No/no/

https://codereview.appspot.com/72500043/diff/160001/cmd/jujud/machine.go#newc...
cmd/jujud/machine.go:376: return nil, fmt.Errorf("No state info on agent
config")
s/No/no/

s/on/found in/ ?

https://codereview.appspot.com/72500043/diff/160001/replicaset/replicaset.go
File replicaset/replicaset.go (right):

https://codereview.appspot.com/72500043/diff/160001/replicaset/replicaset.go#...
replicaset/replicaset.go:291: return nil, fmt.Errorf("replSetGetStatus error:
%v", err)
cannot get replica set status: %v

?

https://codereview.appspot.com/72500043/diff/160001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/72500043/diff/160001/state/open.go#newcode86
state/open.go:86: logger.Infof("dialing mongo")
s/Infof/Debugf/

https://codereview.appspot.com/72500043/diff/160001/state/open.go#newcode92
state/open.go:92: logger.Infof("connection established")
ditto

https://codereview.appspot.com/72500043/diff/160001/state/open.go#newcode146
state/open.go:146: // It  returns unauthorizedError if access is unauthorized.
s/ //

https://codereview.appspot.com/72500043/diff/160001/state/open.go#newcode249
state/open.go:249: logger.Debugf("starting newState")
d

https://codereview.appspot.com/72500043/diff/160001/worker/instancepoller/upd...
File worker/instancepoller/updater.go (right):

https://codereview.appspot.com/72500043/diff/160001/worker/instancepoller/upd...
worker/instancepoller/updater.go:224: if !state.IsNotProvisionedError(err) {
double negatives are hard to understand.

how about:

if state.IsNotProvisionedError {
   return instInfo, nil
}
return instInfo, err

?

[natefinch, 2014-04-08 19:23:45]

Please take a look.

[natefinch, 2014-04-08 20:16:38]

Thanks for the review, Rog. All changes suggested were made.

[natefinch, 2014-04-08 20:45:52]

Please take a look.

[natefinch, 2014-04-09 10:46:47]

Please take a look.

[rog, 2014-04-09 12:50:00]

stil LGTM

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

https://codereview.appspot.com/72500043/diff/220001/cmd/jujud/bootstrap_test....
cmd/jujud/bootstrap_test.go:438: maybePanic(err)
sorry for not picking up on this earlier, but why aren't these maybePanics just
c.Assert(err, gc.IsNil)s ?

https://codereview.appspot.com/72500043/diff/220001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/72500043/diff/220001/state/open.go#newcode249
state/open.go:249: logger.Debugf("starting newState")
this should go i think, as suggested - it's not that significant, is it?

[rog, 2014-04-09 12:58:42]

actually this does not LGTM without the following fix.

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

https://codereview.appspot.com/72500043/diff/220001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:167: MemberHostPort: mongo.SelectPeerAddress(addrs),
This doesn't seem right - SelectPeerAddress will not return a host:port.

Have you actually tried this live?

I think you probably want:

peerAddr := mongo.SelectPeerAddress(addrs)
if peerAddr == "" {
    return fmt.Errorf("no appropriate peer address found in %q", addrs)
}
peerHostPort := net.JoinHostPort(peerAddr, fmt.Sprint(port))
memberHostPort := net.JoinHostPort(mongo.SelectPeer

...
    MemberHostPort: memberHostPort,

[rog, 2014-04-09 12:59:41]

> memberHostPort := net.JoinHostPort(mongo.SelectPeer

ignore this line

>     MemberHostPort: memberHostPort,

s/memberHostPort/peerHostPort/

[natefinch, 2014-04-09 15:43:39]

Please take a look.

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

https://codereview.appspot.com/72500043/diff/220001/cmd/jujud/bootstrap.go#ne...
cmd/jujud/bootstrap.go:167: MemberHostPort: mongo.SelectPeerAddress(addrs),
On 2014/04/09 12:58:43, rog wrote:
> This doesn't seem right - SelectPeerAddress will not return a host:port.
> 
> Have you actually tried this live?
> 
> I think you probably want:
> 
> peerAddr := mongo.SelectPeerAddress(addrs)
> if peerAddr == "" {
>     return fmt.Errorf("no appropriate peer address found in %q", addrs)
> }
> peerHostPort := net.JoinHostPort(peerAddr, fmt.Sprint(port))
> memberHostPort := net.JoinHostPort(mongo.SelectPeer
> 
> ...
>     MemberHostPort: memberHostPort,

Done.

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

https://codereview.appspot.com/72500043/diff/220001/cmd/jujud/bootstrap_test....
cmd/jujud/bootstrap_test.go:438: maybePanic(err)
On 2014/04/09 12:50:01, rog wrote:
> sorry for not picking up on this earlier, but why aren't these maybePanics
just
> c.Assert(err, gc.IsNil)s ?
> 

Done.

https://codereview.appspot.com/72500043/diff/220001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/72500043/diff/220001/state/open.go#newcode249
state/open.go:249: logger.Debugf("starting newState")
On 2014/04/09 12:50:01, rog wrote:
> this should go i think, as suggested - it's not that significant, is it?

Done.

[rog, 2014-04-09 15:59:58]

On 2014/04/09 15:43:39, nate.finch wrote:
> Please take a look.
> 
> https://codereview.appspot.com/72500043/diff/220001/cmd/jujud/bootstrap.go
> File cmd/jujud/bootstrap.go (right):
> 
>
https://codereview.appspot.com/72500043/diff/220001/cmd/jujud/bootstrap.go#ne...
> cmd/jujud/bootstrap.go:167: MemberHostPort: mongo.SelectPeerAddress(addrs),
> On 2014/04/09 12:58:43, rog wrote:
> > This doesn't seem right - SelectPeerAddress will not return a host:port.
> > 
> > Have you actually tried this live?
> > 
> > I think you probably want:
> > 
> > peerAddr := mongo.SelectPeerAddress(addrs)
> > if peerAddr == "" {
> >     return fmt.Errorf("no appropriate peer address found in %q", addrs)
> > }
> > peerHostPort := net.JoinHostPort(peerAddr, fmt.Sprint(port))
> > memberHostPort := net.JoinHostPort(mongo.SelectPeer
> > 
> > ...
> >     MemberHostPort: memberHostPort,
> 
> Done.
> 
>
https://codereview.appspot.com/72500043/diff/220001/cmd/jujud/bootstrap_test.go
> File cmd/jujud/bootstrap_test.go (right):
> 
>
https://codereview.appspot.com/72500043/diff/220001/cmd/jujud/bootstrap_test....
> cmd/jujud/bootstrap_test.go:438: maybePanic(err)
> On 2014/04/09 12:50:01, rog wrote:
> > sorry for not picking up on this earlier, but why aren't these maybePanics
> just
> > c.Assert(err, gc.IsNil)s ?
> > 
> 
> Done.
> 
> https://codereview.appspot.com/72500043/diff/220001/state/open.go
> File state/open.go (right):
> 
> https://codereview.appspot.com/72500043/diff/220001/state/open.go#newcode249
> state/open.go:249: logger.Debugf("starting newState")
> On 2014/04/09 12:50:01, rog wrote:
> > this should go i think, as suggested - it's not that significant, is it?
> 
> Done.

LGTM