Merge lp:~themue/golxc/003-added-network into lp:golxc
- 003-added-network
- Merge into trunk
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Frank Mueller | Pending | ||
Review via email: mp+136220@code.launchpad.net |
Commit message
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.
- 6. By Frank Mueller
-
golxc: minor changes after review
- 7. By Frank Mueller
-
golxc: merged prerequisite, missed it before
William Reade (fwereade) wrote : | # |
- 8. By Frank Mueller
-
golxc: changes after review
William Reade (fwereade) wrote : | # |
Couple of minors
https:/
File golxc_test.go (right):
https:/
golxc_test.go:160: orig :=
golxc.SetLXCDef
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:/
File golxc.go (right):
https:/
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:/
File golxc_test.go (right):
https:/
golxc_test.go:164: orig :=
golxc.SetLXCDef
I don't think you can guarantee this path doesn't exist either ;).
- 9. By Frank Mueller
-
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:/
File golxc.go (right):
https:/
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:/
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:/
golxc.go:326: if strings.
I'm not sure what we get out of hiding parts of the output in the case
of error. Comment perhaps?
https:/
File golxc_test.go (right):
https:/
golxc_test.go:173: // nonExistingPath returns a non-existing file path.
I'm not sure how this solution is superior to a
`filepath.
Preview Diff
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 | +} |
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 put()
golxc.go:311: out, err := cmd.CombinedOut
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 TempFile( "/tmp", "lxc-test")
golxc_test.go:140: f, err := ioutil.
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 aultFile( "/tmp/not- existing- exc-test" )
golxc_test.go:160: orig :=
golxc.SetLXCDef
c.MkDir()
https:/ /codereview. appspot. com/6849102/