Code review comment for lp:~axwalk/juju-core/lp1271144-maas-bridge-utils

Reviewers: mp+211886_code.launchpad.net,

Message:
Please take a look.

Description:
Fix maas bridge-utils installation

Need to do an apt-get update first. I've updated
the code to use the appropriate options to disable
all interactivity.

Also, changed the code to the ifdown/ifup correctly.

Live tested with "juju deploy ubuntu --to lxc:0"

Fixes lp:1271144
Fixes lp:1248283

https://code.launchpad.net/~axwalk/juju-core/lp1271144-maas-bridge-utils/+merge/211886

(do not edit description out of merge proposal)

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

Affected files (+67, -12 lines):
   A [revision details]
   M provider/maas/environ.go
   M provider/maas/environ_test.go
   M provider/maas/export_test.go
   M utils/apt.go
   M utils/apt_test.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-20140320055725-noj3sbg573beucbk
+New revision: <email address hidden>

Index: utils/apt.go
=== modified file 'utils/apt.go'
--- utils/apt.go 2014-03-13 20:51:48 +0000
+++ utils/apt.go 2014-03-20 09:08:27 +0000
@@ -81,6 +81,16 @@
   }
  }

+// AptGetCommand returns a command to execute apt-get
+// with the specified arguments, and the appropriate
+// environment variables and options for a non-interactive
+// session.
+func AptGetCommand(args ...string) []string {
+ cmd := append([]string{"env"}, aptGetEnvOptions...)
+ cmd = append(cmd, aptGetCommand...)
+ return append(cmd, args...)
+}
+
  // AptGetPreparePackages returns a slice of installCommands. Each item
  // in the slice is suitable for passing directly to AptGetInstall.
  //

Index: utils/apt_test.go
=== modified file 'utils/apt_test.go'
--- utils/apt_test.go 2014-03-13 20:52:41 +0000
+++ utils/apt_test.go 2014-03-20 09:08:27 +0000
@@ -39,6 +39,22 @@
   c.Assert(packagesList[1], gc.DeepEquals, []string{"bridge-utils", "git"})
  }

+func (s *AptSuite) TestAptGetCommand(c *gc.C) {
+ s.testAptGetCommand(c)
+ s.testAptGetCommand(c, "install", "foo")
+}
+
+func (s *AptSuite) testAptGetCommand(c *gc.C, args ...string) {
+ commonArgs := []string{
+ "env", "DEBIAN_FRONTEND=noninteractive",
+ "apt-get", "--option=Dpkg::Options::=--force-confold",
+ "--option=Dpkg::options::=--force-unsafe-io", "--assume-yes", "--quiet",
+ }
+ expected := append(commonArgs, args...)
+ cmd := utils.AptGetCommand(args...)
+ c.Assert(cmd, gc.DeepEquals, expected)
+}
+
  func (s *AptSuite) TestAptGetError(c *gc.C) {
   const expected = `E: frobnicator failure detected`
   cmdError := fmt.Errorf("error")

Index: provider/maas/environ.go
=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go 2014-03-19 19:35:33 +0000
+++ provider/maas/environ.go 2014-03-20 09:08:27 +0000
@@ -276,8 +276,7 @@
   if err != nil {
    return nil, nil, err
   }
- info := machineInfo{hostname}
- runCmd, err := info.cloudinitRunCmd()
+ additionalScripts, err := additionalScripts(hostname)
   if err != nil {
    return nil, nil, err
   }
@@ -288,14 +287,7 @@
   // The machine envronment config values are being moved to the agent
config.
   // Explicitly specify that the lxc containers use the network bridge
defined above.
   args.MachineConfig.AgentEnvironment[agent.LxcBridge] = "br0"
- userdata, err := environs.ComposeUserData(
- args.MachineConfig,
- runCmd,
- "apt-get install bridge-utils",
- createBridgeNetwork(),
- linkBridgeInInterfaces(),
- "service networking restart",
- )
+ userdata, err := environs.ComposeUserData(args.MachineConfig,
additionalScripts...)
   if err != nil {
    msg := fmt.Errorf("could not compose userdata for bootstrap node: %v",
err)
    return nil, nil, msg
@@ -311,6 +303,25 @@
   return inst, nil, nil
  }

+// additionalScripts is an additional set of commands
+// to run during cloud-init (before the synchronous phase).
+func additionalScripts(hostname string) ([]string, error) {
+ info := machineInfo{hostname}
+ runCmd, err := info.cloudinitRunCmd()
+ if err != nil {
+ return nil, err
+ }
+ return []string{
+ runCmd,
+ utils.CommandString(utils.AptGetCommand("update")...),
+ utils.CommandString(utils.AptGetCommand("install", "bridge-utils")...),
+ "ifdown eth0",
+ createBridgeNetwork(),
+ linkBridgeInInterfaces(),
+ "ifup br0",
+ }, nil
+}
+
  // StartInstance is specified in the InstanceBroker interface.
  func (environ *maasEnviron) StopInstances(instances []instance.Instance)
error {
   // Shortcut to exit quickly if 'instances' is an empty slice or nil.

Index: provider/maas/environ_test.go
=== modified file 'provider/maas/environ_test.go'
--- provider/maas/environ_test.go 2014-03-19 01:20:14 +0000
+++ provider/maas/environ_test.go 2014-03-20 09:08:27 +0000
@@ -203,3 +203,18 @@
   c.Assert(err, gc.IsNil)
   c.Assert(a, gc.DeepEquals, arch.AllSupportedArches)
  }
+
+func (*environSuite) TestAdditionalSCripts(c *gc.C) {
+ const aptGetPrefix = "env DEBIAN_FRONTEND=noninteractive apt-get
--option=Dpkg::Options::=--force-confold
--option=Dpkg::options::=--force-unsafe-io --assume-yes --quiet"
+ scripts, err := maas.AdditionalScripts("testing.invalid")
+ c.Assert(err, gc.IsNil)
+ c.Assert(scripts, gc.DeepEquals, []string{
+ "mkdir -p '/var/lib/juju'; echo -n 'hostname: testing.invalid\n'
> '/var/lib/juju/MAASmachine.txt'",
+ aptGetPrefix + " update",
+ aptGetPrefix + " install bridge-utils",
+ "ifdown eth0",
+ "cat > /etc/network/eth0.config << EOF\niface eth0 inet manual\n\nauto
br0\niface br0 inet dhcp\n bridge_ports eth0\nEOF\n",
+ `sed -i "s/iface eth0 inet dhcp/source \/etc\/network\/eth0.config/"
/etc/network/interfaces`,
+ "ifup br0",
+ })
+}

Index: provider/maas/export_test.go
=== modified file 'provider/maas/export_test.go'
--- provider/maas/export_test.go 2013-12-06 11:00:58 +0000
+++ provider/maas/export_test.go 2014-03-20 09:08:27 +0000
@@ -10,8 +10,9 @@
  )

  var (
- ShortAttempt = &shortAttempt
- APIVersion = apiVersion
+ ShortAttempt = &shortAttempt
+ APIVersion = apiVersion
+ AdditionalScripts = additionalScripts
  )

  func MAASAgentName(env environs.Environ) string {