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

[Frank Mueller]

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/

[lp:~themue/golxc/003-added-network]

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)]

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]

8. By Frank Mueller on 2013-01-31

golxc: changes after review

[William Reade (fwereade)]

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]

9. By Frank Mueller on 2013-02-05

golxc: changes after review

[William Reade (fwereade)]

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/

=== 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 @@
 
 // List lists the existing containers on the system.
 func List() ([]*Container, error) {
-	stdout, err := run("lxc-ls", "-1")
+	out, err := run("lxc-ls", "-1")
 	if err != nil {
 		return nil, err
 	}
-	names := nameSet(stdout)
+	names := nameSet(out)
 	containers := make([]*Container, len(names))
 	for i, name := range names {
 		containers[i] = New(name)
@@ -254,11 +254,11 @@
 
 // Info returns the status and the process id of the container.
 func (c *Container) Info() (State, int, error) {
-	stdout, err := run("lxc-info", "-n", c.name)
+	out, err := run("lxc-info", "-n", c.name)
 	if err != nil {
 		return StateUnknown, -1, err
 	}
-	kv := keyValues(stdout, ": ")
+	kv := keyValues(out, ": ")
 	state := State(kv["state"])
 	pid, err := strconv.Atoi(kv["pid"])
 	if err != nil {
@@ -305,20 +305,24 @@
 	return c.containerHome() + "/rootfs/"
 }
 
-// run executes the passed command and returns the output.
+// run executes the passed command and returns the out.
 func run(name string, args ...string) (string, error) {
 	cmd := exec.Command(name, args...)
-	output, err := cmd.CombinedOutput()
+	// LXC message and error printing is inconsistent
+	// using stdout and stderr mixed and in different
+	// message formats. So using CombinedOutput() and
+	// analyzing it in runError() in case of an error.
+	out, err := cmd.CombinedOutput()
 	if err != nil {
-		return "", runError(name, err, output)
+		return "", runError(name, err, out)
 	}
-	return string(output), nil
+	return string(out), nil
 }
 
 // runError creates an error if run fails.
-func runError(name string, err error, output []byte) error {
+func runError(name string, err error, out []byte) error {
 	e := &Error{name, err, nil}
-	for _, l := range strings.Split(string(output), "\n") {
+	for _, l := range strings.Split(string(out), "\n") {
 		if strings.HasPrefix(l, name+": ") {
 			l = l[len(name)+2:]
 		}
@@ -329,7 +333,7 @@
 	return e
 }
 
-// keyValues retrieves key/value pairs out of a command output.
+// keyValues retrieves key/value pairs out of a command out.
 func keyValues(raw string, sep string) map[string]string {
 	kv := map[string]string{}
 	lines := strings.Split(raw, "\n")
@@ -342,7 +346,7 @@
 	return kv
 }
 
-// nameSet retrieves a set of names out of a command output.
+// nameSet retrieves a set of names out of a command out.
 func nameSet(raw string) []string {
 	collector := map[string]struct{}{}
 	set := []string{}

=== 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 @@
 package golxc_test
 
 import (
+	"fmt"
 	"io/ioutil"
 	. "launchpad.net/gocheck"
-	"launchpad.net/golxc"
 	"os"
 	"os/user"
+	"path/filepath"
 	"strings"
 	"testing"
+
+	"launchpad.net/golxc"
 )
 
 func Test(t *testing.T) { TestingT(t) }
@@ -74,10 +77,10 @@
 # LXC_BRIDGE and LXC_ADDR are changed against original for
 # testing purposes.
 LXC_BRIDGE="lxcbr9"
-LXC_ADDR="10.0.9.9"
+LXC_ADDR="10.0.9.1"
 LXC_NETMASK="255.255.255.0"
-LXC_NETWORK="10.0.3.0/24"
-LXC_DHCP_RANGE="10.0.3.2,10.0.3.254"
+LXC_NETWORK="10.0.9.0/24"
+LXC_DHCP_RANGE="10.0.9.2,10.0.9.254"
 LXC_DHCP_MAX="253"
 
 LXC_SHUTDOWN_TIMEOUT=120`
@@ -99,19 +102,90 @@
 	env, err := golxc.ReadDefaultEnvironment()
 	c.Assert(err, IsNil)
 	c.Assert(env["LXC_BRIDGE"], Equals, "lxcbr9")
-	c.Assert(env["LXC_ADDR"], Equals, "10.0.9.9")
+	c.Assert(env["LXC_ADDR"], Equals, "10.0.9.1")
 	c.Assert(env["MIRROR"], Equals, "")
 }
 
 func (s *ConfigurationSuite) TestReadNotExistingDefaultEnvironment(c *C) {
 	// Test reading a not existing environment.
-	orig := golxc.SetLXCDefaultFile("/tmp/not-existing-exc-test")
+	orig := golxc.SetLXCDefaultFile(nonExistingPath())
 	defer golxc.SetLXCDefaultFile(orig)
 
 	_, err := golxc.ReadDefaultEnvironment()
 	c.Assert(err, ErrorMatches, "open .*: no such file or directory")
 }
 
+type NetworkSuite struct{}
+
+var _ = Suite(&NetworkSuite{})
+
+func (s *NetworkSuite) SetUpSuite(c *C) {
+	u, err := user.Current()
+	c.Assert(err, IsNil)
+	if u.Uid != "0" {
+		// Has to be run as root!
+		c.Skip("tests must run as root")
+	}
+}
+
+func (s *NetworkSuite) TestStartStopNetwork(c *C) {
+	// Test starting and stoping of the LXC network.
+	initialRunning, err := golxc.IsNetworkRunning()
+	c.Assert(err, IsNil)
+	defer func() {
+		if initialRunning {
+			c.Assert(golxc.StartNetwork(), IsNil)
+		}
+	}()
+	c.Assert(golxc.StartNetwork(), IsNil)
+	running, err := golxc.IsNetworkRunning()
+	c.Assert(err, IsNil)
+	c.Assert(running, Equals, true)
+	c.Assert(golxc.StopNetwork(), IsNil)
+	running, err = golxc.IsNetworkRunning()
+	c.Assert(err, IsNil)
+	c.Assert(running, Equals, false)
+}
+
+func (s *ConfigurationSuite) TestNetworkAttributes(c *C) {
+	// Test reading the network attribute form an environment.
+	cf := filepath.Join(c.MkDir(), "lxc-test")
+	c.Assert(ioutil.WriteFile(cf, []byte(env), 0555), IsNil)
+
+	orig := golxc.SetLXCDefaultFile(cf)
+	defer golxc.SetLXCDefaultFile(orig)
+
+	addr, bridge, err := golxc.NetworkAttributes()
+	c.Assert(err, IsNil)
+	c.Assert(addr, Equals, "10.0.9.1")
+	c.Assert(bridge, Equals, "lxcbr9")
+}
+
+func (s *NetworkSuite) TestNotExistingNetworkAttributes(c *C) {
+	// Test reading of network attributes from a not existing environment.
+	orig := golxc.SetLXCDefaultFile(nonExistingPath())
+	defer golxc.SetLXCDefaultFile(orig)
+
+	_, _, err := golxc.NetworkAttributes()
+	c.Assert(err, ErrorMatches, "open .*: no such file or directory")
+}
+
+// nonExistingPath returns a non-existing file path.
+func nonExistingPath() string {
+	base := "/any/path/to/non-existing/lxc-file-%d"
+	index := 0
+	for {
+		path := fmt.Sprintf(base, index)
+		f, err := os.Open(path)
+		if os.IsNotExist(err) {
+			return path
+		}
+		f.Close()
+		index++
+	}
+	panic("unreachable")
+}
+
 type LXCSuite struct{}
 
 var _ = Suite(&LXCSuite{})

=== 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 @@
+package golxc
+
+import (
+	"fmt"
+	"strings"
+)
+
+const (
+	defaultAddr   = "10.0.3.1"
+	defaultBridge = "lxcbr0"
+)
+
+// StartNetwork starts the lxc network subsystem.
+func StartNetwork() error {
+	goal, _, err := networkStatus()
+	if err != nil {
+		return err
+	}
+	if goal != "start" {
+		_, err := run("start", "lxc-net")
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// StopNetwork stops the lxc network subsystem.
+func StopNetwork() error {
+	goal, _, err := networkStatus()
+	if err != nil {
+		return err
+	}
+	if goal != "stop" {
+		_, err := run("stop", "lxc-net")
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// IsNotworkRunning checks if the lxc network subsystem
+// is running.
+func IsNetworkRunning() (bool, error) {
+	_, status, err := networkStatus()
+	if err != nil {
+		return false, err
+	}
+	return status == "running", nil
+}
+
+// NetworkAttributes returns the lxc network attributes: 
+// starting IP address and bridge name.
+func NetworkAttributes() (addr, bridge string, err error) {
+	env, err := ReadDefaultEnvironment()
+	if err != nil {
+		return "", "", err
+	}
+	addr = env["LXC_ADDR"]
+	if addr == "" {
+		addr = defaultAddr
+	}
+	bridge = env["LXC_BRIDGE"]
+	if bridge == "" {
+		bridge = defaultBridge
+	}
+	return addr, bridge, nil
+}
+
+// networkStatus returns the status of the lxc network subsystem.
+func networkStatus() (goal, status string, err error) {
+	output, err := run("status", "lxc-net")
+	if err != nil {
+		return "", "", err
+	}
+	fields := strings.Fields(output)
+	if len(fields) != 2 {
+		return "", "", fmt.Errorf("unexpected status output: %q", output)
+	}
+	fields = strings.Split(fields[1], "/")
+	if len(fields) != 2 {
+		return "", "", fmt.Errorf("unexpected status output: %q", output)
+	}
+	return fields[0], fields[1], nil
+}