provider/local: always use filestorage on CLI

provider/local: always use filestorage on CLI

The local provider is modified such that all
storage interactions from the CLI go directly
to disk, and the provider code no longer starts
an httpstorage listener. To achieve this, we
no longer store bootstrap-ip in the .jenv, but
instead add it to the environment config during
bootstrap. Thus, we can use the presence of the
attribute to decide whether or not to use the
file or http storage. BootstrapStorager no longer
needs to be implemented, so EnableBootsrapStorage
has been eliminated from the implementation.

For old environments, bootstrap-ip will be set
in the .jenv and thus httpstorage is used;
this is fine, as long as the machine agent is
running. There's an edge case that will no longer
work, where the environment is bootstrapped but
machine-0 agent is not running. In this case,
storage is inaccessible.

There are some other drive-by fixes to tests
that improve isolation, and some redundant tests
relating to the old sudo behaviour are removed.
All tests now pass with or without a local provider
running on the machine.

Fixes lp:1271672

https://code.launchpad.net/~axwalk/juju-core/local-provider-testability/+merge/204169

(do not edit description out of merge proposal)

[axw, 2014-01-31 09:03:13]

Please take a look.

[rog, 2014-02-03 09:37:40]

LGTM although I'd like a second opinion from someone that's more familiar with
local provider issues.

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

https://codereview.appspot.com/49640050/diff/1/provider/local/environ.go#newc...
provider/local/environ.go:235: // continue on to set up things prior to
bootstrap.
This comment doesn't read easily to me.

How about something like this:

// When the localEnviron value is created on the client
// side, the bootstrap-ip attribute will not exist,
// because it is only set *within* the running
// environment, not in the configuration created
// by Prepare.
// Thus when bootstrapIPAddress returns a non-empty
// string, we know we are running server-side and
// thus must use the network API to access the storage.

?

https://codereview.appspot.com/49640050/diff/1/provider/local/environprovider...
File provider/local/environprovider_test.go (left):

https://codereview.appspot.com/49640050/diff/1/provider/local/environprovider...
provider/local/environprovider_test.go:59: c.Skip("fails if local provider
running already")
I presume you've checked that this doesn't still
fail if a local environment is currently running.

https://codereview.appspot.com/49640050/diff/1/provider/local/export_test.go
File provider/local/export_test.go (right):

https://codereview.appspot.com/49640050/diff/1/provider/local/export_test.go#...
provider/local/export_test.go:54: func MockAddressForInterface() func() {
This might be easier just as:

PatchValue(local.GetAddressForInterface, func(name string) (string, error) etc)

but perhaps that's a change for a future CL.

[axw, 2014-02-11 07:57:23]

Please take a look.

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

https://codereview.appspot.com/49640050/diff/1/provider/local/environ.go#newc...
provider/local/environ.go:235: // continue on to set up things prior to
bootstrap.
On 2014/02/03 09:37:40, rog wrote:
> This comment doesn't read easily to me.
> 
> How about something like this:
> 
> // When the localEnviron value is created on the client
> // side, the bootstrap-ip attribute will not exist,
> // because it is only set *within* the running
> // environment, not in the configuration created
> // by Prepare.
> // Thus when bootstrapIPAddress returns a non-empty
> // string, we know we are running server-side and
> // thus must use the network API to access the storage.
> 
> ?

Done.

https://codereview.appspot.com/49640050/diff/1/provider/local/environprovider...
File provider/local/environprovider_test.go (left):

https://codereview.appspot.com/49640050/diff/1/provider/local/environprovider...
provider/local/environprovider_test.go:59: c.Skip("fails if local provider
running already")
On 2014/02/03 09:37:40, rog wrote:
> I presume you've checked that this doesn't still
> fail if a local environment is currently running.

Correct.

https://codereview.appspot.com/49640050/diff/1/provider/local/export_test.go
File provider/local/export_test.go (right):

https://codereview.appspot.com/49640050/diff/1/provider/local/export_test.go#...
provider/local/export_test.go:54: func MockAddressForInterface() func() {
On 2014/02/03 09:37:40, rog wrote:
> This might be easier just as:
> 
> PatchValue(local.GetAddressForInterface, func(name string) (string, error)
etc)
> 
> but perhaps that's a change for a future CL.

I've changed it to use PatchValue here. Another CL in the future can move that
up the stack if it's deemed desirable.

[thumper, 2014-02-12 02:17:44]

LGTM

https://codereview.appspot.com/49640050/diff/20001/provider/local/environprov...
File provider/local/environprovider_test.go (left):

https://codereview.appspot.com/49640050/diff/20001/provider/local/environprov...
provider/local/environprovider_test.go:59: c.Skip("fails if local provider
running already")
\o/