Merge lp:~themue/golxc/003-added-network into lp:golxc

Proposed by Frank Mueller on 2012-11-26
Status: Merged
Merged at revision: 4
Proposed branch: lp:~themue/golxc/003-added-network
Merge into: lp:golxc
Prerequisite: lp:~themue/golxc/002-added-configuration-reading
Diff against target: 300 lines (+182/-18)
3 files modified
golxc.go (+16/-12)
golxc_test.go (+80/-6)
network.go (+86/-0)
To merge this branch: bzr merge lp:~themue/golxc/003-added-network
Reviewer Review Type Date Requested Status
Frank Mueller Pending
Review via email: mp+136220@code.launchpad.net

Description of the change

golxc: added networking

Added the initial functionallity to start and stop the
LXC networking, to retrieve vital data and to test if
the networking is running.

https://codereview.appspot.com/6849102/

To post a comment you must log in.
lp:~themue/golxc/003-added-network updated on 2012-11-27
6. By Frank Mueller on 2012-11-27

golxc: minor changes after review

7. By Frank Mueller on 2012-11-27

golxc: merged prerequisite, missed it before

William Reade (fwereade) wrote :

I really think we should have tests that run without sudo -- so, just
verifying what commands we call -- that we can run for sanity's sake
before exercising the package against our own machines.

And, for the tests that do hit the real system, I feel we should drop
golxc_test.sh and use something that lets us run "go test ./..." for the
unit tests and `sudo go test ./... -sudo` (or similar) to run the real
tests as well.

A few comments below

https://codereview.appspot.com/6849102/diff/5002/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6849102/diff/5002/golxc.go#newcode311
golxc.go:311: out, err := cmd.CombinedOutput()
Again, I'm not sure about CombinedOutput. If it's sane, there should be
a comment explaining why the streams cannot pollute one another in
confusing ways.

https://codereview.appspot.com/6849102/diff/5002/golxc_test.go
File golxc_test.go (right):

https://codereview.appspot.com/6849102/diff/5002/golxc_test.go#newcode140
golxc_test.go:140: f, err := ioutil.TempFile("/tmp", "lxc-test")
c.MkDir()

https://codereview.appspot.com/6849102/diff/5002/golxc_test.go#newcode142
golxc_test.go:142: defer func() {
...then you can drop this defer, the test will clean up for you.

https://codereview.appspot.com/6849102/diff/5002/golxc_test.go#newcode160
golxc_test.go:160: orig :=
golxc.SetLXCDefaultFile("/tmp/not-existing-exc-test")
c.MkDir()

https://codereview.appspot.com/6849102/

lp:~themue/golxc/003-added-network updated on 2013-01-31
8. By Frank Mueller on 2013-01-31

golxc: changes after review

William Reade (fwereade) wrote :

Couple of minors

https://codereview.appspot.com/6849102/diff/5002/golxc_test.go
File golxc_test.go (right):

https://codereview.appspot.com/6849102/diff/5002/golxc_test.go#newcode160
golxc_test.go:160: orig :=
golxc.SetLXCDefaultFile("/tmp/not-existing-exc-test")
On 2013/01/31 17:14:22, TheMue wrote:
> On 2013/01/30 16:44:58, fwereade wrote:
> > c.MkDir()

> No file written, only name of a non-existing file.

How do you know it doesn't exist if you don't control the directory?

https://codereview.appspot.com/6849102/diff/11001/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6849102/diff/11001/golxc.go#newcode318
golxc.go:318: }
And if there was no error, but extra stuff was printed to stderr, we'd
just hand it back below... so the comment doesn't quite address my
concern, which is about getting unwanted output but no error.

https://codereview.appspot.com/6849102/diff/11001/golxc_test.go
File golxc_test.go (right):

https://codereview.appspot.com/6849102/diff/11001/golxc_test.go#newcode164
golxc_test.go:164: orig :=
golxc.SetLXCDefaultFile("/any/path/to/non-existing/lxc-file")
I don't think you can guarantee this path doesn't exist either ;).

https://codereview.appspot.com/6849102/

lp:~themue/golxc/003-added-network updated on 2013-02-05
9. By Frank Mueller on 2013-02-05

golxc: changes after review

William Reade (fwereade) wrote :

Sorry to keep on about these things :(. I'm ok to say LGTM in the
interest of progress, assuming the points raised below are addressed or
refuted :). Chat to me if there's doubt.

https://codereview.appspot.com/6849102/diff/11001/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6849102/diff/11001/golxc.go#newcode314
golxc.go:314: // analyzing it in runError() in case of an error.
// LXC tools do not use stdout and stderr in a predictable
// way; based on experimentation, the most convenient
// solution is to combine them and leave the client to
// determine sanity as best it can.

https://codereview.appspot.com/6849102/diff/11001/golxc.go#newcode318
golxc.go:318: }
On 2013/02/05 14:21:32, TheMue wrote:
> On 2013/02/04 14:51:30, fwereade wrote:
> > And if there was no error, but extra stuff was printed to stderr,
we'd just
> hand
> > it back below... so the comment doesn't quite address my concern,
which is
> about
> > getting unwanted output but no error.

> That's exactly the problem. Some of the lxc commands simply use stdout
for both
> types, positive and error messages, others use stderr. So far I've
never seen
> mixed output and also signalling an error seems to work correctly.

> How would you write this as comment? Simply longer and more explicit?

Suggested above?

https://codereview.appspot.com/6849102/diff/11001/golxc.go#newcode326
golxc.go:326: if strings.HasPrefix(l, name+": ") {
I'm not sure what we get out of hiding parts of the output in the case
of error. Comment perhaps?

https://codereview.appspot.com/6849102/diff/18001/golxc_test.go
File golxc_test.go (right):

https://codereview.appspot.com/6849102/diff/18001/golxc_test.go#newcode173
golxc_test.go:173: // nonExistingPath returns a non-existing file path.
I'm not sure how this solution is superior to a
`filepath.Join(c.MkDir(), "foo")` at the call site.

https://codereview.appspot.com/6849102/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'golxc.go'
--- golxc.go 2012-11-23 14:26:38 +0000
+++ golxc.go 2013-02-05 14:23:18 +0000
@@ -76,11 +76,11 @@
7676
77// List lists the existing containers on the system.77// List lists the existing containers on the system.
78func List() ([]*Container, error) {78func List() ([]*Container, error) {
79 stdout, err := run("lxc-ls", "-1")79 out, err := run("lxc-ls", "-1")
80 if err != nil {80 if err != nil {
81 return nil, err81 return nil, err
82 }82 }
83 names := nameSet(stdout)83 names := nameSet(out)
84 containers := make([]*Container, len(names))84 containers := make([]*Container, len(names))
85 for i, name := range names {85 for i, name := range names {
86 containers[i] = New(name)86 containers[i] = New(name)
@@ -254,11 +254,11 @@
254254
255// Info returns the status and the process id of the container.255// Info returns the status and the process id of the container.
256func (c *Container) Info() (State, int, error) {256func (c *Container) Info() (State, int, error) {
257 stdout, err := run("lxc-info", "-n", c.name)257 out, err := run("lxc-info", "-n", c.name)
258 if err != nil {258 if err != nil {
259 return StateUnknown, -1, err259 return StateUnknown, -1, err
260 }260 }
261 kv := keyValues(stdout, ": ")261 kv := keyValues(out, ": ")
262 state := State(kv["state"])262 state := State(kv["state"])
263 pid, err := strconv.Atoi(kv["pid"])263 pid, err := strconv.Atoi(kv["pid"])
264 if err != nil {264 if err != nil {
@@ -305,20 +305,24 @@
305 return c.containerHome() + "/rootfs/"305 return c.containerHome() + "/rootfs/"
306}306}
307307
308// run executes the passed command and returns the output.308// run executes the passed command and returns the out.
309func run(name string, args ...string) (string, error) {309func run(name string, args ...string) (string, error) {
310 cmd := exec.Command(name, args...)310 cmd := exec.Command(name, args...)
311 output, err := cmd.CombinedOutput()311 // LXC message and error printing is inconsistent
312 // using stdout and stderr mixed and in different
313 // message formats. So using CombinedOutput() and
314 // analyzing it in runError() in case of an error.
315 out, err := cmd.CombinedOutput()
312 if err != nil {316 if err != nil {
313 return "", runError(name, err, output)317 return "", runError(name, err, out)
314 }318 }
315 return string(output), nil319 return string(out), nil
316}320}
317321
318// runError creates an error if run fails.322// runError creates an error if run fails.
319func runError(name string, err error, output []byte) error {323func runError(name string, err error, out []byte) error {
320 e := &Error{name, err, nil}324 e := &Error{name, err, nil}
321 for _, l := range strings.Split(string(output), "\n") {325 for _, l := range strings.Split(string(out), "\n") {
322 if strings.HasPrefix(l, name+": ") {326 if strings.HasPrefix(l, name+": ") {
323 l = l[len(name)+2:]327 l = l[len(name)+2:]
324 }328 }
@@ -329,7 +333,7 @@
329 return e333 return e
330}334}
331335
332// keyValues retrieves key/value pairs out of a command output.336// keyValues retrieves key/value pairs out of a command out.
333func keyValues(raw string, sep string) map[string]string {337func keyValues(raw string, sep string) map[string]string {
334 kv := map[string]string{}338 kv := map[string]string{}
335 lines := strings.Split(raw, "\n")339 lines := strings.Split(raw, "\n")
@@ -342,7 +346,7 @@
342 return kv346 return kv
343}347}
344348
345// nameSet retrieves a set of names out of a command output.349// nameSet retrieves a set of names out of a command out.
346func nameSet(raw string) []string {350func nameSet(raw string) []string {
347 collector := map[string]struct{}{}351 collector := map[string]struct{}{}
348 set := []string{}352 set := []string{}
349353
=== modified file 'golxc_test.go'
--- golxc_test.go 2013-02-05 14:23:18 +0000
+++ golxc_test.go 2013-02-05 14:23:18 +0000
@@ -1,13 +1,16 @@
1package golxc_test1package golxc_test
22
3import (3import (
4 "fmt"
4 "io/ioutil"5 "io/ioutil"
5 . "launchpad.net/gocheck"6 . "launchpad.net/gocheck"
6 "launchpad.net/golxc"
7 "os"7 "os"
8 "os/user"8 "os/user"
9 "path/filepath"
9 "strings"10 "strings"
10 "testing"11 "testing"
12
13 "launchpad.net/golxc"
11)14)
1215
13func Test(t *testing.T) { TestingT(t) }16func Test(t *testing.T) { TestingT(t) }
@@ -74,10 +77,10 @@
74# LXC_BRIDGE and LXC_ADDR are changed against original for77# LXC_BRIDGE and LXC_ADDR are changed against original for
75# testing purposes.78# testing purposes.
76LXC_BRIDGE="lxcbr9"79LXC_BRIDGE="lxcbr9"
77LXC_ADDR="10.0.9.9"80LXC_ADDR="10.0.9.1"
78LXC_NETMASK="255.255.255.0"81LXC_NETMASK="255.255.255.0"
79LXC_NETWORK="10.0.3.0/24"82LXC_NETWORK="10.0.9.0/24"
80LXC_DHCP_RANGE="10.0.3.2,10.0.3.254"83LXC_DHCP_RANGE="10.0.9.2,10.0.9.254"
81LXC_DHCP_MAX="253"84LXC_DHCP_MAX="253"
8285
83LXC_SHUTDOWN_TIMEOUT=120`86LXC_SHUTDOWN_TIMEOUT=120`
@@ -99,19 +102,90 @@
99 env, err := golxc.ReadDefaultEnvironment()102 env, err := golxc.ReadDefaultEnvironment()
100 c.Assert(err, IsNil)103 c.Assert(err, IsNil)
101 c.Assert(env["LXC_BRIDGE"], Equals, "lxcbr9")104 c.Assert(env["LXC_BRIDGE"], Equals, "lxcbr9")
102 c.Assert(env["LXC_ADDR"], Equals, "10.0.9.9")105 c.Assert(env["LXC_ADDR"], Equals, "10.0.9.1")
103 c.Assert(env["MIRROR"], Equals, "")106 c.Assert(env["MIRROR"], Equals, "")
104}107}
105108
106func (s *ConfigurationSuite) TestReadNotExistingDefaultEnvironment(c *C) {109func (s *ConfigurationSuite) TestReadNotExistingDefaultEnvironment(c *C) {
107 // Test reading a not existing environment.110 // Test reading a not existing environment.
108 orig := golxc.SetLXCDefaultFile("/tmp/not-existing-exc-test")111 orig := golxc.SetLXCDefaultFile(nonExistingPath())
109 defer golxc.SetLXCDefaultFile(orig)112 defer golxc.SetLXCDefaultFile(orig)
110113
111 _, err := golxc.ReadDefaultEnvironment()114 _, err := golxc.ReadDefaultEnvironment()
112 c.Assert(err, ErrorMatches, "open .*: no such file or directory")115 c.Assert(err, ErrorMatches, "open .*: no such file or directory")
113}116}
114117
118type NetworkSuite struct{}
119
120var _ = Suite(&NetworkSuite{})
121
122func (s *NetworkSuite) SetUpSuite(c *C) {
123 u, err := user.Current()
124 c.Assert(err, IsNil)
125 if u.Uid != "0" {
126 // Has to be run as root!
127 c.Skip("tests must run as root")
128 }
129}
130
131func (s *NetworkSuite) TestStartStopNetwork(c *C) {
132 // Test starting and stoping of the LXC network.
133 initialRunning, err := golxc.IsNetworkRunning()
134 c.Assert(err, IsNil)
135 defer func() {
136 if initialRunning {
137 c.Assert(golxc.StartNetwork(), IsNil)
138 }
139 }()
140 c.Assert(golxc.StartNetwork(), IsNil)
141 running, err := golxc.IsNetworkRunning()
142 c.Assert(err, IsNil)
143 c.Assert(running, Equals, true)
144 c.Assert(golxc.StopNetwork(), IsNil)
145 running, err = golxc.IsNetworkRunning()
146 c.Assert(err, IsNil)
147 c.Assert(running, Equals, false)
148}
149
150func (s *ConfigurationSuite) TestNetworkAttributes(c *C) {
151 // Test reading the network attribute form an environment.
152 cf := filepath.Join(c.MkDir(), "lxc-test")
153 c.Assert(ioutil.WriteFile(cf, []byte(env), 0555), IsNil)
154
155 orig := golxc.SetLXCDefaultFile(cf)
156 defer golxc.SetLXCDefaultFile(orig)
157
158 addr, bridge, err := golxc.NetworkAttributes()
159 c.Assert(err, IsNil)
160 c.Assert(addr, Equals, "10.0.9.1")
161 c.Assert(bridge, Equals, "lxcbr9")
162}
163
164func (s *NetworkSuite) TestNotExistingNetworkAttributes(c *C) {
165 // Test reading of network attributes from a not existing environment.
166 orig := golxc.SetLXCDefaultFile(nonExistingPath())
167 defer golxc.SetLXCDefaultFile(orig)
168
169 _, _, err := golxc.NetworkAttributes()
170 c.Assert(err, ErrorMatches, "open .*: no such file or directory")
171}
172
173// nonExistingPath returns a non-existing file path.
174func nonExistingPath() string {
175 base := "/any/path/to/non-existing/lxc-file-%d"
176 index := 0
177 for {
178 path := fmt.Sprintf(base, index)
179 f, err := os.Open(path)
180 if os.IsNotExist(err) {
181 return path
182 }
183 f.Close()
184 index++
185 }
186 panic("unreachable")
187}
188
115type LXCSuite struct{}189type LXCSuite struct{}
116190
117var _ = Suite(&LXCSuite{})191var _ = Suite(&LXCSuite{})
118192
=== added file 'network.go'
--- network.go 1970-01-01 00:00:00 +0000
+++ network.go 2013-02-05 14:23:18 +0000
@@ -0,0 +1,86 @@
1package golxc
2
3import (
4 "fmt"
5 "strings"
6)
7
8const (
9 defaultAddr = "10.0.3.1"
10 defaultBridge = "lxcbr0"
11)
12
13// StartNetwork starts the lxc network subsystem.
14func StartNetwork() error {
15 goal, _, err := networkStatus()
16 if err != nil {
17 return err
18 }
19 if goal != "start" {
20 _, err := run("start", "lxc-net")
21 if err != nil {
22 return err
23 }
24 }
25 return nil
26}
27
28// StopNetwork stops the lxc network subsystem.
29func StopNetwork() error {
30 goal, _, err := networkStatus()
31 if err != nil {
32 return err
33 }
34 if goal != "stop" {
35 _, err := run("stop", "lxc-net")
36 if err != nil {
37 return err
38 }
39 }
40 return nil
41}
42
43// IsNotworkRunning checks if the lxc network subsystem
44// is running.
45func IsNetworkRunning() (bool, error) {
46 _, status, err := networkStatus()
47 if err != nil {
48 return false, err
49 }
50 return status == "running", nil
51}
52
53// NetworkAttributes returns the lxc network attributes:
54// starting IP address and bridge name.
55func NetworkAttributes() (addr, bridge string, err error) {
56 env, err := ReadDefaultEnvironment()
57 if err != nil {
58 return "", "", err
59 }
60 addr = env["LXC_ADDR"]
61 if addr == "" {
62 addr = defaultAddr
63 }
64 bridge = env["LXC_BRIDGE"]
65 if bridge == "" {
66 bridge = defaultBridge
67 }
68 return addr, bridge, nil
69}
70
71// networkStatus returns the status of the lxc network subsystem.
72func networkStatus() (goal, status string, err error) {
73 output, err := run("status", "lxc-net")
74 if err != nil {
75 return "", "", err
76 }
77 fields := strings.Fields(output)
78 if len(fields) != 2 {
79 return "", "", fmt.Errorf("unexpected status output: %q", output)
80 }
81 fields = strings.Split(fields[1], "/")
82 if len(fields) != 2 {
83 return "", "", fmt.Errorf("unexpected status output: %q", output)
84 }
85 return fields[0], fields[1], nil
86}

Subscribers

People subscribed via source and target branches

to all changes: