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
1=== modified file 'golxc.go'
2--- golxc.go 2012-11-23 14:26:38 +0000
3+++ golxc.go 2013-02-05 14:23:18 +0000
4@@ -76,11 +76,11 @@
5
6 // List lists the existing containers on the system.
7 func List() ([]*Container, error) {
8- stdout, err := run("lxc-ls", "-1")
9+ out, err := run("lxc-ls", "-1")
10 if err != nil {
11 return nil, err
12 }
13- names := nameSet(stdout)
14+ names := nameSet(out)
15 containers := make([]*Container, len(names))
16 for i, name := range names {
17 containers[i] = New(name)
18@@ -254,11 +254,11 @@
19
20 // Info returns the status and the process id of the container.
21 func (c *Container) Info() (State, int, error) {
22- stdout, err := run("lxc-info", "-n", c.name)
23+ out, err := run("lxc-info", "-n", c.name)
24 if err != nil {
25 return StateUnknown, -1, err
26 }
27- kv := keyValues(stdout, ": ")
28+ kv := keyValues(out, ": ")
29 state := State(kv["state"])
30 pid, err := strconv.Atoi(kv["pid"])
31 if err != nil {
32@@ -305,20 +305,24 @@
33 return c.containerHome() + "/rootfs/"
34 }
35
36-// run executes the passed command and returns the output.
37+// run executes the passed command and returns the out.
38 func run(name string, args ...string) (string, error) {
39 cmd := exec.Command(name, args...)
40- output, err := cmd.CombinedOutput()
41+ // LXC message and error printing is inconsistent
42+ // using stdout and stderr mixed and in different
43+ // message formats. So using CombinedOutput() and
44+ // analyzing it in runError() in case of an error.
45+ out, err := cmd.CombinedOutput()
46 if err != nil {
47- return "", runError(name, err, output)
48+ return "", runError(name, err, out)
49 }
50- return string(output), nil
51+ return string(out), nil
52 }
53
54 // runError creates an error if run fails.
55-func runError(name string, err error, output []byte) error {
56+func runError(name string, err error, out []byte) error {
57 e := &Error{name, err, nil}
58- for _, l := range strings.Split(string(output), "\n") {
59+ for _, l := range strings.Split(string(out), "\n") {
60 if strings.HasPrefix(l, name+": ") {
61 l = l[len(name)+2:]
62 }
63@@ -329,7 +333,7 @@
64 return e
65 }
66
67-// keyValues retrieves key/value pairs out of a command output.
68+// keyValues retrieves key/value pairs out of a command out.
69 func keyValues(raw string, sep string) map[string]string {
70 kv := map[string]string{}
71 lines := strings.Split(raw, "\n")
72@@ -342,7 +346,7 @@
73 return kv
74 }
75
76-// nameSet retrieves a set of names out of a command output.
77+// nameSet retrieves a set of names out of a command out.
78 func nameSet(raw string) []string {
79 collector := map[string]struct{}{}
80 set := []string{}
81
82=== modified file 'golxc_test.go'
83--- golxc_test.go 2013-02-05 14:23:18 +0000
84+++ golxc_test.go 2013-02-05 14:23:18 +0000
85@@ -1,13 +1,16 @@
86 package golxc_test
87
88 import (
89+ "fmt"
90 "io/ioutil"
91 . "launchpad.net/gocheck"
92- "launchpad.net/golxc"
93 "os"
94 "os/user"
95+ "path/filepath"
96 "strings"
97 "testing"
98+
99+ "launchpad.net/golxc"
100 )
101
102 func Test(t *testing.T) { TestingT(t) }
103@@ -74,10 +77,10 @@
104 # LXC_BRIDGE and LXC_ADDR are changed against original for
105 # testing purposes.
106 LXC_BRIDGE="lxcbr9"
107-LXC_ADDR="10.0.9.9"
108+LXC_ADDR="10.0.9.1"
109 LXC_NETMASK="255.255.255.0"
110-LXC_NETWORK="10.0.3.0/24"
111-LXC_DHCP_RANGE="10.0.3.2,10.0.3.254"
112+LXC_NETWORK="10.0.9.0/24"
113+LXC_DHCP_RANGE="10.0.9.2,10.0.9.254"
114 LXC_DHCP_MAX="253"
115
116 LXC_SHUTDOWN_TIMEOUT=120`
117@@ -99,19 +102,90 @@
118 env, err := golxc.ReadDefaultEnvironment()
119 c.Assert(err, IsNil)
120 c.Assert(env["LXC_BRIDGE"], Equals, "lxcbr9")
121- c.Assert(env["LXC_ADDR"], Equals, "10.0.9.9")
122+ c.Assert(env["LXC_ADDR"], Equals, "10.0.9.1")
123 c.Assert(env["MIRROR"], Equals, "")
124 }
125
126 func (s *ConfigurationSuite) TestReadNotExistingDefaultEnvironment(c *C) {
127 // Test reading a not existing environment.
128- orig := golxc.SetLXCDefaultFile("/tmp/not-existing-exc-test")
129+ orig := golxc.SetLXCDefaultFile(nonExistingPath())
130 defer golxc.SetLXCDefaultFile(orig)
131
132 _, err := golxc.ReadDefaultEnvironment()
133 c.Assert(err, ErrorMatches, "open .*: no such file or directory")
134 }
135
136+type NetworkSuite struct{}
137+
138+var _ = Suite(&NetworkSuite{})
139+
140+func (s *NetworkSuite) SetUpSuite(c *C) {
141+ u, err := user.Current()
142+ c.Assert(err, IsNil)
143+ if u.Uid != "0" {
144+ // Has to be run as root!
145+ c.Skip("tests must run as root")
146+ }
147+}
148+
149+func (s *NetworkSuite) TestStartStopNetwork(c *C) {
150+ // Test starting and stoping of the LXC network.
151+ initialRunning, err := golxc.IsNetworkRunning()
152+ c.Assert(err, IsNil)
153+ defer func() {
154+ if initialRunning {
155+ c.Assert(golxc.StartNetwork(), IsNil)
156+ }
157+ }()
158+ c.Assert(golxc.StartNetwork(), IsNil)
159+ running, err := golxc.IsNetworkRunning()
160+ c.Assert(err, IsNil)
161+ c.Assert(running, Equals, true)
162+ c.Assert(golxc.StopNetwork(), IsNil)
163+ running, err = golxc.IsNetworkRunning()
164+ c.Assert(err, IsNil)
165+ c.Assert(running, Equals, false)
166+}
167+
168+func (s *ConfigurationSuite) TestNetworkAttributes(c *C) {
169+ // Test reading the network attribute form an environment.
170+ cf := filepath.Join(c.MkDir(), "lxc-test")
171+ c.Assert(ioutil.WriteFile(cf, []byte(env), 0555), IsNil)
172+
173+ orig := golxc.SetLXCDefaultFile(cf)
174+ defer golxc.SetLXCDefaultFile(orig)
175+
176+ addr, bridge, err := golxc.NetworkAttributes()
177+ c.Assert(err, IsNil)
178+ c.Assert(addr, Equals, "10.0.9.1")
179+ c.Assert(bridge, Equals, "lxcbr9")
180+}
181+
182+func (s *NetworkSuite) TestNotExistingNetworkAttributes(c *C) {
183+ // Test reading of network attributes from a not existing environment.
184+ orig := golxc.SetLXCDefaultFile(nonExistingPath())
185+ defer golxc.SetLXCDefaultFile(orig)
186+
187+ _, _, err := golxc.NetworkAttributes()
188+ c.Assert(err, ErrorMatches, "open .*: no such file or directory")
189+}
190+
191+// nonExistingPath returns a non-existing file path.
192+func nonExistingPath() string {
193+ base := "/any/path/to/non-existing/lxc-file-%d"
194+ index := 0
195+ for {
196+ path := fmt.Sprintf(base, index)
197+ f, err := os.Open(path)
198+ if os.IsNotExist(err) {
199+ return path
200+ }
201+ f.Close()
202+ index++
203+ }
204+ panic("unreachable")
205+}
206+
207 type LXCSuite struct{}
208
209 var _ = Suite(&LXCSuite{})
210
211=== added file 'network.go'
212--- network.go 1970-01-01 00:00:00 +0000
213+++ network.go 2013-02-05 14:23:18 +0000
214@@ -0,0 +1,86 @@
215+package golxc
216+
217+import (
218+ "fmt"
219+ "strings"
220+)
221+
222+const (
223+ defaultAddr = "10.0.3.1"
224+ defaultBridge = "lxcbr0"
225+)
226+
227+// StartNetwork starts the lxc network subsystem.
228+func StartNetwork() error {
229+ goal, _, err := networkStatus()
230+ if err != nil {
231+ return err
232+ }
233+ if goal != "start" {
234+ _, err := run("start", "lxc-net")
235+ if err != nil {
236+ return err
237+ }
238+ }
239+ return nil
240+}
241+
242+// StopNetwork stops the lxc network subsystem.
243+func StopNetwork() error {
244+ goal, _, err := networkStatus()
245+ if err != nil {
246+ return err
247+ }
248+ if goal != "stop" {
249+ _, err := run("stop", "lxc-net")
250+ if err != nil {
251+ return err
252+ }
253+ }
254+ return nil
255+}
256+
257+// IsNotworkRunning checks if the lxc network subsystem
258+// is running.
259+func IsNetworkRunning() (bool, error) {
260+ _, status, err := networkStatus()
261+ if err != nil {
262+ return false, err
263+ }
264+ return status == "running", nil
265+}
266+
267+// NetworkAttributes returns the lxc network attributes:
268+// starting IP address and bridge name.
269+func NetworkAttributes() (addr, bridge string, err error) {
270+ env, err := ReadDefaultEnvironment()
271+ if err != nil {
272+ return "", "", err
273+ }
274+ addr = env["LXC_ADDR"]
275+ if addr == "" {
276+ addr = defaultAddr
277+ }
278+ bridge = env["LXC_BRIDGE"]
279+ if bridge == "" {
280+ bridge = defaultBridge
281+ }
282+ return addr, bridge, nil
283+}
284+
285+// networkStatus returns the status of the lxc network subsystem.
286+func networkStatus() (goal, status string, err error) {
287+ output, err := run("status", "lxc-net")
288+ if err != nil {
289+ return "", "", err
290+ }
291+ fields := strings.Fields(output)
292+ if len(fields) != 2 {
293+ return "", "", fmt.Errorf("unexpected status output: %q", output)
294+ }
295+ fields = strings.Split(fields[1], "/")
296+ if len(fields) != 2 {
297+ return "", "", fmt.Errorf("unexpected status output: %q", output)
298+ }
299+ return fields[0], fields[1], nil
300+}

Subscribers

People subscribed via source and target branches

to all changes: