support mongo namespaces

support mongo namespaces

This change consolidates the name of the juju mongo database where it belongs - in the code that creates the scripts for mongo.  It also fixes several bugs that were introduced due to the name change of the mongo service. Finally, it removes the ability to specify the exact name of the mongo service, but it still lets you add a namespace to prevent conflicts.  This is necessary to support removal of old scripts.... the mongo code needs to control how the names are constructed.

https://code.launchpad.net/~natefinch/juju-core/038-namespace/+merge/213334

(do not edit description out of merge proposal)

[natefinch, 2014-03-28 20:40:26]

Please take a look.

[jameinel, 2014-03-30 10:19:35]

I'm ok with the code, but I'm not sure why we need the juju-db-v2 part to the
name. It just seems like we're going to never remove something newer than what
we think we're doing, (as in a v2 client won't see a v3 when it tries to start a
v2), etc.

I'm happy to have the code consolidated, and it does appear better than what we
have, so while I'd like to understand it better, LGTM.

[rog, 2014-03-31 18:11:50]

Looks good in general.

I tend to agree with john here. As discussed online, can we avoid making a new
service name - to upgrade the service config, we can just check if the content
changes, and change the file only if so. It shouldn't be hard to keep the
upstart file deterministic.

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

https://codereview.appspot.com/81980043/diff/1/agent/mongo/mongo.go#newcode68
agent/mongo/mongo.go:68: func RemoveService(namespace string) error {
doc comment please.

and some explanation of what a "namespace" is (although that doesn't necessarily
need to go on this particular function)

https://codereview.appspot.com/81980043/diff/1/agent/mongo/mongo.go#newcode74
agent/mongo/mongo.go:74: func ServiceName(namespace string) string {
ditto

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

https://codereview.appspot.com/81980043/diff/1/agent/mongo/mongo_test.go#newc...
agent/mongo/mongo_test.go:92: c.Check(err, gc.IsNil)
I'd keep this as an assert.
There's not much point checking if the service has no longer been installed if
the call fails, and the extra messages will just make the original failure less
obvious.

[natefinch, 2014-04-01 20:48:02]

Please take a look.

[rog, 2014-04-02 16:24:15]

Looks much better, thanks. A few comments and suggestions below.

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

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:81: // on this machine from colliding (only really needed
by the local provider).
s/(only really needed by the local provider)/This should be empty unless using
the local provider/

?

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:108: contents, err := service.ReadConf()
perhaps:

if err != nil {
    if os.IsNotExist(err) {
       // service doesn't exist, no problem
       return nil
    }
    return err
}

then we can lose the indent on the rest of the code.

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:112: expected, err := service.Render()
I'm not sure about this. It's kinda second-guessing the behaviour of
upstart.Install. I wonder if it would be better to change upstart.Conf.Install
so that it implements the same behaviour (if you're calling Install, you
probably don't want to stop the service if the contents haven't changed).

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:116: if string(contents) != string(expected) {
if !bytes.Equal(contents, expected) {

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:131: func ServiceName(namespace string) string {
doc comment please

https://codereview.appspot.com/81980043/diff/20001/cmd/plugins/juju-backup/ju...
File cmd/plugins/juju-backup/juju-backup (right):

https://codereview.appspot.com/81980043/diff/20001/cmd/plugins/juju-backup/ju...
cmd/plugins/juju-backup/juju-backup:54: execute 'Starting mongo' start
juju-db-v2
i think this is now wrong.

https://codereview.appspot.com/81980043/diff/20001/cmd/plugins/juju-backup/ju...
cmd/plugins/juju-backup/juju-backup:71: /etc/init/juju-db-v2.conf \
ditto

https://codereview.appspot.com/81980043/diff/20001/cmd/plugins/juju-restore/r...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/81980043/diff/20001/cmd/plugins/juju-restore/r...
cmd/plugins/juju-restore/restore.go:108: initctl stop juju-db-v2
this too

https://codereview.appspot.com/81980043/diff/20001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/81980043/diff/20001/upstart/upstart.go#newcode44
upstart/upstart.go:44: func (s *Service) ReadConf() ([]byte, error) {
If we put the behaviour inside Install, we don't need to expose this, or Render.

[natefinch, 2014-04-07 11:20:42]

Please take a look.

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

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:81: // on this machine from colliding (only really needed
by the local provider).
On 2014/04/02 16:24:15, rog wrote:
> s/(only really needed by the local provider)/This should be empty unless using
> the local provider/
> 
> ?

Done.

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:108: contents, err := service.ReadConf()
On 2014/04/02 16:24:15, rog wrote:
> perhaps:
> 
> if err != nil {
>     if os.IsNotExist(err) {
>        // service doesn't exist, no problem
>        return nil
>     }
>     return err
> }
> 
> then we can lose the indent on the rest of the code.

Done.

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:112: expected, err := service.Render()
On 2014/04/02 16:24:15, rog wrote:
> I'm not sure about this. It's kinda second-guessing the behaviour of
> upstart.Install. I wonder if it would be better to change upstart.Conf.Install
> so that it implements the same behaviour (if you're calling Install, you
> probably don't want to stop the service if the contents haven't changed).

Yeah, I almost put this code in there, but wasn't sure if that was the right
call or not. I'll move it.

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:116: if string(contents) != string(expected) {
On 2014/04/02 16:24:15, rog wrote:
> if !bytes.Equal(contents, expected) {

Done.

https://codereview.appspot.com/81980043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:131: func ServiceName(namespace string) string {
On 2014/04/02 16:24:15, rog wrote:
> doc comment please

Done.

https://codereview.appspot.com/81980043/diff/20001/cmd/plugins/juju-backup/ju...
File cmd/plugins/juju-backup/juju-backup (right):

https://codereview.appspot.com/81980043/diff/20001/cmd/plugins/juju-backup/ju...
cmd/plugins/juju-backup/juju-backup:54: execute 'Starting mongo' start
juju-db-v2
On 2014/04/02 16:24:15, rog wrote:
> i think this is now wrong.

Oh weird,  I definitely fixed this, not sure how it got put back.  I'll re-fix
it.

https://codereview.appspot.com/81980043/diff/20001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/81980043/diff/20001/upstart/upstart.go#newcode44
upstart/upstart.go:44: func (s *Service) ReadConf() ([]byte, error) {
On 2014/04/02 16:24:15, rog wrote:
> If we put the behaviour inside Install, we don't need to expose this, or
Render.

Yep, absolutely.  Good call.

[rog, 2014-04-07 13:09:10]

LGTM with one trivial suggestion.

https://codereview.appspot.com/81980043/diff/40001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/81980043/diff/40001/upstart/upstart.go#newcode168
upstart/upstart.go:168: // Render returns the upstart configuration for the
service as a slice of bytes.
This comment should be reverted.