Implement manual bootstrapping

Implement manual bootstrapping

environs/bootstrap:
  - Added BootstrapStorage interface. If an Environ
    implements this interface, its BootstrapStorage
    will be used by cmd/juju bootstrap.

cmd/juju:
  - Updated bootstrap subcommand to use
    environs/bootstrap.BootstrapStorage,
    as described above.

provider/local/storage:
  - Added LocalStorageConfig interface, which an
    Environ may implement to describe the configuration
    of a "localstorage".

environs/filestorage:
  - Added NewFileStorage (i.e. implement StorageWrite)
    for testing in environs/manual.

environs/manual:
  - Refactored existing code to support bootstrap machines.
  - Added "Bootstrap" function to manually bootstrap an
    existing machine.

To manually bootstrap, you must provide an environment that
implements the new LocalStorageConfig and BootstrapStorage
interfaces. In other words, you can (currently) only manually
bootstrap a machine which is expected to manage the storage
for its environment. This may be relaxed later if needed.

Next up is the null provider, which uses this new Bootstrap
function. I need to address one last issue with it first:
the null provider mustn't narrow the tools to default-series.

https://code.launchpad.net/~axwalk/juju-core/manual-bootstrap/+merge/184714

(do not edit description out of merge proposal)

[axw, 2013-09-10 02:36:14]

Please take a look.

[axw, 2013-09-11 05:16:27]

Please take a look.

[fwereade, 2013-09-11 13:51:21]

Looking very nice; just a few questions, and a suggestion that you consider
moving the local storage worker into the top-level worker package, rather than
hiding it away under local.

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go#newco...
cmd/juju/bootstrap.go:71: if bs, ok := environ.(bootstrap.BootstrapStorage); ok
{
Would that maybe be ".(BootstrapStorager)"? Elsewhere, we're experimenting with
things like:

type HasBootstrapStorage interface {
    BootstrapStorage() (Storage, error)
}

...which IMO leads at least to more-readable casts. YMMV.

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filest...
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filest...
environs/filestorage/filestorage.go:114: _, err = io.CopyN(file, r, length)
As suggested in the previous review, please copy the whole lot somewhere out of
the way and move atomically into place on success; in this case I think we've
definitely vulnerable to errors with longer-term consequences.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.g...
environs/manual/bootstrap.go:143: }
This is really nice; can we produce some output while we're doing so? I guess
it'll have to be straight to the log for now, but a few INFO messages would be
helpful.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.g...
environs/manual/bootstrap.go:146: // get the *state.Machine?
Don't think so...

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
File environs/manual/bootstrap_test.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
environs/manual/bootstrap_test.go:153: c.Assert(Bootstrap(args),
gc.ErrorMatches, "Environ argument is nil")
FWIW, I'm kinda ok with panics for unexpected nils.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
environs/manual/bootstrap_test.go:172: args.MachineId = "1"
Eyebrow slightly raised here... I'm not sure that's actually valid. Do we
initialize the machine id counter in state such that all subsequent numbers are
higher?

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
environs/manual/bootstrap_test.go:204: defer sshresponse(c,
checkProvisionedScript, "", 0)()
This code block is looking awfully familiar now. Can we tidy it up a little?

https://codereview.appspot.com/13635044/diff/4001/environs/manual/tools.go
File environs/manual/tools.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/tools.go#ne...
environs/manual/tools.go:23: arches := possibleTools.Arches()
Why would we be getting multiple arches out of FindInstanceTools? Aren't we
passing one in?

[axw1, 2013-09-12 07:59:00]

Thank you for the review. I have moved provider/local/storage to
worker/localstorage.

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go#newco...
cmd/juju/bootstrap.go:71: if bs, ok := environ.(bootstrap.BootstrapStorage); ok
{
On 2013/09/11 13:51:21, fwereade wrote:
> Would that maybe be ".(BootstrapStorager)"?

Yes, that's a bit better, thanks.

> Elsewhere, we're experimenting with
> things like:
> 
> type HasBootstrapStorage interface {
>     BootstrapStorage() (Storage, error)
> }
> 
> ...which IMO leads at least to more-readable casts. YMMV.

I'm not a big fan of the "HasX" style. IMO, the name should represent the
behaviour or role, not something an implementer has.

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filest...
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filest...
environs/filestorage/filestorage.go:114: _, err = io.CopyN(file, r, length)
On 2013/09/11 13:51:21, fwereade wrote:
> As suggested in the previous review, please copy the whole lot somewhere out
of
> the way and move atomically into place on success; in this case I think we've
> definitely vulnerable to errors with longer-term consequences.

Done. It's a bit simpler here :)

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.g...
environs/manual/bootstrap.go:143: }
On 2013/09/11 13:51:21, fwereade wrote:
> This is really nice; can we produce some output while we're doing so? I guess
> it'll have to be straight to the log for now, but a few INFO messages would be
> helpful.

Sure. I've added some logger.Infofs to checkProvisioned,
detectSeriesAndHardwareCharacteristics and provisionMachineAgent. I've purposely
not added Errorfs, as error results will be logged upstream.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.g...
environs/manual/bootstrap.go:146: // get the *state.Machine?
On 2013/09/11 13:51:21, fwereade wrote:
> Don't think so...

Oops! Nor do I. I had that in there from early on, and forgot to take it out :)

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
File environs/manual/bootstrap_test.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
environs/manual/bootstrap_test.go:153: c.Assert(Bootstrap(args),
gc.ErrorMatches, "Environ argument is nil")
On 2013/09/11 13:51:21, fwereade wrote:
> FWIW, I'm kinda ok with panics for unexpected nils.

Noted.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
environs/manual/bootstrap_test.go:172: args.MachineId = "1"
On 2013/09/11 13:51:21, fwereade wrote:
> Eyebrow slightly raised here... I'm not sure that's actually valid. Do we
> initialize the machine id counter in state such that all subsequent numbers
are
> higher?

The reason for this test is that Environ.Bootstrap is provided with a machine ID
to use. manual.Bootstrap is invoked via the null provider's Bootstrap method.
Thus, the test exists so as not to make assumptions about being on machine "0".

Whether or not the counter is incremented... I don't think that can be its
concern, if it's been provided with the ID.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
environs/manual/bootstrap_test.go:204: defer sshresponse(c,
checkProvisionedScript, "", 0)()
On 2013/09/11 13:51:21, fwereade wrote:
> This code block is looking awfully familiar now. Can we tidy it up a little?

Done.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/tools.go
File environs/manual/tools.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/tools.go#ne...
environs/manual/tools.go:23: arches := possibleTools.Arches()
On 2013/09/11 13:51:21, fwereade wrote:
> Why would we be getting multiple arches out of FindInstanceTools? Aren't we
> passing one in?

Heh, yes. I probably copied that from somewhere. Fixed, thanks.

[axw, 2013-09-12 08:07:52]

Please take a look.

[fwereade, 2013-09-13 09:24:59]

This is great, thanks. Modulo the /tmp-filesystem issue, this LGTM, on the
understanding that the local storage is not yet fit for purpose and needs some
concept of authentication [0] before we can actually let people use this
feature.

Let me know if I've completely misunderstood the overlap between SSHStorage and
LocalStorage, which is not a blocker for this CL but must be resolved in a
followup (if it's how I understood it to be).

[0] or to not be exposed, which we *might* be able to get away with if we always
used SSH storage from outside..?

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go#newco...
cmd/juju/bootstrap.go:71: if bs, ok := environ.(bootstrap.BootstrapStorage); ok
{
On 2013/09/12 07:59:00, axw1 wrote:
> On 2013/09/11 13:51:21, fwereade wrote:
> > Would that maybe be ".(BootstrapStorager)"?
> 
> Yes, that's a bit better, thanks.
> 
> > Elsewhere, we're experimenting with
> > things like:
> > 
> > type HasBootstrapStorage interface {
> >     BootstrapStorage() (Storage, error)
> > }
> > 
> > ...which IMO leads at least to more-readable casts. YMMV.
> 
> I'm not a big fan of the "HasX" style. IMO, the name should represent the
> behaviour or role, not something an implementer has.

Your mileage may vary :). I'm not really sold on either way tbh.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
File environs/manual/bootstrap_test.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_t...
environs/manual/bootstrap_test.go:172: args.MachineId = "1"
On 2013/09/12 07:59:00, axw1 wrote:
> On 2013/09/11 13:51:21, fwereade wrote:
> > Eyebrow slightly raised here... I'm not sure that's actually valid. Do we
> > initialize the machine id counter in state such that all subsequent numbers
> are
> > higher?
> 
> The reason for this test is that Environ.Bootstrap is provided with a machine
ID
> to use. manual.Bootstrap is invoked via the null provider's Bootstrap method.
> Thus, the test exists so as not to make assumptions about being on machine
"0".
> 
> Whether or not the counter is incremented... I don't think that can be its
> concern, if it's been provided with the ID.

Yeah, this isn't your problem. I think it still is a problem that we can allow
bootstrap with arbitrary ids, but that other bits of the system assign special
meaning to "0" (it somehow seems that every time we eliminate a special case for
0 we end up with a new one somewhere else).

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

https://codereview.appspot.com/13635044/diff/14001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:30: localstorage
"launchpad.net/juju-core/worker/localstorage"
drop the explicit name?

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
environs/filestorage/filestorage.go:45: func NewFileStorage(path string)
(environs.Storage, error) {
High-level question while I think of it: does the SSHStorage on bootstrap get
reused but backed with a FileStorage? I sort of imagine so, in which case, cool
...but is it possible for there to be overlap? ie should we be sharing the
flocking everywhere?

or am I just fantasizing?

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
environs/filestorage/filestorage.go:110: file, err := ioutil.TempFile("",
"juju-filestorage-"+name)
Same different-filesystems problem.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
OK, I think my problem here is maybe that it's not *just* about bootstrap
storage, right? It's the same storage we always use, but this is the path to it
that's accessible from a client machine. Yes?

So, at least in the short term, we *will* need SSHStorage and LocalStorage to
coexist.

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker.go
File worker/localstorage/worker.go (left):

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker...
worker/localstorage/worker.go:1: package storage
ah, I see. Can we call this localstorage please?

[axw1, 2013-09-13 09:41:25]

Haven't pushed changes yet, need to sort out the storage issues. Here's my
answers anyway.

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

https://codereview.appspot.com/13635044/diff/14001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:30: localstorage
"launchpad.net/juju-core/worker/localstorage"
On 2013/09/13 09:24:59, fwereade wrote:
> drop the explicit name?

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
environs/filestorage/filestorage.go:45: func NewFileStorage(path string)
(environs.Storage, error) {
On 2013/09/13 09:24:59, fwereade wrote:
> High-level question while I think of it: does the SSHStorage on bootstrap get
> reused but backed with a FileStorage? I sort of imagine so, in which case,
cool
> ...but is it possible for there to be overlap? ie should we be sharing the
> flocking everywhere?
> 
> or am I just fantasizing?

filestorage is only used for testing at the moment.

environs/localstorage takes over from environs/sshstorage, reusing the storage
directory. However, there is no overlap in use; environs/sshstorage is only used
to write the tools and bootstrap state file. *If* there's a failure
bootstrapping, the bootstrap storage may also be used to remove the state file.

Since there's no overlap, there's no need to share the locking. I did just
realise that I totally broke the manual bootstrap code though, when I changed
sshstorage's directory structure. I'll have to revisit that before merging.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
environs/filestorage/filestorage.go:110: file, err := ioutil.TempFile("",
"juju-filestorage-"+name)
On 2013/09/13 09:24:59, fwereade wrote:
> Same different-filesystems problem.

This is only for testing, but I will fix it anyway. I think localstorage ought
to be renamed httpstorage, and have the backend contain one of these.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
On 2013/09/13 09:24:59, fwereade wrote:
> OK, I think my problem here is maybe that it's not *just* about bootstrap
> storage, right? It's the same storage we always use, but this is the path to
it
> that's accessible from a client machine. Yes?
> 
> So, at least in the short term, we *will* need SSHStorage and LocalStorage to
> coexist.

They're pointing to the same location, but they're never used concurrently.

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker.go
File worker/localstorage/worker.go (left):

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker...
worker/localstorage/worker.go:1: package storage
On 2013/09/13 09:24:59, fwereade wrote:
> ah, I see. Can we call this localstorage please?

Sorry, fixed.

[rog, 2013-09-14 07:28:03]

Looks good. Some minor comments and suggestions below.

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:76: environ = &bootstrapStorageEnviron{environ,
bootstrapStorage}
nice

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstr...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstr...
environs/bootstrap/bootstrap.go:24: // BootstrapStorager is an interface that
returns a environs.Storage that may
I think I'd define this inside environs/interface.go

This package doesn't define or use it, and that file
is a useful reference for people implementing providers.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
environs/filestorage/filestorage.go:45: func NewFileStorage(path string)
(environs.Storage, error) {
Doc comment please.

Also, wouldn't it make more sense to define NewFileStorageReader in terms of
NewFileStorage? That way you wouldn't need the dynamic type conversion, as
Storage can be assigned to StorageReader.

For future consideration, I think filestorage.New and filestorage.NewReader
would be more idiomatic names than filestorage.NewFileStorage and
filestorage.NewFileStorageReader.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go#n...
environs/manual/agent.go:41: machineenv map[string]string
s/machineenv/machineEnv/

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:42: var errHostEmpty = errors.New("Host argument is
empty")
Errors don't generally start with a capital letter.

Also, if it's only used once, I wouldn't bother defining a variable for it.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:43: var errEnvironNil = errors.New("Environ
argument is nil")
ditto

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:51: // host will manually bootstrapped.
s/will/will be/ ?

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:65: err = fmt.Errorf("error checking if
provisioned: %v", err)
"failed to check provisioned status: %v" ?

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:78: defer func() {
Why not defer this after the call to SaveState and thereby lose the need for the
savedState variable?

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:87: err = fmt.Errorf("error detecting hardware
characteristics: %v", err)
s/err = /return /

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:123: machineenv := map[string]string{
s/machineenv/machineEnv/

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go
File environs/manual/detection.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection....
environs/manual/detection.go:22: const checkProvisionedScript = "ls /etc/init/ |
grep juju.*\\.conf || exit 0"
How about a more direct test?

"ls /etc/init/juju*.conf"

which will succeed if provisioned and fail otherwise.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection....
environs/manual/detection.go:28: cmd := exec.Command("ssh", sshHost, "bash")
or

cmd := exec.Command("ssh", sshHost, "bash", "-c", checkProvisionedScript)

?

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection....
environs/manual/detection.go:29: cmd.Stdin =
bytes.NewBufferString(checkProvisionedScript)
strings.NewReader would seem more appropriate if you continue to use Stdin.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection....
environs/manual/detection.go:30: out, err := cmd.CombinedOutput()
I think you want Output not CombinedOutput here.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection....
environs/manual/detection.go:125: lsb_release -cs
I wonder if this should include a "set -e"

https://codereview.appspot.com/13635044/diff/14001/environs/manual/provisione...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/provisione...
environs/manual/provisioner_test.go:58: for i, errorCode := range []int{255, 0}
{
Please add a log statement here, so that if the test fails, we
know which iteration we were on.

Something like this is conventional:

c.Logf("test %d: code %d", i, errorCode)

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
File environs/manual/sshresponse_test.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
environs/manual/sshresponse_test.go:41: func sshresponse(c *gc.C, input, output
string, rc int) func() {
I found these names quite confusing; the respond method doesn't
respond to anything...

How about something like "installFakeSSH" instead instead of sshresponse?

Then the type could be fakeSSH, and the respond method could be named SetUp or
similar.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
environs/manual/sshresponse_test.go:70: func (r sshresponder) respond(c *gc.C)
func() {
I'd like to see a doc comment on this please.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
environs/manual/sshresponse_test.go:87: f := sshresponse(c, input, output, rc)
I think things can be simpler here. How about something like:

var restores := []func()
add := func(input, output string, rc int) {
    restore := sshresponse(c, input, output, rc)
    restores = append(restores, restore)
}
if !r.skipProvisionAgent {
    add("", "", r.provisionAgentExitCode)
}
add(detectionScript, detectionoutput, 0)
add(checkProvisionedScript, "", 0)
return func() {
    for i := len(restores)-1; i >= 0; i-- {
        restores[i]()
    }
}


alternatively, you could use jc.Restorer throughout, and
add a method to it:

// Add returns a Restorer that restores first f1
// and then f. It is valid to call this on a nil
// Restorer.
func (f Restorer) Add(f1 Restorer) Restorer {
     return func() { 
         f1.Restore()
         if f != nil {
             f.Restore()
         }
     }
}

then you could do:

var restore jc.Restorer
add := func(input, output string, rc int) {
    restore = restore.Add(sshresponse(c, input, output, rc))
}
if !r.skipProvisionAgent {
    add("", "", r.provisionAgentExitCode)
}
add(detectionScript, detectionoutput, 0)
add(checkProvisionedScript, "", 0)
return restore


On balance I prefer the latter, because the defer
calls can be:

defer sshresponder{...}.respond().Restore()

which is more obvious to my eyes (I had missed the extra set of () in the call).

[thumper, 2013-09-16 03:02:08]

Still some changes needed here I think

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:76: environ = &bootstrapStorageEnviron{environ,
bootstrapStorage}
If all we are caring about is the storage, why are we changing the environ?

Does it not make sense to change the things that depend on the storage to just
depend on the storage and not the entire environment, then you don't need to
mock out the Storage method.

Obviously here the important thing is that we want to use a particular storage
for bootstrap, so I'd expect to be assigning the bootstrapStorage to a storage
variable, not mocking out the method with a different structure.

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstr...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstr...
environs/bootstrap/bootstrap.go:24: // BootstrapStorager is an interface that
returns a environs.Storage that may
On 2013/09/14 07:28:03, rog wrote:
> I think I'd define this inside environs/interface.go
> 
> This package doesn't define or use it, and that file
> is a useful reference for people implementing providers.

Agreed.  It is something that is used by the environment, so it makes more sense
to live there.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
environs/filestorage/filestorage.go:110: file, err := ioutil.TempFile("",
"juju-filestorage-"+name)
On 2013/09/13 09:41:26, axw1 wrote:
> On 2013/09/13 09:24:59, fwereade wrote:
> > Same different-filesystems problem.
> 
> This is only for testing, but I will fix it anyway. I think localstorage ought
> to be renamed httpstorage, and have the backend contain one of these.

That sounds like a good idea IMO

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go#n...
environs/manual/agent.go:82: err := environs.FinishMachineConfig(mcfg,
args.envcfg, constraints.Value{})
I fear that using names like "envcfg" means that we have more and more assumed
knowledge encoded into our variable names.  Here I like some parts of the Zen of
Python "Explicit is better than implicit", yes it is python, but I think the
principle holds.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
Arg, here I know that I'm going to clash on names with Rog, but IMO
  BootstrapStorager
is a terrible name.
  HasBootstrapStorage
makes a lot more sense to me.  Firstly you can look at the name and know what is
going on, and we aren't inventing words to go by the weird idea that interfaces
should end with "er".

bootstrapStorage, ok := sometype.(HasBootstrapStorage)

reads ok to me (FSVO ok given the syntax of the type assertion).

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
File environs/manual/sshresponse_test.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
environs/manual/sshresponse_test.go:53: return testing.PatchEnvironment("PATH",
fakebin+":"+os.Getenv("PATH"))
I've been thinking of better ways to get test code to clean up after itself.

If we had an interface called:
  type TestCleanup interface {
    AddCleanup(func())

we could then have this function take an interface pointer,
and instead of returning a function that the caller needs to defer,
you could just go:

func sshresponse(c *gc.C, cleaner testing.TestCleanup, input, output string, rc
int) {
  // everything else
  cleaner.AddCleanup(testing.PatchEnvironment("PATH",
fakebin+":"+os.Getenv("PATH")))
}

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/config.go
File worker/localstorage/config.go (right):

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/config...
worker/localstorage/config.go:4: package storage
why is the package "storage" when the directory is "localstorage" ?

This kind of goes against a lot of our coding standards.

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker.go
File worker/localstorage/worker.go (right):

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker...
worker/localstorage/worker.go:54: sharedStorageDir :=
os.Getenv(osenv.JujuSharedStorageDir)
I think these have all been moved into the agent config now.

Changes may have been lost during the move from the old directory.

[axw1, 2013-09-16 04:56:48]

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:76: environ = &bootstrapStorageEnviron{environ,
bootstrapStorage}
On 2013/09/16 03:02:08, thumper wrote:
> If all we are caring about is the storage, why are we changing the environ?
> 
> Does it not make sense to change the things that depend on the storage to just
> depend on the storage and not the entire environment, then you don't need to
> mock out the Storage method.

The storage must be passed down to environs/bootstrap Bootstrap, and then to
environs/tools FindTools. Possibly more places. Those things also want an
Environ (in the case of FindTools, it's hidden in an interface conversion for
the legacy fallback codepath).

Bootstrap storage must be used for *everything* until the environment is
bootstrapped. Leaving a method for acquiring broken/unusable storage seems like
a mistake to me.

> Obviously here the important thing is that we want to use a particular storage
> for bootstrap, so I'd expect to be assigning the bootstrapStorage to a storage
> variable, not mocking out the method with a different structure.

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstr...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstr...
environs/bootstrap/bootstrap.go:24: // BootstrapStorager is an interface that
returns a environs.Storage that may
On 2013/09/14 07:28:03, rog wrote:
> I think I'd define this inside environs/interface.go
> 
> This package doesn't define or use it, and that file
> is a useful reference for people implementing providers.

Fair point, done.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
environs/filestorage/filestorage.go:45: func NewFileStorage(path string)
(environs.Storage, error) {
On 2013/09/14 07:28:03, rog wrote:
> Doc comment please.

Done. Seems someone else came along and wrote a writer constructor in the mean
time. It lacks a doc comment, so I'll add to that :)

> Also, wouldn't it make more sense to define NewFileStorageReader in terms of
> NewFileStorage? That way you wouldn't need the dynamic type conversion, as
> Storage can be assigned to StorageReader.

That would simplify the code here. My rationale for doing it this way is that it
limits exposure.

IMO, you shouldn't be able to convert a StorageReader to a Storage, unless you
originally created it with NewStorage and converted it to StorageReader.

> For future consideration, I think filestorage.New and filestorage.NewReader
> would be more idiomatic names than filestorage.NewFileStorage and
> filestorage.NewFileStorageReader.

I do agree, but sadly it seems there's no consensus.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/files...
environs/filestorage/filestorage.go:110: file, err := ioutil.TempFile("",
"juju-filestorage-"+name)
On 2013/09/13 09:41:26, axw1 wrote:
> On 2013/09/13 09:24:59, fwereade wrote:
> > Same different-filesystems problem.
> 
> This is only for testing, but I will fix it anyway. I think localstorage ought
> to be renamed httpstorage, and have the backend contain one of these.

It would seem someone else has come along and written a filestorage writer in
the mean time. I'm going to have to touch a bunch more files, so I'll do this in
a followup if that's okay.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go#n...
environs/manual/agent.go:41: machineenv map[string]string
On 2013/09/14 07:28:03, rog wrote:
> s/machineenv/machineEnv/

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go#n...
environs/manual/agent.go:82: err := environs.FinishMachineConfig(mcfg,
args.envcfg, constraints.Value{})
On 2013/09/16 03:02:08, thumper wrote:
> I fear that using names like "envcfg" means that we have more and more assumed
> knowledge encoded into our variable names.  Here I like some parts of the Zen
of
> Python "Explicit is better than implicit", yes it is python, but I think the
> principle holds.

I've renamed it to environConfig.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
On 2013/09/16 03:02:08, thumper wrote:
> Arg, here I know that I'm going to clash on names with Rog, but IMO
>   BootstrapStorager
> is a terrible name.
>   HasBootstrapStorage
> makes a lot more sense to me.  Firstly you can look at the name and know what
is
> going on, and we aren't inventing words to go by the weird idea that
interfaces
> should end with "er".
> 
> bootstrapStorage, ok := sometype.(HasBootstrapStorage)
> 
> reads ok to me (FSVO ok given the syntax of the type assertion).

Copy and paste of my reply to William: I'm not a big fan of the "HasX" style.
IMO, the name should represent the behaviour or role, not something an
implementer has.

Either way, we ought to be consistent. I lean towards being consistent with
"idiomatic Go", where an interface is typically named after an implementer's
behaviour.

I think the variable name you've assigned to in your example is telling. The
name "bootstrapStorage" implies to me that it is itself a Storage. What else
might you call it? Maybe hasBootstrapStorage? That would imply to me it's a
boolean.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:42: var errHostEmpty = errors.New("Host argument is
empty")
On 2013/09/14 07:28:03, rog wrote:
> Errors don't generally start with a capital letter.

That was meant to relate to the field name, but that was probably a bit
misguided.

> Also, if it's only used once, I wouldn't bother defining a variable for it.

Fair enough, inlined.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:43: var errEnvironNil = errors.New("Environ
argument is nil")
On 2013/09/14 07:28:03, rog wrote:
> ditto

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:51: // host will manually bootstrapped.
On 2013/09/14 07:28:03, rog wrote:
> s/will/will be/ ?

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:65: err = fmt.Errorf("error checking if
provisioned: %v", err)
On 2013/09/14 07:28:03, rog wrote:
> "failed to check provisioned status: %v" ?

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:78: defer func() {
On 2013/09/14 07:28:03, rog wrote:
> Why not defer this after the call to SaveState and thereby lose the need for
the
> savedState variable?

Done. Not sure why I did it like that. I may have had something outside the
condition before.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:87: err = fmt.Errorf("error detecting hardware
characteristics: %v", err)
On 2013/09/14 07:28:03, rog wrote:
> s/err = /return /

Done. I think at one stage it must've spilled over 80 chars.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:123: machineenv := map[string]string{
On 2013/09/14 07:28:03, rog wrote:
> s/machineenv/machineEnv/

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go
File environs/manual/detection.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection....
environs/manual/detection.go:22: const checkProvisionedScript = "ls /etc/init/ |
grep juju.*\\.conf || exit 0"
On 2013/09/14 07:28:03, rog wrote:
> How about a more direct test?
> 
> "ls /etc/init/juju*.conf"
> 
> which will succeed if provisioned and fail otherwise.

I didn't do that because I wanted to be able to distinguish between an ssh
failure and a command failure.

The alternative is to check error code 255. This requires OS-specific code for
converting to WaitStatus, or whatever the client is running.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection....
environs/manual/detection.go:28: cmd := exec.Command("ssh", sshHost, "bash")
On 2013/09/14 07:28:03, rog wrote:
> or
> 
> cmd := exec.Command("ssh", sshHost, "bash", "-c", checkProvisionedScript)
> 
> ?

Indeed. No idea why I didn't do that in the first place.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection....
environs/manual/detection.go:30: out, err := cmd.CombinedOutput()
On 2013/09/14 07:28:03, rog wrote:
> I think you want Output not CombinedOutput here.

Yep, this has been fixed in another MP.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection....
environs/manual/detection.go:125: lsb_release -cs
On 2013/09/14 07:28:03, rog wrote:
> I wonder if this should include a "set -e"

Yes, I think so. Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/provisione...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/provisione...
environs/manual/provisioner_test.go:58: for i, errorCode := range []int{255, 0}
{
On 2013/09/14 07:28:03, rog wrote:
> Please add a log statement here, so that if the test fails, we
> know which iteration we were on.
> 
> Something like this is conventional:
> 
> c.Logf("test %d: code %d", i, errorCode)

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
File environs/manual/sshresponse_test.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
environs/manual/sshresponse_test.go:41: func sshresponse(c *gc.C, input, output
string, rc int) func() {
On 2013/09/14 07:28:03, rog wrote:
> I found these names quite confusing; the respond method doesn't
> respond to anything...
> 
> How about something like "installFakeSSH" instead instead of sshresponse?
> 
> Then the type could be fakeSSH, and the respond method could be named SetUp or
> similar.

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
environs/manual/sshresponse_test.go:53: return testing.PatchEnvironment("PATH",
fakebin+":"+os.Getenv("PATH"))
On 2013/09/16 03:02:08, thumper wrote:
> I've been thinking of better ways to get test code to clean up after itself.
> 
> If we had an interface called:
>   type TestCleanup interface {
>     AddCleanup(func())
> 
> we could then have this function take an interface pointer,
> and instead of returning a function that the caller needs to defer,
> you could just go:
> 
> func sshresponse(c *gc.C, cleaner testing.TestCleanup, input, output string,
rc
> int) {
>   // everything else
>   cleaner.AddCleanup(testing.PatchEnvironment("PATH",
> fakebin+":"+os.Getenv("PATH")))
> }
> 

That sounds fine. Since there's no way in hell our tests could run in parallel,
we could probably go a step further and make it a top-level function in
juju-core/testing. Then PatchEnvironment itself could be responsible for calling
AddCleanup.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
environs/manual/sshresponse_test.go:70: func (r sshresponder) respond(c *gc.C)
func() {
On 2013/09/14 07:28:03, rog wrote:
> I'd like to see a doc comment on this please.

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshrespons...
environs/manual/sshresponse_test.go:87: f := sshresponse(c, input, output, rc)
On 2013/09/14 07:28:03, rog wrote:
> I think things can be simpler here. How about something like:
> 
> var restores := []func()
> add := func(input, output string, rc int) {
>     restore := sshresponse(c, input, output, rc)
>     restores = append(restores, restore)
> }
> if !r.skipProvisionAgent {
>     add("", "", r.provisionAgentExitCode)
> }
> add(detectionScript, detectionoutput, 0)
> add(checkProvisionedScript, "", 0)
> return func() {
>     for i := len(restores)-1; i >= 0; i-- {
>         restores[i]()
>     }
> }
> 
> 
> alternatively, you could use jc.Restorer throughout, and
> add a method to it:
> 
> // Add returns a Restorer that restores first f1
> // and then f. It is valid to call this on a nil
> // Restorer.
> func (f Restorer) Add(f1 Restorer) Restorer {
>      return func() { 
>          f1.Restore()
>          if f != nil {
>              f.Restore()
>          }
>      }
> }
> 
> then you could do:
> 
> var restore jc.Restorer
> add := func(input, output string, rc int) {
>     restore = restore.Add(sshresponse(c, input, output, rc))
> }
> if !r.skipProvisionAgent {
>     add("", "", r.provisionAgentExitCode)
> }
> add(detectionScript, detectionoutput, 0)
> add(checkProvisionedScript, "", 0)
> return restore
> 
> 
> On balance I prefer the latter, because the defer
> calls can be:
> 
> defer sshresponder{...}.respond().Restore()
> 
> which is more obvious to my eyes (I had missed the extra set of () in the
call).

Much nicer, thank you.

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/config.go
File worker/localstorage/config.go (right):

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/config...
worker/localstorage/config.go:4: package storage
On 2013/09/16 03:02:08, thumper wrote:
> why is the package "storage" when the directory is "localstorage" ?
> 
> This kind of goes against a lot of our coding standards.

Yep, that was a mistake.

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker.go
File worker/localstorage/worker.go (right):

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker...
worker/localstorage/worker.go:54: sharedStorageDir :=
os.Getenv(osenv.JujuSharedStorageDir)
On 2013/09/16 03:02:08, thumper wrote:
> I think these have all been moved into the agent config now.
> 
> Changes may have been lost during the move from the old directory.

Yeah I hadn't merged yet. Done now.

[axw, 2013-09-16 06:12:37]

Please take a look.

[rog, 2013-09-16 16:51:34]

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
On 2013/09/16 04:56:48, axw1 wrote:
> On 2013/09/16 03:02:08, thumper wrote:
> > Arg, here I know that I'm going to clash on names with Rog, but IMO
> >   BootstrapStorager
> > is a terrible name.
> >   HasBootstrapStorage
> > makes a lot more sense to me.  Firstly you can look at the name and know
what
> is
> > going on, and we aren't inventing words to go by the weird idea that
> interfaces
> > should end with "er".
> > 
> > bootstrapStorage, ok := sometype.(HasBootstrapStorage)
> > 
> > reads ok to me (FSVO ok given the syntax of the type assertion).
> 
> Copy and paste of my reply to William: I'm not a big fan of the "HasX" style.
> IMO, the name should represent the behaviour or role, not something an
> implementer has.

+lots.

I'm not keen on "BootstrapStorager" as a name either,
but I think it's important that interface names are nouns
not predicates - an interface name should represent what the
object *is*, not what it *can be*.

[thumper, 2013-09-16 21:01:31]

On 2013/09/16 16:51:34, rog wrote:
>
https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go
> File environs/manual/bootstrap.go (right):
> 
>
https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap....
> environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
> On 2013/09/16 04:56:48, axw1 wrote:
> > On 2013/09/16 03:02:08, thumper wrote:
> > > Arg, here I know that I'm going to clash on names with Rog, but IMO
> > >   BootstrapStorager
> > > is a terrible name.
> > >   HasBootstrapStorage
> > > makes a lot more sense to me.  Firstly you can look at the name and know
> what
> > is
> > > going on, and we aren't inventing words to go by the weird idea that
> > interfaces
> > > should end with "er".
> > > 
> > > bootstrapStorage, ok := sometype.(HasBootstrapStorage)
> > > 
> > > reads ok to me (FSVO ok given the syntax of the type assertion).
> > 
> > Copy and paste of my reply to William: I'm not a big fan of the "HasX"
style.
> > IMO, the name should represent the behaviour or role, not something an
> > implementer has.
> 
> +lots.
> 
> I'm not keen on "BootstrapStorager" as a name either,
> but I think it's important that interface names are nouns
> not predicates - an interface name should represent what the
> object *is*, not what it *can be*.

Why not then just call the interface "BootstrapStorage".

[rog, 2013-09-16 21:41:47]

On 16 September 2013 22:01,  <tim.penhey@canonical.com> wrote:
> Why not then just call the interface "BootstrapStorage".

That would be appropriate for the storage itself, but this
interface represents an object that can be asked *for* the storage,
which doesn't seem quite the same thing to me.

[fwereade, 2013-09-19 09:28:28]

If the only remaining contentious point is (Has)BootstrapStorage(r), then I call
dealer's choice and LGTM whatever he picks; I don't think any of the names are
great, but I also don't think any of them are so bad as to unacceptably reduce
the quality of the codebase ;).

If there are other problems, carry on as usual, ofc, but let's not block this on
naming arguments.