Fixed bug #1240667: pin priority for cloud-tools

Fixed bug #1240667: pin priority for cloud-tools

This introduces several new calls in cloudinit,
which deal with apt sources, their preferences
(/etc/apt/preferences.d/) and packages that we
install from there.

Basically, when deploying a precise machine that
needs to install mongodb-server package, we add
the cloud-tools pocket, but by default this will
add it as an apt source with a higher priority
than the main archive. As a consequence, charms
that try to install packages from main, which
are also in the cloud-tools pocket get the latter,
rather than the former (i.e. the described problem
with python-django's version 1.5 in cloud-tools vs.
1.14 in main, which breaks openstack-dashboard charm).

Now, when adding the cloud-tools archive we also
change its apt preferences, so that its priority
as an apt source is lower than main, which means
when installing mongodb-server during cloudinit
we need to explicitly specity --target-release
'precise-updates/cloud-tools' to pick the version
from cloud-tools.

Live tested on EC2 and works as expected.

https://code.launchpad.net/~dimitern/juju-core/290-lp-1240667-cloud-tools-priority/+merge/205816

(do not edit description out of merge proposal)

[dimitern, 2014-02-11 16:50:30]

Please take a look.

[gz, 2014-02-11 17:05:01]

LGTM if we get sign-off from smoser.

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go#newcode134
cloudinit/options.go:134: cfg.AddPackage(fmt.Sprintf("--target-release '%s'
'%s'", targetRelease, packageName))
This is the only dodgy bit of the operation, I'm not sure if we should really
rely on cloud-init passing through flags on the packages key like this, but
given our tie to a specific older cloud-init on precise, as it happens to work
we can probably get away with it. Let's check with Scott.

[rog, 2014-02-11 17:31:54]

LGTM modulo mgz's concerns and some suggestions below.

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go#newcode17
cloudinit/options.go:17: // AptPreferencesTemplate defines the format used to
create an
I don't really like using printf format strings as poor-man's
templates, particularly when the data is already in a struct,
and we have text/template.


I'd do something like:

var aptPreferences = template.Must(template.New("").Parse(
`Explanation: {{.Explanation}}
Package: {{.Package}}
Pin: {{.Pin}}
Pin-Priority: {{.PinPriority}}
`))

Then below, you can do:

if prefs != nil {
    var buf bytes.Buffer
    err := aptPreferencesTemplate.Execute(&buf, prefs)
    if err != nil {
        panic(err)
    }
    cfg.AddFile(prefs.Path, buf.String(), 0644)
}


then the template is easier to read and easier to change
in the future.

Also, rather than exporting the template, you could
define a method on AptPreferences to return the
file contents:

e.g.

// FileContents returns file contents suitable
// for setting up the given apt preferences.
func (prefs *AptPreferences) FileContents() string

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go#newcode134
cloudinit/options.go:134: cfg.AddPackage(fmt.Sprintf("--target-release '%s'
'%s'", targetRelease, packageName))
+1 to mgz's remarks.
Also:

cfg.AddPackage(fmt.Sprintf("--target-release %s %s",  
utils.ShQuote(targetRelease), utils.ShQuote(packageName)))

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

https://codereview.appspot.com/61410051/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:213: c.AddFile(noncefile, cfg.MachineNonce,
0644)
+1 to this drive-by, assuming it's tested.

[dimitern, 2014-02-11 20:02:50]

Please take a look.

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go#newcode17
cloudinit/options.go:17: // AptPreferencesTemplate defines the format used to
create an
On 2014/02/11 17:31:54, rog wrote:
> I don't really like using printf format strings as poor-man's
> templates, particularly when the data is already in a struct,
> and we have text/template.
> 
> 
> I'd do something like:
> 
> var aptPreferences = template.Must(template.New("").Parse(
> `Explanation: {{.Explanation}}
> Package: {{.Package}}
> Pin: {{.Pin}}
> Pin-Priority: {{.PinPriority}}
> `))
> 
> Then below, you can do:
> 
> if prefs != nil {
>     var buf bytes.Buffer
>     err := aptPreferencesTemplate.Execute(&buf, prefs)
>     if err != nil {
>         panic(err)
>     }
>     cfg.AddFile(prefs.Path, buf.String(), 0644)
> }
> 
> 
> then the template is easier to read and easier to change
> in the future.
> 
> Also, rather than exporting the template, you could
> define a method on AptPreferences to return the
> file contents:
> 
> e.g.
> 
> // FileContents returns file contents suitable
> // for setting up the given apt preferences.
> func (prefs *AptPreferences) FileContents() string

Good point, done!

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go#newcode134
cloudinit/options.go:134: cfg.AddPackage(fmt.Sprintf("--target-release '%s'
'%s'", targetRelease, packageName))
On 2014/02/11 17:05:01, gz wrote:
> This is the only dodgy bit of the operation, I'm not sure if we should really
> rely on cloud-init passing through flags on the packages key like this, but
> given our tie to a specific older cloud-init on precise, as it happens to work
> we can probably get away with it. Let's check with Scott.

As discussed on IRC with smoser, this is fine and I tested it to do the right
thing. It's much simpler than passing an entire certificate file with rigid
formatting through cloudinit. I added tests for all newly generated code.

https://codereview.appspot.com/61410051/diff/1/cloudinit/options.go#newcode134
cloudinit/options.go:134: cfg.AddPackage(fmt.Sprintf("--target-release '%s'
'%s'", targetRelease, packageName))
On 2014/02/11 17:31:54, rog wrote:
> +1 to mgz's remarks.
> Also:
> 
> cfg.AddPackage(fmt.Sprintf("--target-release %s %s",  
> utils.ShQuote(targetRelease), utils.ShQuote(packageName)))

I don't want to shquote these here, because they weren't escaped before and they
get escaped at rendering time, somewhat differently in cloudinit and sshinit,
after doing some checks.

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

https://codereview.appspot.com/61410051/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:213: c.AddFile(noncefile, cfg.MachineNonce,
0644)
On 2014/02/11 17:31:54, rog wrote:
> +1 to this drive-by, assuming it's tested.

Yep, it is - works fine.

[axw, 2014-02-12 01:25:41]

https://codereview.appspot.com/61410051/diff/20001/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/61410051/diff/20001/cloudinit/options.go#newco...
cloudinit/options.go:91: cfg.AddFile(prefs.Path, prefs.FileContents(), 0644)
Did you live test both add-machine and bootstrap?

I don't see how this could have worked as expected in a live test of add-machine
(which uses cloud-init all the way, as opposed to cloud-init + "sshinit").
AddFile is implemented in terms of runcmd; runcmd runs after packages are
installed.

I'm really not keen on this approach at all. I think environs/cloudinit should
be modified to add a bootcmd in ConfigureBasic. Bootcmds are run before packages
are installed.

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

https://codereview.appspot.com/61410051/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:144: if src.Prefs != nil {
Is there a good reason to duplicate this logic here and in cloudinit/options.go?
Anything that's not core cloud-init functionality should really belong in
environs/cloudinit.

[dimitern, 2014-02-12 08:49:36]

https://codereview.appspot.com/61410051/diff/20001/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/61410051/diff/20001/cloudinit/options.go#newco...
cloudinit/options.go:91: cfg.AddFile(prefs.Path, prefs.FileContents(), 0644)
On 2014/02/12 01:25:42, axw wrote:
> Did you live test both add-machine and bootstrap?
> 
> I don't see how this could have worked as expected in a live test of
add-machine
> (which uses cloud-init all the way, as opposed to cloud-init + "sshinit").
> AddFile is implemented in terms of runcmd; runcmd runs after packages are
> installed.
> 
> I'm really not keen on this approach at all. I think environs/cloudinit should
> be modified to add a bootcmd in ConfigureBasic. Bootcmds are run before
packages
> are installed.

It works all the same - the repository is added and with it the prefs file gets
created. Just tested it carefully again with add-machine and a fresh precise VM.

About the approach, since it fixes the bug, which I was aiming to do, it's
adequate, if not the best one, I agree. If you feel strongly that it should be
changed, please propose a follow-up?

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

https://codereview.appspot.com/61410051/diff/20001/cloudinit/sshinit/configur...
cloudinit/sshinit/configure.go:144: if src.Prefs != nil {
On 2014/02/12 01:25:42, axw wrote:
> Is there a good reason to duplicate this logic here and in
cloudinit/options.go?
> Anything that's not core cloud-init functionality should really belong in
> environs/cloudinit.

This is due to the fact that runcmds are run after everything else, as you
mentioned. So I had to pull it out here in order for it to work.