Merge lp:~thumper/juju-core/kvm-manager-start-stop into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Implement kvm start, stop and list.

The implementation here uses a mix of:
 * uvt-simplestream-libvirt
 * uvt-kvm
 * virsh

These are provided by the uvtool package and the
dependencies. The implementation of the low level
calls out to the shell are in container/kvm/libvirt.go

These low level calls are then used by the container
factory and container instances in container/kvm.

These are used by the container manager instance.

There are some live tests for the kvm calls.
  cd container/kvm
  sudo -E go test
This will take quite some time the first time it is
run on any particular machine as it is syncing the
kvm machine instances. But it does work \o/
The machines won't actually boot functional juju
machines in the test because the tools location is
fake.

https://codereview.appspot.com/30020044/

Implement kvm start, stop and list.

The implementation here uses a mix of:
 * uvt-simplestream-libvirt
 * uvt-kvm
 * virsh

These are provided by the uvtool package and the
dependencies. The implementation of the low level
calls out to the shell are in container/kvm/libvirt.go

These low level calls are then used by the container
factory and container instances in container/kvm.

These are used by the container manager instance.

There are some live tests for the kvm calls.
  cd container/kvm
  sudo -E go test
This will take quite some time the first time it is
run on any particular machine as it is syncing the
kvm machine instances. But it does work \o/
The machines won't actually boot functional juju
machines in the test because the tools location is
fake.

https://codereview.appspot.com/30020044/

[Tim Penhey (thumper)]

Reviewers: mp+196072_code.launchpad.net,

Message:
Please take a look.

Description:
Implement kvm start, stop and list.

The implementation here uses a mix of:
  * uvt-simplestream-libvirt
  * uvt-kvm
  * virsh

These are provided by the uvtool package and the
dependencies. The implementation of the low level
calls out to the shell are in container/kvm/libvirt.go

These low level calls are then used by the container
factory and container instances in container/kvm.

These are used by the container manager instance.

There are some live tests for the kvm calls.
   cd container/kvm
   sudo -E go test
This will take quite some time the first time it is
run on any particular machine as it is syncing the
kvm machine instances. But it does work \o/
The machines won't actually boot functional juju
machines in the test because the tools location is
fake.

https://code.launchpad.net/~thumper/juju-core/kvm-manager-start-stop/+merge/196072

Requires:
https://code.launchpad.net/~thumper/juju-core/local-provider-syslog/+merge/196048

(do not edit description out of merge proposal)

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

Affected files (+424, -29 lines):
   A [revision details]
   M container/kvm/container.go
   M container/kvm/containerfactory.go
   M container/kvm/interface.go
   M container/kvm/kvm.go
   M container/kvm/kvm_test.go
   A container/kvm/libvirt.go
   A container/kvm/live_test.go
   M container/kvm/mock/mock-kvm.go
   M container/kvm/mock/mock-kvm_test.go
   M container/userdata.go
   M log/log.go

[Ian Booth (wallyworld)]

LTGM with some suggestions.

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go
File container/kvm/container.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode16
container/kvm/container.go:16: started *bool
Perhaps a comment explaining why a pointer? ie inband signalling of
unknown state

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode36
container/kvm/container.go:36: bridge := ""
I'd prefer var bridge string here because "" is not a valid value

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode60
container/kvm/container.go:60: if c.IsRunning() {
Perhaps
id !c.isRunning() {
   logger.Debugf(...)
   return nil
}
other stuff....

because the other stuff is more lines of code and I think it reads
better that way

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go
File container/kvm/kvm.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go#newcode78
container/kvm/kvm.go:78: return nil, err
Generally, inside these sorts of methods implementing an interface, I
think we have been doing stuff like
return nil, fmt.Errorf("doing blah blah: %v", err)

to give some content to the final error message, rather that just return
nil, err

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go#newcode84
container/kvm/kvm.go:84: return nil, err
ditto

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm_test.go
File container/kvm/kvm_test.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm_test.go#newcode24
container/kvm/kvm_test.go:24:
Aren't there tests we can do for start, stop etc when using mocks?

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go
File container/kvm/libvirt.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode5
container/kvm/libvirt.go:5:
Could we add some docstring saying what this file does and also what
packages it depends on being installed?

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode22
container/kvm/libvirt.go:22: func run(command string, args ...string)
(output string, err error) {
This looks like pretty generic boilerplate. Shouldn't we put this in a
utils package somewhere?

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode53
container/kvm/libvirt.go:53: UserData string
Could we rename this to UserDataFile cause that's what it is and it
could be confused for the actual user data content otherwise. That's
what I thought till I read the rest of the code.

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode107
container/kvm/libvirt.go:107: // Perhaps regex matching is the easiest
way to match the lines.
s/Perhaps//

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode112
container/kvm/libvirt.go:112: machineStatus :=
machineListPattern.ExpandString(nil, "$hostname $status", output, s)
machineStatus is confusing as a variable name, since it could look like
is is being used to store the value of "status" from "id hostname
stat...

[Tim Penhey (thumper)]

I'll push the diff once I have fixed the first one. It seems a bit
screwed up right now.

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go
File container/kvm/container.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode16
container/kvm/container.go:16: started *bool
On 2013/11/21 23:57:26, wallyworld wrote:
> Perhaps a comment explaining why a pointer? ie inband signalling of
unknown
> state

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode36
container/kvm/container.go:36: bridge := ""
On 2013/11/21 23:57:26, wallyworld wrote:
> I'd prefer var bridge string here because "" is not a valid value

Changed it, but "" is a technically valid value. If it isn't set, we
don't pass the --bridge value down to the underlying uvt-kvm call.

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode60
container/kvm/container.go:60: if c.IsRunning() {
On 2013/11/21 23:57:26, wallyworld wrote:
> Perhaps
> id !c.isRunning() {
> logger.Debugf(...)
> return nil
> }
> other stuff....

> because the other stuff is more lines of code and I think it reads
better that
> way

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go
File container/kvm/kvm.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go#newcode78
container/kvm/kvm.go:78: return nil, err
On 2013/11/21 23:57:26, wallyworld wrote:
> Generally, inside these sorts of methods implementing an interface, I
think we
> have been doing stuff like
> return nil, fmt.Errorf("doing blah blah: %v", err)

> to give some content to the final error message, rather that just
return nil,
> err

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go#newcode84
container/kvm/kvm.go:84: return nil, err
On 2013/11/21 23:57:26, wallyworld wrote:
> ditto

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm_test.go
File container/kvm/kvm_test.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm_test.go#newcode24
container/kvm/kvm_test.go:24:
On 2013/11/21 23:57:26, wallyworld wrote:
> Aren't there tests we can do for start, stop etc when using mocks?

Yes, and we actually do already. This comment was to remind myself to
implement the live tests, which I did.

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go
File container/kvm/libvirt.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode5
container/kvm/libvirt.go:5:
On 2013/11/21 23:57:26, wallyworld wrote:
> Could we add some docstring saying what this file does and also what
packages it
> depends on being installed?

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode22
container/kvm/libvirt.go:22: func run(command string, args ...string)
(output string, err error) {
On 2013/11/21 23:57:26, wallyworld wrote:
> This looks like pretty generic boilerplate. Shouldn't we put this in a
utils
> package somewhere?

Possibly, but it did provide me a very place to ramp up the logging to
test what was going on. I'd be a little loathed to rem...

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/30020044/

=== modified file 'container/kvm/container.go'
--- container/kvm/container.go	2013-11-12 21:00:12 +0000
+++ container/kvm/container.go	2013-11-25 04:11:41 +0000
@@ -5,12 +5,18 @@
 
 import (
 	"fmt"
+
+	"launchpad.net/juju-core/container"
+	"launchpad.net/juju-core/log"
 )
 
 type kvmContainer struct {
 	factory *containerFactory
 	name    string
-	started bool
+	// started is a three state boolean, true, false, or unknown
+	// this allows for checking when we don't know, but using a
+	// value if we already know it (like in the list situation).
+	started *bool
 }
 
 var _ Container = (*kvmContainer)(nil)
@@ -19,16 +25,61 @@
 	return c.name
 }
 
-func (c *kvmContainer) Start() error {
-	return fmt.Errorf("not implemented")
+func (c *kvmContainer) Start(
+	series string,
+	arch string,
+	userDataFile string,
+	network *container.NetworkConfig,
+) error {
+	// TODO: handle memory, cpu, disk etc.
+	logger.Debugf("Synchronise images for %s %s", series, arch)
+	if err := SyncImages(series, arch); err != nil {
+		return err
+	}
+	var bridge string
+	if network != nil {
+		if network.NetworkType == container.BridgeNetwork {
+			bridge = network.Device
+		} else {
+			return log.LoggedErrorf(logger, "Non-bridge network devices not yet supported")
+		}
+	}
+	logger.Debugf("Create the machine %s", c.name)
+	if err := CreateMachine(CreateMachineParams{
+		Hostname:      c.name,
+		Series:        series,
+		Arch:          arch,
+		UserDataFile:  userDataFile,
+		NetworkBridge: bridge,
+	}); err != nil {
+		return err
+	}
+
+	logger.Debugf("Set machine %s to autostart", c.name)
+	return AutostartMachine(c.name)
 }
 
 func (c *kvmContainer) Stop() error {
-	return fmt.Errorf("not implemented")
+	if !c.IsRunning() {
+		logger.Debugf("%s is already stopped", c.name)
+		return nil
+	}
+	// Make started state unknown again.
+	c.started = nil
+	logger.Debugf("Stop %s", c.name)
+	return DestroyMachine(c.name)
 }
 
 func (c *kvmContainer) IsRunning() bool {
-	return c.started
+	if c.started != nil {
+		return *c.started
+	}
+	machines, err := ListMachines()
+	if err != nil {
+		return false
+	}
+	c.started = isRunning(machines[c.name])
+	return *c.started
 }
 
 func (c *kvmContainer) String() string {

=== modified file 'container/kvm/containerfactory.go'
--- container/kvm/containerfactory.go	2013-11-12 21:00:12 +0000
+++ container/kvm/containerfactory.go	2013-11-25 04:11:41 +0000
@@ -3,10 +3,6 @@
 
 package kvm
 
-import (
-	"fmt"
-)
-
 type containerFactory struct {
 }
 
@@ -19,6 +15,26 @@
 	}
 }
 
-func (factory *containerFactory) List() ([]Container, error) {
-	return nil, fmt.Errorf("Not yet implemented")
+func isRunning(value string) *bool {
+	var result *bool = new(bool)
+	if value == "running" {
+		*result = true
+	}
+	return result
+}
+
+func (factory *containerFactory) List() (result []Container, err error) {
+	machines, err := ListMachines()
+	if err != nil {
+		return nil, err
+	}
+	for hostname, status := range machines {
+		result = append(result, &kvmContainer{
+			factory: factory,
+			name:    hostname,
+			started: isRunning(status),
+		})
+
+	}
+	return result, nil
 }

=== modified file 'container/kvm/interface.go'
--- container/kvm/interface.go	2013-11-12 21:00:12 +0000
+++ container/kvm/interface.go	2013-11-25 04:11:41 +0000
@@ -3,6 +3,10 @@
 
 package kvm
 
+import (
+	"launchpad.net/juju-core/container"
+)
+
 // Container represents a virtualized container instance and provides
 // operations to create, maintain and destroy the container.
 type Container interface {
@@ -11,8 +15,13 @@
 	Name() string
 
 	// Start runs the container as a daemon.
-	// TODO: determine parameters
-	Start() error
+	// TODO: add constraints.
+	Start(
+		series string,
+		arch string,
+		userDataFile string,
+		network *container.NetworkConfig,
+	) error
 
 	// Stop terminates the running container.
 	Stop() error

=== modified file 'container/kvm/kvm.go'
--- container/kvm/kvm.go	2013-11-13 01:35:46 +0000
+++ container/kvm/kvm.go	2013-11-25 04:11:41 +0000
@@ -13,6 +13,9 @@
 	"launchpad.net/juju-core/container"
 	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/log"
+	"launchpad.net/juju-core/names"
+	"launchpad.net/juju-core/version"
 )
 
 var (
@@ -60,11 +63,39 @@
 	machineConfig *cloudinit.MachineConfig,
 	series string,
 	network *container.NetworkConfig) (instance.Instance, error) {
-	return nil, fmt.Errorf("not yet implemented")
+
+	name := names.MachineTag(machineConfig.MachineId)
+	if manager.name != "" {
+		name = fmt.Sprintf("%s-%s", manager.name, name)
+	}
+	// Note here that the kvmObjectFacotry only returns a valid container
+	// object, and doesn't actually construct the underlying kvm container on
+	// disk.
+	kvmContainer := KvmObjectFactory.New(name)
+
+	// Create the cloud-init.
+	directory, err := container.NewDirectory(name)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create container directory: %v", err)
+	}
+	logger.Tracef("write cloud-init")
+	userDataFilename, err := container.WriteUserData(machineConfig, directory)
+	if err != nil {
+		return nil, log.LoggedErrorf(logger, "failed to write user data: %v", err)
+	}
+	// Create the container.
+	logger.Tracef("create the container")
+	if err := kvmContainer.Start(series, version.Current.Arch, userDataFilename, network); err != nil {
+		return nil, log.LoggedErrorf(logger, "kvm container creation failed: %v", err)
+	}
+	logger.Tracef("kvm container created")
+	return &kvmInstance{kvmContainer, name}, nil
 }
 
-func (manager *containerManager) StopContainer(instance.Instance) error {
-	return fmt.Errorf("not yet implemented")
+func (manager *containerManager) StopContainer(instance instance.Instance) error {
+	name := string(instance.Id())
+	kvmContainer := KvmObjectFactory.New(name)
+	return kvmContainer.Stop()
 }
 
 func (manager *containerManager) ListContainers() (result []instance.Instance, err error) {

=== modified file 'container/kvm/kvm_test.go'
--- container/kvm/kvm_test.go	2013-11-12 21:00:12 +0000
+++ container/kvm/kvm_test.go	2013-11-25 04:11:41 +0000
@@ -11,6 +11,7 @@
 	kvmtesting "launchpad.net/juju-core/container/kvm/testing"
 	"launchpad.net/juju-core/instance"
 	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/version"
 )
 
 type KVMSuite struct {
@@ -19,7 +20,11 @@
 
 var _ = gc.Suite(&KVMSuite{})
 
-// TODO: work out how to test the actual kvm implementations.
+func (*KVMSuite) TestManagerNameNeeded(c *gc.C) {
+	manager, err := kvm.NewContainerManager(container.ManagerConfig{})
+	c.Assert(err, gc.ErrorMatches, "name is required")
+	c.Assert(manager, gc.IsNil)
+}
 
 func (*KVMSuite) TestListInitiallyEmpty(c *gc.C) {
 	manager, err := kvm.NewContainerManager(container.ManagerConfig{Name: "test"})
@@ -30,9 +35,10 @@
 }
 
 func (s *KVMSuite) createRunningContainer(c *gc.C, name string) kvm.Container {
-	container := s.Factory.New(name)
-	c.Assert(container.Start(), gc.IsNil)
-	return container
+	kvmContainer := s.Factory.New(name)
+	network := container.BridgeNetworkConfig("testbr0")
+	c.Assert(kvmContainer.Start("quantal", version.Current.Arch, "userdata.txt", network), gc.IsNil)
+	return kvmContainer
 }
 
 func (s *KVMSuite) TestListMatchesManagerName(c *gc.C) {

=== added file 'container/kvm/libvirt.go'
--- container/kvm/libvirt.go	1970-01-01 00:00:00 +0000
+++ container/kvm/libvirt.go	2013-11-25 04:11:41 +0000
@@ -0,0 +1,128 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package kvm
+
+// This file contains wrappers around the following executables:
+//   uvt-simplestreams-libvirt
+//   uvt-kvm
+//   virsh
+// Those executables are found in the following packages:
+//   uvtool-libvirt
+//   libvirt-bin
+//
+// These executables provide Juju's interface to dealing with kvm containers.
+// The define how we start, stop and list running containers on the host
+
+import (
+	"fmt"
+	"os/exec"
+	"regexp"
+	"strings"
+)
+
+var (
+	// The regular expression for breaking up the results of 'virsh list'
+	// (?m) - specify that this is a multiline regex
+	// first part is the opaque identifier we don't care about
+	// then the hostname, and lastly the status.
+	machineListPattern = regexp.MustCompile(`(?m)^\s+\d+\s+(?P<hostname>[-\w]+)\s+(?P<status>.+)\s*$`)
+)
+
+// run the command and return the combined output.
+func run(command string, args ...string) (output string, err error) {
+	logger.Tracef("%s %v", command, args)
+	cmd := exec.Command(command, args...)
+	out, err := cmd.CombinedOutput()
+	output = string(out)
+	logger.Tracef("output: %v", output)
+	if err != nil {
+		return output, err
+	}
+	if !cmd.ProcessState.Success() {
+		return output, fmt.Errorf("%s returned non-zero exit", command)
+	}
+	return output, nil
+}
+
+// SyncImages updates the local cached images by reading the simplestreams
+// data and downloading the cloud images to the uvtool pool (used by libvirt).
+func SyncImages(series string, arch string) error {
+	args := []string{
+		"sync",
+		fmt.Sprintf("arch=%s", arch),
+		fmt.Sprintf("release=%s", series),
+	}
+	_, err := run("uvt-simplestreams-libvirt", args...)
+	return err
+}
+
+type CreateMachineParams struct {
+	Hostname      string
+	Series        string
+	Arch          string
+	UserDataFile  string
+	NetworkBridge string
+	// TODO memory, cpu and disk
+}
+
+// CreateMachine creates a virtual machine and starts it.
+func CreateMachine(params CreateMachineParams) error {
+	if params.Hostname == "" {
+		return fmt.Errorf("Hostname is required")
+	}
+	args := []string{
+		"create",
+		"--log-console-output", // do wonder where this goes...
+	}
+	if params.UserDataFile != "" {
+		args = append(args, "--user-data", params.UserDataFile)
+	}
+	if params.NetworkBridge != "" {
+		args = append(args, "--bridge", params.NetworkBridge)
+	}
+	// TODO add memory, cpu and disk prior to hostname
+	args = append(args, params.Hostname)
+	if params.Series != "" {
+		args = append(args, fmt.Sprintf("release=%s", params.Series))
+	}
+	if params.Arch != "" {
+		args = append(args, fmt.Sprintf("arch=%s", params.Arch))
+	}
+	output, err := run("uvt-kvm", args...)
+	logger.Debugf("is this the logged output?:\n%s", output)
+	return err
+}
+
+// DestroyMachine destroys the virtual machine identified by hostname.
+func DestroyMachine(hostname string) error {
+	_, err := run("uvt-kvm", "destroy", hostname)
+	return err
+}
+
+// AutostartMachine indicates that the virtual machines should automatically
+// restart when the host restarts.
+func AutostartMachine(hostname string) error {
+	_, err := run("virsh", "autostart", hostname)
+	return err
+}
+
+// ListMachines returns a map of machine name to state, where state is one of:
+// running, idle, paused, shutdown, shut off, crashed, dying, pmsuspended.
+func ListMachines() (map[string]string, error) {
+	output, err := run("virsh", "-q", "list", "--all")
+	if err != nil {
+		return nil, err
+	}
+	// Split the output into lines.
+	// Regex matching is the easiest way to match the lines.
+	//   id hostname status
+	// separated by whitespace, with whitespace at the start too.
+	result := make(map[string]string)
+	for _, s := range machineListPattern.FindAllStringSubmatchIndex(output, -1) {
+		hostnameAndStatus := machineListPattern.ExpandString(nil, "$hostname $status", output, s)
+		parts := strings.SplitN(string(hostnameAndStatus), " ", 2)
+		result[parts[0]] = parts[1]
+	}
+	return result, nil
+}

=== added file 'container/kvm/live_test.go'
--- container/kvm/live_test.go	1970-01-01 00:00:00 +0000
+++ container/kvm/live_test.go	2013-11-25 04:11:41 +0000
@@ -0,0 +1,139 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package kvm_test
+
+import (
+	"os"
+	"runtime"
+
+	gc "launchpad.net/gocheck"
+	"launchpad.net/loggo"
+
+	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/container"
+	"launchpad.net/juju-core/container/kvm"
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/instance"
+	jujutesting "launchpad.net/juju-core/juju/testing"
+	coretesting "launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/tools"
+	"launchpad.net/juju-core/version"
+)
+
+type LiveSuite struct {
+	testbase.LoggingSuite
+	ContainerDir string
+	RemovedDir   string
+}
+
+var _ = gc.Suite(&LiveSuite{})
+
+func (s *LiveSuite) SetUpTest(c *gc.C) {
+	s.LoggingSuite.SetUpTest(c)
+	// Skip if not linux
+	if runtime.GOOS != "linux" {
+		c.Skip("not running linux")
+	}
+	// Skip if not running as root.
+	if os.Getuid() != 0 {
+		c.Skip("not running as root")
+	}
+	s.ContainerDir = c.MkDir()
+	s.PatchValue(&container.ContainerDir, s.ContainerDir)
+	s.RemovedDir = c.MkDir()
+	s.PatchValue(&container.RemovedContainerDir, s.RemovedDir)
+	loggo.GetLogger("juju.container").SetLogLevel(loggo.TRACE)
+}
+
+func (s *LiveSuite) newManager(c *gc.C, name string) container.Manager {
+	manager, err := kvm.NewContainerManager(
+		container.ManagerConfig{
+			Name:   name,
+			LogDir: c.MkDir(),
+		})
+	c.Assert(err, gc.IsNil)
+	return manager
+}
+
+func assertNumberOfContainers(c *gc.C, manager container.Manager, count int) {
+	containers, err := manager.ListContainers()
+	c.Assert(err, gc.IsNil)
+	c.Assert(containers, gc.HasLen, count)
+}
+
+func (s *LiveSuite) TestNoInitialContainers(c *gc.C) {
+	manager := s.newManager(c, "test")
+	assertNumberOfContainers(c, manager, 0)
+}
+
+func shutdownMachines(manager container.Manager) func(*gc.C) {
+	return func(c *gc.C) {
+		instances, err := manager.ListContainers()
+		c.Assert(err, gc.IsNil)
+		for _, instance := range instances {
+			err := manager.StopContainer(instance)
+			c.Check(err, gc.IsNil)
+		}
+	}
+}
+
+func startContainer(c *gc.C, manager container.Manager, machineId string) instance.Instance {
+	machineNonce := "fake-nonce"
+	stateInfo := jujutesting.FakeStateInfo(machineId)
+	apiInfo := jujutesting.FakeAPIInfo(machineId)
+	machineConfig := environs.NewMachineConfig(machineId, machineNonce, stateInfo, apiInfo)
+	network := container.BridgeNetworkConfig("virbr0")
+
+	machineConfig.Tools = &tools.Tools{
+		Version: version.MustParseBinary("2.3.4-foo-bar"),
+		URL:     "http://tools.testing.invalid/2.3.4-foo-bar.tgz",
+	}
+	environConfig := dummyConfig(c)
+	err := environs.FinishMachineConfig(machineConfig, environConfig, constraints.Value{})
+	c.Assert(err, gc.IsNil)
+
+	instance, err := manager.StartContainer(machineConfig, "precise", network)
+	c.Assert(err, gc.IsNil)
+	return instance
+}
+
+func (s *LiveSuite) TestShutdownMachines(c *gc.C) {
+	manager := s.newManager(c, "test")
+	startContainer(c, manager, "1/kvm/0")
+	startContainer(c, manager, "1/kvm/1")
+	assertNumberOfContainers(c, manager, 2)
+
+	shutdownMachines(manager)(c)
+	assertNumberOfContainers(c, manager, 0)
+}
+
+func (s *LiveSuite) TestManagerIsolation(c *gc.C) {
+	firstManager := s.newManager(c, "first")
+	s.AddCleanup(shutdownMachines(firstManager))
+
+	startContainer(c, firstManager, "1/kvm/0")
+	startContainer(c, firstManager, "1/kvm/1")
+
+	secondManager := s.newManager(c, "second")
+	s.AddCleanup(shutdownMachines(secondManager))
+
+	startContainer(c, secondManager, "1/kvm/0")
+
+	assertNumberOfContainers(c, firstManager, 2)
+	assertNumberOfContainers(c, secondManager, 1)
+}
+
+func dummyConfig(c *gc.C) *config.Config {
+	testConfig, err := config.New(config.UseDefaults, coretesting.FakeConfig())
+	c.Assert(err, gc.IsNil)
+	testConfig, err = testConfig.Apply(map[string]interface{}{
+		"type":          "dummy",
+		"state-server":  false,
+		"agent-version": version.Current.Number.String(),
+	})
+	c.Assert(err, gc.IsNil)
+	return testConfig
+}

=== modified file 'container/kvm/mock/mock-kvm.go'
--- container/kvm/mock/mock-kvm.go	2013-11-12 21:00:12 +0000
+++ container/kvm/mock/mock-kvm.go	2013-11-25 04:11:41 +0000
@@ -6,6 +6,7 @@
 import (
 	"fmt"
 
+	"launchpad.net/juju-core/container"
 	"launchpad.net/juju-core/container/kvm"
 )
 
@@ -66,7 +67,12 @@
 	return mock.name
 }
 
-func (mock *mockContainer) Start() error {
+func (mock *mockContainer) Start(
+	series string,
+	arch string,
+	userDataFile string,
+	network *container.NetworkConfig,
+) error {
 	if mock.started {
 		return fmt.Errorf("container is already running")
 	}

=== modified file 'container/kvm/mock/mock-kvm_test.go'
--- container/kvm/mock/mock-kvm_test.go	2013-11-12 21:13:21 +0000
+++ container/kvm/mock/mock-kvm_test.go	2013-11-25 04:11:41 +0000
@@ -6,17 +6,27 @@
 import (
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/container"
 	"launchpad.net/juju-core/container/kvm"
 	"launchpad.net/juju-core/container/kvm/mock"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/version"
 )
 
 type MockSuite struct {
 	testbase.LoggingSuite
 }
 
-var _ = gc.Suite(&MockSuite{})
+var (
+	_ = gc.Suite(&MockSuite{})
+
+	// Start args
+	series   = "quantal"
+	arch     = version.Current.Arch
+	userData = "/some/path/to/userdata.txt"
+	network  = container.BridgeNetworkConfig("testbr0")
+)
 
 func (*MockSuite) TestListInitiallyEmpty(c *gc.C) {
 	factory := mock.MockFactory()
@@ -52,7 +62,7 @@
 func (*MockSuite) TestContainerStartStarts(c *gc.C) {
 	factory := mock.MockFactory()
 	container := factory.New("first")
-	err := container.Start()
+	err := container.Start(series, arch, userData, network)
 	c.Assert(err, gc.IsNil)
 	c.Assert(container.IsRunning(), jc.IsTrue)
 }
@@ -60,16 +70,16 @@
 func (*MockSuite) TestContainerStartingRunningErrors(c *gc.C) {
 	factory := mock.MockFactory()
 	container := factory.New("first")
-	err := container.Start()
+	err := container.Start(series, arch, userData, network)
 	c.Assert(err, gc.IsNil)
-	err = container.Start()
+	err = container.Start(series, arch, userData, network)
 	c.Assert(err, gc.ErrorMatches, "container is already running")
 }
 
 func (*MockSuite) TestContainerStoppingRunningStops(c *gc.C) {
 	factory := mock.MockFactory()
 	container := factory.New("first")
-	err := container.Start()
+	err := container.Start(series, arch, userData, network)
 	c.Assert(err, gc.IsNil)
 	err = container.Stop()
 	c.Assert(err, gc.IsNil)
@@ -132,8 +142,8 @@
 
 	first := factory.New("first")
 	second := factory.New("second")
-	first.Start()
-	second.Start()
+	first.Start(series, arch, userData, network)
+	second.Start(series, arch, userData, network)
 	second.Stop()
 	first.Stop()
 
@@ -151,7 +161,7 @@
 	factory.AddListener(second)
 
 	container := factory.New("container")
-	container.Start()
+	container.Start(series, arch, userData, network)
 	container.Stop()
 
 	c.Assert(<-first, gc.Equals, mock.Event{mock.Started, "container"})

=== modified file 'container/userdata.go'
--- container/userdata.go	2013-11-14 08:27:15 +0000
+++ container/userdata.go	2013-11-25 04:11:41 +0000
@@ -17,7 +17,7 @@
 )
 
 var (
-	logger         = loggo.GetLogger("juju.container.lxc")
+	logger         = loggo.GetLogger("juju.container")
 	aptHTTPProxyRE = regexp.MustCompile(`(?i)^Acquire::HTTP::Proxy\s+"([^"]+)";$`)
 )
 

=== modified file 'log/log.go'
--- log/log.go	2013-06-10 04:25:51 +0000
+++ log/log.go	2013-11-25 04:11:41 +0000
@@ -4,6 +4,8 @@
 package log
 
 import (
+	"fmt"
+
 	"launchpad.net/loggo"
 )
 
@@ -41,3 +43,9 @@
 	logger.Logf(loggo.DEBUG, format, a...)
 	return nil
 }
+
+// Log the error and return an error with the same text.
+func LoggedErrorf(logger loggo.Logger, format string, a ...interface{}) error {
+	logger.Logf(loggo.ERROR, format, a...)
+	return fmt.Errorf(format, a...)
+}