Enable the local provider.

Enable the local provider.

This branch changes how the local storage is handled.
It really wasn't possible to have the client keep serving
the storage on dynamic ports and expect other running
services to work.

Also we need something always acting as the storage. After
discussion with William last night, we agreed that this
should be a worker in the machine agent.

https://code.launchpad.net/~thumper/juju-core/local-provider/+merge/174930

Requires: https://code.launchpad.net/~thumper/juju-core/selectively-install-lxc/+merge/174928

(do not edit description out of merge proposal)

[thumper, 2013-07-16 06:34:52]

Please take a look.

[mue, 2013-07-16 10:16:35]

LGTM, great work, but some comments.

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

https://codereview.appspot.com/11333043/diff/1/cmd/jujud/machine.go#newcode189
cmd/jujud/machine.go:189: if providerType != "local" && m.ContainerType() !=
instance.LXC {
Make "local" a const.

https://codereview.appspot.com/11333043/diff/1/cmd/jujud/machine.go#newcode197
cmd/jujud/machine.go:197: if providerType == "local" && m.Id() == "0" {
Also a const with a speaking name for the "0".

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go
File environs/local/environ.go (right):

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:199: // If we get to here, it is because we haven't
yet bootstrapped an
Interesting hurdle. ;) But I would like a construct like 

if !config.bootstrapped() {
    return env.<findAGoodNameForTheStuffBelow>()
}
return nil

more. So that logical block of code has its own method.

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:229: func (env *localEnviron) setupLocalStorage()
error {
Appreciate comment event for private methods.

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:287: if inst.Id() == "localhost" {
Again I like a const here. In the error you name it "bootstrap instance". So 

if inst.Id() == bootstrapInstance { ... }

would read better.

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:311: instances = append(instances,
&localInstance{"localhost", env})
Const, see above.

[gz, 2013-07-16 10:44:18]

Unless I'm missing something, having both config and JUJU_ envvars appears to be
redundant. I would strongly prefer not having the storage ports configurable,
and just dynamically assign them and pass through.

Have yet to actually play with this, but looks pretty close to useful.

https://codereview.appspot.com/11333043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11333043/diff/1/environs/local/config.go#newco...
environs/local/config.go:31: "shared-storage-port": 8041,
Any reasoning behind these default ports?

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go
File environs/local/environ.go (right):

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:298: func (env *localEnviron) Instances(ids
[]instance.Id) ([]instance.Instance, error) {
It's now possible to implement this correctly, by for instance calling
AllInstances then filtering.

[thumper, 2013-07-17 03:10:11]

Please take a look.

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

https://codereview.appspot.com/11333043/diff/1/cmd/jujud/machine.go#newcode189
cmd/jujud/machine.go:189: if providerType != "local" && m.ContainerType() !=
instance.LXC {
On 2013/07/16 10:16:35, mue wrote:
> Make "local" a const.

Done.

https://codereview.appspot.com/11333043/diff/1/cmd/jujud/machine.go#newcode197
cmd/jujud/machine.go:197: if providerType == "local" && m.Id() == "0" {
On 2013/07/16 10:16:35, mue wrote:
> Also a const with a speaking name for the "0".

Done.

https://codereview.appspot.com/11333043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11333043/diff/1/environs/local/config.go#newco...
environs/local/config.go:31: "shared-storage-port": 8041,
On 2013/07/16 10:44:18, gz wrote:
> Any reasoning behind these default ports?

None at all, but I needed some defaults.

8000 and 8080 are often taken by other web thingies, and in the 8000 range is
often thought of as http type ones, so I thought it reasonable.

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go
File environs/local/environ.go (right):

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:229: func (env *localEnviron) setupLocalStorage()
error {
On 2013/07/16 10:16:35, mue wrote:
> Appreciate comment event for private methods.

Done.

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:287: if inst.Id() == "localhost" {
On 2013/07/16 10:16:35, mue wrote:
> Again I like a const here. In the error you name it "bootstrap instance". So 
> 
> if inst.Id() == bootstrapInstance { ... }
> 
> would read better.

Done.

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:311: instances = append(instances,
&localInstance{"localhost", env})
On 2013/07/16 10:16:35, mue wrote:
> Const, see above.

Done.

[thumper, 2013-07-17 03:40:42]

On 2013/07/16 10:44:18, gz wrote:
> Unless I'm missing something, having both config and JUJU_ envvars appears to
be
> redundant. I would strongly prefer not having the storage ports configurable,
> and just dynamically assign them and pass through.

Perhaps this was just being a little lazy as it meant that the worker didn't
need to watch for environ changes in the configuration.  It made the worker
simpler, and that was what I was going for initially.

>
https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
> environs/local/environ.go:298: func (env *localEnviron) Instances(ids
> []instance.Id) ([]instance.Instance, error) {
> It's now possible to implement this correctly, by for instance calling
> AllInstances then filtering.

I still kinda argue the point about caring for this.

[wallyworld, 2013-07-17 03:40:55]

I shared Martin's concerns about the env variables. But we could land this and
fix it before the next release.

LGTM

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go
File environs/local/environ.go (right):

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:341: return err
Should we try and do as much removal as we can here even if something errors?

[thumper, 2013-07-17 03:45:05]

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go
File environs/local/environ.go (right):

https://codereview.appspot.com/11333043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:341: return err
On 2013/07/17 03:40:55, wallyworld wrote:
> Should we try and do as much removal as we can here even if something errors?

While I don't expect errors here, it seems weird to try to special case it.