Merge lp:~rogpeppe/juju-core/512-maas-bootstrap-bridge-utils into lp:~go-bot/juju-core/trunk

[Roger Peppe]

provider/maas: apt-get install bridge-utils

Previously we added bridge-utils to the required packages
in environs/cloudinit, but at bootstrap time, those packages
are not installed before the additional commands specified
by a provider are run (we want any failures to
be visible, so we wait until the synchronous part of the
bootstrap process).

To fix this, we explicitly add "apt-get install bridge-utils"
to provider/maas and remove it from environs/cloudinit.

Fixes:

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

https://codereview.appspot.com/72230045/

provider/maas: apt-get install bridge-utils

Previously we added bridge-utils to the required packages
in environs/cloudinit, but at bootstrap time, those packages
are not installed before the additional commands specified
by a provider are run (we want any failures to
be visible, so we wait until the synchronous part of the
bootstrap process).

To fix this, we explicitly add "apt-get install bridge-utils"
to provider/maas and remove it from environs/cloudinit.

Fixes:

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

https://codereview.appspot.com/72230045/

[Roger Peppe (rogpeppe)]

Reviewers: mp+209885_code.launchpad.net,

Message:
Please take a look.

Description:
provider/maas: apt-get install bridge-utils

Previously we added bridge-utils to the required packages
in environs/cloudinit, but at bootstrap time, those packages
are not installed before the additional commands specified
by a provider are run (we want any failures to
be visible, so we wait until the synchronous part of the
bootstrap process).

To fix this, we explicitly add "apt-get install bridge-utils"
to provider/maas and remove it from environs/cloudinit.

Fixes:

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

https://code.launchpad.net/~rogpeppe/juju-core/512-maas-bootstrap-bridge-utils/+merge/209885

(do not edit description out of merge proposal)

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

Affected files (+3, -1 lines):
   A [revision details]
   M environs/cloudinit/cloudinit.go
   M provider/maas/environ.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140307010349-qiohhcs2paafuj1o
+New revision: <email address hidden>

Index: environs/cloudinit/cloudinit.go
=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go 2014-03-06 20:35:21 +0000
+++ environs/cloudinit/cloudinit.go 2014-03-07 10:20:30 +0000
@@ -238,7 +238,6 @@
    // juju requires git for managing charm directories.
    c.AddPackage("git")
    c.AddPackage("cpu-checker")
- c.AddPackage("bridge-utils")
    c.AddPackage("rsyslog-gnutls")

    // Write out the apt proxy settings

Index: provider/maas/environ.go
=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go 2014-03-03 10:10:44 +0000
+++ provider/maas/environ.go 2014-03-07 10:20:30 +0000
@@ -268,6 +268,7 @@
   userdata, err := environs.ComposeUserData(
    machineConfig,
    runCmd,
+ "apt-get install bridge-utils",
    createBridgeNetwork(),
    linkBridgeInInterfaces(),
    "service networking restart",

[William Reade (fwereade)]

On 2014/03/07 12:21:24, rog wrote:
> Please take a look.

I has a sad about the lack of tests.

https://codereview.appspot.com/72230045/

[Andrew Wilkins (axwalk)]

On 2014/03/07 12:34:32, fwereade wrote:
> On 2014/03/07 12:21:24, rog wrote:
> > Please take a look.

> I has a sad about the lack of tests.

Apart from this, looks good.

You might be able to knock this one off at the same time, but feel free
to ignore me: https://bugs.launchpad.net/maas/+bug/1248283

(We should be using ifdown/ifup, rather than "service networking
restart")

https://codereview.appspot.com/72230045/

[John A Meinel (jameinel)]

doesn't this mean that we aren't installing bridge-utils for all the things that aren't maas? It seems strange that we would only put it into one of the providers.

[John A Meinel (jameinel)]

I guess https://bugs.launchpad.net/juju-core/+bug/1277359 is that we should only be installing it for MaaS, but given we are trying to make containers accessible on every provider, I think we'd end up back where we are and just installing it everywhere. So I think that bug might actually be Won't Fix unless we have a really strong reason.

https://bugs.launchpad.net/juju-core/+bug/1271144 is a bigger deal, but from what I can tell this may or may not actually fix it.

[Martin Packman (gz)]

Not sure this is landable as is.

https://codereview.appspot.com/72230045/diff/20001/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/72230045/diff/20001/provider/maas/environ.go#newcode271
provider/maas/environ.go:271: "apt-get install bridge-utils",
This misses the niceties of utils/apt.go aptGetCommand, some of which
like -y are really requirements. We should try to share good apt
practices across the code base.

It also will not play nicely with Wayne's in-progress change to make
sure we try to install packages-for-juju from the cloud-archive where
possible:

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

https://codereview.appspot.com/72230045/

[Wayne Witzel III (wwitzel3)]

After discussion with Martin and Roger I believe we plan to land this as-is for now? I tested this yesterday with my local maas setup. Was able to reproduce the error on trunk, unable to reproduce the error on this branch.

[Roger Peppe (rogpeppe)]

Tested live and landing as is, because it fixes a real bug.

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

On 7 March 2014 16:30, Martin Packman <email address hidden> wrote:
> Not sure this is landable as is.
>
>
> https://codereview.appspot.com/72230045/diff/20001/provider/maas/environ.go
> File provider/maas/environ.go (right):
>
> https://codereview.appspot.com/72230045/diff/20001/provider/maas/environ.go#newcode271
> provider/maas/environ.go:271: "apt-get install bridge-utils",
> This misses the niceties of utils/apt.go aptGetCommand, some of which
> like -y are really requirements. We should try to share good apt
> practices across the code base.
>
> It also will not play nicely with Wayne's in-progress change to make
> sure we try to install packages-for-juju from the cloud-archive where
> possible:
>
> https://code.launchpad.net/~wwitzel3/juju-core/lp-1289316-lxc-maas-precise
>
> https://codereview.appspot.com/72230045/
>
> --
> https://code.launchpad.net/~rogpeppe/juju-core/512-maas-bootstrap-bridge-utils/+merge/209885
> You are the owner of lp:~rogpeppe/juju-core/512-maas-bootstrap-bridge-utils.

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in environs/cloudinit/cloudinit.go

=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go	2014-03-13 05:09:14 +0000
+++ provider/maas/environ.go	2014-03-14 13:58:03 +0000
@@ -266,6 +266,7 @@
 	userdata, err := environs.ComposeUserData(
 		args.MachineConfig,
 		runCmd,
+		"apt-get install bridge-utils",
 		createBridgeNetwork(),
 		linkBridgeInInterfaces(),
 		"service networking restart",