Implement synchronous bootstrap

Implement synchronous bootstrap

environs/manual has been further refactored to
split the "manual cloud-init over SSH" out into
a separate package (cloudinit/sshinit).

provider/common now starts an instance with the
basic cloud-init only (SSH keys + logging), then
waits for a DNS name, waits to be able to connect
via SSH, and then defers to sshinit to execute the
remaining cloud-init steps.

If the user hits Ctrl-C, SIGINT will terminate
the SSH connection, and the bootstrap process will
attempt to clean up by stopping the instance and
removing the state file (if the instance could be
cleanly stopped). We also ignore SIGINT in the
juju process so a second Ctrl-C will not terminate
the cleanup procedure.

Fixes #1224230

https://code.launchpad.net/~axwalk/juju-core/synchronous-bootstrap/+merge/196058

(do not edit description out of merge proposal)

[axw, 2013-11-21 06:33:20]

Please take a look.

[wallyworld, 2013-11-22 01:53:59]

This looks pretty solid. I like how manual provisioning and cloud provisioning
now share more code. 

Before landing, please test live on each of the providers. 

I have a nagging concern about the finishBootstrap step. WaitDNS and the ssh
connect retry should work most if not all of the time, but there are times when
canonistack for example is reaaaally slow to start an instance. It worries me
that we could needlessly kill bootstrap if we think we have waited long enough
but the provider is just slow. Thoughts?

https://codereview.appspot.com/30190043/diff/1/cloudinit/sshinit/configure_te...
File cloudinit/sshinit/configure_test.go (left):

https://codereview.appspot.com/30190043/diff/1/cloudinit/sshinit/configure_te...
cloudinit/sshinit/configure_test.go:3: 
Is there a test that "apt-get -y upgrade" is only used when asked for? I can't
see it if there is. I think this is a pretty important change so warrants an
explicit test.

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

https://codereview.appspot.com/30190043/diff/1/cmd/juju/bootstrap.go#newcode126
cmd/juju/bootstrap.go:126: signal.Notify(make(chan os.Signal), os.Interrupt)
Just a thought. One other option is to install a handler which prints a message
informing the user that their Ctrl-C cannot terminate the cmd because cleanup is
being done.

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit.go#newcode135
environs/cloudinit.go:135: // and setup the SSH keys. The rest we leave to
environs/manual.
environs/manual or cloudinit/sshinit?

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit_test.go
File environs/cloudinit_test.go (right):

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit_test.go#new...
environs/cloudinit_test.go:207: // for MAAS. MAAS needs to configure and thren
bounce the
typo

https://codereview.appspot.com/30190043/diff/1/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:35: defer func() {
Could we break this logic out into a separate function outside of Bootstrap for
clarity. The func could be 

handleBootstrapError(err error, inst instance.Instance)

https://codereview.appspot.com/30190043/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:95: func TestingDisableFinishBootstrap() func() {
I was going to suggest move this to an export_tests.go file but I see it's
called outside this package. Bollocks.

https://codereview.appspot.com/30190043/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:104: func TestingPatchFinishBootstrap(f
func(instance.Instance, *cloudinit.MachineConfig) error) func() {
Does this need to be exported? It's only called from the above.

[axw, 2013-11-22 02:52:01]

I've tested against ec2, will do openstack and azure later. I don't have access
to a MAAS installation, so that may take a little while.

I've not made any changes to the signal handling yet, but I will do so. I need
to think a bit more about how to handle it.

https://codereview.appspot.com/30190043/diff/1/cloudinit/sshinit/configure_te...
File cloudinit/sshinit/configure_test.go (left):

https://codereview.appspot.com/30190043/diff/1/cloudinit/sshinit/configure_te...
cloudinit/sshinit/configure_test.go:3: 
On 2013/11/22 01:54:00, wallyworld wrote:
> Is there a test that "apt-get -y upgrade" is only used when asked for? I can't
> see it if there is. I think this is a pretty important change so warrants an
> explicit test.

Added a test, also for apt-get update.

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

https://codereview.appspot.com/30190043/diff/1/cmd/juju/bootstrap.go#newcode126
cmd/juju/bootstrap.go:126: signal.Notify(make(chan os.Signal), os.Interrupt)
On 2013/11/22 01:54:00, wallyworld wrote:
> Just a thought. One other option is to install a handler which prints a
message
> informing the user that their Ctrl-C cannot terminate the cmd because cleanup
is
> being done.

Yeah, that's not a bad idea. The only catch is, what do we do before finish
bootstrap is entered? That's got me thinking about your concerns some more: we
need to be able to cancel the WaitDNS/ssh phase too, I think, and that's not
currently handled.

I think what I'll have to do is move the signal handling inside finishBootstrap.
This is in the same realm as direct-os.Stdin access, so it should probably
(eventually? now?) go into some kind of OS-context structure that gets passed to
the Environment (or maybe just to Bootstrap).

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit.go#newcode135
environs/cloudinit.go:135: // and setup the SSH keys. The rest we leave to
environs/manual.
On 2013/11/22 01:54:00, wallyworld wrote:
> environs/manual or cloudinit/sshinit?

cloudinit/sshinit; fixed, thanks

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit_test.go
File environs/cloudinit_test.go (right):

https://codereview.appspot.com/30190043/diff/1/environs/cloudinit_test.go#new...
environs/cloudinit_test.go:207: // for MAAS. MAAS needs to configure and thren
bounce the
On 2013/11/22 01:54:00, wallyworld wrote:
> typo

Done.

https://codereview.appspot.com/30190043/diff/1/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:35: defer func() {
On 2013/11/22 01:54:00, wallyworld wrote:
> Could we break this logic out into a separate function outside of Bootstrap
for
> clarity. The func could be 
> 
> handleBootstrapError(err error, inst instance.Instance)
> 

Done.

https://codereview.appspot.com/30190043/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:104: func TestingPatchFinishBootstrap(f
func(instance.Instance, *cloudinit.MachineConfig) error) func() {
On 2013/11/22 01:54:00, wallyworld wrote:
> Does this need to be exported? It's only called from the above.

Unexported. I was going to use it in a test, but it wasn't worthwhile.

[axw, 2013-11-22 06:05:03]

Please take a look.

[wallyworld, 2013-11-22 06:37:49]

Thanks for dealing with the finishBootstrap issues - I think it's much better
now that it allows the process to complete in it's own time, but still allows a
Ctrl-C. The code looks ok from a reading it perspective, and you said you've
tested it live :-) One final suggestion - after each "tick" in the for loops
while waiting for dns and ssh, should we print a "." or something so the user
knows the system is alive. Every one second would be too often so perhaps after
each 10 1 second timeouts we could do it?

We do need to test on maas before landing. 

LGTM after a MAAS test and final move of those test helpers to a testing
package.

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configur...
File cloudinit/sshinit/configure_test.go (right):

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure_test.go:136: assertScriptMatches(c, cfg,
"(.|\n)*apt-get -y update(.|\n)*", false)
To prevent typos causing the test to pass, I'd love to see the regexp string
which is cut and pasted be put into a const or var.

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure_test.go:149: assertScriptMatches(c, cfg,
"(.|\n)*apt-get -y upgrade(.|\n)*", false)
ditto

https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap....
provider/common/bootstrap.go:37: // TODO(axw) 2013-11-22 #XXX
a bug number would be great here

https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap....
provider/common/bootstrap.go:90: fmt.Fprintln(ctx.Stderr, "Stopping
instance...")
pedant alert - I think we should use case consistently for out messages.
"Stopping...." vs "cleaning up..." etc

[axw, 2013-11-22 07:54:35]

On 2013/11/22 06:37:49, wallyworld wrote:
> Thanks for dealing with the finishBootstrap issues - I think it's much better
> now that it allows the process to complete in it's own time, but still allows
a
> Ctrl-C. The code looks ok from a reading it perspective, and you said you've
> tested it live :-) One final suggestion - after each "tick" in the for loops
> while waiting for dns and ssh, should we print a "." or something so the user
> knows the system is alive. Every one second would be too often so perhaps
after
> each 10 1 second timeouts we could do it?

I've added the dots. I think 1 second is fine; the DNS wait is quick anyway. The
difference in rate should also give an indication of the relative speed of the
two phases.

> We do need to test on maas before landing. 
> 
> LGTM after a MAAS test and final move of those test helpers to a testing
> package.

Thanks, I will do that. I might just run this by thumper (and fwereade?) as
well, as I know he was interested in it to begin with.

>
https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configur...
> File cloudinit/sshinit/configure_test.go (right):
> 
>
https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configur...
> cloudinit/sshinit/configure_test.go:136: assertScriptMatches(c, cfg,
> "(.|\n)*apt-get -y update(.|\n)*", false)
> To prevent typos causing the test to pass, I'd love to see the regexp string
> which is cut and pasted be put into a const or var.
> 
>
https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configur...
> cloudinit/sshinit/configure_test.go:149: assertScriptMatches(c, cfg,
> "(.|\n)*apt-get -y upgrade(.|\n)*", false)
> ditto
> 
>
https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap.go
> File provider/common/bootstrap.go (right):
> 
>
https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap....
> provider/common/bootstrap.go:37: // TODO(axw) 2013-11-22 #XXX
> a bug number would be great here
> 
>
https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap....
> provider/common/bootstrap.go:90: fmt.Fprintln(ctx.Stderr, "Stopping
> instance...")
> pedant alert - I think we should use case consistently for out messages.
> "Stopping...." vs "cleaning up..." etc

[axw, 2013-11-22 08:07:50]

Please take a look.

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configur...
File cloudinit/sshinit/configure_test.go (right):

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure_test.go:136: assertScriptMatches(c, cfg,
"(.|\n)*apt-get -y update(.|\n)*", false)
On 2013/11/22 06:37:50, wallyworld wrote:
> To prevent typos causing the test to pass, I'd love to see the regexp string
> which is cut and pasted be put into a const or var.

Done.

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure_test.go:149: assertScriptMatches(c, cfg,
"(.|\n)*apt-get -y upgrade(.|\n)*", false)
On 2013/11/22 06:37:50, wallyworld wrote:
> ditto

Done.

https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap....
provider/common/bootstrap.go:37: // TODO(axw) 2013-11-22 #XXX
On 2013/11/22 06:37:50, wallyworld wrote:
> a bug number would be great here

Done.

https://codereview.appspot.com/30190043/diff/20001/provider/common/bootstrap....
provider/common/bootstrap.go:90: fmt.Fprintln(ctx.Stderr, "Stopping
instance...")
On 2013/11/22 06:37:50, wallyworld wrote:
> pedant alert - I think we should use case consistently for out messages.
> "Stopping...." vs "cleaning up..." etc

Absolutely - just an oversight, thanks for picking it up.

[jameinel, 2013-11-24 08:11:01]

I tried this, and I *really* like the first steps where it is starting to
connect, letting you know what is going on, etc. However, once it can SSH in, it
just dumps all of cloud-init-output.log to stdout which seems very overly
verbose. I personally would like to know a *summary* of what it is doing
(installing packages) rather than 100s of lines about the exact thing it is
doing.

Thoughts?

[axw, 2013-11-25 09:16:38]

On 2013/11/24 08:11:01, jameinel wrote:
> I tried this, and I *really* like the first steps where it is starting to
> connect, letting you know what is going on, etc. However, once it can SSH in,
it
> just dumps all of cloud-init-output.log to stdout which seems very overly
> verbose. I personally would like to know a *summary* of what it is doing
> (installing packages) rather than 100s of lines about the exact thing it is
> doing.
> 
> Thoughts?

I agree. It's maybe slightly tricky to implement, as it really is just executing
one big script over SSH. I'll have to do some investigation to see what's
practical.

[axw, 2013-12-02 09:53:10]

Please take a look.

[fwereade, 2013-12-02 14:54:09]

This is *great*. Thanks very much. It would be really nice to have an initial
line saying "Launching instance", followed by the actual instance id that got
started -- and maybe to finish by running juju status -- but those are just
details.

So LGTM modulo trivials below, assuming jam is happy; and a few followup
questions:

1) Can we drop the secrets dance soon? (I guess we still need it for the first
connect to 1.16 via state :/... but maybe we don't need it for the API)

2) How close are we to writing the state server address directly into the .jenv,
so we get fast API connections from the word go? I'd be really keen to see that
followup soon :).

3) Which SSH commands do we still use? I think it's just debug-log that goes
over the API now, so ssh, scp, and debug-hooks are all still SSHy AIUI -- can we
consolidate those bits now? Any I didn't think of?

4) How hard will it be to extract the manual-provisioning code such that it's
possible to run a "juju register" command on an instance that you want to
provision and already have direct access to? I think we touched on this a while
back, but I'm not sure we came to any conclusion.

https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configur...
File cloudinit/sshinit/configure_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure_test.go:36: environs.RegisterProvider("sshinit",
&testProvider{})
Can we stick an explicit "test" into the name somewhere please?

https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_t...
File environs/bootstrap/state_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_t...
environs/bootstrap/state_test.go:59: c.Assert(err, gc.IsNil) // doesn't exist,
juju don't carea
s/carea/care/

https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:40: // Std{in,out,err}, and interrupt signal
handling.
I'd like this to be an imminent followup, please :).

https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:61: inst, hw, err = env.StartInstance(cons,
selectedTools, machineConfig)
Wouldn't this still work with :=, even given inst? Maybe I'm still sluggish
after lunch...

https://codereview.appspot.com/30190043/diff/60001/provider/common/testing/bo...
File provider/common/testing/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/60001/provider/common/testing/bo...
provider/common/testing/bootstrap.go:17: f := func(*common.BootstrapContext,
instance.Instance, *cloudinit.MachineConfig) error {
Let's log something in here so that there's some way of diagnosing what happened
if Disable gets called in a surprising situation.

https://codereview.appspot.com/30190043/diff/60001/provider/maas/maas_test.go
File provider/maas/maas_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/provider/maas/maas_test.go...
provider/maas/maas_test.go:14: commontesting
"launchpad.net/juju-core/provider/common/testing"
Oh, a thought. Might DisableFinishBootstrap be happier in environs/testing?

https://codereview.appspot.com/30190043/diff/60001/utils/ssh/ssh.go
File utils/ssh/ssh.go (left):

https://codereview.appspot.com/30190043/diff/60001/utils/ssh/ssh.go#oldcode15
utils/ssh/ssh.go:15: // Move this to a common package for use in cmd/juju, and
others.
Can the SSHy bits in cmd/juju use this now?

[fwereade, 2013-12-02 15:30:09]

Oh! And, can we run in-environment storage everywhere soon? This doesn't handle
the bootstrap-state case ofc, but it would be great to keep charms and tools
inside the environment.

[axw, 2013-12-03 05:17:21]

On 2013/12/02 14:54:09, fwereade wrote:
> This is *great*. Thanks very much. It would be really nice to have an initial
> line saying "Launching instance", followed by the actual instance id that got
> started -- and maybe to finish by running juju status -- but those are just
> details.

I've added the "Launching instance" message. Leaving the status bit for now;
that can easily be added in afterwards, and is somewhat separate from this CL.

> So LGTM modulo trivials below, assuming jam is happy; and a few followup
> questions:
>
> 1) Can we drop the secrets dance soon? (I guess we still need it for the first
> connect to 1.16 via state :/... but maybe we don't need it for the API)

Yes, I intend to kill it soon.

> 2) How close are we to writing the state server address directly into the
.jenv,
> so we get fast API connections from the word go? I'd be really keen to see
that
> followup soon :).

I don't know, that wasn't part of my plan. Sounds easy enough.

> 3) Which SSH commands do we still use? I think it's just debug-log that goes
> over the API now, so ssh, scp, and debug-hooks are all still SSHy AIUI -- can
we
> consolidate those bits now? Any I didn't think of?

debug-log still uses SSH; it only uses the API for getting the address. IIANM,
TheMue is looking at changing this.
ssh, scp, debug-* all use SSH, as do environs/manual, environs/sshstorage, and
cloudinit/sshinit. I've changed them all to use utils/ssh.

> 4) How hard will it be to extract the manual-provisioning code such that it's
> possible to run a "juju register" command on an instance that you want to
> provision and already have direct access to? I think we touched on this a
while
> back, but I'm not sure we came to any conclusion.

The SSH-bits could easily be pretty easily swapped to use os/exec Command, so
it's all on localhost. The challenging bit will be how to package configuration
in a nice way.

> Oh! And, can we run in-environment storage everywhere soon? This doesn't
handle
> the bootstrap-state case ofc, but it would be great to keep charms and tools
> inside the environment.

I can still see one major problem, which is that you couldn't then sync-tools
before bootstrapping. Other than that, I think it should be doable.

>
https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configur...
> File cloudinit/sshinit/configure_test.go (right):
> 
>
https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configur...
> cloudinit/sshinit/configure_test.go:36: environs.RegisterProvider("sshinit",
> &testProvider{})
> Can we stick an explicit "test" into the name somewhere please?
> 
>
https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_t...
> File environs/bootstrap/state_test.go (right):
> 
>
https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_t...
> environs/bootstrap/state_test.go:59: c.Assert(err, gc.IsNil) // doesn't exist,
> juju don't carea
> s/carea/care/
> 
>
https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap.go
> File provider/common/bootstrap.go (right):
> 
>
https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap....
> provider/common/bootstrap.go:40: // Std{in,out,err}, and interrupt signal
> handling.
> I'd like this to be an imminent followup, please :).
> 
>
https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap....
> provider/common/bootstrap.go:61: inst, hw, err = env.StartInstance(cons,
> selectedTools, machineConfig)
> Wouldn't this still work with :=, even given inst? Maybe I'm still sluggish
> after lunch...
> 
>
https://codereview.appspot.com/30190043/diff/60001/provider/common/testing/bo...
> File provider/common/testing/bootstrap.go (right):
> 
>
https://codereview.appspot.com/30190043/diff/60001/provider/common/testing/bo...
> provider/common/testing/bootstrap.go:17: f := func(*common.BootstrapContext,
> instance.Instance, *cloudinit.MachineConfig) error {
> Let's log something in here so that there's some way of diagnosing what
happened
> if Disable gets called in a surprising situation.
> 
> https://codereview.appspot.com/30190043/diff/60001/provider/maas/maas_test.go
> File provider/maas/maas_test.go (right):
> 
>
https://codereview.appspot.com/30190043/diff/60001/provider/maas/maas_test.go...
> provider/maas/maas_test.go:14: commontesting
> "launchpad.net/juju-core/provider/common/testing"
> Oh, a thought. Might DisableFinishBootstrap be happier in environs/testing?
> 
> https://codereview.appspot.com/30190043/diff/60001/utils/ssh/ssh.go
> File utils/ssh/ssh.go (left):
> 
> https://codereview.appspot.com/30190043/diff/60001/utils/ssh/ssh.go#oldcode15
> utils/ssh/ssh.go:15: // Move this to a common package for use in cmd/juju, and
> others.
> Can the SSHy bits in cmd/juju use this now?

[axw, 2013-12-03 05:54:12]

Please take a look.

https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configur...
File cloudinit/sshinit/configure_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure_test.go:36: environs.RegisterProvider("sshinit",
&testProvider{})
On 2013/12/02 14:54:09, fwereade wrote:
> Can we stick an explicit "test" into the name somewhere please?

Done.

https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_t...
File environs/bootstrap/state_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_t...
environs/bootstrap/state_test.go:59: c.Assert(err, gc.IsNil) // doesn't exist,
juju don't carea
On 2013/12/02 14:54:09, fwereade wrote:
> s/carea/care/

Done.

https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:40: // Std{in,out,err}, and interrupt signal
handling.
On 2013/12/02 14:54:09, fwereade wrote:
> I'd like this to be an imminent followup, please :).

No problems.

https://codereview.appspot.com/30190043/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:61: inst, hw, err = env.StartInstance(cons,
selectedTools, machineConfig)
On 2013/12/02 14:54:09, fwereade wrote:
> Wouldn't this still work with :=, even given inst? Maybe I'm still sluggish
> after lunch...

No, you're right, that'll work. Fixed.

https://codereview.appspot.com/30190043/diff/60001/provider/common/testing/bo...
File provider/common/testing/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/60001/provider/common/testing/bo...
provider/common/testing/bootstrap.go:17: f := func(*common.BootstrapContext,
instance.Instance, *cloudinit.MachineConfig) error {
On 2013/12/02 14:54:09, fwereade wrote:
> Let's log something in here so that there's some way of diagnosing what
happened
> if Disable gets called in a surprising situation.

Done.

https://codereview.appspot.com/30190043/diff/60001/provider/maas/maas_test.go
File provider/maas/maas_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/provider/maas/maas_test.go...
provider/maas/maas_test.go:14: commontesting
"launchpad.net/juju-core/provider/common/testing"
On 2013/12/02 14:54:09, fwereade wrote:
> Oh, a thought. Might DisableFinishBootstrap be happier in environs/testing?

Hmm yeah that's fine, and stops the package proliferation. Moved.

https://codereview.appspot.com/30190043/diff/60001/utils/ssh/ssh.go
File utils/ssh/ssh.go (left):

https://codereview.appspot.com/30190043/diff/60001/utils/ssh/ssh.go#oldcode15
utils/ssh/ssh.go:15: // Move this to a common package for use in cmd/juju, and
others.
On 2013/12/02 14:54:09, fwereade wrote:
> Can the SSHy bits in cmd/juju use this now?

Done. Also updated environs/sshstorage, and environs/manual. Had to tweak
utils/ssh a bit, and add an ScpCommand function.