Code review comment for lp:~thumper/juju-core/container-dir-filesystem-type

Reviewers: mp+210101_code.launchpad.net,

Message:
Please take a look.

Description:
Detect backing filesystem for lxc container dir.

Have the LXC container manager know the underlying
filesystem of the container directory.

This is necessary for the container manager to become
btrfs aware, for snapshotting containers on clone.

https://code.launchpad.net/~thumper/juju-core/container-dir-filesystem-type/+merge/210101

Requires:
https://code.launchpad.net/~thumper/juju-core/autostart-containers-after-creation/+merge/210099

(do not edit description out of merge proposal)

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

Affected files (+71, -5 lines):
   A [revision details]
   M container/lxc/export_test.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.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/export_test.go
=== modified file 'container/lxc/export_test.go'
--- container/lxc/export_test.go 2014-03-07 02:44:39 +0000
+++ container/lxc/export_test.go 2014-03-07 02:53:53 +0000
@@ -4,8 +4,9 @@
  package lxc

  var (
+ ContainerConfigFilename = containerConfigFilename
+ ContainerDirFilesystem = containerDirFilesystem
+ GenerateNetworkConfig = generateNetworkConfig
   NetworkConfigTemplate = networkConfigTemplate
- GenerateNetworkConfig = generateNetworkConfig
   RestartSymlink = restartSymlink
- ContainerConfigFilename = containerConfigFilename
  )

Index: container/lxc/lxc.go
=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go 2014-03-09 20:53:16 +0000
+++ container/lxc/lxc.go 2014-03-09 21:33:22 +0000
@@ -7,6 +7,7 @@
   "fmt"
   "io/ioutil"
   "os"
+ "os/exec"
   "path/filepath"
   "strings"

@@ -33,6 +34,8 @@
  const (
   // DefaultLxcBridge is the package created container bridge
   DefaultLxcBridge = "lxcbr0"
+ // Btrfs is special as we treat it differently for create and clone.
+ Btrfs = "btrfs"
  )

  // DefaultNetworkConfig returns a valid NetworkConfig to use the
@@ -41,9 +44,29 @@
   return container.BridgeNetworkConfig(DefaultLxcBridge)
  }

+// FsCommandOutput calls cmd.Output, this is used as an overloading point
so
+// we can test what *would* be run without actually executing another
program
+var FsCommandOutput = (*exec.Cmd).CombinedOutput
+
+func containerDirFilesystem() (string, error) {
+ cmd := exec.Command("df", "--output=fstype", LxcContainerDir)
+ out, err := FsCommandOutput(cmd)
+ if err != nil {
+ return "", err
+ }
+ // The filesystem is the second line.
+ lines := strings.Split(string(out), "\n")
+ if len(lines) < 2 {
+ logger.Errorf("unexpected output: ", out)
+ return "", fmt.Errorf("could not determine filesystem type")
+ }
+ return lines[1], nil
+}
+
  type containerManager struct {
- name string
- logdir string
+ name string
+ logdir string
+ backingFilesystem string
  }

  // containerManager implements container.Manager.
@@ -61,7 +84,16 @@
   if logDir == "" {
    logDir = agent.DefaultLogDir
   }
- return &containerManager{name: name, logdir: logDir}, nil
+ backingFS, err := containerDirFilesystem()
+ if err != nil {
+ return nil, err
+ }
+ logger.Tracef("backing filesystem: %q", backingFS)
+ return &containerManager{
+ name: name,
+ logdir: logDir,
+ backingFilesystem: backingFS,
+ }, nil
  }

  func (manager *containerManager) StartContainer(

Index: container/lxc/lxc_test.go
=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go 2014-03-09 20:53:16 +0000
+++ container/lxc/lxc_test.go 2014-03-09 20:53:17 +0000
@@ -41,6 +41,37 @@
   loggo.GetLogger("juju.container.lxc").SetLogLevel(loggo.TRACE)
  }

+func (s *LxcSuite) TestContainerDirFilesystem(c *gc.C) {
+ for i, test := range []struct {
+ message string
+ output string
+ expected string
+ errorMatch string
+ }{{
+ message: "btrfs",
+ output: "Type\nbtrfs\n",
+ expected: lxc.Btrfs,
+ }, {
+ message: "ext4",
+ output: "Type\next4\n",
+ expected: "ext4",
+ }, {
+ message: "not enough output",
+ output: "foo",
+ errorMatch: "could not determine filesystem type",
+ }} {
+ c.Logf("%v: %s", i, test.message)
+ s.HookCommandOutput(&lxc.FsCommandOutput, []byte(test.output), nil)
+ value, err := lxc.ContainerDirFilesystem()
+ if test.errorMatch == "" {
+ c.Check(err, gc.IsNil)
+ c.Check(value, gc.Equals, test.expected)
+ } else {
+ c.Check(err, gc.ErrorMatches, test.errorMatch)
+ }
+ }
+}
+
  func (s *LxcSuite) makeManager(c *gc.C, name string) container.Manager {
   manager, err := lxc.NewContainerManager(container.ManagerConfig{
    container.ConfigName: name,