lxc: Fixed target-release issue precise/maas

lxc: Fixed target-release issue precise/maas

Mitigates issue lp:1289316

https://code.launchpad.net/~wwitzel3/juju-core/lp-1289316-lxc-maas-precise/+merge/209974

(do not edit description out of merge proposal)

[wwitzel, 2014-03-07 17:44:23]

Please take a look.

[dimitern, 2014-03-07 17:50:41]

LGTM with a few minor suggestions, assuming you tested this live on EC2 at
least.

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

https://codereview.appspot.com/72270044/diff/1/cloudinit/options.go#newcode110
cloudinit/options.go:110: // argument. If targetRelease is an empty string, no
--target-release.
s/no --target-release./no --target-release is passed to apt-get install./ ?

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

https://codereview.appspot.com/72270044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:214: if cfg.Tools.Version.Series == "precise" {
I'd add a comment here like this:

// Only supported LTS releases have cloud-tools pocket support.
// When Trusty is released, it will need a target release as well.

[wwitzel, 2014-03-09 18:25:41]

Please take a look.

[wwitzel, 2014-03-09 18:34:58]

Please take a look.

https://codereview.appspot.com/72270044/diff/20001/environs/cloudinit/cloudin...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/72270044/diff/20001/environs/cloudinit/cloudin...
environs/cloudinit/cloudinit.go:218: targetRelease := cfg.TargetRelease()
Don't like there here. Going to move the assignment down to where I use it.

[dimitern, 2014-03-09 18:40:50]

https://codereview.appspot.com/72270044/diff/40001/environs/cloudinit/cloudin...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/72270044/diff/40001/environs/cloudinit/cloudin...
environs/cloudinit/cloudinit.go:403: func (cfg *MachineConfig) TargetRelease()
string {
doc comment please:
// TargetRelease returns ...

[wwitzel, 2014-03-09 19:02:26]

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

https://codereview.appspot.com/72270044/diff/1/cloudinit/options.go#newcode110
cloudinit/options.go:110: // argument. If targetRelease is an empty string, no
--target-release.
On 2014/03/07 17:50:42, dimitern wrote:
> s/no --target-release./no --target-release is passed to apt-get install./ ?

Done.

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

https://codereview.appspot.com/72270044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:214: if cfg.Tools.Version.Series == "precise" {
On 2014/03/07 17:50:42, dimitern wrote:
> I'd add a comment here like this:
> 
> // Only supported LTS releases have cloud-tools pocket support.
> // When Trusty is released, it will need a target release as well.

Done.

https://codereview.appspot.com/72270044/diff/40001/environs/cloudinit/cloudin...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/72270044/diff/40001/environs/cloudinit/cloudin...
environs/cloudinit/cloudinit.go:403: func (cfg *MachineConfig) TargetRelease()
string {
On 2014/03/09 18:40:50, dimitern wrote:
> doc comment please:
> // TargetRelease returns ...

Done.

[wwitzel, 2014-03-09 19:03:27]

Please take a look.

[wwitzel, 2014-03-10 11:08:14]

Please take a look.

[wwitzel, 2014-03-10 19:31:18]

Please take a look.

[wwitzel, 2014-03-10 19:32:58]

On 2014/03/10 19:31:18, wwitzel wrote:
> Please take a look.

LGTM - Fixing tests, self approving.

[wwitzel, 2014-03-12 18:19:15]

Please take a look.

[wwitzel3, 2014-03-12 18:46:38]

https://codereview.appspot.com/72270044/diff/120001/container/lxc/initialisat...
File container/lxc/initialisation.go (right):

https://codereview.appspot.com/72270044/diff/120001/container/lxc/initialisat...
container/lxc/initialisation.go:35: func targetReleasePackages(targetRelease
string) []string {
After talking with rogpeppe we will add an ltsPackages slice. This will contain
the packages that will require a --target-release and we will merge that with
the requiredPackages slice.

In the case of an empty string for targetRelease we will just append them
together and not provide a --target-release

[wwitzel, 2014-03-13 11:40:06]

Please take a look.

[wwitzel, 2014-03-13 11:40:58]

https://codereview.appspot.com/72270044/diff/120001/container/lxc/initialisat...
File container/lxc/initialisation.go (right):

https://codereview.appspot.com/72270044/diff/120001/container/lxc/initialisat...
container/lxc/initialisation.go:35: func targetReleasePackages(targetRelease
string) []string {
On 2014/03/12 18:46:39, wwitzel3 wrote:
> After talking with rogpeppe we will add an ltsPackages slice. This will
contain
> the packages that will require a --target-release and we will merge that with
> the requiredPackages slice.
> 
> In the case of an empty string for targetRelease we will just append them
> together and not provide a --target-release

Done.

[wwitzel, 2014-03-13 11:47:00]

Please take a look.

[rog, 2014-03-13 12:17:51]

This is mostly great, but we should really not ignore errors.

For the record, acronyms in identifiers should be either
fully capitalised, or not capitalised at all.

So ltsFoo or fooLTS, but not fooLts.

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
File container/lxc/initialisation.go (right):

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
container/lxc/initialisation.go:13: // during AptGetInstall
// ltsPackages holds any required packages that may
// install with a different version from their default.
// The installed versions of these packages will be selected
// with targetRelease argument to NewContainerInitialiser.
//
// Specifically, ltsPackages should
// include any required packages from the cloud archive.

?

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
container/lxc/initialisation.go:18: var requiredPackages = []string{}
// requiredPackages holds any required packages that
// will always install with their default version.

?

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
container/lxc/initialisation.go:35: return
ensureDependencies((*ci).targetRelease)
ci.targetRelease

(the indirection is automatic)

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
container/lxc/initialisation.go:41: packages := []string{
How about putting all the targetRelease-specific logic in here?

func installLTSPackages(targetRelease string) error {
    var args []string
    if targetRelease != "" {
      args = append(args, "--target-release", targetRelease)
    }
    args = append(args, ltsPackages...)
    return utils.AptGetInstall(args...)
}


Then the logic in ensureDependencies can be much simpler:

func ensureDependencies(targetRelease string) error {
    if err := installLTSPackages(targetRelease); err != nil { 
        return err
    }
    return utils.AptGetInstall(requiredPackages)
}


it's potentially slightly more inefficient, but do we actually care?

https://codereview.appspot.com/72270044/diff/160001/state/api/provisioner/mac...
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/72270044/diff/160001/state/api/provisioner/mac...
state/api/provisioner/machine.go:172: series, _ := m.Series()
We should not ignore the error here - it may represent a connection
failure. That means we should return an error from TargetRelease too.

https://codereview.appspot.com/72270044/diff/160001/state/api/provisioner/mac...
state/api/provisioner/machine.go:175: targetRelease =
"precise-updates/cloud-tools"
Or lose the targetRelease variable:

switch series {
case "precise"
    return "precise-updates/cloud-tools"
}
return ""

[wwitzel3, 2014-03-13 13:05:09]

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
File container/lxc/initialisation.go (right):

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
container/lxc/initialisation.go:13: // during AptGetInstall
On 2014/03/13 12:17:52, rog wrote:
> // ltsPackages holds any required packages that may
> // install with a different version from their default.
> // The installed versions of these packages will be selected
> // with targetRelease argument to NewContainerInitialiser.
> //
> // Specifically, ltsPackages should
> // include any required packages from the cloud archive.
> 
> ?

Done.

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
container/lxc/initialisation.go:18: var requiredPackages = []string{}
On 2014/03/13 12:17:52, rog wrote:
> // requiredPackages holds any required packages that
> // will always install with their default version.
> 
> ?

Done.

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
container/lxc/initialisation.go:35: return
ensureDependencies((*ci).targetRelease)
On 2014/03/13 12:17:52, rog wrote:
> ci.targetRelease
> 
> (the indirection is automatic)

Done.

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
container/lxc/initialisation.go:41: packages := []string{
On 2014/03/13 12:17:52, rog wrote:
> How about putting all the targetRelease-specific logic in here?
> 
> func installLTSPackages(targetRelease string) error {
>     var args []string
>     if targetRelease != "" {
>       args = append(args, "--target-release", targetRelease)
>     }
>     args = append(args, ltsPackages...)
>     return utils.AptGetInstall(args...)
> }
> 
> 
> Then the logic in ensureDependencies can be much simpler:
> 
> func ensureDependencies(targetRelease string) error {
>     if err := installLTSPackages(targetRelease); err != nil { 
>         return err
>     }
>     return utils.AptGetInstall(requiredPackages)
> }
> 
> 
> it's potentially slightly more inefficient, but do we actually care?

I like that, it was a carry over from the old version where that method returned
a string of packages to be appended. Now that it is actually calling
AptGetInstall, moving all of that logic there makes sense.

https://codereview.appspot.com/72270044/diff/160001/container/lxc/initialisat...
container/lxc/initialisation.go:41: packages := []string{
On 2014/03/13 12:17:52, rog wrote:
> How about putting all the targetRelease-specific logic in here?
> 
> func installLTSPackages(targetRelease string) error {
>     var args []string
>     if targetRelease != "" {
>       args = append(args, "--target-release", targetRelease)
>     }
>     args = append(args, ltsPackages...)
>     return utils.AptGetInstall(args...)
> }
> 
> 
> Then the logic in ensureDependencies can be much simpler:
> 
> func ensureDependencies(targetRelease string) error {
>     if err := installLTSPackages(targetRelease); err != nil { 
>         return err
>     }
>     return utils.AptGetInstall(requiredPackages)
> }
> 
> 
> it's potentially slightly more inefficient, but do we actually care?

Done.

https://codereview.appspot.com/72270044/diff/160001/state/api/provisioner/mac...
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/72270044/diff/160001/state/api/provisioner/mac...
state/api/provisioner/machine.go:172: series, _ := m.Series()
On 2014/03/13 12:17:52, rog wrote:
> We should not ignore the error here - it may represent a connection
> failure. That means we should return an error from TargetRelease too.

Done.

[wwitzel, 2014-03-13 13:06:45]

Please take a look.

[wwitzel, 2014-03-13 17:27:18]

Please take a look.

[gz, 2014-03-13 17:59:19]

LGTM witha few comments.

https://codereview.appspot.com/72270044/diff/200001/container/lxc/initialisat...
File container/lxc/initialisation.go (right):

https://codereview.appspot.com/72270044/diff/200001/container/lxc/initialisat...
container/lxc/initialisation.go:37: packagesList :=
utils.AptGetPreparePackages(requiredPackages, series)
I find 'packagesList' a little confusing. It's really a aptGetInstallCommandList
or something...

https://codereview.appspot.com/72270044/diff/200001/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/72270044/diff/200001/utils/apt.go#newcode46
utils/apt.go:46: // should reference when determining the --target-release for a
given series.
Would be good to include a url that has the currently list of packages in the
cloud-tools pocket.

https://codereview.appspot.com/72270044/diff/200001/utils/apt.go#newcode72
utils/apt.go:72: // targetRelesase returns a string base on the current series
typo, targetRelease

https://codereview.appspot.com/72270044/diff/200001/utils/apt_test.go
File utils/apt_test.go (right):

https://codereview.appspot.com/72270044/diff/200001/utils/apt_test.go#newcode37
utils/apt_test.go:37: packagesList :=
utils.AptGetPreparePackages([]string{"lxc", "bridge-utils"}, "precise")
I'd pass at least three things, so we see the grouping into two sets works.

[wwitzel3, 2014-03-13 18:07:38]

https://codereview.appspot.com/72270044/diff/200001/container/lxc/initialisat...
File container/lxc/initialisation.go (right):

https://codereview.appspot.com/72270044/diff/200001/container/lxc/initialisat...
container/lxc/initialisation.go:37: packagesList :=
utils.AptGetPreparePackages(requiredPackages, series)
On 2014/03/13 17:59:19, gz wrote:
> I find 'packagesList' a little confusing. It's really a
aptGetInstallCommandList
> or something...

Done.

https://codereview.appspot.com/72270044/diff/200001/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/72270044/diff/200001/utils/apt.go#newcode46
utils/apt.go:46: // should reference when determining the --target-release for a
given series.
On 2014/03/13 17:59:19, gz wrote:
> Would be good to include a url that has the currently list of packages in the
> cloud-tools pocket.

Done.

https://codereview.appspot.com/72270044/diff/200001/utils/apt.go#newcode72
utils/apt.go:72: // targetRelesase returns a string base on the current series
On 2014/03/13 17:59:19, gz wrote:
> typo, targetRelease

Done.

https://codereview.appspot.com/72270044/diff/200001/utils/apt_test.go
File utils/apt_test.go (right):

https://codereview.appspot.com/72270044/diff/200001/utils/apt_test.go#newcode37
utils/apt_test.go:37: packagesList :=
utils.AptGetPreparePackages([]string{"lxc", "bridge-utils"}, "precise")
On 2014/03/13 17:59:19, gz wrote:
> I'd pass at least three things, so we see the grouping into two sets works.

Done.

[wwitzel, 2014-03-13 18:12:41]

Please take a look.

[wwitzel3, 2014-03-13 18:13:07]

Discussed the choice to keep this separated from cloudinit with mgz and
rogpeppe. We will address the two places doing the same thing later.

[natefinch, 2014-03-13 20:20:04]

LGTM with just one tiny nitpick

https://codereview.appspot.com/72270044/diff/220001/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/72270044/diff/220001/utils/apt.go#newcode92
utils/apt.go:92: if target := targetRelease(series); target != "" {
nitpicky, but can you do 
if target == "" { 
   return append(installCommands, packages)
}

// else all the rest of the stuff

It makes it a lot easier to read, since you don't have to keep in your head what
the if statement is talking about 30 lines later.

[wwitzel3, 2014-03-13 20:33:45]

https://codereview.appspot.com/72270044/diff/220001/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/72270044/diff/220001/utils/apt.go#newcode92
utils/apt.go:92: if target := targetRelease(series); target != "" {
On 2014/03/13 20:20:04, nate.finch wrote:
> nitpicky, but can you do 
> if target == "" { 
>    return append(installCommands, packages)
> }
> 
> // else all the rest of the stuff
> 
> It makes it a lot easier to read, since you don't have to keep in your head
what
> the if statement is talking about 30 lines later.  

Done.