juju-core

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

  1. configurable-lxc-bridge
  2. Merge into trunk
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 1938
Proposed branch: lp:~thumper/juju-core/configurable-lxc-bridge
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 236 lines (+68/-40)
5 files modified
provider/local/config.go (+6/-0)
provider/local/config_test.go (+39/-28)
provider/local/environ.go (+15/-11)
provider/local/environprovider.go (+7/-0)
provider/local/instance.go (+1/-1)
To merge this branch: bzr merge lp:~thumper/juju-core/configurable-lxc-bridge
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+188980@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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/

Revision history for this message
Dave Cheney (dave-cheney) wrote :

> 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
21 configFields = schema.Fields{21 configFields = schema.Fields{
22 "root-dir": schema.String(),22 "root-dir": schema.String(),
23 "bootstrap-ip": schema.String(),23 "bootstrap-ip": schema.String(),
24 "network-bridge": schema.String(),
24 "storage-port": schema.ForceInt(),25 "storage-port": schema.ForceInt(),
25 "shared-storage-port": schema.ForceInt(),26 "shared-storage-port": schema.ForceInt(),
26 }27 }
@@ -30,6 +31,7 @@
30 // range.31 // range.
31 configDefaults = schema.Defaults{32 configDefaults = schema.Defaults{
32 "root-dir": "",33 "root-dir": "",
34 "network-bridge": "lxcbr0",
33 "bootstrap-ip": schema.Omit,35 "bootstrap-ip": schema.Omit,
34 "storage-port": 8040,36 "storage-port": 8040,
35 "shared-storage-port": 8041,37 "shared-storage-port": 8041,
@@ -71,6 +73,10 @@
71 return c.attrs["root-dir"].(string)73 return c.attrs["root-dir"].(string)
72}74}
7375
76func (c *environConfig) networkBridge() string {
77 return c.attrs["network-bridge"].(string)
78}
79
74func (c *environConfig) sharedStorageDir() string {80func (c *environConfig) sharedStorageDir() string {
75 return filepath.Join(c.rootDir(), "shared-storage")81 return filepath.Join(c.rootDir(), "shared-storage")
76}82}
7783
=== 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 @@
40 minimal := minimalConfigValues()40 minimal := minimalConfigValues()
41 testConfig, err := config.New(config.NoDefaults, minimal)41 testConfig, err := config.New(config.NoDefaults, minimal)
42 c.Assert(err, gc.IsNil)42 c.Assert(err, gc.IsNil)
43 return testConfig43 valid, err := local.Provider.Validate(testConfig, nil)
44 c.Assert(err, gc.IsNil)
45 return valid
46}
47
48func localConfig(c *gc.C, extra map[string]interface{}) *config.Config {
49 values := minimalConfigValues()
50 for key, value := range extra {
51 values[key] = value
52 }
53 testConfig, err := config.New(config.NoDefaults, values)
54 c.Assert(err, gc.IsNil)
55 valid, err := local.Provider.Validate(testConfig, nil)
56 c.Assert(err, gc.IsNil)
57 return valid
58}
59
60func (s *configSuite) TestDefaultNetworkBridge(c *gc.C) {
61 config := minimalConfig(c)
62 unknownAttrs := config.UnknownAttrs()
63 c.Assert(unknownAttrs["network-bridge"], gc.Equals, "lxcbr0")
64}
65
66func (s *configSuite) TestSetNetworkBridge(c *gc.C) {
67 config := localConfig(c, map[string]interface{}{
68 "network-bridge": "br0",
69 })
70 unknownAttrs := config.UnknownAttrs()
71 c.Assert(unknownAttrs["network-bridge"], gc.Equals, "br0")
44}72}
4573
46func (s *configSuite) TestValidateConfig(c *gc.C) {74func (s *configSuite) TestValidateConfig(c *gc.C) {
47 testConfig := minimalConfig(c)75 valid := minimalConfig(c)
48
49 valid, err := local.Provider.Validate(testConfig, nil)
50 c.Assert(err, gc.IsNil)
51
52 expectedRootDir := filepath.Join(osenv.Home(), ".juju", "test")76 expectedRootDir := filepath.Join(osenv.Home(), ".juju", "test")
53 unknownAttrs := valid.UnknownAttrs()77 unknownAttrs := valid.UnknownAttrs()
54 c.Assert(unknownAttrs["root-dir"], gc.Equals, expectedRootDir)78 c.Assert(unknownAttrs["root-dir"], gc.Equals, expectedRootDir)
55}79}
5680
57func (s *configSuite) TestValidateConfigWithRootDir(c *gc.C) {81func (s *configSuite) TestValidateConfigWithRootDir(c *gc.C) {
58 values := minimalConfigValues()
59 root := c.MkDir()82 root := c.MkDir()
60 values["root-dir"] = root83 valid := localConfig(c, map[string]interface{}{
61 testConfig, err := config.New(config.NoDefaults, values)84 "root-dir": root,
62 c.Assert(err, gc.IsNil)85 })
63
64 valid, err := local.Provider.Validate(testConfig, nil)
65 c.Assert(err, gc.IsNil)
66 unknownAttrs := valid.UnknownAttrs()86 unknownAttrs := valid.UnknownAttrs()
67 c.Assert(unknownAttrs["root-dir"], gc.Equals, root)87 c.Assert(unknownAttrs["root-dir"], gc.Equals, root)
68}88}
6989
70func (s *configSuite) TestValidateConfigWithTildeInRootDir(c *gc.C) {90func (s *configSuite) TestValidateConfigWithTildeInRootDir(c *gc.C) {
71 values := minimalConfigValues()91 valid := localConfig(c, map[string]interface{}{
72 values["root-dir"] = "~/.juju/foo"92 "root-dir": "~/.juju/foo",
73 testConfig, err := config.New(config.NoDefaults, values)93 })
74 c.Assert(err, gc.IsNil)
75
76 valid, err := local.Provider.Validate(testConfig, nil)
77 c.Assert(err, gc.IsNil)
78
79 expectedRootDir := filepath.Join(osenv.Home(), ".juju", "foo")94 expectedRootDir := filepath.Join(osenv.Home(), ".juju", "foo")
80 unknownAttrs := valid.UnknownAttrs()95 unknownAttrs := valid.UnknownAttrs()
81 c.Assert(unknownAttrs["root-dir"], gc.Equals, expectedRootDir)96 c.Assert(unknownAttrs["root-dir"], gc.Equals, expectedRootDir)
@@ -84,13 +99,9 @@
84func (s *configSuite) TestValidateConfigWithFloatPort(c *gc.C) {99func (s *configSuite) TestValidateConfigWithFloatPort(c *gc.C) {
85 // When the config values get serialized through JSON, the integers100 // When the config values get serialized through JSON, the integers
86 // get coerced to float64 values. The parsing needs to handle this.101 // get coerced to float64 values. The parsing needs to handle this.
87 values := minimalConfigValues()102 valid := localConfig(c, map[string]interface{}{
88 values["storage-port"] = float64(8040)103 "storage-port": float64(8040),
89 testConfig, err := config.New(config.NoDefaults, values)104 })
90 c.Assert(err, gc.IsNil)
91
92 valid, err := local.Provider.Validate(testConfig, nil)
93 c.Assert(err, gc.IsNil)
94 unknownAttrs := valid.UnknownAttrs()105 unknownAttrs := valid.UnknownAttrs()
95 c.Assert(unknownAttrs["storage-port"], gc.Equals, int(8040))106 c.Assert(unknownAttrs["storage-port"], gc.Equals, int(8040))
96}107}
97108
=== 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 @@
38 "launchpad.net/juju-core/version"38 "launchpad.net/juju-core/version"
39)39)
4040
41// lxcBridgeName is the name of the network interface that the local provider
42// uses to determine the ip address to use for machine-0 such that the
43// containers being created are able to communicate with it simply.
44const lxcBridgeName = "lxcbr0"
45
46// boostrapInstanceId is just the name we give to the bootstrap machine.41// boostrapInstanceId is just the name we give to the bootstrap machine.
47// Using "localhost" because it is, and it makes sense.42// Using "localhost" because it is, and it makes sense.
48const boostrapInstanceId = "localhost"43const boostrapInstanceId = "localhost"
@@ -233,11 +228,19 @@
233 return err228 return err
234 }229 }
235230
236 bridgeAddress, err := env.findBridgeAddress()231 // We need the provider config to get the network bridge.
232 config, err := providerInstance.newConfig(cfg)
237 if err != nil {233 if err != nil {
234 logger.Errorf("failed to create new environ config: %v", err)
238 return err235 return err
239 }236 }
240 logger.Debugf("found %q as address for %q", bridgeAddress, lxcBridgeName)237 networkBridge := config.networkBridge()
238 bridgeAddress, err := env.findBridgeAddress(networkBridge)
239 if err != nil {
240 logger.Infof("configure a different bridge using 'network-bridge' in the config file")
241 return fmt.Errorf("cannot find address of network-bridge: %q", networkBridge)
242 }
243 logger.Debugf("found %q as address for %q", bridgeAddress, networkBridge)
241 cfg, err = cfg.Apply(map[string]interface{}{244 cfg, err = cfg.Apply(map[string]interface{}{
242 "bootstrap-ip": bridgeAddress,245 "bootstrap-ip": bridgeAddress,
243 })246 })
@@ -245,7 +248,8 @@
245 logger.Errorf("failed to apply new addresses to config: %v", err)248 logger.Errorf("failed to apply new addresses to config: %v", err)
246 return err249 return err
247 }250 }
248 config, err := providerInstance.newConfig(cfg)251 // Now recreate the config based on the settings with the bootstrap id.
252 config, err = providerInstance.newConfig(cfg)
249 if err != nil {253 if err != nil {
250 logger.Errorf("failed to create new environ config: %v", err)254 logger.Errorf("failed to create new environ config: %v", err)
251 return err255 return err
@@ -293,7 +297,7 @@
293 agenttools := possibleTools[0]297 agenttools := possibleTools[0]
294 logger.Debugf("tools: %#v", agenttools)298 logger.Debugf("tools: %#v", agenttools)
295299
296 network := lxc.DefaultNetworkConfig()300 network := lxc.BridgeNetworkConfig(env.config.networkBridge())
297 inst, err := env.containerManager.StartContainer(301 inst, err := env.containerManager.StartContainer(
298 machineId, series, machineConfig.MachineNonce, network,302 machineId, series, machineConfig.MachineNonce, network,
299 agenttools, env.config.Config,303 agenttools, env.config.Config,
@@ -509,8 +513,8 @@
509 return nil513 return nil
510}514}
511515
512func (env *localEnviron) findBridgeAddress() (string, error) {516func (env *localEnviron) findBridgeAddress(networkBridge string) (string, error) {
513 return getAddressForInterface(lxcBridgeName)517 return getAddressForInterface(networkBridge)
514}518}
515519
516func (env *localEnviron) writeBootstrapAgentConfFile(secret string, cert, key []byte) (agent.Config, error) {520func (env *localEnviron) writeBootstrapAgentConfFile(secret string, cert, key []byte) (agent.Config, error) {
517521
=== 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 @@
81 oldLocalConfig.rootDir(),81 oldLocalConfig.rootDir(),
82 localConfig.rootDir())82 localConfig.rootDir())
83 }83 }
84 if localConfig.networkBridge() != oldLocalConfig.networkBridge() {
85 return nil, fmt.Errorf("cannot change network-bridge from %q to %q",
86 oldLocalConfig.rootDir(),
87 localConfig.rootDir())
88 }
84 if localConfig.storagePort() != oldLocalConfig.storagePort() {89 if localConfig.storagePort() != oldLocalConfig.storagePort() {
85 return nil, fmt.Errorf("cannot change storage-port from %v to %v",90 return nil, fmt.Errorf("cannot change storage-port from %v to %v",
86 oldLocalConfig.storagePort(),91 oldLocalConfig.storagePort(),
@@ -120,6 +125,8 @@
120 # Override the shared storage port if you have multiple local providers, or if the125 # Override the shared storage port if you have multiple local providers, or if the
121 # default port is used by another program.126 # default port is used by another program.
122 # shared-storage-port: 8041127 # shared-storage-port: 8041
128 # Override the network bridge if you have changed the default lxc bridge
129 # network-bridge: lxcbr0
123130
124`[1:]131`[1:]
125}132}
126133
=== 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 @@
36func (inst *localInstance) DNSName() (string, error) {36func (inst *localInstance) DNSName() (string, error) {
37 if string(inst.id) == "localhost" {37 if string(inst.id) == "localhost" {
38 // get the bridge address from the environment38 // get the bridge address from the environment
39 addr, err := inst.env.findBridgeAddress()39 addr, err := inst.env.findBridgeAddress(inst.env.config.networkBridge())
40 if err != nil {40 if err != nil {
41 logger.Errorf("failed to get bridge address: %v", err)41 logger.Errorf("failed to get bridge address: %v", err)
42 return "", instance.ErrNoDNSName42 return "", instance.ErrNoDNSName

Subscribers

People subscribed via source and target branches

to status/vote changes: