mongo keyfiles and refactoring

mongo keyfiles and refactoring

Write out mongo keyfiles,move initiate under peergrouper.

https://code.launchpad.net/~natefinch/juju-core/041-moremongo/+merge/215464

(do not edit description out of merge proposal)

[natefinch, 2014-04-11 17:00:52]

Please take a look.

[jameinel, 2014-04-12 09:57:43]

A quick thought about hard-coded Identifiers.

https://codereview.appspot.com/86920043/diff/1/worker/peergrouper/initiate.go
File worker/peergrouper/initiate.go (right):

https://codereview.appspot.com/86920043/diff/1/worker/peergrouper/initiate.go...
worker/peergrouper/initiate.go:67: jujuMachineTag: agent.BootstrapMachineId,
can we look this up, or pass this in via InitiateMongoParams rather than
hard-coding it?
I can probably live with this on the idea that we won't have an environment
where machine-0 is not the API server without HA, because you needed HA to get
here.

Though potentially, you could start with machine-0, get HA, stop HA, and end up
with only machine-1.

So if it is hard to pass it in as data, I can live with it, but it seems better
to be given this, rather than hard-coding it.

[axw, 2014-04-14 04:50:54]

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (left):

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#oldcode245
agent/mongo/mongo.go:245: // NOTE: ensure that the right package is installed?
This NOTE can go I think?

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode265
agent/mongo/mongo.go:265: func aptGetInstallMongod() error {
It would be good to add a log message here, otherwise I think "juju bootstrap"
will just sit there with no feedback while it downloads mongodb-server.

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode266
agent/mongo/mongo.go:266: // Only Quantal requires the PPA (for mongo).
I think the "(for mongo)" can be dropped now.

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode272
agent/mongo/mongo.go:272: cmds :=
utils.AptGetPreparePackages([]string{"mongodb-server"}, version.Current.Series)
I think it would be quite nice to see some progress out of apt-get here (for
bootstrap), but I understand if it's a pain to implement.

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode272
agent/mongo/mongo.go:272: cmds :=
utils.AptGetPreparePackages([]string{"mongodb-server"}, version.Current.Series)
We should be using juju-mongodb if it's available?

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode282
agent/mongo/mongo.go:282: cmd := exec.Command("add-apt-repository", "-y", name)
This requires python-software-properties to be installed; please AptGetInstall
that first.

https://codereview.appspot.com/86920043/diff/1/worker/peergrouper/initiate.go
File worker/peergrouper/initiate.go (right):

https://codereview.appspot.com/86920043/diff/1/worker/peergrouper/initiate.go...
worker/peergrouper/initiate.go:31: func MaybeInitiateMongoServer(p
InitiateMongoParams) error {
Why move this to peergrouper? agent/mongo seems better to me, since it's used
only during agent initialisation. Is there some intended change in use that
motivates this?

[rog, 2014-04-14 15:10:59]

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode272
agent/mongo/mongo.go:272: cmds :=
utils.AptGetPreparePackages([]string{"mongodb-server"}, version.Current.Series)
On 2014/04/14 04:50:54, axw wrote:
> We should be using juju-mongodb if it's available?

The existing code doesn't use juju-mongodb. What should we do to check if it's
available?

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode272
agent/mongo/mongo.go:272: cmds :=
utils.AptGetPreparePackages([]string{"mongodb-server"}, version.Current.Series)
On 2014/04/14 04:50:54, axw wrote:
> I think it would be quite nice to see some progress out of apt-get here (for
> bootstrap), but I understand if it's a pain to implement.

Let's not block HA on this - we can add a bug for it.

https://codereview.appspot.com/86920043/diff/1/worker/peergrouper/initiate.go
File worker/peergrouper/initiate.go (right):

https://codereview.appspot.com/86920043/diff/1/worker/peergrouper/initiate.go...
worker/peergrouper/initiate.go:31: func MaybeInitiateMongoServer(p
InitiateMongoParams) error {
On 2014/04/14 04:50:54, axw wrote:
> Why move this to peergrouper? agent/mongo seems better to me, since it's used
> only during agent initialisation. Is there some intended change in use that
> motivates this?

The reason is that this code needs to have knowledge of the machine tags on
replica set members. I don't want that knowledge scattered around the code base.

We can't get agent/mongo to invoke worker/peergrouper, because
worker/peergrouper wants information from agent/mongo (SelectPeerAddress for
one).

https://codereview.appspot.com/86920043/diff/1/worker/peergrouper/initiate.go...
worker/peergrouper/initiate.go:67: jujuMachineTag: agent.BootstrapMachineId,
On 2014/04/12 09:57:44, jameinel wrote:
> can we look this up, or pass this in via InitiateMongoParams rather than
> hard-coding it?
> I can probably live with this on the idea that we won't have an environment
> where machine-0 is not the API server without HA, because you needed HA to get
> here.

I don't really see a problem with hard-coding it. There can only ever
be one bootstrap machine.

> Though potentially, you could start with machine-0, get HA, stop HA, and end
up
> with only machine-1.

Even when we can stop HA, we won't need to call InitiateMongoServer again.

[natefinch, 2014-04-14 15:37:42]

Please take a look.

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (left):

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#oldcode245
agent/mongo/mongo.go:245: // NOTE: ensure that the right package is installed?
On 2014/04/14 04:50:54, axw wrote:
> This NOTE can go I think?

Done.

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode265
agent/mongo/mongo.go:265: func aptGetInstallMongod() error {
On 2014/04/14 04:50:54, axw wrote:
> It would be good to add a log message here, otherwise I think "juju bootstrap"
> will just sit there with no feedback while it downloads mongodb-server.

Done.

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode266
agent/mongo/mongo.go:266: // Only Quantal requires the PPA (for mongo).
On 2014/04/14 04:50:54, axw wrote:
> I think the "(for mongo)" can be dropped now.

Done.

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode272
agent/mongo/mongo.go:272: cmds :=
utils.AptGetPreparePackages([]string{"mongodb-server"}, version.Current.Series)
On 2014/04/14 15:11:00, rog wrote:
> On 2014/04/14 04:50:54, axw wrote:
> > We should be using juju-mongodb if it's available?
> 
> The existing code doesn't use juju-mongodb. What should we do to check if it's
> available?

We don't install it right now, we can modify this behavior later, but for now,
let's not block this branch on that.

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode272
agent/mongo/mongo.go:272: cmds :=
utils.AptGetPreparePackages([]string{"mongodb-server"}, version.Current.Series)
On 2014/04/14 04:50:54, axw wrote:
> I think it would be quite nice to see some progress out of apt-get here (for
> bootstrap), but I understand if it's a pain to implement.

I think it would be nice, but I think it'll have to wait.

https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode282
agent/mongo/mongo.go:282: cmd := exec.Command("add-apt-repository", "-y", name)
On 2014/04/14 04:50:54, axw wrote:
> This requires python-software-properties to be installed; please AptGetInstall
> that first.

Done.

[dimitern, 2014-04-14 16:52:56]

Looks solid, and since it's a blocker for HA I'll give it an LGTM, but I have
some suggestions (mostly readability and documenting code) and questions below,
as not everything is clear.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:36: ReplicaSetName = "juju"
doc comment?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:143: return fmt.Errorf("cannot create mongo dbdir: %v",
err)
s/dbdir/db dir/ ?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:161: err = ioutil.WriteFile(
Shouldn't we use here utils.AtomicWriteFile as well in case of a crash?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:195: journalDir := path.Join(dataDir, "journal")
Why not filepath.Join instead?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:249: " --dbpath=" + dbDir +
utils.ShQuote(dbDir) ?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:258: " --replSet " + ReplicaSetName +
utils.ShQuote(ReplicaSetName) ?

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

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:39: _        = gc.Suite(&MongoSuite{})
This looks a bit awkward like this. How about separating them:

var _ = gc.Suite(&MongoSuite{})

var testInfo = params.StateServingInfo{
    StatePort:    25252,
    Cert:         "foobar-cert",
    PrivateKey:   "foobar-privkey",
    SharedSecret: "foobar-sharedsecret",
}

The first var is perceived more like a "directive" than a variable declaration
(i.e. "register this suite").

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:69: s.removeError = nil
Why do you need to explicitly initialize these?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:132: c.Assert(conf.Desc, gc.Not(gc.Equals), "")
Maybe check for a more specific description?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:134: // TODO set Out so that mongod output goes
somewhere useful?
// TODO(rog) ...

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:153: c.Assert(s.installed, gc.HasLen, 1)
Extract lines 128-135 in an inline helper so you can call it here (rather than
only checking the length) and on line 127?

assertInstalled := func() {
    c.Assert(s.installed, gc.HasLen, 1)
    conf := s.installed[0]
    ...
}

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:265: // that is written by each call to the command -
mockShellCalls
s/mockShellCalls/getMockShellArgs/

Also, I'd like a sentence or two explaining the format of the output file that
gets created (in the doc comment or inside the function).

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:275: echo +"$i"+
Why +"$i"+ ? Are the pluses used to separate arguments so they can be parsed
later?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:285: // Given a file name returned by
mockShellCommands, getMockShellCalls
// getMockShellCalls, when given a file name returned by mockShellCommands,
returns a slice .... ?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:289: func getMockShellCalls(c *gc.C, file string)
[][]string {
s/getMockShellCalls/getMockShellArgs/ ? Since it actually returns arguments, not
calls.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:305: func splitCall(c *gc.C, part string) []string {
doc comment

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:316: func failCmd(path string) {
doc comment

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

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:110: if err := c.ChangeConfig(func(agentConfig
agent.ConfigSetter) {
That's a clever but not quite readable nesting IMO.

I'd prefer simply:

err = c.ChangeConfig(func(agentConfig agent.ConfigSetter) {
 ...
})
if err != nil {
...
}

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:121: logger.Debugf("started mongo")
Shouldn't this be an Infof as with the other "X started" or "Y stopped" similar
messages?

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:168: logger.Infof("calling ensureMongoServer")
Now this looks more suitable for a Debugf.

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

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap_test.g...
cmd/jujud/bootstrap_test.go:170: c.Assert(len(servingInfo.SharedSecret),
gc.Not(gc.Equals), 0)
Don't you know exactly what's the length here?

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go
File replicaset/replicaset.go (right):

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go#n...
replicaset/replicaset.go:28: func Initiate(session *mgo.Session, address, name
string, tags map[string]string) error {
I can't see description of tags or tests about it.

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go#n...
replicaset/replicaset.go:277: monotonicSession := session.Clone()
Why the need to Clone() the session ?

https://codereview.appspot.com/86920043/diff/20001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/86920043/diff/20001/state/api/apiclient.go#new...
state/api/apiclient.go:128: defer try.Kill()
Oops :)

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/desired...
worker/peergrouper/desired.go:15: const jujuMachineTag = "juju-machine-id"
doc comment?

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
File worker/peergrouper/initiate.go (right):

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate.go:1: package peergrouper
Copyright header.

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate.go:7: "launchpad.net/juju-core/agent"
Separate juju-core from mgo imports.

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate.go:12: const bootstrapMachineId = "0"
d? it seems unused; otherwise doc comment to explain how it's different from
agent.BootstrapMachineId.

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
File worker/peergrouper/initiate_test.go (right):

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate_test.go:1: package peergrouper_test
Copyright header.

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate_test.go:28: err =
peergrouper.MaybeInitiateMongoServer(peergrouper.InitiateMongoParams{
args := peergrouper.InitiateMongoParams{
    DialInfo:       info,
    MemberHostPort: inst.Addr(),
}
err = peergrouper.MaybeInitiateMongoServer(args)
...

(both places where it's called)

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate_test.go:35: // configuration already existed but we
tried to created
s/created/create/

[dimitern, 2014-04-14 16:52:56]

Looks solid, and since it's a blocker for HA I'll give it an LGTM, but I have
some suggestions (mostly readability and documenting code) and questions below,
as not everything is clear.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:36: ReplicaSetName = "juju"
doc comment?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:143: return fmt.Errorf("cannot create mongo dbdir: %v",
err)
s/dbdir/db dir/ ?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:161: err = ioutil.WriteFile(
Shouldn't we use here utils.AtomicWriteFile as well in case of a crash?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:195: journalDir := path.Join(dataDir, "journal")
Why not filepath.Join instead?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:249: " --dbpath=" + dbDir +
utils.ShQuote(dbDir) ?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:258: " --replSet " + ReplicaSetName +
utils.ShQuote(ReplicaSetName) ?

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

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:39: _        = gc.Suite(&MongoSuite{})
This looks a bit awkward like this. How about separating them:

var _ = gc.Suite(&MongoSuite{})

var testInfo = params.StateServingInfo{
    StatePort:    25252,
    Cert:         "foobar-cert",
    PrivateKey:   "foobar-privkey",
    SharedSecret: "foobar-sharedsecret",
}

The first var is perceived more like a "directive" than a variable declaration
(i.e. "register this suite").

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:69: s.removeError = nil
Why do you need to explicitly initialize these?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:132: c.Assert(conf.Desc, gc.Not(gc.Equals), "")
Maybe check for a more specific description?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:134: // TODO set Out so that mongod output goes
somewhere useful?
// TODO(rog) ...

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:153: c.Assert(s.installed, gc.HasLen, 1)
Extract lines 128-135 in an inline helper so you can call it here (rather than
only checking the length) and on line 127?

assertInstalled := func() {
    c.Assert(s.installed, gc.HasLen, 1)
    conf := s.installed[0]
    ...
}

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:265: // that is written by each call to the command -
mockShellCalls
s/mockShellCalls/getMockShellArgs/

Also, I'd like a sentence or two explaining the format of the output file that
gets created (in the doc comment or inside the function).

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:275: echo +"$i"+
Why +"$i"+ ? Are the pluses used to separate arguments so they can be parsed
later?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:285: // Given a file name returned by
mockShellCommands, getMockShellCalls
// getMockShellCalls, when given a file name returned by mockShellCommands,
returns a slice .... ?

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:289: func getMockShellCalls(c *gc.C, file string)
[][]string {
s/getMockShellCalls/getMockShellArgs/ ? Since it actually returns arguments, not
calls.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:305: func splitCall(c *gc.C, part string) []string {
doc comment

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:316: func failCmd(path string) {
doc comment

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

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:110: if err := c.ChangeConfig(func(agentConfig
agent.ConfigSetter) {
That's a clever but not quite readable nesting IMO.

I'd prefer simply:

err = c.ChangeConfig(func(agentConfig agent.ConfigSetter) {
 ...
})
if err != nil {
...
}

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:121: logger.Debugf("started mongo")
Shouldn't this be an Infof as with the other "X started" or "Y stopped" similar
messages?

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:168: logger.Infof("calling ensureMongoServer")
Now this looks more suitable for a Debugf.

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

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap_test.g...
cmd/jujud/bootstrap_test.go:170: c.Assert(len(servingInfo.SharedSecret),
gc.Not(gc.Equals), 0)
Don't you know exactly what's the length here?

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go
File replicaset/replicaset.go (right):

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go#n...
replicaset/replicaset.go:28: func Initiate(session *mgo.Session, address, name
string, tags map[string]string) error {
I can't see description of tags or tests about it.

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go#n...
replicaset/replicaset.go:277: monotonicSession := session.Clone()
Why the need to Clone() the session ?

https://codereview.appspot.com/86920043/diff/20001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/86920043/diff/20001/state/api/apiclient.go#new...
state/api/apiclient.go:128: defer try.Kill()
Oops :)

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/desired...
worker/peergrouper/desired.go:15: const jujuMachineTag = "juju-machine-id"
doc comment?

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
File worker/peergrouper/initiate.go (right):

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate.go:1: package peergrouper
Copyright header.

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate.go:7: "launchpad.net/juju-core/agent"
Separate juju-core from mgo imports.

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate.go:12: const bootstrapMachineId = "0"
d? it seems unused; otherwise doc comment to explain how it's different from
agent.BootstrapMachineId.

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
File worker/peergrouper/initiate_test.go (right):

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate_test.go:1: package peergrouper_test
Copyright header.

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate_test.go:28: err =
peergrouper.MaybeInitiateMongoServer(peergrouper.InitiateMongoParams{
args := peergrouper.InitiateMongoParams{
    DialInfo:       info,
    MemberHostPort: inst.Addr(),
}
err = peergrouper.MaybeInitiateMongoServer(args)
...

(both places where it's called)

https://codereview.appspot.com/86920043/diff/20001/worker/peergrouper/initiat...
worker/peergrouper/initiate_test.go:35: // configuration already existed but we
tried to created
s/created/create/

[rog, 2014-04-14 19:03:10]

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:161: err = ioutil.WriteFile(
On 2014/04/14 16:52:57, dimitern wrote:
> Shouldn't we use here utils.AtomicWriteFile as well in case of a crash?

+1

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:195: journalDir := path.Join(dataDir, "journal")
On 2014/04/14 16:52:57, dimitern wrote:
> Why not filepath.Join instead?

+1

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:249: " --dbpath=" + dbDir +
On 2014/04/14 16:52:57, dimitern wrote:
> utils.ShQuote(dbDir) ?

+1

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:258: " --replSet " + ReplicaSetName +
On 2014/04/14 16:52:57, dimitern wrote:
> utils.ShQuote(ReplicaSetName) ?

That's probably not strictly necessary - that's a name we choose, and we'd be
really stupid to choose a name containing shell metacharacters.

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

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:39: _        = gc.Suite(&MongoSuite{})
On 2014/04/14 16:52:57, dimitern wrote:
> This looks a bit awkward like this. How about separating them:
> 
> var _ = gc.Suite(&MongoSuite{})
> 
> var testInfo = params.StateServingInfo{
>     StatePort:    25252,
>     Cert:         "foobar-cert",
>     PrivateKey:   "foobar-privkey",
>     SharedSecret: "foobar-sharedsecret",
> }
> 
> The first var is perceived more like a "directive" than a variable declaration
> (i.e. "register this suite").

+1. var() is useful for grouping related stuff.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:69: s.removeError = nil
On 2014/04/14 16:52:57, dimitern wrote:
> Why do you need to explicitly initialize these?

Because they're set when the patched functions are called, which should be
evident from the lines above, but perhaps it could do with a comment.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:132: c.Assert(conf.Desc, gc.Not(gc.Equals), "")
On 2014/04/14 16:52:57, dimitern wrote:
> Maybe check for a more specific description?

I don't think there's a point - we're just duplicating the code. We care that
it's set, but the code can control what it's set to.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:153: c.Assert(s.installed, gc.HasLen, 1)
On 2014/04/14 16:52:57, dimitern wrote:
> Extract lines 128-135 in an inline helper so you can call it here (rather than
> only checking the length) and on line 127?
> 
> assertInstalled := func() {
>     c.Assert(s.installed, gc.HasLen, 1)
>     conf := s.installed[0]
>     ...
> }

meh, maybe. I'm happy to trust the earlier test and go with a simpler check the
second time, for the sake of easier-to-read test logic.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:265: // that is written by each call to the command -
mockShellCalls
> Also, I'd like a sentence or two explaining the format of the output file that
> gets created (in the doc comment or inside the function).

Inside the function would be appropriate, as it's really an implementation
detail.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:275: echo +"$i"+
On 2014/04/14 16:52:57, dimitern wrote:
> Why +"$i"+ ? Are the pluses used to separate arguments so they can be parsed
> later?

yes. it would be nicer if there was a neat way of quoting newlines in bash.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:289: func getMockShellCalls(c *gc.C, file string)
[][]string {
On 2014/04/14 16:52:57, dimitern wrote:
> s/getMockShellCalls/getMockShellArgs/ ? Since it actually returns arguments,
not
> calls.

It does return calls - each call is one element in the returned slice. Each
element of *that* contains an argument.

getMockShellArgs makes it sound like it gets the args for a single call.

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

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:121: logger.Debugf("started mongo")
On 2014/04/14 16:52:57, dimitern wrote:
> Shouldn't this be an Infof as with the other "X started" or "Y stopped"
similar
> messages?

+1

https://codereview.appspot.com/86920043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:168: logger.Infof("calling ensureMongoServer")
On 2014/04/14 16:52:57, dimitern wrote:
> Now this looks more suitable for a Debugf.

+1. or delete.

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go
File replicaset/replicaset.go (right):

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go#n...
replicaset/replicaset.go:277: monotonicSession := session.Clone()
On 2014/04/14 16:52:57, dimitern wrote:
> Why the need to Clone() the session ?

Because we don't want any side effects on the session (it can cause other things
to fail) but we do need the session in monotonic mode when we manipulate the
replica set.

[axw, 2014-04-15 01:39:28]

On 2014/04/14 15:37:42, nate.finch wrote:
> Please take a look.
> 
> https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go
> File agent/mongo/mongo.go (left):
> 
> https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#oldcode245
> agent/mongo/mongo.go:245: // NOTE: ensure that the right package is installed?
> On 2014/04/14 04:50:54, axw wrote:
> > This NOTE can go I think?
> 
> Done.
> 
> https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go
> File agent/mongo/mongo.go (right):
> 
> https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode265
> agent/mongo/mongo.go:265: func aptGetInstallMongod() error {
> On 2014/04/14 04:50:54, axw wrote:
> > It would be good to add a log message here, otherwise I think "juju
bootstrap"
> > will just sit there with no feedback while it downloads mongodb-server.
> 
> Done.
> 
> https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode266
> agent/mongo/mongo.go:266: // Only Quantal requires the PPA (for mongo).
> On 2014/04/14 04:50:54, axw wrote:
> > I think the "(for mongo)" can be dropped now.
> 
> Done.
> 
> https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode272
> agent/mongo/mongo.go:272: cmds :=
> utils.AptGetPreparePackages([]string{"mongodb-server"},
version.Current.Series)
> On 2014/04/14 15:11:00, rog wrote:
> > On 2014/04/14 04:50:54, axw wrote:
> > > We should be using juju-mongodb if it's available?
> > 
> > The existing code doesn't use juju-mongodb. What should we do to check if
it's
> > available?
> 
> We don't install it right now, we can modify this behavior later, but for now,
> let's not block this branch on that.

No problems, I was under the mistaken impression we were already.

> https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode272
> agent/mongo/mongo.go:272: cmds :=
> utils.AptGetPreparePackages([]string{"mongodb-server"},
version.Current.Series)
> On 2014/04/14 04:50:54, axw wrote:
> > I think it would be quite nice to see some progress out of apt-get here (for
> > bootstrap), but I understand if it's a pain to implement.
> 
> I think it would be nice, but I think it'll have to wait.

No problem.

> https://codereview.appspot.com/86920043/diff/1/agent/mongo/mongo.go#newcode282
> agent/mongo/mongo.go:282: cmd := exec.Command("add-apt-repository", "-y",
name)
> On 2014/04/14 04:50:54, axw wrote:
> > This requires python-software-properties to be installed; please
AptGetInstall
> > that first.
> 
> Done.

Thanks for the updates. LGTM with Dimiter's suggestions.

[rog, 2014-04-15 15:51:27]

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

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:305: func splitCall(c *gc.C, part string) []string {
On 2014/04/14 16:52:57, dimitern wrote:
> doc comment

Done.

https://codereview.appspot.com/86920043/diff/20001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:316: func failCmd(path string) {
On 2014/04/14 16:52:57, dimitern wrote:
> doc comment

Done.

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go
File replicaset/replicaset.go (right):

https://codereview.appspot.com/86920043/diff/20001/replicaset/replicaset.go#n...
replicaset/replicaset.go:28: func Initiate(session *mgo.Session, address, name
string, tags map[string]string) error {
On 2014/04/14 16:52:57, dimitern wrote:
> I can't see description of tags or tests about it.

Done.

[natefinch, 2014-04-15 16:02:36]

Please take a look.