Merge lp:~thumper/juju-core/kvm-provisioner into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Hook up the kvm broker and provisioner

This branch adds a kvm broker and hooks it up to the
existing container initialization.

The cpu-checker package is added to every machine to
allow kvm-ok to be executed.

A few drive by fixes related to kvm:
 * added a default bridge, used by the broker when
there is no other config.
 * the container wasn't removing the container dir
 * the kvm package isn't actually needed
 * the local provider was setting the wrong container
type (in the case of kvm)

https://codereview.appspot.com/36910043/

Hook up the kvm broker and provisioner

This branch adds a kvm broker and hooks it up to the
existing container initialization.

The cpu-checker package is added to every machine to
allow kvm-ok to be executed.

A few drive by fixes related to kvm:
 * added a default bridge, used by the broker when
there is no other config.
 * the container wasn't removing the container dir
 * the kvm package isn't actually needed
 * the local provider was setting the wrong container
type (in the case of kvm)

https://codereview.appspot.com/36910043/

[Tim Penhey (thumper)]

Reviewers: mp+197629_code.launchpad.net,

Message:
Please take a look.

Description:
Hook up the kvm broker and provisioner

This branch adds a kvm broker and hooks it up to the
existing container initialization.

The cpu-checker package is added to every machine to
allow kvm-ok to be executed.

A few drive by fixes related to kvm:
  * added a default bridge, used by the broker when
there is no other config.
  * the container wasn't removing the container dir
  * the kvm package isn't actually needed
  * the local provider was setting the wrong container
type (in the case of kvm)

https://code.launchpad.net/~thumper/juju-core/kvm-provisioner/+merge/197629

(do not edit description out of merge proposal)

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

Affected files (+388, -4 lines):
   A [revision details]
   M container/kvm/initialisation.go
   M container/kvm/kvm.go
   M environs/cloudinit/cloudinit.go
   M provider/local/environ.go
   M worker/provisioner/container_initialisation.go
   A worker/provisioner/kvm-broker.go
   A worker/provisioner/kvm-broker_test.go

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/36910043/

[Ian Booth (wallyworld)]

LGTM
Please add a todo to factor out the common lxc and kvm code in the
respective brokers. And a todo to sort out the kvm bridge stuff. I know
there's a comment but a todo is better.

https://codereview.appspot.com/36910043/

=== modified file 'container/kvm/initialisation.go'
--- container/kvm/initialisation.go	2013-12-02 21:16:18 +0000
+++ container/kvm/initialisation.go	2013-12-04 04:19:23 +0000
@@ -14,7 +14,6 @@
 var requiredPackages = []string{
 	"uvtool-libvirt",
 	"uvtool",
-	"kvm",
 }
 
 type containerInitialiser struct{}

=== modified file 'container/kvm/kvm.go'
--- container/kvm/kvm.go	2013-11-25 03:36:54 +0000
+++ container/kvm/kvm.go	2013-12-04 04:19:23 +0000
@@ -22,6 +22,7 @@
 	logger = loggo.GetLogger("juju.container.kvm")
 
 	KvmObjectFactory ContainerFactory = &containerFactory{}
+	DefaultKvmBridge                  = "virbr0"
 )
 
 // IsKVMSupported calls into the kvm-ok executable from the cpu-checkers package.
@@ -95,7 +96,11 @@
 func (manager *containerManager) StopContainer(instance instance.Instance) error {
 	name := string(instance.Id())
 	kvmContainer := KvmObjectFactory.New(name)
-	return kvmContainer.Stop()
+	if err := kvmContainer.Stop(); err != nil {
+		logger.Errorf("failed to stop kvm container: %v", err)
+		return err
+	}
+	return container.RemoveDirectory(name)
 }
 
 func (manager *containerManager) ListContainers() (result []instance.Instance, err error) {

=== modified file 'container/kvm/kvm_test.go'
--- container/kvm/kvm_test.go	2013-11-25 03:36:54 +0000
+++ container/kvm/kvm_test.go	2013-12-04 04:19:23 +0000
@@ -4,11 +4,14 @@
 package kvm_test
 
 import (
+	"path/filepath"
+
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/container"
 	"launchpad.net/juju-core/container/kvm"
 	kvmtesting "launchpad.net/juju-core/container/kvm/testing"
+	containertesting "launchpad.net/juju-core/container/testing"
 	"launchpad.net/juju-core/instance"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/version"
@@ -16,20 +19,26 @@
 
 type KVMSuite struct {
 	kvmtesting.TestSuite
+	manager container.Manager
 }
 
 var _ = gc.Suite(&KVMSuite{})
 
+func (s *KVMSuite) SetUpTest(c *gc.C) {
+	s.TestSuite.SetUpTest(c)
+	var err error
+	s.manager, err = kvm.NewContainerManager(container.ManagerConfig{Name: "test"})
+	c.Assert(err, gc.IsNil)
+}
+
 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"})
-	c.Assert(err, gc.IsNil)
-	containers, err := manager.ListContainers()
+func (s *KVMSuite) TestListInitiallyEmpty(c *gc.C) {
+	containers, err := s.manager.ListContainers()
 	c.Assert(err, gc.IsNil)
 	c.Assert(containers, gc.HasLen, 0)
 }
@@ -42,13 +51,11 @@
 }
 
 func (s *KVMSuite) TestListMatchesManagerName(c *gc.C) {
-	manager, err := kvm.NewContainerManager(container.ManagerConfig{Name: "test"})
-	c.Assert(err, gc.IsNil)
 	s.createRunningContainer(c, "test-match1")
 	s.createRunningContainer(c, "test-match2")
 	s.createRunningContainer(c, "testNoMatch")
 	s.createRunningContainer(c, "other")
-	containers, err := manager.ListContainers()
+	containers, err := s.manager.ListContainers()
 	c.Assert(err, gc.IsNil)
 	c.Assert(containers, gc.HasLen, 2)
 	expectedIds := []instance.Id{"test-match1", "test-match2"}
@@ -57,12 +64,30 @@
 }
 
 func (s *KVMSuite) TestListMatchesRunningContainers(c *gc.C) {
-	manager, err := kvm.NewContainerManager(container.ManagerConfig{Name: "test"})
-	c.Assert(err, gc.IsNil)
 	running := s.createRunningContainer(c, "test-running")
 	s.Factory.New("test-stopped")
-	containers, err := manager.ListContainers()
+	containers, err := s.manager.ListContainers()
 	c.Assert(err, gc.IsNil)
 	c.Assert(containers, gc.HasLen, 1)
 	c.Assert(string(containers[0].Id()), gc.Equals, running.Name())
 }
+
+func (s *KVMSuite) TestStartContainer(c *gc.C) {
+	instance := containertesting.StartContainer(c, s.manager, "1/kvm/0")
+	name := string(instance.Id())
+	cloudInitFilename := filepath.Join(s.ContainerDir, name, "cloud-init")
+	containertesting.AssertCloudInit(c, cloudInitFilename)
+}
+
+func (s *KVMSuite) TestStopContainer(c *gc.C) {
+	instance := containertesting.StartContainer(c, s.manager, "1/lxc/0")
+
+	err := s.manager.StopContainer(instance)
+	c.Assert(err, gc.IsNil)
+
+	name := string(instance.Id())
+	// Check that the container dir is no longer in the container dir
+	c.Assert(filepath.Join(s.ContainerDir, name), jc.DoesNotExist)
+	// but instead, in the removed container dir
+	c.Assert(filepath.Join(s.RemovedDir, name), jc.IsDirectory)
+}

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2013-12-01 23:54:35 +0000
+++ container/lxc/lxc_test.go	2013-12-04 04:19:23 +0000
@@ -18,14 +18,10 @@
 	"launchpad.net/juju-core/container"
 	"launchpad.net/juju-core/container/lxc"
 	lxctesting "launchpad.net/juju-core/container/lxc/testing"
-	"launchpad.net/juju-core/environs"
-	"launchpad.net/juju-core/instance"
+	containertesting "launchpad.net/juju-core/container/testing"
 	instancetest "launchpad.net/juju-core/instance/testing"
-	jujutesting "launchpad.net/juju-core/juju/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
-	"launchpad.net/juju-core/tools"
-	"launchpad.net/juju-core/version"
 )
 
 func Test(t *stdtesting.T) {
@@ -66,26 +62,9 @@
 	aptConfigScript = fmt.Sprintf("#!/bin/sh\n echo '%s\n%s'", configHttpProxy, configProxyExtra)
 )
 
-func StartContainer(c *gc.C, manager container.Manager, machineId string) instance.Instance {
-	stateInfo := jujutesting.FakeStateInfo(machineId)
-	apiInfo := jujutesting.FakeAPIInfo(machineId)
-	machineConfig := environs.NewMachineConfig(machineId, "fake-nonce", stateInfo, apiInfo)
-	machineConfig.SyslogPort = 2345
-	machineConfig.Tools = &tools.Tools{
-		Version: version.MustParseBinary("2.3.4-foo-bar"),
-		URL:     "http://tools.testing.invalid/2.3.4-foo-bar.tgz",
-	}
-
-	series := "series"
-	network := container.BridgeNetworkConfig("nic42")
-	inst, err := manager.StartContainer(machineConfig, series, network)
-	c.Assert(err, gc.IsNil)
-	return inst
-}
-
 func (s *LxcSuite) TestStartContainer(c *gc.C) {
 	manager := lxc.NewContainerManager(container.ManagerConfig{})
-	instance := StartContainer(c, manager, "1/lxc/0")
+	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
 
 	name := string(instance.Id())
 	// Check our container config files.
@@ -94,10 +73,7 @@
 	c.Assert(string(lxcConfContents), jc.Contains, "lxc.network.link = nic42")
 
 	cloudInitFilename := filepath.Join(s.ContainerDir, name, "cloud-init")
-	c.Assert(cloudInitFilename, jc.IsNonEmptyFile)
-	data, err := ioutil.ReadFile(cloudInitFilename)
-	c.Assert(err, gc.IsNil)
-	c.Assert(string(data), jc.HasPrefix, "#cloud-config\n")
+	data := containertesting.AssertCloudInit(c, cloudInitFilename)
 
 	x := make(map[interface{}]interface{})
 	err = goyaml.Unmarshal(data, &x)
@@ -133,7 +109,7 @@
 
 func (s *LxcSuite) TestContainerState(c *gc.C) {
 	manager := lxc.NewContainerManager(container.ManagerConfig{})
-	instance := StartContainer(c, manager, "1/lxc/0")
+	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
 
 	// The mock container will be immediately "running".
 	c.Assert(instance.Status(), gc.Equals, string(golxc.StateRunning))
@@ -147,7 +123,7 @@
 
 func (s *LxcSuite) TestStopContainer(c *gc.C) {
 	manager := lxc.NewContainerManager(container.ManagerConfig{})
-	instance := StartContainer(c, manager, "1/lxc/0")
+	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
 
 	err := manager.StopContainer(instance)
 	c.Assert(err, gc.IsNil)
@@ -161,7 +137,7 @@
 
 func (s *LxcSuite) TestStopContainerNameClash(c *gc.C) {
 	manager := lxc.NewContainerManager(container.ManagerConfig{})
-	instance := StartContainer(c, manager, "1/lxc/0")
+	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
 
 	name := string(instance.Id())
 	targetDir := filepath.Join(s.RemovedDir, name)
@@ -179,7 +155,7 @@
 
 func (s *LxcSuite) TestNamedManagerPrefix(c *gc.C) {
 	manager := lxc.NewContainerManager(container.ManagerConfig{Name: "eric"})
-	instance := StartContainer(c, manager, "1/lxc/0")
+	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
 	c.Assert(string(instance.Id()), gc.Equals, "eric-machine-1-lxc-0")
 }
 
@@ -187,12 +163,12 @@
 	foo := lxc.NewContainerManager(container.ManagerConfig{Name: "foo"})
 	bar := lxc.NewContainerManager(container.ManagerConfig{Name: "bar"})
 
-	foo1 := StartContainer(c, foo, "1/lxc/0")
-	foo2 := StartContainer(c, foo, "1/lxc/1")
-	foo3 := StartContainer(c, foo, "1/lxc/2")
+	foo1 := containertesting.StartContainer(c, foo, "1/lxc/0")
+	foo2 := containertesting.StartContainer(c, foo, "1/lxc/1")
+	foo3 := containertesting.StartContainer(c, foo, "1/lxc/2")
 
-	bar1 := StartContainer(c, bar, "1/lxc/0")
-	bar2 := StartContainer(c, bar, "1/lxc/1")
+	bar1 := containertesting.StartContainer(c, bar, "1/lxc/0")
+	bar2 := containertesting.StartContainer(c, bar, "1/lxc/1")
 
 	result, err := foo.ListContainers()
 	c.Assert(err, gc.IsNil)
@@ -205,14 +181,14 @@
 
 func (s *LxcSuite) TestStartContainerAutostarts(c *gc.C) {
 	manager := lxc.NewContainerManager(container.ManagerConfig{})
-	instance := StartContainer(c, manager, "1/lxc/0")
+	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
 	autostartLink := lxc.RestartSymlink(string(instance.Id()))
 	c.Assert(autostartLink, jc.IsSymlink)
 }
 
 func (s *LxcSuite) TestStopContainerRemovesAutostartLink(c *gc.C) {
 	manager := lxc.NewContainerManager(container.ManagerConfig{})
-	instance := StartContainer(c, manager, "1/lxc/0")
+	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
 	err := manager.StopContainer(instance)
 	c.Assert(err, gc.IsNil)
 	autostartLink := lxc.RestartSymlink(string(instance.Id()))

=== added directory 'container/testing'
=== added file 'container/testing/common.go'
--- container/testing/common.go	1970-01-01 00:00:00 +0000
+++ container/testing/common.go	2013-12-04 04:19:23 +0000
@@ -0,0 +1,43 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package testing
+
+import (
+	"io/ioutil"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/container"
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/instance"
+	jujutesting "launchpad.net/juju-core/juju/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/tools"
+	"launchpad.net/juju-core/version"
+)
+
+func StartContainer(c *gc.C, manager container.Manager, machineId string) instance.Instance {
+	stateInfo := jujutesting.FakeStateInfo(machineId)
+	apiInfo := jujutesting.FakeAPIInfo(machineId)
+	machineConfig := environs.NewMachineConfig(machineId, "fake-nonce", stateInfo, apiInfo)
+	machineConfig.SyslogPort = 2345
+	machineConfig.Tools = &tools.Tools{
+		Version: version.MustParseBinary("2.3.4-foo-bar"),
+		URL:     "http://tools.testing.invalid/2.3.4-foo-bar.tgz",
+	}
+
+	series := "series"
+	network := container.BridgeNetworkConfig("nic42")
+	inst, err := manager.StartContainer(machineConfig, series, network)
+	c.Assert(err, gc.IsNil)
+	return inst
+}
+
+func AssertCloudInit(c *gc.C, filename string) []byte {
+	c.Assert(filename, jc.IsNonEmptyFile)
+	data, err := ioutil.ReadFile(filename)
+	c.Assert(err, gc.IsNil)
+	c.Assert(string(data), jc.HasPrefix, "#cloud-config\n")
+	return data
+}

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2013-12-03 06:04:43 +0000
+++ environs/cloudinit/cloudinit.go	2013-12-04 04:19:23 +0000
@@ -187,6 +187,7 @@
 
 	// juju requires git for managing charm directories.
 	c.AddPackage("git")
+	c.AddPackage("cpu-checker")
 
 	c.AddScripts(
 		"set -xe", // ensure we run all the scripts or abort.

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2013-12-01 23:54:35 +0000
+++ provider/local/environ.go	2013-12-04 04:19:23 +0000
@@ -288,7 +288,7 @@
 	series := possibleTools.OneSeries()
 	logger.Debugf("StartInstance: %q, %s", machineConfig.MachineId, series)
 	machineConfig.Tools = possibleTools[0]
-	machineConfig.MachineContainerType = instance.LXC
+	machineConfig.MachineContainerType = env.config.container()
 	logger.Debugf("tools: %#v", machineConfig.Tools)
 	network := container.BridgeNetworkConfig(env.config.networkBridge())
 	if err := environs.FinishMachineConfig(machineConfig, env.config.Config, cons); err != nil {

=== modified file 'worker/provisioner/container_initialisation.go'
--- worker/provisioner/container_initialisation.go	2013-11-25 05:03:44 +0000
+++ worker/provisioner/container_initialisation.go	2013-12-04 04:19:23 +0000
@@ -148,7 +148,11 @@
 		broker = NewLxcBroker(cs.provisioner, tools, cs.config)
 	case instance.KVM:
 		initialiser = kvm.NewContainerInitialiser()
-		//TODO - implement kvm broker
+		broker, err = NewKvmBroker(cs.provisioner, tools, cs.config)
+		if err != nil {
+			logger.Errorf("failed to create new kvm broker")
+			return nil, nil, err
+		}
 	default:
 		return nil, nil, fmt.Errorf("unknown container type: %v", containerType)
 	}

=== modified file 'worker/provisioner/container_initialisation_test.go'
--- worker/provisioner/container_initialisation_test.go	2013-11-25 00:35:32 +0000
+++ worker/provisioner/container_initialisation_test.go	2013-12-04 04:19:23 +0000
@@ -187,7 +187,7 @@
 		packages []string
 	}{
 		{instance.LXC, []string{"lxc"}},
-		{instance.KVM, []string{"uvtool-libvirt", "uvtool", "kvm"}},
+		{instance.KVM, []string{"uvtool-libvirt", "uvtool"}},
 	} {
 		s.assertContainerInitialised(c, test.ctype, test.packages)
 	}

=== added file 'worker/provisioner/kvm-broker.go'
--- worker/provisioner/kvm-broker.go	1970-01-01 00:00:00 +0000
+++ worker/provisioner/kvm-broker.go	2013-12-04 04:19:23 +0000
@@ -0,0 +1,118 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package provisioner
+
+import (
+	"launchpad.net/loggo"
+
+	"launchpad.net/juju-core/agent"
+	"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/cloudinit"
+	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/tools"
+)
+
+var kvmLogger = loggo.GetLogger("juju.provisioner.kvm")
+
+var _ environs.InstanceBroker = (*kvmBroker)(nil)
+var _ tools.HasTools = (*kvmBroker)(nil)
+
+func NewKvmBroker(
+	api APICalls,
+	tools *tools.Tools,
+	agentConfig agent.Config,
+) (environs.InstanceBroker, error) {
+	manager, err := kvm.NewContainerManager(container.ManagerConfig{Name: "juju"})
+	if err != nil {
+		return nil, err
+	}
+	return &kvmBroker{
+		manager:     manager,
+		api:         api,
+		tools:       tools,
+		agentConfig: agentConfig,
+	}, nil
+}
+
+type kvmBroker struct {
+	manager     container.Manager
+	api         APICalls
+	tools       *tools.Tools
+	agentConfig agent.Config
+}
+
+func (broker *kvmBroker) Tools() tools.List {
+	return tools.List{broker.tools}
+}
+
+// StartInstance is specified in the Broker interface.
+func (broker *kvmBroker) StartInstance(
+	cons constraints.Value,
+	possibleTools tools.List,
+	machineConfig *cloudinit.MachineConfig,
+) (instance.Instance, *instance.HardwareCharacteristics, error) {
+
+	// TODO: refactor common code out of the container brokers.
+	machineId := machineConfig.MachineId
+	kvmLogger.Infof("starting kvm container for machineId: %s", machineId)
+
+	// TODO: Default to using the host network until we can configure.  Yes,
+	// this is using the LxcBridge value, we should put it in the api call for
+	// container config.
+	bridgeDevice := broker.agentConfig.Value(agent.LxcBridge)
+	if bridgeDevice == "" {
+		bridgeDevice = kvm.DefaultKvmBridge
+	}
+	network := container.BridgeNetworkConfig(bridgeDevice)
+
+	// TODO: series doesn't necessarily need to be the same as the host.
+	series := possibleTools.OneSeries()
+	machineConfig.MachineContainerType = instance.KVM
+	machineConfig.Tools = possibleTools[0]
+
+	config, err := broker.api.ContainerConfig()
+	if err != nil {
+		kvmLogger.Errorf("failed to get container config: %v", err)
+		return nil, nil, err
+	}
+	if err := environs.PopulateMachineConfig(
+		machineConfig,
+		config.ProviderType,
+		config.AuthorizedKeys,
+		config.SSLHostnameVerification,
+		config.SyslogPort,
+	); err != nil {
+		kvmLogger.Errorf("failed to populate machine config: %v", err)
+		return nil, nil, err
+	}
+
+	inst, err := broker.manager.StartContainer(machineConfig, series, network)
+	if err != nil {
+		kvmLogger.Errorf("failed to start container: %v", err)
+		return nil, nil, err
+	}
+	kvmLogger.Infof("started kvm container for machineId: %s, %s", machineId, inst.Id())
+	return inst, nil, nil
+}
+
+// StopInstances shuts down the given instances.
+func (broker *kvmBroker) StopInstances(instances []instance.Instance) error {
+	// TODO: potentially parallelise.
+	for _, instance := range instances {
+		kvmLogger.Infof("stopping kvm container for instance: %s", instance.Id())
+		if err := broker.manager.StopContainer(instance); err != nil {
+			kvmLogger.Errorf("container did not stop: %v", err)
+			return err
+		}
+	}
+	return nil
+}
+
+// AllInstances only returns running containers.
+func (broker *kvmBroker) AllInstances() (result []instance.Instance, err error) {
+	return broker.manager.ListContainers()
+}

=== added file 'worker/provisioner/kvm-broker_test.go'
--- worker/provisioner/kvm-broker_test.go	1970-01-01 00:00:00 +0000
+++ worker/provisioner/kvm-broker_test.go	2013-12-04 04:19:23 +0000
@@ -0,0 +1,256 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package provisioner_test
+
+import (
+	"fmt"
+	"path/filepath"
+	"time"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/agent"
+	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/container/kvm/mock"
+	kvmtesting "launchpad.net/juju-core/container/kvm/testing"
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/instance"
+	instancetest "launchpad.net/juju-core/instance/testing"
+	jujutesting "launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/names"
+	"launchpad.net/juju-core/state"
+	coretesting "launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
+	coretools "launchpad.net/juju-core/tools"
+	"launchpad.net/juju-core/version"
+	"launchpad.net/juju-core/worker/provisioner"
+)
+
+type kvmSuite struct {
+	kvmtesting.TestSuite
+	events chan mock.Event
+}
+
+type kvmBrokerSuite struct {
+	kvmSuite
+	broker      environs.InstanceBroker
+	agentConfig agent.Config
+}
+
+var _ = gc.Suite(&kvmBrokerSuite{})
+
+func (s *kvmSuite) SetUpTest(c *gc.C) {
+	s.TestSuite.SetUpTest(c)
+	s.events = make(chan mock.Event)
+	go func() {
+		for event := range s.events {
+			c.Output(3, fmt.Sprintf("kvm event: <%s, %s>", event.Action, event.InstanceId))
+		}
+	}()
+	s.TestSuite.Factory.AddListener(s.events)
+}
+
+func (s *kvmSuite) TearDownTest(c *gc.C) {
+	close(s.events)
+	s.TestSuite.TearDownTest(c)
+}
+
+func (s *kvmBrokerSuite) SetUpTest(c *gc.C) {
+	s.kvmSuite.SetUpTest(c)
+	tools := &coretools.Tools{
+		Version: version.MustParseBinary("2.3.4-foo-bar"),
+		URL:     "http://tools.testing.invalid/2.3.4-foo-bar.tgz",
+	}
+	var err error
+	s.agentConfig, err = agent.NewAgentConfig(
+		agent.AgentConfigParams{
+			DataDir:      "/not/used/here",
+			Tag:          "tag",
+			Password:     "dummy-secret",
+			Nonce:        "nonce",
+			APIAddresses: []string{"10.0.0.1:1234"},
+			CACert:       []byte(coretesting.CACert),
+		})
+	c.Assert(err, gc.IsNil)
+	s.broker, err = provisioner.NewKvmBroker(&fakeAPI{}, tools, s.agentConfig)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *kvmBrokerSuite) startInstance(c *gc.C, machineId string) instance.Instance {
+	machineNonce := "fake-nonce"
+	stateInfo := jujutesting.FakeStateInfo(machineId)
+	apiInfo := jujutesting.FakeAPIInfo(machineId)
+	machineConfig := environs.NewMachineConfig(machineId, machineNonce, stateInfo, apiInfo)
+	cons := constraints.Value{}
+	possibleTools := s.broker.(coretools.HasTools).Tools()
+	kvm, _, err := s.broker.StartInstance(cons, possibleTools, machineConfig)
+	c.Assert(err, gc.IsNil)
+	return kvm
+}
+
+func (s *kvmBrokerSuite) TestStopInstance(c *gc.C) {
+	kvm0 := s.startInstance(c, "1/kvm/0")
+	kvm1 := s.startInstance(c, "1/kvm/1")
+	kvm2 := s.startInstance(c, "1/kvm/2")
+
+	err := s.broker.StopInstances([]instance.Instance{kvm0})
+	c.Assert(err, gc.IsNil)
+	s.assertInstances(c, kvm1, kvm2)
+	c.Assert(s.kvmContainerDir(kvm0), jc.DoesNotExist)
+	c.Assert(s.kvmRemovedContainerDir(kvm0), jc.IsDirectory)
+
+	err = s.broker.StopInstances([]instance.Instance{kvm1, kvm2})
+	c.Assert(err, gc.IsNil)
+	s.assertInstances(c)
+}
+
+func (s *kvmBrokerSuite) TestAllInstances(c *gc.C) {
+	kvm0 := s.startInstance(c, "1/kvm/0")
+	kvm1 := s.startInstance(c, "1/kvm/1")
+	s.assertInstances(c, kvm0, kvm1)
+
+	err := s.broker.StopInstances([]instance.Instance{kvm1})
+	c.Assert(err, gc.IsNil)
+	kvm2 := s.startInstance(c, "1/kvm/2")
+	s.assertInstances(c, kvm0, kvm2)
+}
+
+func (s *kvmBrokerSuite) assertInstances(c *gc.C, inst ...instance.Instance) {
+	results, err := s.broker.AllInstances()
+	c.Assert(err, gc.IsNil)
+	instancetest.MatchInstances(c, results, inst...)
+}
+
+func (s *kvmBrokerSuite) kvmContainerDir(inst instance.Instance) string {
+	return filepath.Join(s.ContainerDir, string(inst.Id()))
+}
+
+func (s *kvmBrokerSuite) kvmRemovedContainerDir(inst instance.Instance) string {
+	return filepath.Join(s.RemovedDir, string(inst.Id()))
+}
+
+type kvmProvisionerSuite struct {
+	CommonProvisionerSuite
+	kvmSuite
+	machineId string
+	events    chan mock.Event
+}
+
+var _ = gc.Suite(&kvmProvisionerSuite{})
+
+func (s *kvmProvisionerSuite) SetUpSuite(c *gc.C) {
+	s.CommonProvisionerSuite.SetUpSuite(c)
+	s.kvmSuite.SetUpSuite(c)
+}
+
+func (s *kvmProvisionerSuite) TearDownSuite(c *gc.C) {
+	s.kvmSuite.TearDownSuite(c)
+	s.CommonProvisionerSuite.TearDownSuite(c)
+}
+
+func (s *kvmProvisionerSuite) SetUpTest(c *gc.C) {
+	s.CommonProvisionerSuite.SetUpTest(c)
+	s.kvmSuite.SetUpTest(c)
+
+	// The kvm provisioner actually needs the machine it is being created on
+	// to be in state, in order to get the watcher.
+	m, err := s.State.AddMachine(config.DefaultSeries, state.JobHostUnits, state.JobManageState)
+	c.Assert(err, gc.IsNil)
+	err = m.SetAddresses([]instance.Address{
+		instance.NewAddress("0.1.2.3"),
+	})
+	c.Assert(err, gc.IsNil)
+	s.machineId = m.Id()
+	s.APILogin(c, m)
+	err = m.SetAgentVersion(version.Current)
+	c.Assert(err, gc.IsNil)
+
+	s.events = make(chan mock.Event, 25)
+	s.Factory.AddListener(s.events)
+}
+
+func (s *kvmProvisionerSuite) expectStarted(c *gc.C, machine *state.Machine) string {
+	s.State.StartSync()
+	event := <-s.events
+	c.Assert(event.Action, gc.Equals, mock.Started)
+	err := machine.Refresh()
+	c.Assert(err, gc.IsNil)
+	s.waitInstanceId(c, machine, instance.Id(event.InstanceId))
+	return event.InstanceId
+}
+
+func (s *kvmProvisionerSuite) expectStopped(c *gc.C, instId string) {
+	s.State.StartSync()
+	event := <-s.events
+	c.Assert(event.Action, gc.Equals, mock.Stopped)
+	c.Assert(event.InstanceId, gc.Equals, instId)
+}
+
+func (s *kvmProvisionerSuite) expectNoEvents(c *gc.C) {
+	select {
+	case event := <-s.events:
+		c.Fatalf("unexpected event %#v", event)
+	case <-time.After(coretesting.ShortWait):
+		return
+	}
+}
+
+func (s *kvmProvisionerSuite) TearDownTest(c *gc.C) {
+	close(s.events)
+	s.kvmSuite.TearDownTest(c)
+	s.CommonProvisionerSuite.TearDownTest(c)
+}
+
+func (s *kvmProvisionerSuite) newKvmProvisioner(c *gc.C) provisioner.Provisioner {
+	machineTag := names.MachineTag(s.machineId)
+	agentConfig := s.AgentConfigForTag(c, machineTag)
+	tools, err := s.provisioner.Tools(agentConfig.Tag())
+	c.Assert(err, gc.IsNil)
+	broker, err := provisioner.NewKvmBroker(s.provisioner, tools, agentConfig)
+	c.Assert(err, gc.IsNil)
+	return provisioner.NewContainerProvisioner(instance.KVM, s.provisioner, agentConfig, broker)
+}
+
+func (s *kvmProvisionerSuite) TestProvisionerStartStop(c *gc.C) {
+	p := s.newKvmProvisioner(c)
+	c.Assert(p.Stop(), gc.IsNil)
+}
+
+func (s *kvmProvisionerSuite) TestDoesNotStartEnvironMachines(c *gc.C) {
+	p := s.newKvmProvisioner(c)
+	defer stop(c, p)
+
+	// Check that an instance is not provisioned when the machine is created.
+	_, err := s.State.AddMachine(config.DefaultSeries, state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+
+	s.expectNoEvents(c)
+}
+
+func (s *kvmProvisionerSuite) addContainer(c *gc.C) *state.Machine {
+	params := state.AddMachineParams{
+		ParentId:      s.machineId,
+		ContainerType: instance.KVM,
+		Series:        config.DefaultSeries,
+		Jobs:          []state.MachineJob{state.JobHostUnits},
+	}
+	container, err := s.State.AddMachineWithConstraints(&params)
+	c.Assert(err, gc.IsNil)
+	return container
+}
+
+func (s *kvmProvisionerSuite) TestContainerStartedAndStopped(c *gc.C) {
+	p := s.newKvmProvisioner(c)
+	defer stop(c, p)
+
+	container := s.addContainer(c)
+
+	instId := s.expectStarted(c, container)
+
+	// ...and removed, along with the machine, when the machine is Dead.
+	c.Assert(container.EnsureDead(), gc.IsNil)
+	s.expectStopped(c, instId)
+	s.waitRemoved(c, container)
+}

=== modified file 'worker/provisioner/lxc-broker.go'
--- worker/provisioner/lxc-broker.go	2013-12-01 23:54:35 +0000
+++ worker/provisioner/lxc-broker.go	2013-12-04 04:19:23 +0000
@@ -49,7 +49,7 @@
 // StartInstance is specified in the Broker interface.
 func (broker *lxcBroker) StartInstance(cons constraints.Value, possibleTools tools.List,
 	machineConfig *cloudinit.MachineConfig) (instance.Instance, *instance.HardwareCharacteristics, error) {
-
+	// TODO: refactor common code out of the container brokers.
 	machineId := machineConfig.MachineId
 	lxcLogger.Infof("starting lxc container for machineId: %s", machineId)