environs/cloudinit: fix raring issue

environs/cloudinit: fix raring issue

This works around LP bug #1103881, so cloudinit will
succeed on raring, rather than failing to execute
any scripts. The problem seems to be upgrading upstart
causes unexpected failures.

This has been tested live on ec2/eu-west-1 as well.

https://code.launchpad.net/~dimitern/juju-core/036-cloudinit-fix-for-raring/+merge/159852

(do not edit description out of merge proposal)

[dimitern, 2013-04-19 16:03:54]

Please take a look.

[fwereade, 2013-04-19 16:27:48]

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

https://codereview.appspot.com/8648047/diff/1/environs/cloudinit/cloudinit.go...
environs/cloudinit/cloudinit.go:189: }
I'd quite like to put the upgrade stuff together. It doesn't matter when we do
SetAptUpdate or SetOutput or whatever, so maybe just put those up at the top as
well.

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

https://codereview.appspot.com/8648047/diff/1/environs/cloudinit/cloudinit_te...
environs/cloudinit/cloudinit_test.go:252: // TODO(dimitern) raring does apt-get
upgrade differently, due to LP bug #1103881
Please just suffer the vast copy/paste necessary to test both the raring and
non-raring paths ;p.

[fwereade, 2013-04-19 16:30:37]

ok, sorry about the bad testing advice. Put the apt stuff up with the upgrade
script and LGTM

[dimitern, 2013-04-19 16:55:56]

Please take a look.

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

https://codereview.appspot.com/8648047/diff/1/environs/cloudinit/cloudinit.go...
environs/cloudinit/cloudinit.go:189: }
On 2013/04/19 16:27:48, fwereade wrote:
> I'd quite like to put the upgrade stuff together. It doesn't matter when we do
> SetAptUpdate or SetOutput or whatever, so maybe just put those up at the top
as
> well.

Done.

[rog, 2013-04-19 17:05:44]

LGTM

[dimitern, 2013-04-19 17:09:28]

*** Submitted:

environs/cloudinit: fix raring issue

This works around LP bug #1103881, so cloudinit will
succeed on raring, rather than failing to execute
any scripts. The problem seems to be upgrading upstart
causes unexpected failures.

This has been tested live on ec2/eu-west-1 as well.

R=fwereade, rog
CC=
https://codereview.appspot.com/8648047

[gz, 2013-04-22 13:53:50]

Did we get smoser to have a look at this as well? I take it the purpose is just
to move the upgrade later, that's not completely clear from the code. We've also
lost a few params from the builtin cloudinit call, (--quiet is just a
convienience, but I'm not sure about the option and the envvar), perhaps they
should be brought over as well.

[smoser, 2013-04-22 18:13:20]

The suggested change/workaround of:
 | #cloud-config
 | apt_upgrade: True
to:
 | #cloud-config
 | runcmd:
 |  ...
 |  - [apt-get, upgrade, -y]

has at least the following concerns:
 a.) I'm not sure just by looking at the diff if 'apt-get update' is ever
invoked prior to the apt-get upgrade.  If it is not, then that is effectively a
no-op.
 b.) For entirely non-interactive apt-get, you need to specify
     1.) --option=Dpkg::Options::=--force-confold
     2.) you need 'DEBIAN_FRONTEND=noninteractive' in the environment
 c.) I unfortunately do not have an understanding of the problem, so I can't say
with certainty that this will fix it.  My testing indicates that 'apt-get
upgrade' after a "boot finished" is not problematic to upstart, but the general
philosophy of upstart is that there is no such thing as "boot finished".
 d.) you're possibly unintentionally changing the order of other things in boot.

Per discussion in #ubuntu-devel, and advice from Ubuntu Foundations team, it
seems the right thing to do there is to *not* work around in juju.

Your potential for failure will be reduced dramatically as soon as raring
releases, as an upgrade to upstart or one of its dependencies will have to
follow SRU criteria, and the fix for this bug is supposed to be "measured in
days".

[gz, 2013-04-22 21:06:29]

As requested, I've proposed a change backing out this one:

<https://codereview.appspot.com/8913044/>