Merge lp:~axwalk/juju-core/synchronous-bootstrap into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

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://codereview.appspot.com/30190043/

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://codereview.appspot.com/30190043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+196058_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/30190043/

Affected files (+334, -88 lines):
   A [revision details]
   M cloudinit/options.go
   M cloudinit/sshinit/configure.go
   M cloudinit/sshinit/configure_test.go
   A cloudinit/sshinit/suite_test.go
   M cmd/juju/bootstrap.go
   M environs/bootstrap/state.go
   M environs/bootstrap/state_test.go
   M environs/cloudinit.go
   M environs/cloudinit/cloudinit.go
   M environs/cloudinit_test.go
   M environs/manual/bootstrap.go
   M environs/manual/detection.go
   M environs/manual/provisioner.go
   M provider/common/bootstrap.go
   M provider/common/bootstrap_test.go
   M provider/ec2/local_test.go
   M provider/maas/maas_test.go
   M provider/openstack/local_test.go
   M utils/ssh/ssh.go

[Ian Booth (wallyworld)]

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_test.go
File cloudinit/sshinit/configure_test.go (left):

https://codereview.appspot.com/30190043/diff/1/cloudinit/sshinit/configure_test.go#oldcode3
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#newcode207
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#newcode35
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#newcode95
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#newcode104
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.

https://codereview.appspot.com/30190043/

[Andrew Wilkins (axwalk)]

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_test.go
File cloudinit/sshinit/configure_test.go (left):

https://codereview.appspot.com/30190043/diff/1/cloudinit/sshinit/configure_test.go#oldcode3
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#newcode207
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#newcode35
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#newcode104
provider/common/bootstrap.go:104: func TestingPatchFinishBootstrap(f
func(instance.Inst...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/30190043/

[Ian Booth (wallyworld)]

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/configure_test.go
File cloudinit/sshinit/configure_test.go (right):

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configure_test.go#newcode136
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/configure_test.go#newcode149
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.go#newcode37
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.go#newcode90
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

https://codereview.appspot.com/30190043/

[Andrew Wilkins (axwalk)]

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/configure_test.go
> File cloudinit/sshinit/configure_test.go (right):

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configure_test.go#newcode136
> 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/configure_test.go#newcode149
> 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.go#newcode37
> 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.go#newcode90
> 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

https://codereview.appspot.com/30190043/

[Andrew Wilkins (axwalk)]

Please take a look.

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

https://codereview.appspot.com/30190043/diff/20001/cloudinit/sshinit/configure_test.go#newcode136
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/configure_test.go#newcode149
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.go#newcode37
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.go#newcode90
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.

https://codereview.appspot.com/30190043/

[John A Meinel (jameinel)]

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?

https://codereview.appspot.com/30190043/

[Andrew Wilkins (axwalk)]

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.

https://codereview.appspot.com/30190043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/30190043/

[William Reade (fwereade)]

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/configure_test.go
File cloudinit/sshinit/configure_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configure_test.go#newcode36
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_test.go
File environs/bootstrap/state_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_test.go#newcode59
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.go#newcode40
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.go#newcode61
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/bootstrap.go
File provider/common/testing/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/60001/provider/common/testing/bootstrap.go#newcode17
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...

[William Reade (fwereade)]

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.

https://codereview.appspot.com/30190043/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

> 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?

Right now debug-log still goes via ssh + tail, though Frank is working
on moving us to an actual API call.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlKdYRYACgkQJdeBCYSNAAPUsgCfTgnsDcRvG+/CyNLYoptrrENk
sSAAn2dC2nbwNAT76g3n701gOdt1Yt9c
=X9FF
-----END PGP SIGNATURE-----

[Andrew Wilkins (axwalk)]

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/configure_test.go
> File cloudinit/sshinit/configure_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configure_test.go#newcode36
> 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_test.go
> File environs/bootstrap/state_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_test.go#newcode59
> 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....

[Andrew Wilkins (axwalk)]

Please take a look.

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

https://codereview.appspot.com/30190043/diff/60001/cloudinit/sshinit/configure_test.go#newcode36
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_test.go
File environs/bootstrap/state_test.go (right):

https://codereview.appspot.com/30190043/diff/60001/environs/bootstrap/state_test.go#newcode59
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.go#newcode40
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.go#newcode61
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/bootstrap.go
File provider/common/testing/bootstrap.go (right):

https://codereview.appspot.com/30190043/diff/60001/provider/common/testing/bootstrap.go#newcode17
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#newcode14
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.

https://codereview.appspot.com/30190043/

=== modified file 'cloudinit/cloudinit_test.go'
--- cloudinit/cloudinit_test.go	2013-11-26 12:24:48 +0000
+++ cloudinit/cloudinit_test.go	2013-12-03 06:15:09 +0000
@@ -269,6 +269,37 @@
 	c.Assert(cfg.Packages(), gc.DeepEquals, []string{"a b c", "d!"})
 }
 
+func (S) TestSetOutput(c *gc.C) {
+	type test struct {
+		kind   cloudinit.OutputKind
+		stdout string
+		stderr string
+	}
+	tests := []test{{
+		cloudinit.OutAll, "a", "",
+	}, {
+		cloudinit.OutAll, "", "b",
+	}, {
+		cloudinit.OutInit, "a", "b",
+	}, {
+		cloudinit.OutAll, "a", "b",
+	}, {
+		cloudinit.OutAll, "", "",
+	}}
+
+	cfg := cloudinit.New()
+	stdout, stderr := cfg.Output(cloudinit.OutAll)
+	c.Assert(stdout, gc.Equals, "")
+	c.Assert(stderr, gc.Equals, "")
+	for i, t := range tests {
+		c.Logf("test %d: %+v", i, t)
+		cfg.SetOutput(t.kind, t.stdout, t.stderr)
+		stdout, stderr = cfg.Output(t.kind)
+		c.Assert(stdout, gc.Equals, t.stdout)
+		c.Assert(stderr, gc.Equals, t.stderr)
+	}
+}
+
 //#cloud-config
 //packages:
 //- juju

=== modified file 'cloudinit/options.go'
--- cloudinit/options.go	2013-11-26 12:24:48 +0000
+++ cloudinit/options.go	2013-12-03 06:15:09 +0000
@@ -31,6 +31,13 @@
 	cfg.set("apt_upgrade", yes, yes)
 }
 
+// AptUpgrade returns the value set by SetAptUpgrade, or
+// false if no call to SetAptUpgrade has been made.
+func (cfg *Config) AptUpgrade() bool {
+	update, _ := cfg.attrs["apt_upgrade"].(bool)
+	return update
+}
+
 // SetUpdate sets whether cloud-init runs "apt-get update"
 // on first boot.
 func (cfg *Config) SetAptUpdate(yes bool) {
@@ -233,6 +240,20 @@
 	cfg.attrs["output"] = out
 }
 
+// Output returns the output destination passed to SetOutput for
+// the specified output kind.
+func (cfg *Config) Output(kind OutputKind) (stdout, stderr string) {
+	if out, ok := cfg.attrs["output"].(map[string]interface{}); ok {
+		switch out := out[string(kind)].(type) {
+		case string:
+			stdout = out
+		case []string:
+			stdout, stderr = out[0], out[1]
+		}
+	}
+	return stdout, stderr
+}
+
 // AddSSHKey adds a pre-generated ssh key to the
 // server keyring. Keys that are added like this will be
 // written to /etc/ssh and new random keys will not

=== added file 'cloudinit/progress.go'
--- cloudinit/progress.go	1970-01-01 00:00:00 +0000
+++ cloudinit/progress.go	2013-12-03 06:15:09 +0000
@@ -0,0 +1,43 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package cloudinit
+
+import (
+	"fmt"
+
+	"launchpad.net/juju-core/utils"
+)
+
+// progressFd is the file descriptor to which progress is logged.
+// This is necessary so that we can redirect all non-progress
+// related stderr away.
+//
+// Note, from the Bash manual:
+//   "Redirections using file descriptors greater than 9 should be
+//   used with care, as they may conflict with file descriptors the
+//   shell uses internally."
+const progressFd = 9
+
+// InitProgressCmd will return a command to initialise progress
+// reporting, sending messages to stderr. If LogProgressCmd is
+// used in a script, InitProgressCmd MUST be executed beforehand.
+//
+// The returned command is idempotent; this is important, to
+// allow a script to be embedded in another with stderr redirected,
+// in which case InitProgressCmd must precede the redirection.
+func InitProgressCmd() string {
+	return fmt.Sprintf("test -e /proc/self/fd/%d || exec %d>&2", progressFd, progressFd)
+}
+
+// LogProgressCmd will return a command to log the specified progress
+// message to stderr; the resultant command should be added to the
+// configuration as a runcmd or bootcmd as appropriate.
+//
+// If there are any uses of LogProgressCmd in a configuration, the
+// configuration MUST precede the command with the result of
+// InitProgressCmd.
+func LogProgressCmd(format string, args ...interface{}) string {
+	msg := utils.ShQuote(fmt.Sprintf(format, args...))
+	return fmt.Sprintf("echo %s >&%d", msg, progressFd)
+}

=== added file 'cloudinit/progress_test.go'
--- cloudinit/progress_test.go	1970-01-01 00:00:00 +0000
+++ cloudinit/progress_test.go	2013-12-03 06:15:09 +0000
@@ -0,0 +1,27 @@
+// Copyright 2011, 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package cloudinit_test
+
+import (
+	"regexp"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/cloudinit"
+)
+
+type progressSuite struct{}
+
+var _ = gc.Suite(&progressSuite{})
+
+func (*progressSuite) TestProgressCmds(c *gc.C) {
+	initCmd := cloudinit.InitProgressCmd()
+	re := regexp.MustCompile(`test -e /proc/self/fd/([0-9]+) \|\| exec ([0-9]+)>&2`)
+	submatch := re.FindStringSubmatch(initCmd)
+	c.Assert(submatch, gc.HasLen, 3)
+	c.Assert(submatch[0], gc.Equals, initCmd)
+	c.Assert(submatch[1], gc.Equals, submatch[2])
+	logCmd := cloudinit.LogProgressCmd("he'llo\"!")
+	c.Assert(logCmd, gc.Equals, `echo 'he'"'"'llo"!' >&`+submatch[1])
+}

=== added directory 'cloudinit/sshinit'
=== renamed file 'environs/manual/agent.go' => 'cloudinit/sshinit/configure.go'
--- environs/manual/agent.go	2013-11-14 08:27:15 +0000
+++ cloudinit/sshinit/configure.go	2013-12-03 06:15:09 +0000
@@ -1,7 +1,7 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package manual
+package sshinit
 
 import (
 	"encoding/base64"
@@ -9,25 +9,29 @@
 	"os"
 	"strings"
 
-	corecloudinit "launchpad.net/juju-core/cloudinit"
-	"launchpad.net/juju-core/environs/cloudinit"
+	"launchpad.net/loggo"
+
+	"launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/utils/ssh"
 )
 
-// ProvisionMachineAgent connects to a machine over SSH,
-// and installs a machine agent.
-func ProvisionMachineAgent(host string, mcfg *cloudinit.MachineConfig) error {
+var logger = loggo.GetLogger("juju.cloudinit.sshinit")
+
+// Configure connects to the specified host over SSH,
+// and executes a script that carries out cloud-config.
+func Configure(host string, cfg *cloudinit.Config) error {
 	logger.Infof("Provisioning machine agent on %s", host)
-	script, err := provisionMachineAgentScript(mcfg)
+	script, err := generateScript(cfg)
 	if err != nil {
 		return err
 	}
 	scriptBase64 := base64.StdEncoding.EncodeToString([]byte(script))
 	script = fmt.Sprintf(`F=$(mktemp); echo %s | base64 -d > $F; . $F`, scriptBase64)
-	cmd := sshCommand(
+	cmd := ssh.Command(
 		host,
-		fmt.Sprintf("sudo bash -c '%s'", script),
-		allocateTTY,
+		[]string{"sudo", fmt.Sprintf("bash -c '%s'", script)},
+		ssh.AllocateTTY,
 	)
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
@@ -35,17 +39,9 @@
 	return cmd.Run()
 }
 
-// provisionMachineAgentScript generates the script necessary
-// to install a machine agent with the provided MachineConfig.
-func provisionMachineAgentScript(mcfg *cloudinit.MachineConfig) (string, error) {
-	// We generate a cloud-init config, which we'll then pull the runcmds
-	// and prerequisite packages out of. Rather than generating a cloud-config,
-	// we'll just generate a shell script.
-	cloudcfg := corecloudinit.New()
-	if err := cloudinit.Configure(mcfg, cloudcfg); err != nil {
-		return "", err
-	}
-
+// generateScript generates the script that applies
+// the specified cloud-config.
+func generateScript(cloudcfg *cloudinit.Config) (string, error) {
 	// TODO(axw): 2013-08-23 bug 1215777
 	// Carry out configuration for ssh-keys-per-user,
 	// machine-updates-authkeys, using cloud-init config.
@@ -78,23 +74,38 @@
 	// We prepend "set -xe". This is already in runcmds,
 	// but added here to avoid relying on that to be
 	// invariant.
-	script := []string{"#!/bin/bash", "set -xe"}
+	script := []string{"#!/bin/bash", "set -e"}
+	// We must initialise progress reporting before entering
+	// the subshell and redirecting stderr.
+	script = append(script, cloudinit.InitProgressCmd())
+	stdout, stderr := cloudcfg.Output(cloudinit.OutAll)
+	script = append(script, "(")
+	if stderr != "" {
+		script = append(script, "(")
+	}
 	script = append(script, bootcmds...)
 	script = append(script, pkgcmds...)
 	script = append(script, runcmds...)
+	if stderr != "" {
+		script = append(script, ") "+stdout)
+		script = append(script, ") "+stderr)
+	} else {
+		script = append(script, ") "+stdout+" 2>&1")
+	}
 	return strings.Join(script, "\n"), nil
 }
 
 // addPackageCommands returns a slice of commands that, when run,
 // will add the required apt repositories and packages.
-func addPackageCommands(cfg *corecloudinit.Config) ([]string, error) {
+func addPackageCommands(cfg *cloudinit.Config) ([]string, error) {
 	var cmds []string
 	if len(cfg.AptSources()) > 0 {
-		// Ensure apt-add-repository is available.
+		// Ensure add-apt-repository is available.
+		cmds = append(cmds, cloudinit.LogProgressCmd("Installing add-apt-repository"))
 		cmds = append(cmds, "apt-get -y install python-software-properties")
 	}
 	for _, src := range cfg.AptSources() {
-		// PPA keys are obtained by apt-add-repository, from launchpad.
+		// PPA keys are obtained by add-apt-repository, from launchpad.
 		if !strings.HasPrefix(src.Source, "ppa:") {
 			if src.Key != "" {
 				key := utils.ShQuote(src.Key)
@@ -102,14 +113,19 @@
 				cmds = append(cmds, cmd)
 			}
 		}
-		cmds = append(cmds, "apt-add-repository -y "+utils.ShQuote(src.Source))
+		cmds = append(cmds, cloudinit.LogProgressCmd("Adding apt repository: %s", src.Source))
+		cmds = append(cmds, "add-apt-repository -y "+utils.ShQuote(src.Source))
 	}
-	if len(cfg.AptSources()) > 0 {
+	if len(cfg.AptSources()) > 0 || cfg.AptUpdate() {
+		cmds = append(cmds, cloudinit.LogProgressCmd("Running apt-get update"))
 		cmds = append(cmds, "apt-get -y update")
 	}
-	// Note: explicitly ignoring apt_upgrade, so as not to trample the target
-	// machine's existing configuration.
+	if cfg.AptUpgrade() {
+		cmds = append(cmds, cloudinit.LogProgressCmd("Running apt-get upgrade"))
+		cmds = append(cmds, "apt-get -y upgrade")
+	}
 	for _, pkg := range cfg.Packages() {
+		cmds = append(cmds, cloudinit.LogProgressCmd("Installing package: %s", pkg))
 		cmd := fmt.Sprintf("apt-get -y install %s", utils.ShQuote(pkg))
 		cmds = append(cmds, cmd)
 	}

=== renamed file 'environs/manual/agent_test.go' => 'cloudinit/sshinit/configure_test.go'
--- environs/manual/agent_test.go	2013-11-14 08:27:15 +0000
+++ cloudinit/sshinit/configure_test.go	2013-12-03 06:15:09 +0000
@@ -1,35 +1,46 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package manual
+package sshinit
 
 import (
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
-	"launchpad.net/juju-core/environs/cloudinit"
+	envcloudinit "launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/environs/config"
 	envtools "launchpad.net/juju-core/environs/tools"
-	_ "launchpad.net/juju-core/provider/dummy"
 	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/version"
 )
 
-type agentSuite struct {
+type configureSuite struct {
 	testbase.LoggingSuite
 }
 
-var _ = gc.Suite(&agentSuite{})
-
-func dummyConfig(c *gc.C, stateServer bool, vers version.Binary) *config.Config {
+var _ = gc.Suite(&configureSuite{})
+
+type testProvider struct {
+	environs.EnvironProvider
+}
+
+func (p *testProvider) SecretAttrs(cfg *config.Config) (map[string]string, error) {
+	return map[string]string{}, nil
+}
+
+func init() {
+	environs.RegisterProvider("sshinit_test", &testProvider{})
+}
+
+func testConfig(c *gc.C, stateServer bool, vers version.Binary) *config.Config {
 	testConfig, err := config.New(config.UseDefaults, coretesting.FakeConfig())
 	c.Assert(err, gc.IsNil)
 	testConfig, err = testConfig.Apply(map[string]interface{}{
-		"type":           "dummy",
-		"state-server":   stateServer,
+		"type":           "sshinit_test",
 		"default-series": vers.Series,
 		"agent-version":  vers.Number.String(),
 	})
@@ -37,8 +48,8 @@
 	return testConfig
 }
 
-func (s *agentSuite) getMachineConfig(c *gc.C, stateServer bool, vers version.Binary) *cloudinit.MachineConfig {
-	var mcfg *cloudinit.MachineConfig
+func (s *configureSuite) getCloudConfig(c *gc.C, stateServer bool, vers version.Binary) *cloudinit.Config {
+	var mcfg *envcloudinit.MachineConfig
 	if stateServer {
 		mcfg = environs.NewBootstrapMachineConfig("http://whatever/dotcom")
 	} else {
@@ -48,10 +59,13 @@
 		Version: vers,
 		URL:     "file:///var/lib/juju/storage/" + envtools.StorageName(vers),
 	}
-	environConfig := dummyConfig(c, stateServer, vers)
+	environConfig := testConfig(c, stateServer, vers)
 	err := environs.FinishMachineConfig(mcfg, environConfig, constraints.Value{})
 	c.Assert(err, gc.IsNil)
-	return mcfg
+	cloudcfg := cloudinit.New()
+	err = envcloudinit.Configure(mcfg, cloudcfg)
+	c.Assert(err, gc.IsNil)
+	return cloudcfg
 }
 
 var allSeries = [...]string{"precise", "quantal", "raring", "saucy"}
@@ -63,10 +77,10 @@
 	return gc.Not(checker)
 }
 
-func (s *agentSuite) TestAptSources(c *gc.C) {
+func (s *configureSuite) TestAptSources(c *gc.C) {
 	for _, series := range allSeries {
 		vers := version.MustParseBinary("1.16.0-" + series + "-amd64")
-		script, err := provisionMachineAgentScript(s.getMachineConfig(c, true, vers))
+		script, err := generateScript(s.getCloudConfig(c, true, vers))
 		c.Assert(err, gc.IsNil)
 
 		// Only Precise requires the cloud-tools pocket.
@@ -82,7 +96,7 @@
 		c.Assert(
 			script,
 			checkIff(gc.Matches, needsCloudTools),
-			"(.|\n)*apt-add-repository.*cloud-tools(.|\n)*",
+			"(.|\n)*add-apt-repository.*cloud-tools(.|\n)*",
 		)
 
 		// Only Quantal requires the PPA (for mongo).
@@ -90,10 +104,10 @@
 		c.Assert(
 			script,
 			checkIff(gc.Matches, needsJujuPPA),
-			"(.|\n)*apt-add-repository.*ppa:juju/stable(.|\n)*",
+			"(.|\n)*add-apt-repository.*ppa:juju/stable(.|\n)*",
 		)
 
-		// Only install python-software-properties (apt-add-repository)
+		// Only install python-software-properties (add-apt-repository)
 		// if we need to.
 		c.Assert(
 			script,
@@ -102,3 +116,39 @@
 		)
 	}
 }
+
+func assertScriptMatches(c *gc.C, cfg *cloudinit.Config, pattern string, match bool) {
+	script, err := generateScript(cfg)
+	c.Assert(err, gc.IsNil)
+	checker := gc.Matches
+	if !match {
+		checker = gc.Not(checker)
+	}
+	c.Assert(script, checker, pattern)
+}
+
+func (s *configureSuite) TestAptUpdate(c *gc.C) {
+	// apt-get update is run if either AptUpdate is set,
+	// or apt sources are defined.
+	const aptGetUpdatePattern = "(.|\n)*apt-get -y update(.|\n)*"
+	cfg := cloudinit.New()
+	c.Assert(cfg.AptUpdate(), gc.Equals, false)
+	c.Assert(cfg.AptSources(), gc.HasLen, 0)
+	assertScriptMatches(c, cfg, aptGetUpdatePattern, false)
+	cfg.SetAptUpdate(true)
+	assertScriptMatches(c, cfg, aptGetUpdatePattern, true)
+	cfg.SetAptUpdate(false)
+	cfg.AddAptSource("source", "key")
+	assertScriptMatches(c, cfg, aptGetUpdatePattern, true)
+}
+
+func (s *configureSuite) TestAptUpgrade(c *gc.C) {
+	// apt-get upgrade is only run if AptUpgrade is set.
+	const aptGetUpgradePattern = "(.|\n)*apt-get -y upgrade(.|\n)*"
+	cfg := cloudinit.New()
+	cfg.SetAptUpdate(true)
+	cfg.AddAptSource("source", "key")
+	assertScriptMatches(c, cfg, aptGetUpgradePattern, false)
+	cfg.SetAptUpgrade(true)
+	assertScriptMatches(c, cfg, aptGetUpgradePattern, true)
+}

=== added file 'cloudinit/sshinit/suite_test.go'
--- cloudinit/sshinit/suite_test.go	1970-01-01 00:00:00 +0000
+++ cloudinit/sshinit/suite_test.go	2013-12-03 06:15:09 +0000
@@ -0,0 +1,14 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package sshinit
+
+import (
+	stdtesting "testing"
+
+	gc "launchpad.net/gocheck"
+)
+
+func Test(t *stdtesting.T) {
+	gc.TestingT(t)
+}

=== modified file 'cmd/juju/debughooks.go'
--- cmd/juju/debughooks.go	2013-11-28 11:57:57 +0000
+++ cmd/juju/debughooks.go	2013-12-03 06:15:09 +0000
@@ -146,7 +146,7 @@
 	debugctx := unitdebug.NewHooksContext(c.Target)
 	script := base64.StdEncoding.EncodeToString([]byte(unitdebug.ClientScript(debugctx, c.hooks)))
 	innercmd := fmt.Sprintf(`F=$(mktemp); echo %s | base64 -d > $F; . $F`, script)
-	args := []string{"--", fmt.Sprintf("sudo /bin/bash -c '%s'", innercmd)}
+	args := []string{fmt.Sprintf("sudo /bin/bash -c '%s'", innercmd)}
 	c.Args = args
 	return c.SSHCommand.Run(ctx)
 }

=== modified file 'cmd/juju/debughooks_test.go'
--- cmd/juju/debughooks_test.go	2013-11-08 04:45:20 +0000
+++ cmd/juju/debughooks_test.go	2013-12-03 06:15:09 +0000
@@ -19,7 +19,7 @@
 	SSHCommonSuite
 }
 
-const debugHooksArgs = `-l ubuntu -t ` + commonArgs
+const debugHooksArgs = sshArgs
 
 var debugHooksTests = []struct {
 	info   string
@@ -29,10 +29,10 @@
 	stderr string
 }{{
 	args:   []string{"mysql/0"},
-	result: regexp.QuoteMeta(debugHooksArgs + "dummyenv-0.dns -- sudo /bin/bash -c 'F=$(mktemp); echo IyEvYmluL2Jhc2gKKAojIExvY2sgdGhlIGp1anUtPHVuaXQ+LWRlYnVnIGxvY2tmaWxlLgpmbG9jayAtbiA4IHx8IChlY2hvICJGYWlsZWQgdG8gYWNxdWlyZSAvdG1wL2p1anUtdW5pdC1teXNxbC0wLWRlYnVnLWhvb2tzOiB1bml0IGlzIGFscmVhZHkgYmVpbmcgZGVidWdnZWQiIDI+JjE7IGV4aXQgMSkKKAojIENsb3NlIHRoZSBpbmhlcml0ZWQgbG9jayBGRCwgb3IgdG11eCB3aWxsIGtlZXAgaXQgb3Blbi4KZXhlYyA4PiYtCgojIFdyaXRlIG91dCB0aGUgZGVidWctaG9va3MgYXJncy4KZWNobyAiZTMwSyIgfCBiYXNlNjQgLWQgPiAvdG1wL2p1anUtdW5pdC1teXNxbC0wLWRlYnVnLWhvb2tzCgojIExvY2sgdGhlIGp1anUtPHVuaXQ+LWRlYnVnLWV4aXQgbG9ja2ZpbGUuCmZsb2NrIC1uIDkgfHwgZXhpdCAxCgojIFdhaXQgZm9yIHRtdXggdG8gYmUgaW5zdGFsbGVkLgp3aGlsZSBbICEgLWYgL3Vzci9iaW4vdG11eCBdOyBkbwogICAgc2xlZXAgMQpkb25lCgppZiBbICEgLWYgfi8udG11eC5jb25mIF07IHRoZW4KICAgICAgICBpZiBbIC1mIC91c3Ivc2hhcmUvYnlvYnUvcHJvZmlsZXMvdG11eCBdOyB0aGVuCiAgICAgICAgICAgICAgICAjIFVzZSBieW9idS90bXV4IHByb2ZpbGUgZm9yIGZhbWlsaWFyIGtleWJpbmRpbmdzIGFuZCBicmFuZGluZwogICAgICAgICAgICAgICAgZWNobyAic291cmNlLWZpbGUgL3Vzci9zaGFyZS9ieW9idS9wcm9maWxlcy90bXV4IiA+IH4vLnRtdXguY29uZgogICAgICAgIGVsc2UKICAgICAgICAgICAgICAgICMgT3RoZXJ3aXNlLCB1c2UgdGhlIGxlZ2FjeSBqdWp1L3RtdXggY29uZmlndXJhdGlvbgogICAgICAgICAgICAgICAgY2F0ID4gfi8udG11eC5jb25mIDw8RU5ECiAgICAgICAgICAgICAgICAKIyBTdGF0dXMgYmFyCnNldC1vcHRpb24gLWcgc3RhdHVzLWJnIGJsYWNrCnNldC1vcHRpb24gLWcgc3RhdHVzLWZnIHdoaXRlCgpzZXQtd2luZG93LW9wdGlvbiAtZyB3aW5kb3ctc3RhdHVzLWN1cnJlbnQtYmcgcmVkCnNldC13aW5kb3ctb3B0aW9uIC1nIHdpbmRvdy1zdGF0dXMtY3VycmVudC1hdHRyIGJyaWdodAoKc2V0LW9wdGlvbiAtZyBzdGF0dXMtcmlnaHQgJycKCiMgUGFuZXMKc2V0LW9wdGlvbiAtZyBwYW5lLWJvcmRlci1mZyB3aGl0ZQpzZXQtb3B0aW9uIC1nIHBhbmUtYWN0aXZlLWJvcmRlci1mZyB3aGl0ZQoKIyBNb25pdG9yIGFjdGl2aXR5IG9uIHdpbmRvd3MKc2V0LXdpbmRvdy1vcHRpb24gLWcgbW9uaXRvci1hY3Rpdml0eSBvbgoKIyBTY3JlZW4gYmluZGluZ3MsIHNpbmNlIHBlb3BsZSBhcmUgbW9yZSBmYW1pbGlhciB3aXRoIHRoYXQuCnNldC1vcHRpb24gLWcgcHJlZml4IEMtYQpiaW5kIEMtYSBsYXN0LXdpbmRvdwpiaW5kIGEgc2VuZC1rZXkgQy1hCgpiaW5kIHwgc3BsaXQtd2luZG93IC1oCmJpbmQgLSBzcGxpdC13aW5kb3cgLXYKCiMgRml4IENUUkwtUEdVUC9QR0RPV04gZm9yIHZpbQpzZXQtd2luZG93LW9wdGlvbiAtZyB4dGVybS1rZXlzIG9uCgojIFByZXZlbnQgRVNDIGtleSBmcm9tIGFkZGluZyBkZWxheSBhbmQgYnJlYWtpbmcgVmltJ3MgRVNDID4gYXJyb3cga2V5CnNldC1vcHRpb24gLXMgZXNjYXBlLXRpbWUgMAoKRU5ECiAgICAgICAgZmkKZmkKCigKICAgICMgQ2xvc2UgdGhlIGluaGVyaXRlZCBsb2NrIEZELCBvciB0bXV4IHdpbGwga2VlcCBpdCBvcGVuLgogICAgZXhlYyA5PiYtCiAgICBleGVjIHRtdXggbmV3LXNlc3Npb24gLXMgbXlzcWwvMAopCikgOT4vdG1wL2p1anUtdW5pdC1teXNxbC0wLWRlYnVnLWhvb2tzLWV4aXQKKSA4Pi90bXAvanVqdS11bml0LW15c3FsLTAtZGVidWctaG9va3MKZXhpdCAkPwo= | base64 -d > $F; . $F'\n"),
+	result: regexp.QuoteMeta(debugHooksArgs + "[e-mail] -- sudo /bin/bash -c 'F=$(mktemp); echo IyEvYmluL2Jhc2gKKAojIExvY2sgdGhlIGp1anUtPHVuaXQ+LWRlYnVnIGxvY2tmaWxlLgpmbG9jayAtbiA4IHx8IChlY2hvICJGYWlsZWQgdG8gYWNxdWlyZSAvdG1wL2p1anUtdW5pdC1teXNxbC0wLWRlYnVnLWhvb2tzOiB1bml0IGlzIGFscmVhZHkgYmVpbmcgZGVidWdnZWQiIDI+JjE7IGV4aXQgMSkKKAojIENsb3NlIHRoZSBpbmhlcml0ZWQgbG9jayBGRCwgb3IgdG11eCB3aWxsIGtlZXAgaXQgb3Blbi4KZXhlYyA4PiYtCgojIFdyaXRlIG91dCB0aGUgZGVidWctaG9va3MgYXJncy4KZWNobyAiZTMwSyIgfCBiYXNlNjQgLWQgPiAvdG1wL2p1anUtdW5pdC1teXNxbC0wLWRlYnVnLWhvb2tzCgojIExvY2sgdGhlIGp1anUtPHVuaXQ+LWRlYnVnLWV4aXQgbG9ja2ZpbGUuCmZsb2NrIC1uIDkgfHwgZXhpdCAxCgojIFdhaXQgZm9yIHRtdXggdG8gYmUgaW5zdGFsbGVkLgp3aGlsZSBbICEgLWYgL3Vzci9iaW4vdG11eCBdOyBkbwogICAgc2xlZXAgMQpkb25lCgppZiBbICEgLWYgfi8udG11eC5jb25mIF07IHRoZW4KICAgICAgICBpZiBbIC1mIC91c3Ivc2hhcmUvYnlvYnUvcHJvZmlsZXMvdG11eCBdOyB0aGVuCiAgICAgICAgICAgICAgICAjIFVzZSBieW9idS90bXV4IHByb2ZpbGUgZm9yIGZhbWlsaWFyIGtleWJpbmRpbmdzIGFuZCBicmFuZGluZwogICAgICAgICAgICAgICAgZWNobyAic291cmNlLWZpbGUgL3Vzci9zaGFyZS9ieW9idS9wcm9maWxlcy90bXV4IiA+IH4vLnRtdXguY29uZgogICAgICAgIGVsc2UKICAgICAgICAgICAgICAgICMgT3RoZXJ3aXNlLCB1c2UgdGhlIGxlZ2FjeSBqdWp1L3RtdXggY29uZmlndXJhdGlvbgogICAgICAgICAgICAgICAgY2F0ID4gfi8udG11eC5jb25mIDw8RU5ECiAgICAgICAgICAgICAgICAKIyBTdGF0dXMgYmFyCnNldC1vcHRpb24gLWcgc3RhdHVzLWJnIGJsYWNrCnNldC1vcHRpb24gLWcgc3RhdHVzLWZnIHdoaXRlCgpzZXQtd2luZG93LW9wdGlvbiAtZyB3aW5kb3ctc3RhdHVzLWN1cnJlbnQtYmcgcmVkCnNldC13aW5kb3ctb3B0aW9uIC1nIHdpbmRvdy1zdGF0dXMtY3VycmVudC1hdHRyIGJyaWdodAoKc2V0LW9wdGlvbiAtZyBzdGF0dXMtcmlnaHQgJycKCiMgUGFuZXMKc2V0LW9wdGlvbiAtZyBwYW5lLWJvcmRlci1mZyB3aGl0ZQpzZXQtb3B0aW9uIC1nIHBhbmUtYWN0aXZlLWJvcmRlci1mZyB3aGl0ZQoKIyBNb25pdG9yIGFjdGl2aXR5IG9uIHdpbmRvd3MKc2V0LXdpbmRvdy1vcHRpb24gLWcgbW9uaXRvci1hY3Rpdml0eSBvbgoKIyBTY3JlZW4gYmluZGluZ3MsIHNpbmNlIHBlb3BsZSBhcmUgbW9yZSBmYW1pbGlhciB3aXRoIHRoYXQuCnNldC1vcHRpb24gLWcgcHJlZml4IEMtYQpiaW5kIEMtYSBsYXN0LXdpbmRvdwpiaW5kIGEgc2VuZC1rZXkgQy1hCgpiaW5kIHwgc3BsaXQtd2luZG93IC1oCmJpbmQgLSBzcGxpdC13aW5kb3cgLXYKCiMgRml4IENUUkwtUEdVUC9QR0RPV04gZm9yIHZpbQpzZXQtd2luZG93LW9wdGlvbiAtZyB4dGVybS1rZXlzIG9uCgojIFByZXZlbnQgRVNDIGtleSBmcm9tIGFkZGluZyBkZWxheSBhbmQgYnJlYWtpbmcgVmltJ3MgRVNDID4gYXJyb3cga2V5CnNldC1vcHRpb24gLXMgZXNjYXBlLXRpbWUgMAoKRU5ECiAgICAgICAgZmkKZmkKCigKICAgICMgQ2xvc2UgdGhlIGluaGVyaXRlZCBsb2NrIEZELCBvciB0bXV4IHdpbGwga2VlcCBpdCBvcGVuLgogICAgZXhlYyA5PiYtCiAgICBleGVjIHRtdXggbmV3LXNlc3Npb24gLXMgbXlzcWwvMAopCikgOT4vdG1wL2p1anUtdW5pdC1teXNxbC0wLWRlYnVnLWhvb2tzLWV4aXQKKSA4Pi90bXAvanVqdS11bml0LW15c3FsLTAtZGVidWctaG9va3MKZXhpdCAkPwo= | base64 -d > $F; . $F'\n"),
 }, {
 	args:   []string{"mongodb/1"},
-	result: regexp.QuoteMeta(debugHooksArgs + "dummyenv-2.dns -- sudo /bin/bash -c 'F=$(mktemp); echo IyEvYmluL2Jhc2gKKAojIExvY2sgdGhlIGp1anUtPHVuaXQ+LWRlYnVnIGxvY2tmaWxlLgpmbG9jayAtbiA4IHx8IChlY2hvICJGYWlsZWQgdG8gYWNxdWlyZSAvdG1wL2p1anUtdW5pdC1tb25nb2RiLTEtZGVidWctaG9va3M6IHVuaXQgaXMgYWxyZWFkeSBiZWluZyBkZWJ1Z2dlZCIgMj4mMTsgZXhpdCAxKQooCiMgQ2xvc2UgdGhlIGluaGVyaXRlZCBsb2NrIEZELCBvciB0bXV4IHdpbGwga2VlcCBpdCBvcGVuLgpleGVjIDg+Ji0KCiMgV3JpdGUgb3V0IHRoZSBkZWJ1Zy1ob29rcyBhcmdzLgplY2hvICJlMzBLIiB8IGJhc2U2NCAtZCA+IC90bXAvanVqdS11bml0LW1vbmdvZGItMS1kZWJ1Zy1ob29rcwoKIyBMb2NrIHRoZSBqdWp1LTx1bml0Pi1kZWJ1Zy1leGl0IGxvY2tmaWxlLgpmbG9jayAtbiA5IHx8IGV4aXQgMQoKIyBXYWl0IGZvciB0bXV4IHRvIGJlIGluc3RhbGxlZC4Kd2hpbGUgWyAhIC1mIC91c3IvYmluL3RtdXggXTsgZG8KICAgIHNsZWVwIDEKZG9uZQoKaWYgWyAhIC1mIH4vLnRtdXguY29uZiBdOyB0aGVuCiAgICAgICAgaWYgWyAtZiAvdXNyL3NoYXJlL2J5b2J1L3Byb2ZpbGVzL3RtdXggXTsgdGhlbgogICAgICAgICAgICAgICAgIyBVc2UgYnlvYnUvdG11eCBwcm9maWxlIGZvciBmYW1pbGlhciBrZXliaW5kaW5ncyBhbmQgYnJhbmRpbmcKICAgICAgICAgICAgICAgIGVjaG8gInNvdXJjZS1maWxlIC91c3Ivc2hhcmUvYnlvYnUvcHJvZmlsZXMvdG11eCIgPiB+Ly50bXV4LmNvbmYKICAgICAgICBlbHNlCiAgICAgICAgICAgICAgICAjIE90aGVyd2lzZSwgdXNlIHRoZSBsZWdhY3kganVqdS90bXV4IGNvbmZpZ3VyYXRpb24KICAgICAgICAgICAgICAgIGNhdCA+IH4vLnRtdXguY29uZiA8PEVORAogICAgICAgICAgICAgICAgCiMgU3RhdHVzIGJhcgpzZXQtb3B0aW9uIC1nIHN0YXR1cy1iZyBibGFjawpzZXQtb3B0aW9uIC1nIHN0YXR1cy1mZyB3aGl0ZQoKc2V0LXdpbmRvdy1vcHRpb24gLWcgd2luZG93LXN0YXR1cy1jdXJyZW50LWJnIHJlZApzZXQtd2luZG93LW9wdGlvbiAtZyB3aW5kb3ctc3RhdHVzLWN1cnJlbnQtYXR0ciBicmlnaHQKCnNldC1vcHRpb24gLWcgc3RhdHVzLXJpZ2h0ICcnCgojIFBhbmVzCnNldC1vcHRpb24gLWcgcGFuZS1ib3JkZXItZmcgd2hpdGUKc2V0LW9wdGlvbiAtZyBwYW5lLWFjdGl2ZS1ib3JkZXItZmcgd2hpdGUKCiMgTW9uaXRvciBhY3Rpdml0eSBvbiB3aW5kb3dzCnNldC13aW5kb3ctb3B0aW9uIC1nIG1vbml0b3ItYWN0aXZpdHkgb24KCiMgU2NyZWVuIGJpbmRpbmdzLCBzaW5jZSBwZW9wbGUgYXJlIG1vcmUgZmFtaWxpYXIgd2l0aCB0aGF0LgpzZXQtb3B0aW9uIC1nIHByZWZpeCBDLWEKYmluZCBDLWEgbGFzdC13aW5kb3cKYmluZCBhIHNlbmQta2V5IEMtYQoKYmluZCB8IHNwbGl0LXdpbmRvdyAtaApiaW5kIC0gc3BsaXQtd2luZG93IC12CgojIEZpeCBDVFJMLVBHVVAvUEdET1dOIGZvciB2aW0Kc2V0LXdpbmRvdy1vcHRpb24gLWcgeHRlcm0ta2V5cyBvbgoKIyBQcmV2ZW50IEVTQyBrZXkgZnJvbSBhZGRpbmcgZGVsYXkgYW5kIGJyZWFraW5nIFZpbSdzIEVTQyA+IGFycm93IGtleQpzZXQtb3B0aW9uIC1zIGVzY2FwZS10aW1lIDAKCkVORAogICAgICAgIGZpCmZpCgooCiAgICAjIENsb3NlIHRoZSBpbmhlcml0ZWQgbG9jayBGRCwgb3IgdG11eCB3aWxsIGtlZXAgaXQgb3Blbi4KICAgIGV4ZWMgOT4mLQogICAgZXhlYyB0bXV4IG5ldy1zZXNzaW9uIC1zIG1vbmdvZGIvMQopCikgOT4vdG1wL2p1anUtdW5pdC1tb25nb2RiLTEtZGVidWctaG9va3MtZXhpdAopIDg+L3RtcC9qdWp1LXVuaXQtbW9uZ29kYi0xLWRlYnVnLWhvb2tzCmV4aXQgJD8K | base64 -d > $F; . $F'\n"),
+	result: regexp.QuoteMeta(debugHooksArgs + "[e-mail] -- sudo /bin/bash -c 'F=$(mktemp); echo IyEvYmluL2Jhc2gKKAojIExvY2sgdGhlIGp1anUtPHVuaXQ+LWRlYnVnIGxvY2tmaWxlLgpmbG9jayAtbiA4IHx8IChlY2hvICJGYWlsZWQgdG8gYWNxdWlyZSAvdG1wL2p1anUtdW5pdC1tb25nb2RiLTEtZGVidWctaG9va3M6IHVuaXQgaXMgYWxyZWFkeSBiZWluZyBkZWJ1Z2dlZCIgMj4mMTsgZXhpdCAxKQooCiMgQ2xvc2UgdGhlIGluaGVyaXRlZCBsb2NrIEZELCBvciB0bXV4IHdpbGwga2VlcCBpdCBvcGVuLgpleGVjIDg+Ji0KCiMgV3JpdGUgb3V0IHRoZSBkZWJ1Zy1ob29rcyBhcmdzLgplY2hvICJlMzBLIiB8IGJhc2U2NCAtZCA+IC90bXAvanVqdS11bml0LW1vbmdvZGItMS1kZWJ1Zy1ob29rcwoKIyBMb2NrIHRoZSBqdWp1LTx1bml0Pi1kZWJ1Zy1leGl0IGxvY2tmaWxlLgpmbG9jayAtbiA5IHx8IGV4aXQgMQoKIyBXYWl0IGZvciB0bXV4IHRvIGJlIGluc3RhbGxlZC4Kd2hpbGUgWyAhIC1mIC91c3IvYmluL3RtdXggXTsgZG8KICAgIHNsZWVwIDEKZG9uZQoKaWYgWyAhIC1mIH4vLnRtdXguY29uZiBdOyB0aGVuCiAgICAgICAgaWYgWyAtZiAvdXNyL3NoYXJlL2J5b2J1L3Byb2ZpbGVzL3RtdXggXTsgdGhlbgogICAgICAgICAgICAgICAgIyBVc2UgYnlvYnUvdG11eCBwcm9maWxlIGZvciBmYW1pbGlhciBrZXliaW5kaW5ncyBhbmQgYnJhbmRpbmcKICAgICAgICAgICAgICAgIGVjaG8gInNvdXJjZS1maWxlIC91c3Ivc2hhcmUvYnlvYnUvcHJvZmlsZXMvdG11eCIgPiB+Ly50bXV4LmNvbmYKICAgICAgICBlbHNlCiAgICAgICAgICAgICAgICAjIE90aGVyd2lzZSwgdXNlIHRoZSBsZWdhY3kganVqdS90bXV4IGNvbmZpZ3VyYXRpb24KICAgICAgICAgICAgICAgIGNhdCA+IH4vLnRtdXguY29uZiA8PEVORAogICAgICAgICAgICAgICAgCiMgU3RhdHVzIGJhcgpzZXQtb3B0aW9uIC1nIHN0YXR1cy1iZyBibGFjawpzZXQtb3B0aW9uIC1nIHN0YXR1cy1mZyB3aGl0ZQoKc2V0LXdpbmRvdy1vcHRpb24gLWcgd2luZG93LXN0YXR1cy1jdXJyZW50LWJnIHJlZApzZXQtd2luZG93LW9wdGlvbiAtZyB3aW5kb3ctc3RhdHVzLWN1cnJlbnQtYXR0ciBicmlnaHQKCnNldC1vcHRpb24gLWcgc3RhdHVzLXJpZ2h0ICcnCgojIFBhbmVzCnNldC1vcHRpb24gLWcgcGFuZS1ib3JkZXItZmcgd2hpdGUKc2V0LW9wdGlvbiAtZyBwYW5lLWFjdGl2ZS1ib3JkZXItZmcgd2hpdGUKCiMgTW9uaXRvciBhY3Rpdml0eSBvbiB3aW5kb3dzCnNldC13aW5kb3ctb3B0aW9uIC1nIG1vbml0b3ItYWN0aXZpdHkgb24KCiMgU2NyZWVuIGJpbmRpbmdzLCBzaW5jZSBwZW9wbGUgYXJlIG1vcmUgZmFtaWxpYXIgd2l0aCB0aGF0LgpzZXQtb3B0aW9uIC1nIHByZWZpeCBDLWEKYmluZCBDLWEgbGFzdC13aW5kb3cKYmluZCBhIHNlbmQta2V5IEMtYQoKYmluZCB8IHNwbGl0LXdpbmRvdyAtaApiaW5kIC0gc3BsaXQtd2luZG93IC12CgojIEZpeCBDVFJMLVBHVVAvUEdET1dOIGZvciB2aW0Kc2V0LXdpbmRvdy1vcHRpb24gLWcgeHRlcm0ta2V5cyBvbgoKIyBQcmV2ZW50IEVTQyBrZXkgZnJvbSBhZGRpbmcgZGVsYXkgYW5kIGJyZWFraW5nIFZpbSdzIEVTQyA+IGFycm93IGtleQpzZXQtb3B0aW9uIC1zIGVzY2FwZS10aW1lIDAKCkVORAogICAgICAgIGZpCmZpCgooCiAgICAjIENsb3NlIHRoZSBpbmhlcml0ZWQgbG9jayBGRCwgb3IgdG11eCB3aWxsIGtlZXAgaXQgb3Blbi4KICAgIGV4ZWMgOT4mLQogICAgZXhlYyB0bXV4IG5ldy1zZXNzaW9uIC1zIG1vbmdvZGIvMQopCikgOT4vdG1wL2p1anUtdW5pdC1tb25nb2RiLTEtZGVidWctaG9va3MtZXhpdAopIDg+L3RtcC9qdWp1LXVuaXQtbW9uZ29kYi0xLWRlYnVnLWhvb2tzCmV4aXQgJD8K | base64 -d > $F; . $F'\n"),
 }, {
 	info:   `"*" is a valid hook name: it means hook everything`,
 	args:   []string{"mysql/0", "*"},

=== modified file 'cmd/juju/scp.go'
--- cmd/juju/scp.go	2013-11-20 13:29:15 +0000
+++ cmd/juju/scp.go	2013-12-03 06:15:09 +0000
@@ -5,10 +5,10 @@
 
 import (
 	"errors"
-	"os/exec"
 	"strings"
 
 	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/utils/ssh"
 )
 
 // SCPCommand is responsible for launching a scp command to copy files to/from remote machine(s)
@@ -80,9 +80,7 @@
 		}
 	}
 
-	args := []string{"-o", "StrictHostKeyChecking no", "-o", "PasswordAuthentication no"}
-	args = append(args, c.Args...)
-	cmd := exec.Command("scp", args...)
+	cmd := ssh.ScpCommand(c.Args[0], c.Args[1], ssh.NoPasswordAuthentication)
 	cmd.Stdin = ctx.Stdin
 	cmd.Stdout = ctx.Stdout
 	cmd.Stderr = ctx.Stderr

=== modified file 'cmd/juju/scp_test.go'
--- cmd/juju/scp_test.go	2013-11-08 04:45:20 +0000
+++ cmd/juju/scp_test.go	2013-12-03 06:15:09 +0000
@@ -39,7 +39,7 @@
 	},
 	{
 		[]string{"a", "b", "mysql/0"},
-		commonArgs + "a b mysql/0\n",
+		commonArgs + "a b\n",
 	},
 	{
 		[]string{"mongodb/1:foo", "mongodb/0:"},

=== modified file 'cmd/juju/ssh.go'
--- cmd/juju/ssh.go	2013-11-27 12:02:11 +0000
+++ cmd/juju/ssh.go	2013-12-03 06:15:09 +0000
@@ -6,7 +6,6 @@
 import (
 	"errors"
 	"fmt"
-	"os/exec"
 	"time"
 
 	"launchpad.net/juju-core/cmd"
@@ -16,6 +15,7 @@
 	"launchpad.net/juju-core/rpc"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/utils/ssh"
 )
 
 // SSHCommand is responsible for launching a ssh shell on a given unit or machine.
@@ -82,9 +82,13 @@
 	if err != nil {
 		return err
 	}
-	args := []string{"-l", "ubuntu", "-t", "-o", "StrictHostKeyChecking no", "-o", "PasswordAuthentication no", host}
-	args = append(args, c.Args...)
-	cmd := exec.Command("ssh", args...)
+	args := c.Args
+	if len(args) > 0 && args[0] == "--" {
+		// utils/ssh adds "--"; we will continue to accept
+		// it from the CLI for backwards compatibility.
+		args = args[1:]
+	}
+	cmd := ssh.Command("ubuntu@"+host, args, ssh.NoPasswordAuthentication, ssh.AllocateTTY)
 	cmd.Stdin = ctx.Stdin
 	cmd.Stdout = ctx.Stdout
 	cmd.Stderr = ctx.Stderr

=== modified file 'cmd/juju/ssh_test.go'
--- cmd/juju/ssh_test.go	2013-11-08 04:45:20 +0000
+++ cmd/juju/ssh_test.go	2013-12-03 06:15:09 +0000
@@ -60,7 +60,7 @@
 
 const (
 	commonArgs = `-o StrictHostKeyChecking no -o PasswordAuthentication no `
-	sshArgs    = `-l ubuntu -t ` + commonArgs
+	sshArgs    = commonArgs + `-t `
 )
 
 var sshTests = []struct {
@@ -69,30 +69,30 @@
 }{
 	{
 		[]string{"ssh", "0"},
-		sshArgs + "dummyenv-0.dns\n",
+		sshArgs + "[e-mail]\n",
 	},
 	// juju ssh 0 'uname -a'
 	{
 		[]string{"ssh", "0", "uname -a"},
-		sshArgs + "dummyenv-0.dns uname -a\n",
+		sshArgs + "[e-mail] -- uname -a\n",
 	},
 	// juju ssh 0 -- uname -a
 	{
 		[]string{"ssh", "0", "--", "uname", "-a"},
-		sshArgs + "dummyenv-0.dns -- uname -a\n",
+		sshArgs + "[e-mail] -- uname -a\n",
 	},
 	// juju ssh 0 uname -a
 	{
 		[]string{"ssh", "0", "uname", "-a"},
-		sshArgs + "dummyenv-0.dns uname -a\n",
+		sshArgs + "[e-mail] -- uname -a\n",
 	},
 	{
 		[]string{"ssh", "mysql/0"},
-		sshArgs + "dummyenv-0.dns\n",
+		sshArgs + "[e-mail]\n",
 	},
 	{
 		[]string{"ssh", "mongodb/1"},
-		sshArgs + "dummyenv-2.dns\n",
+		sshArgs + "[e-mail]\n",
 	},
 }
 

=== modified file 'environs/bootstrap/state.go'
--- environs/bootstrap/state.go	2013-11-18 04:53:44 +0000
+++ environs/bootstrap/state.go	2013-12-03 06:15:09 +0000
@@ -52,6 +52,11 @@
 	return storage.URL(StateFile)
 }
 
+// DeleteStateFile deletes the state file on the given storage.
+func DeleteStateFile(storage storage.Storage) error {
+	return storage.Remove(StateFile)
+}
+
 // SaveState writes the given state to the given storage.
 func SaveState(storage storage.StorageWriter, state *BootstrapState) error {
 	data, err := goyaml.Marshal(state)

=== modified file 'environs/bootstrap/state_test.go'
--- environs/bootstrap/state_test.go	2013-11-18 04:53:44 +0000
+++ environs/bootstrap/state_test.go	2013-12-03 06:15:09 +0000
@@ -6,6 +6,8 @@
 import (
 	"bytes"
 	"io/ioutil"
+	"os"
+	"path/filepath"
 
 	gc "launchpad.net/gocheck"
 	"launchpad.net/goyaml"
@@ -15,6 +17,7 @@
 	"launchpad.net/juju-core/environs/storage"
 	envtesting "launchpad.net/juju-core/environs/testing"
 	"launchpad.net/juju-core/instance"
+	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
 )
 
@@ -48,6 +51,24 @@
 	c.Check(url, gc.Equals, expectedURL)
 }
 
+func (suite *StateSuite) TestDeleteStateFile(c *gc.C) {
+	closer, stor, dataDir := envtesting.CreateLocalTestStorage(c)
+	defer closer.Close()
+
+	err := bootstrap.DeleteStateFile(stor)
+	c.Assert(err, gc.IsNil) // doesn't exist, juju don't care
+
+	_, err = bootstrap.CreateStateFile(stor)
+	c.Assert(err, gc.IsNil)
+	_, err = os.Stat(filepath.Join(dataDir, bootstrap.StateFile))
+	c.Assert(err, gc.IsNil)
+
+	err = bootstrap.DeleteStateFile(stor)
+	c.Assert(err, gc.IsNil)
+	_, err = os.Stat(filepath.Join(dataDir, bootstrap.StateFile))
+	c.Assert(err, jc.Satisfies, os.IsNotExist)
+}
+
 func (suite *StateSuite) TestSaveStateWritesStateFile(c *gc.C) {
 	stor := suite.newStorage(c)
 	arch := "amd64"

=== modified file 'environs/cloudinit.go'
--- environs/cloudinit.go	2013-11-26 12:24:48 +0000
+++ environs/cloudinit.go	2013-12-03 06:15:09 +0000
@@ -133,9 +133,16 @@
 	for _, script := range additionalScripts {
 		cloudcfg.AddRunCmd(script)
 	}
-	err := cloudinit.Configure(cfg, cloudcfg)
-	if err != nil {
-		return nil, err
+	// When bootstrapping, we only want to apt-get update/upgrade
+	// and setup the SSH keys. The rest we leave to cloudinit/sshinit.
+	if cfg.StateServer {
+		if err := cloudinit.ConfigureBasic(cfg, cloudcfg); err != nil {
+			return nil, err
+		}
+	} else {
+		if err := cloudinit.Configure(cfg, cloudcfg); err != nil {
+			return nil, err
+		}
 	}
 	data, err := cloudcfg.Render()
 	logger.Tracef("Generated cloud init:\n%s", string(data))

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2013-11-26 12:24:48 +0000
+++ environs/cloudinit/cloudinit.go	2013-12-03 06:15:09 +0000
@@ -132,16 +132,60 @@
 // Configure updates the provided cloudinit.Config with
 // configuration to initialize a Juju machine agent.
 func Configure(cfg *MachineConfig, c *cloudinit.Config) error {
+	if err := ConfigureBasic(cfg, c); err != nil {
+		return err
+	}
+	return ConfigureJuju(cfg, c)
+}
+
+const cloudInitOutputLog = "/var/log/cloud-init-output.log"
+
+// ConfigureBasic updates the provided cloudinit.Config with
+// basic configuration to initialise an OS image, such that it can
+// be connected to via SSH, and log to a standard location.
+//
+// Any potentially failing operation should not be added to the
+// configuration, but should instead be done in ConfigureJuju.
+//
+// Note: we don't do apt update/upgrade here so as not to have to wait on
+// apt to finish when performing the second half of image initialisation.
+// Doing it later brings the benefit of feedback in the face of errors,
+// but adds to the running time of initialisation due to lack of activity
+// between image bringup and start of agent installation.
+func ConfigureBasic(cfg *MachineConfig, c *cloudinit.Config) error {
+	c.AddSSHAuthorizedKeys(cfg.AuthorizedKeys)
+	c.SetOutput(cloudinit.OutAll, "| tee -a "+cloudInitOutputLog, "")
+	return nil
+}
+
+// ConfigureJuju updates the provided cloudinit.Config with configuration
+// to initialise a Juju machine agent.
+func ConfigureJuju(cfg *MachineConfig, c *cloudinit.Config) error {
 	if err := verifyConfig(cfg); err != nil {
 		return err
 	}
 
-	// General options.
+	// Initialise progress reporting. We need to do separately for runcmd
+	// and (possibly, below) for bootcmd, as they may be run in different
+	// shell sessions.
+	initProgressCmd := cloudinit.InitProgressCmd()
+	c.AddRunCmd(initProgressCmd)
+
+	// If we're doing synchronous bootstrap or manual provisioning, then
+	// ConfigureBasic won't have been invoked; thus, the output log won't
+	// have been set. We don't want to show the log to the user, so simply
+	// append to the log file rather than teeing.
+	if stdout, _ := c.Output(cloudinit.OutAll); stdout == "" {
+		c.SetOutput(cloudinit.OutAll, ">> "+cloudInitOutputLog, "")
+		c.AddBootCmd(initProgressCmd)
+		c.AddBootCmd(cloudinit.LogProgressCmd("Logging to %s on remote host", cloudInitOutputLog))
+	}
+
+	// Bring packages up-to-date.
+	c.SetAptUpdate(true)
 	c.SetAptUpgrade(true)
-	c.SetAptUpdate(true)
-	c.SetOutput(cloudinit.OutAll, "| tee -a /var/log/cloud-init-output.log", "")
-	c.AddSSHAuthorizedKeys(cfg.AuthorizedKeys)
 
+	// juju requires git for managing charm directories.
 	c.AddPackage("git")
 
 	c.AddScripts(
@@ -149,17 +193,18 @@
 		fmt.Sprintf("mkdir -p %s", cfg.DataDir),
 		"mkdir -p /var/log/juju")
 
-	wgetCommand := "wget"
-	if cfg.DisableSSLHostnameVerification {
-		wgetCommand = "wget --no-check-certificate"
-	}
 	// Make a directory for the tools to live in, then fetch the
 	// tools and unarchive them into it.
 	var copyCmd string
 	if strings.HasPrefix(cfg.Tools.URL, fileSchemePrefix) {
 		copyCmd = fmt.Sprintf("cp %s $bin/tools.tar.gz", shquote(cfg.Tools.URL[len(fileSchemePrefix):]))
 	} else {
+		wgetCommand := "wget"
+		if cfg.DisableSSLHostnameVerification {
+			wgetCommand = "wget --no-check-certificate"
+		}
 		copyCmd = fmt.Sprintf("%s --no-verbose -O $bin/tools.tar.gz %s", wgetCommand, shquote(cfg.Tools.URL))
+		c.AddRunCmd(cloudinit.LogProgressCmd("Fetching tools: %s", copyCmd))
 	}
 	toolsJson, err := json.Marshal(cfg.Tools)
 	if err != nil {
@@ -231,6 +276,7 @@
 		if cons != "" {
 			cons = " --constraints " + shquote(cons)
 		}
+		c.AddRunCmd(cloudinit.LogProgressCmd("Bootstrapping Juju machine agent"))
 		c.AddScripts(
 			fmt.Sprintf("echo %s > %s", shquote(cfg.StateInfoURL), BootstrapStateURLFile),
 			// The bootstrapping is always run with debug on.
@@ -340,6 +386,7 @@
 	if err != nil {
 		return fmt.Errorf("cannot make cloud-init upstart script for the %s agent: %v", tag, err)
 	}
+	c.AddRunCmd(cloudinit.LogProgressCmd("Starting Juju machine agent (%s)", name))
 	c.AddScripts(cmds...)
 	return nil
 }
@@ -361,6 +408,7 @@
 	if err != nil {
 		return fmt.Errorf("cannot make cloud-init upstart script for the state database: %v", err)
 	}
+	c.AddRunCmd(cloudinit.LogProgressCmd("Starting MongoDB server (%s)", name))
 	c.AddScripts(cmds...)
 	return nil
 }

=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go	2013-11-26 12:24:48 +0000
+++ environs/cloudinit/cloudinit_test.go	2013-12-03 06:15:09 +0000
@@ -88,9 +88,11 @@
 		setEnvConfig: true,
 		expectScripts: `
 echo ENABLE_MONGODB="no" > /etc/default/mongodb
+test -e /proc/self/fd/9 \|\| exec 9>&2
 set -xe
 mkdir -p /var/lib/juju
 mkdir -p /var/log/juju
+echo 'Fetching tools.*
 bin='/var/lib/juju/tools/1\.2\.3-precise-amd64'
 mkdir -p \$bin
 wget --no-verbose -O \$bin/tools\.tar\.gz 'http://foo\.com/tools/releases/juju1\.2\.3-precise-amd64\.tgz'
@@ -114,6 +116,7 @@
 dd bs=1M count=1 if=/dev/zero of=/var/lib/juju/db/journal/prealloc\.0
 dd bs=1M count=1 if=/dev/zero of=/var/lib/juju/db/journal/prealloc\.1
 dd bs=1M count=1 if=/dev/zero of=/var/lib/juju/db/journal/prealloc\.2
+echo 'Starting MongoDB server \(juju-db\)'.*
 cat >> /etc/init/juju-db\.conf << 'EOF'\\ndescription "juju state database"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 65000 65000\\nlimit nproc 20000 20000\\n\\nexec /usr/bin/mongod --auth --dbpath=/var/lib/juju/db --sslOnNormalPorts --sslPEMKeyFile '/var/lib/juju/server\.pem' --sslPEMKeyPassword ignored --bind_ip 0\.0\.0\.0 --port 37017 --noprealloc --syslog --smallfiles\\nEOF\\n
 start juju-db
 mkdir -p '/var/lib/juju/agents/bootstrap'
@@ -121,10 +124,12 @@
 printf '%s\\n' '.*' > '/var/lib/juju/agents/bootstrap/format'
 install -m 600 /dev/null '/var/lib/juju/agents/bootstrap/agent\.conf'
 printf '%s\\n' '.*' > '/var/lib/juju/agents/bootstrap/agent\.conf'
+echo 'Bootstrapping Juju machine agent'.*
 echo 'some-url' > /tmp/provider-state-url
 /var/lib/juju/tools/1\.2\.3-precise-amd64/jujud bootstrap-state --data-dir '/var/lib/juju' --env-config '[^']*' --constraints 'mem=2048M' --debug
 rm -rf '/var/lib/juju/agents/bootstrap'
 ln -s 1\.2\.3-precise-amd64 '/var/lib/juju/tools/machine-0'
+echo 'Starting Juju machine agent \(jujud-machine-0\)'.*
 cat >> /etc/init/jujud-machine-0\.conf << 'EOF'\\ndescription "juju machine-0 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-0/jujud machine --data-dir '/var/lib/juju' --machine-id 0 --debug >> /var/log/juju/machine-0\.log 2>&1\\nEOF\\n
 start jujud-machine-0
 `,
@@ -193,9 +198,11 @@
 			SyslogPort: 514,
 		},
 		expectScripts: `
+test -e /proc/self/fd/9 \|\| exec 9>&2
 set -xe
 mkdir -p /var/lib/juju
 mkdir -p /var/log/juju
+echo 'Fetching tools.*
 bin='/var/lib/juju/tools/1\.2\.3-linux-amd64'
 mkdir -p \$bin
 wget --no-verbose -O \$bin/tools\.tar\.gz 'http://foo\.com/tools/releases/juju1\.2\.3-linux-amd64\.tgz'
@@ -213,6 +220,7 @@
 install -m 600 /dev/null '/var/lib/juju/agents/machine-99/agent\.conf'
 printf '%s\\n' '.*' > '/var/lib/juju/agents/machine-99/agent\.conf'
 ln -s 1\.2\.3-linux-amd64 '/var/lib/juju/tools/machine-99'
+echo 'Starting Juju machine agent \(jujud-machine-99\)'.*
 cat >> /etc/init/jujud-machine-99\.conf << 'EOF'\\ndescription "juju machine-99 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-99/jujud machine --data-dir '/var/lib/juju' --machine-id 99 --debug >> /var/log/juju/machine-99\.log 2>&1\\nEOF\\n
 start jujud-machine-99
 `,

=== modified file 'environs/cloudinit_test.go'
--- environs/cloudinit_test.go	2013-11-20 04:28:46 +0000
+++ environs/cloudinit_test.go	2013-12-03 06:15:09 +0000
@@ -139,7 +139,15 @@
 	c.Assert(err, gc.NotNil)
 }
 
-func (*CloudInitSuite) TestUserData(c *gc.C) {
+func (s *CloudInitSuite) TestUserData(c *gc.C) {
+	s.testUserData(c, false)
+}
+
+func (s *CloudInitSuite) TestStateServerUserData(c *gc.C) {
+	s.testUserData(c, true)
+}
+
+func (*CloudInitSuite) testUserData(c *gc.C, stateServer bool) {
 	testJujuHome := c.MkDir()
 	defer config.SetJujuHome(config.SetJujuHome(testJujuHome))
 	tools := &tools.Tools{
@@ -156,20 +164,25 @@
 		StateServerCert: []byte(testing.ServerCert),
 		StateServerKey:  []byte(testing.ServerKey),
 		StateInfo: &state.Info{
+			Addrs:    []string{"127.0.0.1:1234"},
 			Password: "pw1",
 			CACert:   []byte("CA CERT\n" + testing.CACert),
+			Tag:      "machine-10",
 		},
 		APIInfo: &api.Info{
+			Addrs:    []string{"127.0.0.1:1234"},
 			Password: "pw2",
 			CACert:   []byte("CA CERT\n" + testing.CACert),
+			Tag:      "machine-10",
 		},
 		DataDir:          environs.DataDir,
 		Config:           envConfig,
 		StatePort:        envConfig.StatePort(),
 		APIPort:          envConfig.APIPort(),
 		SyslogPort:       envConfig.SyslogPort(),
-		StateServer:      true,
+		StateServer:      stateServer,
 		AgentEnvironment: map[string]string{agent.ProviderType: "dummy"},
+		AuthorizedKeys:   "wheredidileavemykeys",
 	}
 	script1 := "script1"
 	script2 := "script2"
@@ -184,11 +197,32 @@
 	err = goyaml.Unmarshal(unzipped, &config)
 	c.Assert(err, gc.IsNil)
 
-	// Just check that the cloudinit config looks good.
-	c.Check(config["apt_upgrade"], gc.Equals, true)
 	// The scripts given to userData where added as the first
 	// commands to be run.
 	runCmd := config["runcmd"].([]interface{})
 	c.Check(runCmd[0], gc.Equals, script1)
 	c.Check(runCmd[1], gc.Equals, script2)
+
+	if stateServer {
+		// The cloudinit config should have nothing but the basics:
+		// SSH authorized keys, the additional runcmds, and log output.
+		//
+		// Note: the additional runcmds *do* belong here, at least
+		// for MAAS. MAAS needs to configure and then bounce the
+		// network interfaces, which would sever the SSH connection
+		// in the synchronous bootstrap phase.
+		c.Check(config, gc.DeepEquals, map[interface{}]interface{}{
+			"output": map[interface{}]interface{}{
+				"all": "| tee -a /var/log/cloud-init-output.log",
+			},
+			"runcmd":              []interface{}{"script1", "script2"},
+			"ssh_authorized_keys": []interface{}{"wheredidileavemykeys"},
+		})
+	} else {
+		// Just check that the cloudinit config looks good,
+		// and that there are more runcmds than the additional
+		// ones we passed into ComposeUserData.
+		c.Check(config["apt_upgrade"], gc.Equals, true)
+		c.Check(len(runCmd) > 2, jc.IsTrue)
+	}
 }

=== modified file 'environs/manual/bootstrap.go'
--- environs/manual/bootstrap.go	2013-11-18 05:41:36 +0000
+++ environs/manual/bootstrap.go	2013-12-03 06:15:09 +0000
@@ -134,5 +134,5 @@
 	for k, v := range agentEnv {
 		mcfg.AgentEnvironment[k] = v
 	}
-	return ProvisionMachineAgent(args.Host, mcfg)
+	return provisionMachineAgent(args.Host, mcfg)
 }

=== modified file 'environs/manual/detection.go'
--- environs/manual/detection.go	2013-10-09 06:54:15 +0000
+++ environs/manual/detection.go	2013-12-03 06:15:09 +0000
@@ -12,6 +12,7 @@
 
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/utils/ssh"
 )
 
 // checkProvisionedScript is the script to run on the remote machine
@@ -25,7 +26,7 @@
 // exist on the host machine.
 func checkProvisioned(sshHost string) (bool, error) {
 	logger.Infof("Checking if %s is already provisioned", sshHost)
-	cmd := sshCommand(sshHost, fmt.Sprintf("bash -c %s", utils.ShQuote(checkProvisionedScript)))
+	cmd := ssh.Command(sshHost, []string{"bash", "-c", utils.ShQuote(checkProvisionedScript)})
 	var stdout, stderr bytes.Buffer
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
@@ -52,7 +53,7 @@
 // The sshHost argument must be a hostname of the form [user@]host.
 func DetectSeriesAndHardwareCharacteristics(sshHost string) (hc instance.HardwareCharacteristics, series string, err error) {
 	logger.Infof("Detecting series and characteristics on %s", sshHost)
-	cmd := sshCommand(sshHost, "bash")
+	cmd := ssh.Command(sshHost, []string{"bash"})
 	cmd.Stdin = bytes.NewBufferString(detectionScript)
 	out, err := cmd.CombinedOutput()
 	if err != nil {

=== modified file 'environs/manual/provisioner.go'
--- environs/manual/provisioner.go	2013-11-26 14:06:20 +0000
+++ environs/manual/provisioner.go	2013-12-03 06:15:09 +0000
@@ -11,6 +11,8 @@
 
 	"launchpad.net/loggo"
 
+	coreCloudinit "launchpad.net/juju-core/cloudinit"
+	"launchpad.net/juju-core/cloudinit/sshinit"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/cloudinit"
@@ -89,7 +91,7 @@
 	}
 
 	// Finally, provision the machine agent.
-	err = ProvisionMachineAgent(args.Host, mcfg)
+	err = provisionMachineAgent(args.Host, mcfg)
 	if err != nil {
 		return machineId, err
 	}
@@ -202,3 +204,14 @@
 	}
 	return mcfg, nil
 }
+
+func provisionMachineAgent(host string, mcfg *cloudinit.MachineConfig) error {
+	cloudcfg := coreCloudinit.New()
+	if err := cloudinit.ConfigureJuju(mcfg, cloudcfg); err != nil {
+		return err
+	}
+	// Explicitly disabling apt_upgrade so as not to trample
+	// the target machine's existing configuration.
+	cloudcfg.SetAptUpgrade(false)
+	return sshinit.Configure(host, cloudcfg)
+}

=== modified file 'environs/sshstorage/storage.go'
--- environs/sshstorage/storage.go	2013-10-10 01:31:43 +0000
+++ environs/sshstorage/storage.go	2013-12-03 06:15:09 +0000
@@ -20,6 +20,7 @@
 
 	coreerrors "launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/utils/ssh"
 )
 
 // base64LineLength is the default line length for wrapping
@@ -44,12 +45,11 @@
 }
 
 var sshCommand = func(host string, tty bool, command string) *exec.Cmd {
-	sshArgs := []string{host}
+	var options []ssh.Option
 	if tty {
-		sshArgs = append(sshArgs, "-t")
+		options = append(options, ssh.AllocateTTY)
 	}
-	sshArgs = append(sshArgs, "--", command)
-	return exec.Command("ssh", sshArgs...)
+	return ssh.Command(host, []string{command}, options...)
 }
 
 type flockmode string

=== added file 'environs/testing/bootstrap.go'
--- environs/testing/bootstrap.go	1970-01-01 00:00:00 +0000
+++ environs/testing/bootstrap.go	2013-12-03 06:15:09 +0000
@@ -0,0 +1,26 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package testing
+
+import (
+	"launchpad.net/loggo"
+
+	"launchpad.net/juju-core/environs/cloudinit"
+	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/provider/common"
+	"launchpad.net/juju-core/testing/testbase"
+)
+
+var logger = loggo.GetLogger("juju.environs.testing")
+
+// DisableFinishBootstrap disables common.FinishBootstrap so that tests
+// do not attempt to SSH to non-existent machines. The result is a function
+// that restores finishBootstrap.
+func DisableFinishBootstrap() func() {
+	f := func(*common.BootstrapContext, instance.Instance, *cloudinit.MachineConfig) error {
+		logger.Warningf("provider/common.FinishBootstrap is disabled")
+		return nil
+	}
+	return testbase.PatchValue(&common.FinishBootstrap, f)
+}

=== modified file 'provider/common/bootstrap.go'
--- provider/common/bootstrap.go	2013-11-18 04:53:44 +0000
+++ provider/common/bootstrap.go	2013-12-03 06:15:09 +0000
@@ -5,12 +5,21 @@
 
 import (
 	"fmt"
+	"net"
+	"os"
+	"os/signal"
+	"sync"
+	"time"
 
 	"launchpad.net/loggo"
+	"launchpad.net/tomb"
 
+	coreCloudinit "launchpad.net/juju-core/cloudinit"
+	"launchpad.net/juju-core/cloudinit/sshinit"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/bootstrap"
+	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/instance"
 	coretools "launchpad.net/juju-core/tools"
 )
@@ -20,11 +29,20 @@
 // Bootstrap is a common implementation of the Bootstrap method defined on
 // environs.Environ; we strongly recommend that this implementation be used
 // when writing a new provider.
-func Bootstrap(env environs.Environ, cons constraints.Value) error {
+func Bootstrap(env environs.Environ, cons constraints.Value) (err error) {
 	// TODO make safe in the case of racing Bootstraps
 	// If two Bootstraps are called concurrently, there's
 	// no way to make sure that only one succeeds.
 
+	// TODO(axw) 2013-11-22 #1237736
+	// Modify environs/Environ Bootstrap method signature
+	// to take a new context structure, which contains
+	// Std{in,out,err}, and interrupt signal handling.
+	ctx := BootstrapContext{Stderr: os.Stderr}
+
+	var inst instance.Instance
+	defer func() { handleBootstrapError(err, &ctx, inst, env) }()
+
 	// Create an empty bootstrap state file so we can get its URL.
 	// It will be updated with the instance id and hardware characteristics
 	// after the bootstrap instance is started.
@@ -39,10 +57,13 @@
 		return err
 	}
 
+	fmt.Fprintln(ctx.Stderr, "Launching instance")
 	inst, hw, err := env.StartInstance(cons, selectedTools, machineConfig)
 	if err != nil {
 		return fmt.Errorf("cannot start bootstrap instance: %v", err)
 	}
+	fmt.Fprintf(ctx.Stderr, " - %s\n", inst.Id())
+
 	var characteristics []instance.HardwareCharacteristics
 	if hw != nil {
 		characteristics = []instance.HardwareCharacteristics{*hw}
@@ -54,14 +75,148 @@
 			Characteristics: characteristics,
 		})
 	if err != nil {
-		stoperr := env.StopInstances([]instance.Instance{inst})
-		if stoperr != nil {
-			// Failure upon failure.  Log it, but return the original error.
+		return fmt.Errorf("cannot save state: %v", err)
+	}
+	return FinishBootstrap(&ctx, inst, machineConfig)
+}
+
+// handelBootstrapError cleans up after a failed bootstrap.
+func handleBootstrapError(err error, ctx *BootstrapContext, inst instance.Instance, env environs.Environ) {
+	if err == nil {
+		return
+	}
+	ctx.SetInterruptHandler(func() {
+		fmt.Fprintln(ctx.Stderr, "Cleaning up failed bootstrap")
+	})
+	if inst != nil {
+		fmt.Fprintln(ctx.Stderr, "Stopping instance...")
+		if stoperr := env.StopInstances([]instance.Instance{inst}); stoperr != nil {
 			logger.Errorf("cannot stop failed bootstrap instance %q: %v", inst.Id(), stoperr)
-		}
-		return fmt.Errorf("cannot save state: %v", err)
-	}
-	return nil
+		} else {
+			// set to nil so we know we can safely delete the state file
+			inst = nil
+		}
+	}
+	// We only delete the bootstrap state file if either we didn't
+	// start an instance, or we managed to cleanly stop it.
+	if inst == nil {
+		if rmerr := bootstrap.DeleteStateFile(env.Storage()); rmerr != nil {
+			logger.Errorf("cannot delete bootstrap state file: %v", rmerr)
+		}
+	}
+	ctx.SetInterruptHandler(nil)
+}
+
+// FinishBootstrap completes the bootstrap process by connecting
+// to the instance via SSH and carrying out the cloud-config.
+//
+// Note: FinishBootstrap is exposed so it can be replaced for testing.
+var FinishBootstrap = func(ctx *BootstrapContext, inst instance.Instance, machineConfig *cloudinit.MachineConfig) error {
+	var t tomb.Tomb
+	ctx.SetInterruptHandler(func() { t.Killf("interrupted") })
+	dnsName, err := waitSSH(ctx, inst, &t)
+	if err != nil {
+		return err
+	}
+	// Bootstrap is synchronous, and will spawn a subprocess
+	// to complete the procedure. If the user hits Ctrl-C,
+	// SIGINT is sent to the foreground process attached to
+	// the terminal, which will be the ssh subprocess at that
+	// point.
+	ctx.SetInterruptHandler(func() {})
+	cloudcfg := coreCloudinit.New()
+	if err := cloudinit.ConfigureJuju(machineConfig, cloudcfg); err != nil {
+		return err
+	}
+	return sshinit.Configure("ubuntu@"+dnsName, cloudcfg)
+}
+
+// waitSSH waits for the instance to be assigned a DNS
+// entry, then waits until we can connect to it via SSH.
+func waitSSH(ctx *BootstrapContext, inst instance.Instance, t *tomb.Tomb) (dnsName string, err error) {
+	defer t.Done()
+
+	// Wait for a DNS name.
+	fmt.Fprint(ctx.Stderr, "Waiting for DNS name")
+	for {
+		fmt.Fprintf(ctx.Stderr, ".")
+		dnsName, err = inst.DNSName()
+		if err == nil {
+			break
+		} else if err != instance.ErrNoDNSName {
+			fmt.Fprintln(ctx.Stderr)
+			return "", t.Killf("getting DNS name: %v", err)
+		}
+		select {
+		case <-time.After(1 * time.Second):
+		case <-t.Dying():
+			fmt.Fprintln(ctx.Stderr)
+			return "", t.Err()
+		}
+	}
+	fmt.Fprintf(ctx.Stderr, "\n - %v\n", dnsName)
+
+	// Wait until we can open a connection to port 22.
+	fmt.Fprintf(ctx.Stderr, "Attempting to connect to %s:22", dnsName)
+	for {
+		fmt.Fprintf(ctx.Stderr, ".")
+		conn, err := net.DialTimeout("tcp", dnsName+":22", 5*time.Second)
+		if err == nil {
+			conn.Close()
+			fmt.Fprintln(ctx.Stderr)
+			return dnsName, nil
+		} else {
+			logger.Debugf("connection failed: %v", err)
+		}
+		select {
+		case <-time.After(5 * time.Second):
+		case <-t.Dying():
+			return "", t.Err()
+		}
+	}
+}
+
+// TODO(axw) move this to environs; see
+// comment near the top of common.Bootstrap.
+type BootstrapContext struct {
+	once        sync.Once
+	handlerchan chan func()
+
+	Stderr *os.File
+}
+
+func (ctx *BootstrapContext) SetInterruptHandler(f func()) {
+	ctx.once.Do(ctx.initHandler)
+	ctx.handlerchan <- f
+}
+
+func (ctx *BootstrapContext) initHandler() {
+	ctx.handlerchan = make(chan func())
+	go ctx.handleInterrupt()
+}
+
+func (ctx *BootstrapContext) handleInterrupt() {
+	signalchan := make(chan os.Signal, 1)
+	var s chan os.Signal
+	var handler func()
+	for {
+		select {
+		case handler = <-ctx.handlerchan:
+			if handler == nil {
+				if s != nil {
+					signal.Stop(signalchan)
+					s = nil
+				}
+			} else {
+				if s == nil {
+					s = signalchan
+					signal.Notify(signalchan, os.Interrupt)
+				}
+			}
+		case <-s:
+			handler()
+		}
+	}
 }
 
 // EnsureBootstrapTools finds tools, syncing with an external tools source as

=== modified file 'provider/common/bootstrap_test.go'
--- provider/common/bootstrap_test.go	2013-11-18 04:53:44 +0000
+++ provider/common/bootstrap_test.go	2013-12-03 06:15:09 +0000
@@ -197,6 +197,9 @@
 		return minimalConfig(c)
 	}
 
+	restore := envtesting.DisableFinishBootstrap()
+	defer restore()
+
 	env := &mockEnviron{
 		storage:       stor,
 		startInstance: startInstance,

=== modified file 'provider/ec2/local_test.go'
--- provider/ec2/local_test.go	2013-11-18 05:41:36 +0000
+++ provider/ec2/local_test.go	2013-12-03 06:15:09 +0000
@@ -222,26 +222,28 @@
 
 	// check that the user data is configured to start zookeeper
 	// and the machine and provisioning agents.
+	// check that the user data is configured to only configure
+	// authorized SSH keys and set the log output; everything
+	// else happens after the machine is brought up.
 	inst := t.srv.ec2srv.Instance(string(insts[0].Id()))
 	c.Assert(inst, gc.NotNil)
 	bootstrapDNS, err := insts[0].DNSName()
 	c.Assert(err, gc.IsNil)
 	c.Assert(bootstrapDNS, gc.Not(gc.Equals), "")
-
 	userData, err := utils.Gunzip(inst.UserData)
 	c.Assert(err, gc.IsNil)
 	c.Logf("first instance: UserData: %q", userData)
 	var userDataMap map[interface{}]interface{}
 	err = goyaml.Unmarshal(userData, &userDataMap)
 	c.Assert(err, gc.IsNil)
-	CheckPackage(c, userDataMap, "git", true)
-	CheckScripts(c, userDataMap, "jujud bootstrap-state", true)
-	// TODO check for provisioning agent
-	// TODO check for machine agent
+	c.Assert(userDataMap, gc.DeepEquals, map[interface{}]interface{}{
+		"output": map[interface{}]interface{}{
+			"all": "| tee -a /var/log/cloud-init-output.log",
+		},
+		"ssh_authorized_keys": []interface{}{"my-keys"},
+	})
 
-	// check that a new instance will be started without
-	// zookeeper, with a machine agent, and without a
-	// provisioning agent.
+	// check that a new instance will be started with a machine agent
 	inst1, hc := testing.AssertStartInstance(c, env, "1")
 	c.Check(*hc.Arch, gc.Equals, "amd64")
 	c.Check(*hc.Mem, gc.Equals, uint64(1740))
@@ -255,9 +257,11 @@
 	userDataMap = nil
 	err = goyaml.Unmarshal(userData, &userDataMap)
 	c.Assert(err, gc.IsNil)
-	CheckPackage(c, userDataMap, "zookeeperd", false)
+	CheckPackage(c, userDataMap, "git", true)
+	CheckPackage(c, userDataMap, "mongodb-server", false)
+	CheckScripts(c, userDataMap, "jujud bootstrap-state", false)
+	CheckScripts(c, userDataMap, "/var/lib/juju/agents/machine-1/agent.conf", true)
 	// TODO check for provisioning agent
-	// TODO check for machine agent
 
 	err = env.Destroy()
 	c.Assert(err, gc.IsNil)
@@ -408,7 +412,9 @@
 	ec2.UseTestInstanceTypeData(ec2.TestInstanceTypeCosts)
 	ec2.UseTestRegionData(ec2.TestRegions)
 	restoreTimeouts := envtesting.PatchAttemptStrategies(ec2.ShortAttempt, ec2.StorageAttempt)
+	restoreFinishBootstrap := envtesting.DisableFinishBootstrap()
 	return func() {
+		restoreFinishBootstrap()
 		restoreTimeouts()
 		ec2.UseTestImageData(nil)
 		ec2.UseTestInstanceTypeData(nil)

=== modified file 'provider/maas/maas_test.go'
--- provider/maas/maas_test.go	2013-11-26 12:24:48 +0000
+++ provider/maas/maas_test.go	2013-12-03 06:15:09 +0000
@@ -33,6 +33,8 @@
 	s.LoggingSuite.SetUpSuite(c)
 	TestMAASObject := gomaasapi.NewTestMAAS("1.0")
 	s.testMAASObject = TestMAASObject
+	restoreFinishBootstrap := envtesting.DisableFinishBootstrap()
+	s.AddSuiteCleanup(func(*gc.C) { restoreFinishBootstrap() })
 }
 
 func (s *providerSuite) SetUpTest(c *gc.C) {

=== modified file 'provider/openstack/local_test.go'
--- provider/openstack/local_test.go	2013-11-18 05:41:36 +0000
+++ provider/openstack/local_test.go	2013-12-03 06:15:09 +0000
@@ -167,6 +167,8 @@
 	s.srv.start(c, s.cred)
 	s.LiveTests.SetUpSuite(c)
 	openstack.UseTestImageData(openstack.ImageMetadataStorage(s.Env), s.cred)
+	restoreFinishBootstrap := envtesting.DisableFinishBootstrap()
+	s.AddSuiteCleanup(func(*gc.C) { restoreFinishBootstrap() })
 }
 
 func (s *localLiveSuite) TearDownSuite(c *gc.C) {
@@ -203,6 +205,8 @@
 func (s *localServerSuite) SetUpSuite(c *gc.C) {
 	s.LoggingSuite.SetUpSuite(c)
 	s.Tests.SetUpSuite(c)
+	restoreFinishBootstrap := envtesting.DisableFinishBootstrap()
+	s.AddSuiteCleanup(func(*gc.C) { restoreFinishBootstrap() })
 	c.Logf("Running local tests")
 }
 
@@ -740,6 +744,9 @@
 }
 
 func (s *localHTTPSServerSuite) TestCanBootstrap(c *gc.C) {
+	restoreFinishBootstrap := envtesting.DisableFinishBootstrap()
+	defer restoreFinishBootstrap()
+
 	// For testing, we create a storage instance to which is uploaded tools and image metadata.
 	metadataStorage := openstack.MetadataStorage(s.env)
 	url, err := metadataStorage.URL("")

=== added directory 'utils/ssh'
=== renamed file 'environs/manual/ssh.go' => 'utils/ssh/ssh.go'
--- environs/manual/ssh.go	2013-09-12 02:04:05 +0000
+++ utils/ssh/ssh.go	2013-12-03 06:15:09 +0000
@@ -1,25 +1,45 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package manual
+package ssh
 
 import (
 	"os/exec"
 )
 
-type sshOption []string
-
-var allocateTTY sshOption = []string{"-t"}
-
-// TODO(axw) 2013-09-12 bug #1224230
-// Move this to a common package for use in cmd/juju, and others.
-var commonSSHOptions = []string{"-o", "StrictHostKeyChecking no"}
-
-func sshCommand(host string, command string, options ...sshOption) *exec.Cmd {
-	args := append([]string{}, commonSSHOptions...)
+type Option []string
+
+var (
+	commonOptions Option = []string{"-o", "StrictHostKeyChecking no"}
+
+	// AllocateTTY forces pseudo-TTY allocation, which is required,
+	// for example, for sudo password prompts on the target host.
+	AllocateTTY Option = []string{"-t"}
+
+	// NoPasswordAuthentication disallows password-based authentication.
+	NoPasswordAuthentication Option = []string{"-o", "PasswordAuthentication no"}
+)
+
+// Command initialises an os/exec.Cmd to execute the native ssh program.
+func Command(host string, command []string, options ...Option) *exec.Cmd {
+	args := append([]string{}, commonOptions...)
 	for _, option := range options {
 		args = append(args, option...)
 	}
-	args = append(args, host, "--", command)
+	args = append(args, host)
+	if len(command) > 0 {
+		args = append(args, "--")
+		args = append(args, command...)
+	}
 	return exec.Command("ssh", args...)
 }
+
+// ScpCommand initialises an os/exec.Cmd to execute the native scp program.
+func ScpCommand(source, destination string, options ...Option) *exec.Cmd {
+	args := append([]string{}, commonOptions...)
+	for _, option := range options {
+		args = append(args, option...)
+	}
+	args = append(args, source, destination)
+	return exec.Command("scp", args...)
+}