mongo stuff to agent/mongod, add EnsureMongoServer

mongo stuff to agent/mongod, add EnsureMongoServer

This is part of the EnsureMongoServer work. The EnsureMongoServer function will be used in a different CL, but it was useful to commit it separately.

https://code.launchpad.net/~natefinch/juju-core/037-EnsureMongo/+merge/208688

(do not edit description out of merge proposal)

[natefinch, 2014-02-27 21:07:41]

Please take a look.

[wallyworld, 2014-02-28 00:04:23]

We are missing tests for removeOldMongoServices and ensureMongoServer. I don't
think this should land without those tests.

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

https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode25
agent/mongo/mongo.go:25: mongoScriptVersion = 2
I think it would be better to have MongoUpstartService() return the upstart
config and the script version. That way, you change the script, you change the
version at the same time. And you'd add a comment right inside the function so
that it is clear what needs to be done. As it is now, it would be too easy to
change the script and not know to change this value.

https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode36
agent/mongo/mongo.go:36: JujuMongodPath = "/usr/lib/juju/bin/mongod"
Would it be better for this to be unexported and a helper function in
export_test used to change its value?

https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode57
agent/mongo/mongo.go:57: service := MongoUpstartService(mongoServiceName, dir,
port)
So have mongoScriptVersion be returned here

https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode62
agent/mongo/mongo.go:62: if err := removeOldMongoServices(); err != nil {
and mongoScriptVersion passed in as a parameter here

https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode84
agent/mongo/mongo.go:84: if err := service.Install(); err != nil {
Should we remove the prealloc files or do other cleanup if the service fails to
install?

https://codereview.appspot.com/69600043/diff/1/upstart/service.go
File upstart/service.go (right):

https://codereview.appspot.com/69600043/diff/1/upstart/service.go#newcode15
upstart/service.go:15: MaxAgentFiles = 20000
maxMongoFiles is now defined in mongo.go and not used here anymore.
I think MaxAgentFiles should also be moved to mongo.go

[natefinch, 2014-02-28 21:13:01]

Please take a look.

[natefinch, 2014-02-28 21:16:53]

Please take a look.

[natefinch, 2014-02-28 21:57:59]

Please take a look.

[natefinch, 2014-02-28 22:01:32]

On 2014/02/28 00:04:23, wallyworld wrote:
> We are missing tests for removeOldMongoServices and ensureMongoServer. I don't
> think this should land without those tests.

Absolutely. I thought I had already written these, but apparently not.  They're
in there now.

> https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go
> File agent/mongo/mongo.go (right):
> 
> https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode25
> agent/mongo/mongo.go:25: mongoScriptVersion = 2
> I think it would be better to have MongoUpstartService() return the upstart
> config and the script version. That way, you change the script, you change the
> version at the same time. And you'd add a comment right inside the function so
> that it is clear what needs to be done. As it is now, it would be too easy to
> change the script and not know to change this value.

That's a good idea.  I'll do that once we're actually using this code 100%.
Right now I can't, because the code that calls it expects to be able to pass in
to the mongo service name.
 
> https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode36
> agent/mongo/mongo.go:36: JujuMongodPath = "/usr/lib/juju/bin/mongod"
> Would it be better for this to be unexported and a helper function in
> export_test used to change its value?

Normally, yes, but this value needs to be able to be mocked out for local
provider as well, and it doesn't have access to test-only code in this package. 
I changed it so that it's no longer a public variable, but instead there's a
MockPackage() method which will set that value.... that's at least less likely
to get accidentally abused (as has happened with other "only for testing" public
variables).

> https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode57
> agent/mongo/mongo.go:57: service := MongoUpstartService(mongoServiceName, dir,
> port)
> So have mongoScriptVersion be returned here
> 
> https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode62
> agent/mongo/mongo.go:62: if err := removeOldMongoServices(); err != nil {
> and mongoScriptVersion passed in as a parameter here

I did this part, I'll have MongoUpstartServer return the version when I merge
this with the rest of the ensure mongo server stuff

> https://codereview.appspot.com/69600043/diff/1/agent/mongo/mongo.go#newcode84
> agent/mongo/mongo.go:84: if err := service.Install(); err != nil {
> Should we remove the prealloc files or do other cleanup if the service fails
to
> install?

It probably doesn't matter. Not being able to install mongo is catastrophic and
will likely require terminating the instance and starting over.  However, in the
weird case that this is just a temporary error, the code will retry later, in
which case this code is idempotent, so we don't need to worry about it being
rerun.  Either way, deleting those files is probably not necessary.

> https://codereview.appspot.com/69600043/diff/1/upstart/service.go
> File upstart/service.go (right):
> 
> https://codereview.appspot.com/69600043/diff/1/upstart/service.go#newcode15
> upstart/service.go:15: MaxAgentFiles = 20000
> maxMongoFiles is now defined in mongo.go and not used here anymore.
> I think MaxAgentFiles should also be moved to mongo.go

MaxAgentFiles is used by both the Agent upstart script and the mongo upstart
script, so it kinda has to be accessible in both places.

[rog, 2014-03-03 17:04:06]

The business logic looks good. I've made
some suggestions for changes to the mocking approach.

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

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:33: func MockPackage() func() {
I'm not that keen on this. For the purposes of mocking, I think
it would be fine just to export JujuMongoDefaultPath.

How about this:

// PreferredMongodPath holds the preferred path
// of the mongod executable. It is exposed as a variable for
// testing purposes.
var PreferredMongodPath = "/usr/lib/juju/bin/mongod"

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:39: // machine. If the juju-bundled version of mongo
exists, it will return that
I'd be happier if this always returned an absolute path.

// MongodPath returns the mongod executable
// path, PreferredMongodPath by preference.
// It returns an error if it cannot
// find the executable.
func MongodPath() (string, error)

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:55: func ensureMongoServer(dir string, port int) error {
Shouldn't this be exported?

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:71: logger.Errorf("Failed to install mongo service %q: %v",
service.Name, err)
Please return this informative message as part of the error - that
way it will be seen in the correct context eventually:

return fmt.Errorf("failed to install mongo service %q: %v", service.Name, err)

I don't think there's a need for calling logger.Errorf here.

Similarly below.

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:86: zeroes := make([]byte, 1024*1024)
I'd prefer to avoid allocating an unnecessary megabyte
here. Please write the file in chunks (64K for example).

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:91: logger.Errorf("failed to make write mongo prealloc
file: %v", journalDir, err)
Have you run go vet?

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:128: // and the version number of that config.
This doesn't seem to be returning a version number.

https://codereview.appspot.com/69600043/diff/60001/upstart/service.go
File upstart/service.go (right):

https://codereview.appspot.com/69600043/diff/60001/upstart/service.go#newcode15
upstart/service.go:15: MaxAgentFiles = 20000
Doc comment would be good here, now it's exported.

BTW, why does it need to be exported now?

https://codereview.appspot.com/69600043/diff/60001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/69600043/diff/60001/upstart/upstart.go#newcode25
upstart/upstart.go:25: var initDir = defaultInitDir
Why not just export this variable, rather than all the MockPackage stuff?

https://codereview.appspot.com/69600043/diff/60001/upstart/upstart.go#newcode28
upstart/upstart.go:28: rand.Seed(time.Now().UTC().UnixNano())
This is not the right place for this.

If we want the default random number generator
to be randomly seeded, then it should be
done inside main (I actually think that's
something we should do).

https://codereview.appspot.com/69600043/diff/60001/upstart/upstart.go#newcode33
upstart/upstart.go:33: func MockPackage() func() {
I'd prefer not to have this as a precedent.

[natefinch, 2014-03-03 19:04:53]

Please take a look.

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

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:33: func MockPackage() func() {
On 2014/03/03 17:04:07, rog wrote:
> I'm not that keen on this. For the purposes of mocking, I think
> it would be fine just to export JujuMongoDefaultPath.
> 
> How about this:
> 
> // PreferredMongodPath holds the preferred path
> // of the mongod executable. It is exposed as a variable for
> // testing purposes.
> var PreferredMongodPath = "/usr/lib/juju/bin/mongod"

Seems John is also not a fan of the MockPackage approach, especially since he
thinks we may need to expose the variables to production code at some point
anyway.

I kind of like having a separate constant that is the default for the variable,
since it means you can have an idempotent reset to default.

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:39: // machine. If the juju-bundled version of mongo
exists, it will return that
On 2014/03/03 17:04:07, rog wrote:
> I'd be happier if this always returned an absolute path.
> 
> // MongodPath returns the mongod executable
> // path, PreferredMongodPath by preference.
> // It returns an error if it cannot
> // find the executable.
> func MongodPath() (string, error)

The reason I'd done it this way is because the tests often mock out the $PATH
and then which was failing, but I can replace with /usr/bin/which and return an
error, too.

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:55: func ensureMongoServer(dir string, port int) error {
On 2014/03/03 17:04:07, rog wrote:
> Shouldn't this be exported?

Uh yeah.  It was copied from the Machine agent, where it wasn't exported. Fixed.

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:71: logger.Errorf("Failed to install mongo service %q: %v",
service.Name, err)
On 2014/03/03 17:04:07, rog wrote:
> Please return this informative message as part of the error - that
> way it will be seen in the correct context eventually:
> 
> return fmt.Errorf("failed to install mongo service %q: %v", service.Name, err)
> 
> I don't think there's a need for calling logger.Errorf here.
> 
> Similarly below.

Done.

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:86: zeroes := make([]byte, 1024*1024)
On 2014/03/03 17:04:07, rog wrote:
> I'd prefer to avoid allocating an unnecessary megabyte
> here. Please write the file in chunks (64K for example).

Done.

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:91: logger.Errorf("failed to make write mongo prealloc
file: %v", journalDir, err)
On 2014/03/03 17:04:07, rog wrote:
> Have you run go vet?

I just noticed that before seeing this comment.  I hadn't been doing it, since I
thought lbox ran go vet... anyway, I'll start running it manually.  Fixed.

https://codereview.appspot.com/69600043/diff/60001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:128: // and the version number of that config.
On 2014/03/03 17:04:07, rog wrote:
> This doesn't seem to be returning a version number.

Refactored some code and forgot to update the comment. Fixed.

https://codereview.appspot.com/69600043/diff/60001/upstart/service.go
File upstart/service.go (right):

https://codereview.appspot.com/69600043/diff/60001/upstart/service.go#newcode15
upstart/service.go:15: MaxAgentFiles = 20000
On 2014/03/03 17:04:07, rog wrote:
> Doc comment would be good here, now it's exported.
> 
> BTW, why does it need to be exported now?

I unexported it.  It was being used in both the machine agent upstart script and
the mongo upstart script.   But there's no actual need for them both to use the
same constant, so I just copied it to an unexported constant in agent/mongo too.

https://codereview.appspot.com/69600043/diff/60001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/69600043/diff/60001/upstart/upstart.go#newcode25
upstart/upstart.go:25: var initDir = defaultInitDir
On 2014/03/03 17:04:07, rog wrote:
> Why not just export this variable, rather than all the MockPackage stuff?

As I said in my email, I don't really like just exposing a variable and trusting
people not to touch it, especially when it doesn't really make itself very
obvious that it should be untouchable. However, it seems that John agrees and
would rather it was exposed so that if/when production code needs to twiddle
with stuff, it can.  So, I reverted this and instead have just exported the
variable.

https://codereview.appspot.com/69600043/diff/60001/upstart/upstart.go#newcode28
upstart/upstart.go:28: rand.Seed(time.Now().UTC().UnixNano())
On 2014/03/03 17:04:07, rog wrote:
> This is not the right place for this.
> 
> If we want the default random number generator
> to be randomly seeded, then it should be
> done inside main (I actually think that's
> something we should do).

Removed (see above).

https://codereview.appspot.com/69600043/diff/60001/upstart/upstart.go#newcode33
upstart/upstart.go:33: func MockPackage() func() {
On 2014/03/03 17:04:07, rog wrote:
> I'd prefer not to have this as a precedent.

Removed (see above)

[rog, 2014-03-04 16:24:45]

LGTM with a few trivial suggestions.

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:29: JujuMongodPath = JujuMongodPathDefault
or just:
JujuMongodPath = "/usr/lib/juju/bin/mongod"

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:40: s, err := exec.Command("/usr/bin/which",
"mongod").Output()
path, err := exec.LookPath("mongod")
if err != nil {
    return err
}
return path, nil

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:100: for y := 0; y < 16; y++ {
or a little more directly.

for total := 0; total < 1024 * 1024; {
     n, err := f.Write(zeroes)
     if err != nil {
        ...
     }
     total += n
}

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

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:56: err := ioutil.WriteFile(filename, []byte{}, 0777)
s/[]byte{}/nil/

?

https://codereview.appspot.com/69600043/diff/80001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/69600043/diff/80001/upstart/upstart.go#newcode24
upstart/upstart.go:24: var InitDir = InitDirDefault
// InitDir holds the default init directory name.
var InitDir = "/etc/init"

?

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

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:29: JujuMongodPath = JujuMongodPathDefault
On 2014/03/04 16:24:46, rog wrote:
> or just:
> JujuMongodPath = "/usr/lib/juju/bin/mongod"

Done.

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:40: s, err := exec.Command("/usr/bin/which",
"mongod").Output()
On 2014/03/04 16:24:46, rog wrote:
> path, err := exec.LookPath("mongod")
> if err != nil {
>     return err
> }
> return path, nil

I always forget about LookPath.  Done.

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:100: for y := 0; y < 16; y++ {
On 2014/03/04 16:24:46, rog wrote:
> or a little more directly.
> 
> for total := 0; total < 1024 * 1024; {
>      n, err := f.Write(zeroes)
>      if err != nil {
>         ...
>      }
>      total += n
> }

Done.

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

https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo_test.go#...
agent/mongo/mongo_test.go:56: err := ioutil.WriteFile(filename, []byte{}, 0777)
On 2014/03/04 16:24:46, rog wrote:
> s/[]byte{}/nil/
> 
> ?

Done.

https://codereview.appspot.com/69600043/diff/80001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/69600043/diff/80001/upstart/upstart.go#newcode24
upstart/upstart.go:24: var InitDir = InitDirDefault
On 2014/03/04 16:24:46, rog wrote:
> // InitDir holds the default init directory name.
> var InitDir = "/etc/init"
> 
> ?

Done.

[rog, 2014-03-05 16:18:23]

On 2014/03/04 17:01:04, nate.finch wrote:
> https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go
> File agent/mongo/mongo.go (right):
> 
>
https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go#newco...
> agent/mongo/mongo.go:29: JujuMongodPath = JujuMongodPathDefault
> On 2014/03/04 16:24:46, rog wrote:
> > or just:
> > JujuMongodPath = "/usr/lib/juju/bin/mongod"
> 
> Done.
> 
>
https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go#newco...
> agent/mongo/mongo.go:40: s, err := exec.Command("/usr/bin/which",
> "mongod").Output()
> On 2014/03/04 16:24:46, rog wrote:
> > path, err := exec.LookPath("mongod")
> > if err != nil {
> >     return err
> > }
> > return path, nil
> 
> I always forget about LookPath.  Done.
> 
>
https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo.go#newco...
> agent/mongo/mongo.go:100: for y := 0; y < 16; y++ {
> On 2014/03/04 16:24:46, rog wrote:
> > or a little more directly.
> > 
> > for total := 0; total < 1024 * 1024; {
> >      n, err := f.Write(zeroes)
> >      if err != nil {
> >         ...
> >      }
> >      total += n
> > }
> 
> Done.
> 
> https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo_test.go
> File agent/mongo/mongo_test.go (right):
> 
>
https://codereview.appspot.com/69600043/diff/80001/agent/mongo/mongo_test.go#...
> agent/mongo/mongo_test.go:56: err := ioutil.WriteFile(filename, []byte{},
0777)
> On 2014/03/04 16:24:46, rog wrote:
> > s/[]byte{}/nil/
> > 
> > ?
> 
> Done.
> 
> https://codereview.appspot.com/69600043/diff/80001/upstart/upstart.go
> File upstart/upstart.go (right):
> 
>
https://codereview.appspot.com/69600043/diff/80001/upstart/upstart.go#newcode24
> upstart/upstart.go:24: var InitDir = InitDirDefault
> On 2014/03/04 16:24:46, rog wrote:
> > // InitDir holds the default init directory name.
> > var InitDir = "/etc/init"
> > 
> > ?
> 
> Done.

I think these changes still remain to be pushed.

[natefinch, 2014-03-05 16:25:38]

Please take a look.