Merge lp:~thumper/juju-core/no-smash-networking into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Nicer MAAS network change.

The earlier change to create a network bridge for
eth0 was to replace the entire /etc/network/interfaces
file. While this worked, it only worked because MAAS
doesn't yet know much about the network hardware on the
physical machines.

Instead of replacing the entire file, we instead write
out a separate network file and use sed to modify the
existing file to source the new file.

https://codereview.appspot.com/13259043/

Nicer MAAS network change.

The earlier change to create a network bridge for
eth0 was to replace the entire /etc/network/interfaces
file. While this worked, it only worked because MAAS
doesn't yet know much about the network hardware on the
physical machines.

Instead of replacing the entire file, we instead write
out a separate network file and use sed to modify the
existing file to source the new file.

https://codereview.appspot.com/13259043/

[Tim Penhey (thumper)]

Reviewers: mp+182248_code.launchpad.net,

Message:
Please take a look.

Description:
Nicer MAAS network change.

The earlier change to create a network bridge for
eth0 was to replace the entire /etc/network/interfaces
file. While this worked, it only worked because MAAS
doesn't yet know much about the network hardware on the
physical machines.

Instead of replacing the entire file, we instead write
out a separate network file and use sed to modify the
existing file to source the new file.

https://code.launchpad.net/~thumper/juju-core/no-smash-networking/+merge/182248

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/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-20130815174953-arsxsn7541v1jxs5
+New revision: <email address hidden>

Index: environs/maas/environ.go
=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go 2013-08-15 14:13:29 +0000
+++ environs/maas/environ.go 2013-08-16 03:41:38 +0000
@@ -254,11 +254,7 @@
  // createBridgeNetwork returns a string representing the upstart command to
  // create a bridged eth0.
  func createBridgeNetwork() string {
- return `cat > /etc/network/interfaces << EOF
-auto lo
-iface lo inet loopback
-
-auto eth0
+ return `cat > /etc/network/eth0.config << EOF
  iface eth0 inet manual

  auto br0
@@ -268,6 +264,12 @@
  `
  }

+// linkBridgeInInterfaces adds the file created by createBridgeNetwork to
the
+// interfaces file.
+func linkBridgeInInterfaces() string {
+ return `sed -i "s/iface eth0 inet dhcp/source
\/etc\/network\/eth0.config/" etc/network/interfaces`
+}
+
  // internalStartInstance allocates and starts a MAAS node. It is used both
  // for the implementation of StartInstance, and to initialize the bootstrap
  // node.
@@ -314,6 +316,7 @@
    machineConfig,
    runCmd,
    createBridgeNetwork(),
+ linkBridgeInInterfaces(),
    "service networking restart",
   )
   if err != nil {

[John A Meinel (jameinel)]

I'm happy enough with this change, though I wish there was a test that
needed changing at the same time.

We talked about doing this in jujud startup, rather than during
cloud-init. (Then we have the full power of a programming language
rather than just a 'sed' script.)

Anyway, I definitely prefer this to land rather than block waiting for
something else, I'm just opening the conversation about where we might
be headed.

LGTM

https://codereview.appspot.com/13259043/diff/1/environs/maas/environ.go
File environs/maas/environ.go (right):

https://codereview.appspot.com/13259043/diff/1/environs/maas/environ.go#newcode270
environs/maas/environ.go:270: return `sed -i "s/iface eth0 inet
dhcp/source \/etc\/network\/eth0.config/" etc/network/interfaces`
Doesn't the last bit need to be "/etc/network/interfaces"? Are we always
guaranteed to run at '/' ?

https://codereview.appspot.com/13259043/

=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go	2013-08-22 22:24:54 +0000
+++ provider/maas/environ.go	2013-08-27 04:20:38 +0000
@@ -201,11 +201,7 @@
 // createBridgeNetwork returns a string representing the upstart command to
 // create a bridged eth0.
 func createBridgeNetwork() string {
-	return `cat > /etc/network/interfaces << EOF
-auto lo
-iface lo inet loopback
-
-auto eth0
+	return `cat > /etc/network/eth0.config << EOF
 iface eth0 inet manual
 
 auto br0
@@ -215,6 +211,12 @@
 `
 }
 
+// linkBridgeInInterfaces adds the file created by createBridgeNetwork to the
+// interfaces file.
+func linkBridgeInInterfaces() string {
+	return `sed -i "s/iface eth0 inet dhcp/source \/etc\/network\/eth0.config/" /etc/network/interfaces`
+}
+
 // StartInstance is specified in the InstanceBroker interface.
 func (environ *maasEnviron) StartInstance(cons constraints.Value, possibleTools tools.List,
 	machineConfig *cloudinit.MachineConfig) (instance.Instance, *instance.HardwareCharacteristics, error) {
@@ -253,6 +255,7 @@
 		machineConfig,
 		runCmd,
 		createBridgeNetwork(),
+		linkBridgeInInterfaces(),
 		"service networking restart",
 	)
 	if err != nil {