cloudinit/sshinit: wait for cloud-init to finish

cloudinit/sshinit: wait for cloud-init to finish

We must wait for cloud-init to finish doing its
thing, as it will make various modifications to
the base OS. One of the modifications is to set
the cloud-local apt mirror, which we'll make use
of during the synchronous bootstrap phase when
apt updating/upgrading.

Fixes #1259180

https://code.launchpad.net/~axwalk/juju-core/lp1259180-sshinit-wait-cloudinit/+merge/198346

(do not edit description out of merge proposal)

[axw, 2013-12-10 08:39:20]

Please take a look.

[dimitern, 2013-12-10 10:27:57]

LGTM with some minor suggestions.

https://codereview.appspot.com/39940043/diff/1/cloudinit/sshinit/configure.go
File cloudinit/sshinit/configure.go (right):

https://codereview.appspot.com/39940043/diff/1/cloudinit/sshinit/configure.go...
cloudinit/sshinit/configure.go:81: if pkill -0 cloud-init; then
Clever trick, if undocumented, but actually works - good to know!

https://codereview.appspot.com/39940043/diff/1/cloudinit/sshinit/configure.go...
cloudinit/sshinit/configure.go:109: const aptget = "apt-get --option
Dpkg::Options::=--force-confold --assume-yes "
Please add a comment describing the (fairly cryptic) apt-get options. Something
like:

// Force apt-get not to ask for confirmation before installing packages and
overwriting config files.

(if I deciphered it correctly)

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

https://codereview.appspot.com/39940043/diff/1/cloudinit/sshinit/configure_te...
cloudinit/sshinit/configure_test.go:117:
"(.|\n)*"+regexp.QuoteMeta(aptget)+"install.*python-software-properties(.|\n)*",
FWIW this reads better as

var aptgetPrefix = "(.|\n)*" + regexp.QuoteMeta(aptget)

somewhere in the beginning and reusing it in all places below.

[axw, 2013-12-10 14:48:02]

Please take a look.

https://codereview.appspot.com/39940043/diff/1/cloudinit/sshinit/configure.go
File cloudinit/sshinit/configure.go (right):

https://codereview.appspot.com/39940043/diff/1/cloudinit/sshinit/configure.go...
cloudinit/sshinit/configure.go:81: if pkill -0 cloud-init; then
On 2013/12/10 10:27:57, dimitern wrote:
> Clever trick, if undocumented, but actually works - good to know!

:) It is documented, but in man 2 kill.

https://codereview.appspot.com/39940043/diff/1/cloudinit/sshinit/configure.go...
cloudinit/sshinit/configure.go:109: const aptget = "apt-get --option
Dpkg::Options::=--force-confold --assume-yes "
On 2013/12/10 10:27:57, dimitern wrote:
> Please add a comment describing the (fairly cryptic) apt-get options.
Something
> like:
> 
> // Force apt-get not to ask for confirmation before installing packages and
> overwriting config files.
> 
> (if I deciphered it correctly)

Done.

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

https://codereview.appspot.com/39940043/diff/1/cloudinit/sshinit/configure_te...
cloudinit/sshinit/configure_test.go:117:
"(.|\n)*"+regexp.QuoteMeta(aptget)+"install.*python-software-properties(.|\n)*",
On 2013/12/10 10:27:57, dimitern wrote:
> FWIW this reads better as
> 
> var aptgetPrefix = "(.|\n)*" + regexp.QuoteMeta(aptget)
> 
> somewhere in the beginning and reusing it in all places below.

Done.

[dave_cheney.net, 2013-12-11 01:02:11]

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

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:38: cmd.Stdin = os.Stdin
why pass stdin here ? it's not connected to anything ? is it needed for sudo ?

[axw, 2013-12-11 01:09:39]

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

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:38: cmd.Stdin = os.Stdin
On 2013/12/11 01:02:11, dfc wrote:
> why pass stdin here ? it's not connected to anything ? is it needed for sudo ?

Yes, it's required for the remote sudo.

[rog, 2013-12-11 12:34:48]

Looks reasonable, but one reservation outlined below.

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

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:81: if pkill -0 cloud-init; then
This kind of logic makes me uncomfortable. It builds in a deep knowledge of
what's going on on the machine (that cloud-init is implemented by a single
process named "cloud-init" for example) that we don't really need.

I'd prefer to see something that was triggered more directly by logic that we
control. For example we could change our cloud-init script to create a file
which we look for; or we could even tail -f /var/log/cloud-init-output.log
looking for a "cloud-init complete" line that we print (if we did that, we
wouldn't need to poll)

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:113: const aptget = "apt-get --option
Dpkg::Options::=--force-confold --assume-yes "
As an alternative to this, you could do:

apt-get() {
    command apt-get -option Dpkg::Options::=--force-confold --assume-yes "$@"
}

and just use apt-get as usual in all the commands.
It would make the resulting script smaller and easier to read.

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:149: if len(cmds) > 0 {
Why not just do this always, at the start of this function?

[axw, 2013-12-11 13:15:34]

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

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:81: if pkill -0 cloud-init; then
On 2013/12/11 12:34:48, rog wrote:
> This kind of logic makes me uncomfortable. It builds in a deep knowledge of
> what's going on on the machine (that cloud-init is implemented by a single
> process named "cloud-init" for example) that we don't really need.
> 
> I'd prefer to see something that was triggered more directly by logic that we
> control. For example we could change our cloud-init script to create a file
> which we look for; or we could even tail -f /var/log/cloud-init-output.log
> looking for a "cloud-init complete" line that we print (if we did that, we
> wouldn't need to poll)

I would prefer something we control too (I did consider this), but there's a
problem with relying on cloud-init: we don't always use cloud-init (manual
bootstrap). The approach taken works in both cases.

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:113: const aptget = "apt-get --option
Dpkg::Options::=--force-confold --assume-yes "
On 2013/12/11 12:34:48, rog wrote:
> As an alternative to this, you could do:
> 
> apt-get() {
>     command apt-get -option Dpkg::Options::=--force-confold --assume-yes "$@"
> }
> 
> and just use apt-get as usual in all the commands.
> It would make the resulting script smaller and easier to read.

This CL is merged, but I'll bear it in mind next time I'm making changes here.

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:149: if len(cmds) > 0 {
On 2013/12/11 12:34:48, rog wrote:
> Why not just do this always, at the start of this function?

I didn't add it to the script because it doesn't need to be done. Why do it
always?

[rog, 2013-12-11 13:35:32]

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

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:81: if pkill -0 cloud-init; then
On 2013/12/11 13:15:34, axw wrote:
> On 2013/12/11 12:34:48, rog wrote:
> > This kind of logic makes me uncomfortable. It builds in a deep knowledge of
> > what's going on on the machine (that cloud-init is implemented by a single
> > process named "cloud-init" for example) that we don't really need.
> > 
> > I'd prefer to see something that was triggered more directly by logic that
we
> > control. For example we could change our cloud-init script to create a file
> > which we look for; or we could even tail -f /var/log/cloud-init-output.log
> > looking for a "cloud-init complete" line that we print (if we did that, we
> > wouldn't need to poll)
> 
> I would prefer something we control too (I did consider this), but there's a
> problem with relying on cloud-init: we don't always use cloud-init (manual
> bootstrap). The approach taken works in both cases.

It won't work if someone's manual bootstrapped onto a machine where someone
happens to be running a binary named "cloud-init".

Since we can't rely on cloud-init-output, perhaps looking for a file might be
better.

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:149: if len(cmds) > 0 {
On 2013/12/11 13:15:34, axw wrote:
> On 2013/12/11 12:34:48, rog wrote:
> > Why not just do this always, at the start of this function?
> 
> I didn't add it to the script because it doesn't need to be done. Why do it
> always?

simpler code, less garbage, but nothing particularly important.

[axw, 2013-12-11 14:10:43]

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

https://codereview.appspot.com/39940043/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:81: if pkill -0 cloud-init; then
On 2013/12/11 13:35:32, rog wrote:
> On 2013/12/11 13:15:34, axw wrote:
> > On 2013/12/11 12:34:48, rog wrote:
> > > This kind of logic makes me uncomfortable. It builds in a deep knowledge
of
> > > what's going on on the machine (that cloud-init is implemented by a single
> > > process named "cloud-init" for example) that we don't really need.
> > > 
> > > I'd prefer to see something that was triggered more directly by logic that
> we
> > > control. For example we could change our cloud-init script to create a
file
> > > which we look for; or we could even tail -f /var/log/cloud-init-output.log
> > > looking for a "cloud-init complete" line that we print (if we did that, we
> > > wouldn't need to poll)
> > 
> > I would prefer something we control too (I did consider this), but there's a
> > problem with relying on cloud-init: we don't always use cloud-init (manual
> > bootstrap). The approach taken works in both cases.
> 
> It won't work if someone's manual bootstrapped onto a machine where someone
> happens to be running a binary named "cloud-init".

True, that would stall the bootstrap. My answer to that at the moment is "don't
do that". I understand that we can do better, though, so I'm opening a tech debt
bug.

https://bugs.launchpad.net/juju-core/+bug/1259942

> Since we can't rely on cloud-init-output, perhaps looking for a file might be
> better.

Agreed. It's doable by adding an additional first "bootcmd" to the config passed
to sshinit by synchronous bootstrap that waits for a file created by a final
"runcmd" executed by cloud-init.