juju-core

Merge lp:~thumper/juju-core/container-address into lp:~go-bot/juju-core/trunk

  1. container-address
  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: 1733
Proposed branch: lp:~thumper/juju-core/container-address
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 184 lines (+44/-22)
8 files modified
container/lxc/lxc.go (+4/-0)
juju/osenv/vars.go (+1/-0)
provider/local/export_test.go (+2/-1)
provider/local/net.go (+3/-19)
provider/maas/environ.go (+2/-0)
provider/maas/environprovider.go (+9/-0)
utils/network.go (+20/-0)
worker/deployer/simple.go (+3/-2)
To merge this branch: bzr merge lp:~thumper/juju-core/container-address
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+182271@code.launchpad.net

Commit message

Get the address correctly from the container.

This is kind of messy, but it does work.
This uses an extra environment variable from the upstart
script to identify the type of container. MAAS then uses
this when trying to work out the hostname of the machine.

We do need a nicer way to get this done. We also want to
move the environment variables from the upstart script
into the agent config, but this will be a follow up branch.

https://codereview.appspot.com/13261044/

Description of the change

Get the address correctly from the container.

This is kind of messy, but it does work.
This uses an extra environment variable from the upstart
script to identify the type of container. MAAS then uses
this when trying to work out the hostname of the machine.

We do need a nicer way to get this done. We also want to
move the environment variables from the upstart script
into the agent config, but this will be a follow up branch.

https://codereview.appspot.com/13261044/

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

Reviewers: mp+182271_code.launchpad.net,

Message:
Please take a look.

Description:
Get the address correctly from the container.

This is kind of messy, but it does work.
This uses an extra environment variable from the upstart
script to identify the type of container. MAAS then uses
this when trying to work out the hostname of the machine.

We do need a nicer way to get this done. We also want to
move the environment variables from the upstart script
into the agent config, but this will be a follow up branch.

https://code.launchpad.net/~thumper/juju-core/container-address/+merge/182271

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M container/lxc/lxc.go
   M juju/osenv/vars.go
   M provider/local/export_test.go
   M provider/local/net.go
   M provider/maas/environprovider.go
   M utils/network.go
   M worker/deployer/simple.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: <email address hidden>
+New revision: <email address hidden>

Index: container/lxc/lxc.go
=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go 2013-08-21 05:38:38 +0000
+++ container/lxc/lxc.go 2013-08-27 04:29:03 +0000
@@ -19,6 +19,7 @@
   "launchpad.net/juju-core/environs/cloudinit"
   "launchpad.net/juju-core/environs/config"
   "launchpad.net/juju-core/instance"
+ "launchpad.net/juju-core/juju/osenv"
   "launchpad.net/juju-core/names"
   "launchpad.net/juju-core/state"
   "launchpad.net/juju-core/state/api"
@@ -339,6 +340,7 @@
   if err := environs.FinishMachineConfig(machineConfig, environConfig,
constraints.Value{}); err != nil {
    return nil, err
   }
+ machineConfig.MachineEnvironment[osenv.JujuContainerType] =
string(instance.LXC)
   cloudConfig, err := cloudinit.New(machineConfig)
   if err != nil {
    return nil, err

Index: juju/osenv/vars.go
=== modified file 'juju/osenv/vars.go'
--- juju/osenv/vars.go 2013-07-31 05:16:47 +0000
+++ juju/osenv/vars.go 2013-08-01 01:13:46 +0000
@@ -9,6 +9,7 @@
   JujuRepository = "JUJU_REPOSITORY"
   JujuLxcBridge = "JUJU_LXC_BRIDGE"
   JujuProviderType = "JUJU_PROVIDER_TYPE"
+ JujuContainerType = "JUJU_CONTAINER_TYPE"
   JujuStorageDir = "JUJU_STORAGE_DIR"
   JujuStorageAddr = "JUJU_STORAGE_ADDR"
   JujuSharedStorageDir = "JUJU_SHARED_STORAGE_DIR"

Index: provider/local/export_test.go
=== modified file 'provider/local/export_test.go'
--- provider/local/export_test.go 2013-07-17 05:09:42 +0000
+++ provider/local/export_test.go 2013-08-01 01:13:46 +0000
@@ -6,6 +6,7 @@
   gc "launchpad.net/gocheck"

   "launchpad.net/juju-core/environs/config"
+ "launchpad.net/juju-core/utils"
  )

  var (
@@ -62,6 +63,6 @@
    return "127.0.0.1", nil
   }
   return func() {
- getAddressForInterface = getAddressForInterfaceImpl
+ getAddressForInterface = utils.GetAddressForInterface
   }
  }

Index: provider/local/net.go
=== modified file 'provider/local/net.g...

Read more...

Revision history for this message
John A Meinel (jameinel) wrote :

I fully support moving away from environment variables into doing this
properly via something like "agent.conf".

However, I'd rather see us have containers working on MaaS today than
wait for agent.conf refactoring, so this seems ok to me.

Can you add a comment to the "machineConfig.MachineEnvironment" line, to
make it stand out a bit more that we want to change how that works?

Everything else seems pretty straightforward. I wonder if we want a
basic smoke test for utils.GetAddressForInterface.

We could ask for the details of "eth0" and test that we either get a
meaningful error back, or we get an ipv4 address.

LGTM

https://codereview.appspot.com/13261044/

Revision history for this message
William Reade (fwereade) wrote :

LGTM. Are we now settled on using EnvironProvider methods internally to
get addresses across the board? I forget some of the discussions...

https://codereview.appspot.com/13261044/

Revision history for this message
Tim Penhey (thumper) wrote :

On 2013/08/27 06:06:36, jameinel wrote:
> I fully support moving away from environment variables into doing this
 properly
> via something like "agent.conf".

> However, I'd rather see us have containers working on MaaS today than
wait for
> agent.conf refactoring, so this seems ok to me.

> Can you add a comment to the "machineConfig.MachineEnvironment" line,
to make it
> stand out a bit more that we want to change how that works?

OK.

> Everything else seems pretty straightforward. I wonder if we want a
basic smoke
> test for utils.GetAddressForInterface.

We do have tests around the methods that GetAddressForInterface uses.

https://codereview.appspot.com/13261044/

Revision history for this message
Tim Penhey (thumper) wrote :

On 2013/08/27 14:36:18, fwereade wrote:
> LGTM. Are we now settled on using EnvironProvider methods internally
 to get
> addresses across the board? I forget some of the discussions...

I'm not entirely sure, but since I'm still waiting to hear back on a lot
of the addressability issues, this will work for now.

https://codereview.appspot.com/13261044/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (48.8 KiB)

The attempt to merge lp:~thumper/juju-core/container-address into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.721s
ok launchpad.net/juju-core/agent/tools 0.253s
ok launchpad.net/juju-core/bzr 6.628s
ok launchpad.net/juju-core/cert 2.179s
ok launchpad.net/juju-core/charm 0.576s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.021s
ok launchpad.net/juju-core/cmd 0.228s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 114.955s
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x628a9d]

goroutine 743 [running]:
sync/atomic.CompareAndSwapUint32()
 /usr/lib/go/src/pkg/sync/atomic/asm_amd64.s:14 +0xd
sync.(*Mutex).Lock(0x0)
 /usr/lib/go/src/pkg/sync/mutex.go:43 +0x35
launchpad.net/tomb.(*Tomb).init(0x0)
 /home/tarmac/trees/src/src/launchpad.net/tomb/tomb.go:83 +0x25
launchpad.net/tomb.(*Tomb).Kill(0x0, 0x0, 0x0)
 /home/tarmac/trees/src/src/launchpad.net/tomb/tomb.go:135 +0x25
launchpad.net/juju-core/state/api/watcher.(*commonWatcher).Stop(0x0, 0xc2005f5540, 0x0)
 /home/tarmac/trees/src/launchpad.net/juju-core/state/api/watcher/watcher.go:103 +0x31
launchpad.net/juju-core/worker.(*notifyWorker).loop(0xc200492340, 0x0, 0x0)
 /home/tarmac/trees/src/launchpad.net/juju-core/worker/notifyworker.go:86 +0x7e
launchpad.net/juju-core/worker.func·002()
 /home/tarmac/trees/src/launchpad.net/juju-core/worker/notifyworker.go:53 +0x4e
created by launchpad.net/juju-core/worker.NewNotifyWorker
 /home/tarmac/trees/src/launchpad.net/juju-core/worker/notifyworker.go:54 +0xa1

goroutine 1 [chan receive]:
testing.RunTests(0xd999f0, 0x1229700, 0x2, 0x2, 0xc2001af701, ...)
 /usr/lib/go/src/pkg/testing/testing.go:434 +0x88e
testing.Main(0xd999f0, 0x1229700, 0x2, 0x2, 0x1233ea0, ...)
 /usr/lib/go/src/pkg/testing/testing.go:365 +0x8a
main.main()
 launchpad.net/juju-core/cmd/jujud/_test/_testmain.go:45 +0x9a

goroutine 2 [syscall]:

goroutine 5 [chan receive]:
launchpad.net/juju-core/provider/dummy.func·001()
 /home/tarmac/trees/src/launchpad.net/juju-core/provider/dummy/environs.go:177 +0x3a
created by launchpad.net/juju-core/provider/dummy.init·1
 /home/tarmac/trees/src/launchpad.net/juju-core/provider/dummy/environs.go:179 +0xc1

goroutine 7 [chan receive]:
launchpad.net/gocheck.(*suiteRunner).runTest(0xc20060a690, 0xc200650070, 0xc2002575a0)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/gocheck.go:771 +0x4f
launchpad.net/gocheck.(*suiteRunner).run(0xc20060a690, 0xc2001db280)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/gocheck.go:584 +0x1c5
launchpad.net/gocheck.Run(0xc22080, 0xc2001db280, 0xc20030afc0, 0xc200206300)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/run.go:76 +0x47
launchpad.net/gocheck.RunAll(0xc20030afc0, 0xc20030afc0)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/run.go:68 +0xc2
launchpad.net/gocheck.TestingT(0xc2001af870)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/run.go:56 +0x2a2
launchpad.net/juju-core/...

Revision history for this message
John A Meinel (jameinel) wrote :

I think the failure to land here was just a problem on the bot which should have been fixed by Roger's recent change to always require an interface rather than sometimes a pointer-to-struct. So I'm resubmitting it.

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (15.7 KiB)

The attempt to merge lp:~thumper/juju-core/container-address into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.717s
ok launchpad.net/juju-core/agent/tools 0.245s
ok launchpad.net/juju-core/bzr 6.890s
ok launchpad.net/juju-core/cert 2.262s
ok launchpad.net/juju-core/charm 0.628s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.021s
ok launchpad.net/juju-core/cmd 0.219s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 115.193s

----------------------------------------------------------------------
FAIL: machine_test.go:377: MachineSuite.TestManageStateServesAPI

[LOG] 9.43377 INFO juju environs/testing: uploading FAKE tools 1.13.3-precise-amd64
[LOG] 9.45092 INFO juju.environs.boostrap bootstrapping environment "dummyenv"
[LOG] 9.45095 INFO juju.environs.tools reading tools with major version 1
[LOG] 9.45097 INFO juju.environs.tools filtering tools by version: 1.13.3
[LOG] 9.45098 INFO juju.environs.tools filtering tools by series: precise
[LOG] 9.45099 DEBUG juju.environs.tools reading v1.* tools
[LOG] 9.45100 INFO juju.environs.tools falling back to public bucket
[LOG] 9.45101 DEBUG juju.environs.tools reading v1.* tools
[LOG] 9.45105 DEBUG juju.environs.tools found 1.13.3-precise-amd64
[LOG] 9.45109 INFO juju environs/dummy: would pick tools from 1.13.3-precise-amd64
[LOG] 9.47397 INFO juju.state opening state; mongo addresses: ["localhost:35366"]; entity ""
[LOG] 9.49584 INFO juju.state connection established
[LOG] 9.52798 INFO juju.state initializing environment
[LOG] 9.54999 INFO juju state/api: listening on "127.0.0.1:60041"
[LOG] 9.56651 INFO juju.state opening state; mongo addresses: ["localhost:35366"]; entity ""
[LOG] 9.57148 INFO juju.state connection established
[LOG] 9.58529 INFO juju juju: authorization error while connecting to state server; retrying
[LOG] 9.58538 INFO juju.state opening state; mongo addresses: ["localhost:35366"]; entity ""
[LOG] 9.58952 INFO juju.state connection established
[LOG] 9.62028 INFO juju state/api: dialing "wss://127.0.0.1:60041/"
[LOG] 9.62396 INFO juju state/api: connection established
[LOG] 9.62421 DEBUG juju rpc/jsoncodec: <- {"RequestId":1,"Type":"Admin","Request":"Login","Params":{"AuthTag":"user-admin","Password":"dummy-secret","Nonce":""}}
[LOG] 9.62486 DEBUG juju rpc/jsoncodec: -> {"RequestId":1,"Response":{}}
[LOG] 9.62529 INFO juju.container.lxc lxcObjectFactory replaced with mock lxc factory
[LOG] 9.62680 DEBUG juju rpc/jsoncodec: <- {"RequestId":2,"Type":"Pinger","Request":"Ping","Params":{}}
[LOG] 9.62691 DEBUG juju rpc/jsoncodec: -> {"RequestId":2,"Response":{}}
[LOG] 9.70798 INFO juju machine agent machine-0 start
[LOG] 9.72310 INFO juju Starting StateWorker for machine-0
[LOG] 9.72315 INFO juju worker: start "state"
[LOG] 9.72317 INFO juju.state opening state; mongo addresses: ["localhost:35366"]; entity "machine-0"
[LOG] 9.72343 INFO juju worker: start "api"
[LOG] 9.72363 INFO juju state/api: dialing "wss://lo...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'container/lxc/lxc.go'
2--- container/lxc/lxc.go 2013-08-21 05:38:38 +0000
3+++ container/lxc/lxc.go 2013-08-27 23:08:19 +0000
4@@ -19,6 +19,7 @@
5 "launchpad.net/juju-core/environs/cloudinit"
6 "launchpad.net/juju-core/environs/config"
7 "launchpad.net/juju-core/instance"
8+ "launchpad.net/juju-core/juju/osenv"
9 "launchpad.net/juju-core/names"
10 "launchpad.net/juju-core/state"
11 "launchpad.net/juju-core/state/api"
12@@ -339,6 +340,9 @@
13 if err := environs.FinishMachineConfig(machineConfig, environConfig, constraints.Value{}); err != nil {
14 return nil, err
15 }
16+ // TODO(thumper): 2013-08-28 bug 1217614
17+ // The machine envronment config values are being moved to the agent config.
18+ machineConfig.MachineEnvironment[osenv.JujuContainerType] = string(instance.LXC)
19 cloudConfig, err := cloudinit.New(machineConfig)
20 if err != nil {
21 return nil, err
22
23=== modified file 'juju/osenv/vars.go'
24--- juju/osenv/vars.go 2013-07-31 05:16:47 +0000
25+++ juju/osenv/vars.go 2013-08-27 23:08:19 +0000
26@@ -9,6 +9,7 @@
27 JujuRepository = "JUJU_REPOSITORY"
28 JujuLxcBridge = "JUJU_LXC_BRIDGE"
29 JujuProviderType = "JUJU_PROVIDER_TYPE"
30+ JujuContainerType = "JUJU_CONTAINER_TYPE"
31 JujuStorageDir = "JUJU_STORAGE_DIR"
32 JujuStorageAddr = "JUJU_STORAGE_ADDR"
33 JujuSharedStorageDir = "JUJU_SHARED_STORAGE_DIR"
34
35=== modified file 'provider/local/export_test.go'
36--- provider/local/export_test.go 2013-07-17 05:09:42 +0000
37+++ provider/local/export_test.go 2013-08-27 23:08:19 +0000
38@@ -6,6 +6,7 @@
39 gc "launchpad.net/gocheck"
40
41 "launchpad.net/juju-core/environs/config"
42+ "launchpad.net/juju-core/utils"
43 )
44
45 var (
46@@ -62,6 +63,6 @@
47 return "127.0.0.1", nil
48 }
49 return func() {
50- getAddressForInterface = getAddressForInterfaceImpl
51+ getAddressForInterface = utils.GetAddressForInterface
52 }
53 }
54
55=== modified file 'provider/local/net.go'
56--- provider/local/net.go 2013-07-17 05:09:42 +0000
57+++ provider/local/net.go 2013-08-27 23:08:19 +0000
58@@ -3,25 +3,9 @@
59 package local
60
61 import (
62- "net"
63-
64 "launchpad.net/juju-core/utils"
65 )
66
67-var getAddressForInterface = getAddressForInterfaceImpl
68-
69-// getAddressForInterfaceImpl looks for the network interface
70-// and returns the IPv4 address from the possible addresses.
71-func getAddressForInterfaceImpl(interfaceName string) (string, error) {
72- iface, err := net.InterfaceByName(interfaceName)
73- if err != nil {
74- logger.Errorf("cannot find network interface %q: %v", interfaceName, err)
75- return "", err
76- }
77- addrs, err := iface.Addrs()
78- if err != nil {
79- logger.Errorf("cannot get addresses for network interface %q: %v", interfaceName, err)
80- return "", err
81- }
82- return utils.GetIPv4Address(addrs)
83-}
84+// getAddressForInterface is a variable so we can change the implementation
85+// for testing purposes.
86+var getAddressForInterface = utils.GetAddressForInterface
87
88=== modified file 'provider/maas/environ.go'
89--- provider/maas/environ.go 2013-08-27 04:16:59 +0000
90+++ provider/maas/environ.go 2013-08-27 23:08:19 +0000
91@@ -249,6 +249,8 @@
92 if err := environs.FinishMachineConfig(machineConfig, environ.Config(), cons); err != nil {
93 return nil, nil, err
94 }
95+ // TODO(thumper): 2013-08-28 bug 1217614
96+ // The machine envronment config values are being moved to the agent config.
97 // Explicitly specify that the lxc containers use the network bridge defined above.
98 machineConfig.MachineEnvironment[osenv.JujuLxcBridge] = "br0"
99 userdata, err := environs.ComposeUserData(
100
101=== modified file 'provider/maas/environprovider.go'
102--- provider/maas/environprovider.go 2013-08-24 13:50:05 +0000
103+++ provider/maas/environprovider.go 2013-08-27 23:08:19 +0000
104@@ -4,10 +4,15 @@
105 package maas
106
107 import (
108+ "os"
109+
110 "launchpad.net/loggo"
111
112 "launchpad.net/juju-core/environs"
113 "launchpad.net/juju-core/environs/config"
114+ "launchpad.net/juju-core/instance"
115+ "launchpad.net/juju-core/juju/osenv"
116+ "launchpad.net/juju-core/utils"
117 )
118
119 // Logger for the MAAS provider.
120@@ -68,6 +73,10 @@
121 }
122
123 func (maasEnvironProvider) hostname() (string, error) {
124+ // Hack to get container ip addresses properly for MAAS demo.
125+ if os.Getenv(osenv.JujuContainerType) == string(instance.LXC) {
126+ return utils.GetAddressForInterface("eth0")
127+ }
128 info := machineInfo{}
129 err := info.load()
130 if err != nil {
131
132=== modified file 'utils/network.go'
133--- utils/network.go 2013-07-08 02:54:38 +0000
134+++ utils/network.go 2013-08-27 23:08:19 +0000
135@@ -6,8 +6,12 @@
136 import (
137 "fmt"
138 "net"
139+
140+ "launchpad.net/loggo"
141 )
142
143+var logger = loggo.GetLogger("juju.utils")
144+
145 // GetIPv4Address iterates through the addresses expecting the format from
146 // func (ifi *net.Interface) Addrs() ([]net.Addr, error)
147 func GetIPv4Address(addresses []net.Addr) (string, error) {
148@@ -24,3 +28,19 @@
149 }
150 return "", fmt.Errorf("no addresses match")
151 }
152+
153+// GetAddressForInterface looks for the network interface
154+// and returns the IPv4 address from the possible addresses.
155+func GetAddressForInterface(interfaceName string) (string, error) {
156+ iface, err := net.InterfaceByName(interfaceName)
157+ if err != nil {
158+ logger.Errorf("cannot find network interface %q: %v", interfaceName, err)
159+ return "", err
160+ }
161+ addrs, err := iface.Addrs()
162+ if err != nil {
163+ logger.Errorf("cannot get addresses for network interface %q: %v", interfaceName, err)
164+ return "", err
165+ }
166+ return GetIPv4Address(addrs)
167+}
168
169=== modified file 'worker/deployer/simple.go'
170--- worker/deployer/simple.go 2013-08-22 03:39:33 +0000
171+++ worker/deployer/simple.go 2013-08-27 23:08:19 +0000
172@@ -141,9 +141,10 @@
173 Desc: "juju unit agent for " + unitName,
174 Cmd: cmd,
175 Out: logPath,
176- // Propagate the provider type enviroment variable.
177+ // Propagate the provider type and container type enviroment variables.
178 Env: map[string]string{
179- osenv.JujuProviderType: os.Getenv(osenv.JujuProviderType),
180+ osenv.JujuProviderType: os.Getenv(osenv.JujuProviderType),
181+ osenv.JujuContainerType: os.Getenv(osenv.JujuContainerType),
182 },
183 }
184 return uconf.Install()

Subscribers

People subscribed via source and target branches

to status/vote changes: