Merge lp:~thumper/juju-core/configurable-lxc-bridge into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Allow the network-bridge to be configured.

Enable the local provider config to accept network-bridge.
This is then passed through to the container creation
network config.

A slightly more informative message is added when the
bridge is not found.

https://codereview.appspot.com/14321043/

Allow the network-bridge to be configured.

Enable the local provider config to accept network-bridge.
This is then passed through to the container creation
netowrk config.

A slightly more informative message is added when the
bridge is not found.

https://codereview.appspot.com/14321043/

[Tim Penhey (thumper)]

Reviewers: mp+188980_code.launchpad.net,

Message:
Please take a look.

Description:
Allow the network-bridge to be configured.

Enable the local provider config to accept network-bridge.
This is then passed through to the container creation
netowrk config.

A slightly more informative message is added when the
bridge is not found.

https://code.launchpad.net/~thumper/juju-core/configurable-lxc-bridge/+merge/188980

(do not edit description out of merge proposal)

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

Affected files (+70, -40 lines):
   A [revision details]
   M provider/local/config.go
   M provider/local/config_test.go
   M provider/local/environ.go
   M provider/local/environprovider.go
   M provider/local/instance.go

[Andrew Wilkins (axwalk)]

On 2013/10/03 03:19:09, thumper wrote:
> Please take a look.

LGTM, tho I wonder if we should be reading /etc/default/lxc to get the
right default. Fine as it is, just a nicety.

https://codereview.appspot.com/14321043/

[Dave Cheney (dave-cheney)]

> LGTM, tho I wonder if we should be reading /etc/default/lxc to get the
> right default. Fine as it is, just a nicety.

Maybe, that is the gold plated toilet seat option. I'm happy that it
is configurable and called out in the default config.

>
> https://codereview.appspot.com/14321043/
>
> --
> https://code.launchpad.net/~thumper/juju-core/configurable-lxc-bridge/+merge/188980
> You are subscribed to branch lp:juju-core.

=== modified file 'provider/local/config.go'
--- provider/local/config.go	2013-10-02 01:48:17 +0000
+++ provider/local/config.go	2013-10-03 03:13:31 +0000
@@ -21,6 +21,7 @@
 	configFields = schema.Fields{
 		"root-dir":            schema.String(),
 		"bootstrap-ip":        schema.String(),
+		"network-bridge":      schema.String(),
 		"storage-port":        schema.ForceInt(),
 		"shared-storage-port": schema.ForceInt(),
 	}
@@ -30,6 +31,7 @@
 	// range.
 	configDefaults = schema.Defaults{
 		"root-dir":            "",
+		"network-bridge":      "lxcbr0",
 		"bootstrap-ip":        schema.Omit,
 		"storage-port":        8040,
 		"shared-storage-port": 8041,
@@ -71,6 +73,10 @@
 	return c.attrs["root-dir"].(string)
 }
 
+func (c *environConfig) networkBridge() string {
+	return c.attrs["network-bridge"].(string)
+}
+
 func (c *environConfig) sharedStorageDir() string {
 	return filepath.Join(c.rootDir(), "shared-storage")
 }

=== modified file 'provider/local/config_test.go'
--- provider/local/config_test.go	2013-10-02 01:27:45 +0000
+++ provider/local/config_test.go	2013-10-03 03:13:31 +0000
@@ -40,42 +40,57 @@
 	minimal := minimalConfigValues()
 	testConfig, err := config.New(config.NoDefaults, minimal)
 	c.Assert(err, gc.IsNil)
-	return testConfig
+	valid, err := local.Provider.Validate(testConfig, nil)
+	c.Assert(err, gc.IsNil)
+	return valid
+}
+
+func localConfig(c *gc.C, extra map[string]interface{}) *config.Config {
+	values := minimalConfigValues()
+	for key, value := range extra {
+		values[key] = value
+	}
+	testConfig, err := config.New(config.NoDefaults, values)
+	c.Assert(err, gc.IsNil)
+	valid, err := local.Provider.Validate(testConfig, nil)
+	c.Assert(err, gc.IsNil)
+	return valid
+}
+
+func (s *configSuite) TestDefaultNetworkBridge(c *gc.C) {
+	config := minimalConfig(c)
+	unknownAttrs := config.UnknownAttrs()
+	c.Assert(unknownAttrs["network-bridge"], gc.Equals, "lxcbr0")
+}
+
+func (s *configSuite) TestSetNetworkBridge(c *gc.C) {
+	config := localConfig(c, map[string]interface{}{
+		"network-bridge": "br0",
+	})
+	unknownAttrs := config.UnknownAttrs()
+	c.Assert(unknownAttrs["network-bridge"], gc.Equals, "br0")
 }
 
 func (s *configSuite) TestValidateConfig(c *gc.C) {
-	testConfig := minimalConfig(c)
-
-	valid, err := local.Provider.Validate(testConfig, nil)
-	c.Assert(err, gc.IsNil)
-
+	valid := minimalConfig(c)
 	expectedRootDir := filepath.Join(osenv.Home(), ".juju", "test")
 	unknownAttrs := valid.UnknownAttrs()
 	c.Assert(unknownAttrs["root-dir"], gc.Equals, expectedRootDir)
 }
 
 func (s *configSuite) TestValidateConfigWithRootDir(c *gc.C) {
-	values := minimalConfigValues()
 	root := c.MkDir()
-	values["root-dir"] = root
-	testConfig, err := config.New(config.NoDefaults, values)
-	c.Assert(err, gc.IsNil)
-
-	valid, err := local.Provider.Validate(testConfig, nil)
-	c.Assert(err, gc.IsNil)
+	valid := localConfig(c, map[string]interface{}{
+		"root-dir": root,
+	})
 	unknownAttrs := valid.UnknownAttrs()
 	c.Assert(unknownAttrs["root-dir"], gc.Equals, root)
 }
 
 func (s *configSuite) TestValidateConfigWithTildeInRootDir(c *gc.C) {
-	values := minimalConfigValues()
-	values["root-dir"] = "~/.juju/foo"
-	testConfig, err := config.New(config.NoDefaults, values)
-	c.Assert(err, gc.IsNil)
-
-	valid, err := local.Provider.Validate(testConfig, nil)
-	c.Assert(err, gc.IsNil)
-
+	valid := localConfig(c, map[string]interface{}{
+		"root-dir": "~/.juju/foo",
+	})
 	expectedRootDir := filepath.Join(osenv.Home(), ".juju", "foo")
 	unknownAttrs := valid.UnknownAttrs()
 	c.Assert(unknownAttrs["root-dir"], gc.Equals, expectedRootDir)
@@ -84,13 +99,9 @@
 func (s *configSuite) TestValidateConfigWithFloatPort(c *gc.C) {
 	// When the config values get serialized through JSON, the integers
 	// get coerced to float64 values.  The parsing needs to handle this.
-	values := minimalConfigValues()
-	values["storage-port"] = float64(8040)
-	testConfig, err := config.New(config.NoDefaults, values)
-	c.Assert(err, gc.IsNil)
-
-	valid, err := local.Provider.Validate(testConfig, nil)
-	c.Assert(err, gc.IsNil)
+	valid := localConfig(c, map[string]interface{}{
+		"storage-port": float64(8040),
+	})
 	unknownAttrs := valid.UnknownAttrs()
 	c.Assert(unknownAttrs["storage-port"], gc.Equals, int(8040))
 }

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2013-10-02 10:38:04 +0000
+++ provider/local/environ.go	2013-10-03 03:13:31 +0000
@@ -38,11 +38,6 @@
 	"launchpad.net/juju-core/version"
 )
 
-// lxcBridgeName is the name of the network interface that the local provider
-// uses to determine the ip address to use for machine-0 such that the
-// containers being created are able to communicate with it simply.
-const lxcBridgeName = "lxcbr0"
-
 // boostrapInstanceId is just the name we give to the bootstrap machine.
 // Using "localhost" because it is, and it makes sense.
 const boostrapInstanceId = "localhost"
@@ -233,11 +228,19 @@
 		return err
 	}
 
-	bridgeAddress, err := env.findBridgeAddress()
+	// We need the provider config to get the network bridge.
+	config, err := providerInstance.newConfig(cfg)
 	if err != nil {
+		logger.Errorf("failed to create new environ config: %v", err)
 		return err
 	}
-	logger.Debugf("found %q as address for %q", bridgeAddress, lxcBridgeName)
+	networkBridge := config.networkBridge()
+	bridgeAddress, err := env.findBridgeAddress(networkBridge)
+	if err != nil {
+		logger.Infof("configure a different bridge using 'network-bridge' in the config file")
+		return fmt.Errorf("cannot find address of network-bridge: %q", networkBridge)
+	}
+	logger.Debugf("found %q as address for %q", bridgeAddress, networkBridge)
 	cfg, err = cfg.Apply(map[string]interface{}{
 		"bootstrap-ip": bridgeAddress,
 	})
@@ -245,7 +248,8 @@
 		logger.Errorf("failed to apply new addresses to config: %v", err)
 		return err
 	}
-	config, err := providerInstance.newConfig(cfg)
+	// Now recreate the config based on the settings with the bootstrap id.
+	config, err = providerInstance.newConfig(cfg)
 	if err != nil {
 		logger.Errorf("failed to create new environ config: %v", err)
 		return err
@@ -293,7 +297,7 @@
 	agenttools := possibleTools[0]
 	logger.Debugf("tools: %#v", agenttools)
 
-	network := lxc.DefaultNetworkConfig()
+	network := lxc.BridgeNetworkConfig(env.config.networkBridge())
 	inst, err := env.containerManager.StartContainer(
 		machineId, series, machineConfig.MachineNonce, network,
 		agenttools, env.config.Config,
@@ -509,8 +513,8 @@
 	return nil
 }
 
-func (env *localEnviron) findBridgeAddress() (string, error) {
-	return getAddressForInterface(lxcBridgeName)
+func (env *localEnviron) findBridgeAddress(networkBridge string) (string, error) {
+	return getAddressForInterface(networkBridge)
 }
 
 func (env *localEnviron) writeBootstrapAgentConfFile(secret string, cert, key []byte) (agent.Config, error) {

=== modified file 'provider/local/environprovider.go'
--- provider/local/environprovider.go	2013-09-26 23:26:19 +0000
+++ provider/local/environprovider.go	2013-10-03 03:13:31 +0000
@@ -81,6 +81,11 @@
 				oldLocalConfig.rootDir(),
 				localConfig.rootDir())
 		}
+		if localConfig.networkBridge() != oldLocalConfig.networkBridge() {
+			return nil, fmt.Errorf("cannot change network-bridge from %q to %q",
+				oldLocalConfig.rootDir(),
+				localConfig.rootDir())
+		}
 		if localConfig.storagePort() != oldLocalConfig.storagePort() {
 			return nil, fmt.Errorf("cannot change storage-port from %v to %v",
 				oldLocalConfig.storagePort(),
@@ -120,6 +125,8 @@
   # Override the shared storage port if you have multiple local providers, or if the
   # default port is used by another program.
   # shared-storage-port: 8041
+  # Override the network bridge if you have changed the default lxc bridge
+  # network-bridge: lxcbr0
 
 `[1:]
 }

=== modified file 'provider/local/instance.go'
--- provider/local/instance.go	2013-09-30 19:40:06 +0000
+++ provider/local/instance.go	2013-10-03 03:13:31 +0000
@@ -36,7 +36,7 @@
 func (inst *localInstance) DNSName() (string, error) {
 	if string(inst.id) == "localhost" {
 		// get the bridge address from the environment
-		addr, err := inst.env.findBridgeAddress()
+		addr, err := inst.env.findBridgeAddress(inst.env.config.networkBridge())
 		if err != nil {
 			logger.Errorf("failed to get bridge address: %v", err)
 			return "", instance.ErrNoDNSName