Merge lp:~waigani/juju-core/add-start-stop-to-container-manager-interface into lp:~go-bot/juju-core/trunk

[Jesse Meek]

Container Manager Interface

Add StartContainer and StopContainer to
containerManger interface and implement
for lxc and kvm.

https://codereview.appspot.com/77970043/

[Jesse Meek (waigani)]

Reviewers: mp+211832_code.launchpad.net,

Message:
Please take a look.

Description:
Container Manager Interface

Add StartContainer and StopContainer to
containerManger interface and implement
for lxc and kvm.

https://code.launchpad.net/~waigani/juju-core/add-start-stop-to-container-manager-interface/+merge/211832

(do not edit description out of merge proposal)

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

Affected files (+88, -66 lines):
   A [revision details]
   M container/interface.go
   M container/kvm/kvm.go
   M container/kvm/kvm_test.go
   M container/kvm/live_test.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go
   M container/testing/common.go
   M provider/local/environ.go
   M worker/provisioner/kvm-broker.go
   M worker/provisioner/lxc-broker.go

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/77970043/

[William Reade (fwereade)]

I'm a bit nervous about the consistency of start/stop vs create/destroy,
because the InstanceBroker interface has different meanings. Please make
that interface consistent -- assuming you can do so completely
mechanically, LGTM; the rest are just trivials.

https://codereview.appspot.com/77970043/diff/20001/container/interface.go
File container/interface.go (right):

https://codereview.appspot.com/77970043/diff/20001/container/interface.go#newcode31
container/interface.go:31: StartContainer(instance.Instance) error
Blank lines between methods, please, it's getting harder to read.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go
File container/kvm/container.go (left):

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go#oldcode20
container/kvm/container.go:20: }
One concern about caching these values: how will they hold up if someone
messes with our containers out-of-band? I'm mainly worried about the
local provider here.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go
File container/kvm/container.go (right):

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go#newcode19
container/kvm/container.go:19: exists *bool
and between fields as well, actually

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go#newcode96
container/kvm/container.go:96: logger.Debugf("Start %s", c.name)
s/Start/start/

https://codereview.appspot.com/77970043/

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/77970043/diff/20001/container/interface.go
File container/interface.go (right):

https://codereview.appspot.com/77970043/diff/20001/container/interface.go#newcode31
container/interface.go:31: StartContainer(instance.Instance) error
On 2014/03/24 09:51:56, fwereade wrote:
> Blank lines between methods, please, it's getting harder to read.

Done.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go
File container/kvm/container.go (left):

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go#oldcode20
container/kvm/container.go:20: }
On 2014/03/24 09:51:56, fwereade wrote:
> One concern about caching these values: how will they hold up if
someone messes
> with our containers out-of-band? I'm mainly worried about the local
provider
> here.

Good point. What is the cache actually giving us? How often do we call
the container methods Start, Stop, Create and Destroy and does the
expense of calling down to virsh and uvt-kvm each time have any
significant consequence? <- Questions for thumper.

Currently the container methods reset the values to nil after a
successful cache check. We could also reset the cache to nil if the
cache is not as expected and then rerun the check. But really, this is
just circumventing the cache.

On the other hand, what is our policy if people do mess with containers
out-of-band? Is that something we need to seamlessly recover from? We
could reset the cache to nil if the container methods error out for any
reason, so the next time they are called it will work. BUT this is just
adding more complexity at the expense of user experience.

Sigh, thinking out loud here ... ditching the cache seems simplest to
me.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go
File container/kvm/container.go (right):

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go#newcode19
container/kvm/container.go:19: exists *bool
On 2014/03/24 09:51:56, fwereade wrote:
> and between fields as well, actually

Done.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go#newcode96
container/kvm/container.go:96: logger.Debugf("Start %s", c.name)
On 2014/03/24 09:51:56, fwereade wrote:
> s/Start/start/

Done.

https://codereview.appspot.com/77970043/

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/77970043/

[Jesse Meek (waigani)]

https://codereview.appspot.com/77970043/diff/60001/container/kvm/mock/mock-kvm_test.go
File container/kvm/mock/mock-kvm_test.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/mock/mock-kvm_test.go#newcode181
container/kvm/mock/mock-kvm_test.go:181: // first.Start()
The commented lines cause a go routine timeout. I'm not sure why?

https://codereview.appspot.com/77970043/

[Tim Penhey (thumper)]

NOT LGTM yet

https://codereview.appspot.com/77970043/diff/60001/container/interface.go
File container/interface.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/interface.go#newcode35
container/interface.go:35: // StopCOntainer stops a container
s/StopCOntainer/StopContainer/

StopContainer stops a started container?
What if the container is already stopped? Do we error?

https://codereview.appspot.com/77970043/diff/60001/container/kvm/container.go
File container/kvm/container.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/container.go#newcode21
container/kvm/container.go:21: exists *bool
We should move from a tree state boolean (which was sufficient), to an
actual state.

type kvmState string
var (
   unknown kvmState = "unknown"
   created kvmState = "created"
   running kvmState = "running"
   ...
)

https://codereview.appspot.com/77970043/diff/60001/container/kvm/container.go#newcode41
container/kvm/container.go:41: c.exists = nil
wat?

https://codereview.appspot.com/77970043/diff/60001/container/kvm/interface.go
File container/kvm/interface.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/interface.go#newcode32
container/kvm/interface.go:32: Destroy() error
What happens if the container has stopped?

https://codereview.appspot.com/77970043/diff/60001/container/kvm/interface.go#newcode34
container/kvm/interface.go:34: Start() error
need at least minimal docstrings on the methods

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go
File container/kvm/kvm.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go#newcode82
container/kvm/kvm.go:82: return nil
pretty sure that start container shouldn't do nothing

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go#newcode88
container/kvm/kvm.go:88: return kvmContainer.Stop()
doc string should at least say what happens in the error conditions.

what if the container doesn't exist, what if it is already stopped, etc

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go#newcode131
container/kvm/kvm.go:131: if err := kvmContainer.Create(startParams);
err != nil {
Worth noting that that Create both creates and starts the container.

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go#newcode141
container/kvm/kvm.go:141: if err := kvmContainer.Destroy(); err != nil {
equally worth noting that Destroy both stops and destroys the container.

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

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm_test.go#newcode113
container/kvm/kvm_test.go:113:
c.Assert(containertesting.StopContainer(c, s.manager, instance),
gc.IsNil)
What about starting a started container, or stopping a stopped
container? the mock should as closely as possible mirror what the virsh
commands return

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

https://codereview.appspot.com/77970043/diff/60001/container/kvm/libvirt.go#newcode112
container/kvm/...

[Tim Penhey (thumper)]

On 2014/03/24 09:51:56, fwereade wrote:
> I'm a bit nervous about the consistency of start/stop vs
create/destroy, because
> the InstanceBroker interface has different meanings. Please make that
interface
> consistent -- assuming you can do so completely mechanically, LGTM;
the rest are
> just trivials.

The InstanceBroker is aimed at the provisioner, and for the provider to
support the methods
that will be called by the provisioner.

We have the lxcBroker and kvmBroker to do this mapping for us in the
provisioner code.

At the container level, it makes sense to have Create, Start, Stop, and
Destroy. This is
quite independent from the Broker.

https://codereview.appspot.com/77970043/

[William Reade (fwereade)]

On 2014/03/26 02:27:50, thumper wrote:
> On 2014/03/24 09:51:56, fwereade wrote:
> > I'm a bit nervous about the consistency of start/stop vs
create/destroy,
> because
> > the InstanceBroker interface has different meanings. Please make
that
> interface
> > consistent -- assuming you can do so completely mechanically, LGTM;
the rest
> are
> > just trivials.

> The InstanceBroker is aimed at the provisioner, and for the provider
to support
> the methods
> that will be called by the provisioner.

> We have the lxcBroker and kvmBroker to do this mapping for us in the
provisioner
> code.

> At the container level, it makes sense to have Create, Start, Stop,
and Destroy.
> This is
> quite independent from the Broker.

As discussed live: I'd like the Start/Stop/Create/Destroy vocabulary to
have a consistent meaning across the codebase -- at least when talking
about such similar things as different implementations of Instance. If
that means the Broker methods start talking Create/Destroy, that works
just fine. I'm not asking for any sort of rearchitecting, just naming
consistency :).

https://codereview.appspot.com/77970043/

[William Reade (fwereade)]

WIP to address tim's comments.

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/77970043/

[Jesse Meek (waigani)]

On 2014/03/31 06:15:26, waigani wrote:
> Please take a look.

I missed container/lxc/lxc.go:238, passing in instance. I'll follow up
with that. Should I make a new branch for updating the Broker interface?

https://codereview.appspot.com/77970043/

[lp:~waigani/juju-core/add-start-stop-to-container-manager-interface]

2456. By Jesse Meek on 2014-03-31

Checked KVM states via virsh and uvt-kvm and matched them against KvmStatus

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/77970043/

Unmerged revisions

2456. By Jesse Meek on 2014-03-31

Checked KVM states via virsh and uvt-kvm and matched them against KvmStatus

2455. By Jesse Meek on 2014-03-31

Merge trunk.

2454. By Jesse Meek on 2014-03-31

Add KvmState and KvmStatus. Update container methods and tests to use them.

2453. By Jesse Meek on 2014-03-25

added tests to mock-kvm and fixed tests in kvm-broker

2452. By Jesse Meek on 2014-03-24

Code cleanup from review.

2451. By Jesse Meek on 2014-03-21

merge trunk, fix tests

2450. By Jesse Meek on 2014-03-20

implement lxc start/stop container, add tests

2449. By Jesse Meek on 2014-03-19

Added StartContainer and StopContainer

=== modified file 'container/interface.go'
--- container/interface.go	2014-03-19 21:08:58 +0000
+++ container/interface.go	2014-03-31 22:42:50 +0000
@@ -25,8 +25,16 @@
 		machineConfig *cloudinit.MachineConfig,
 		series string,
 		network *NetworkConfig) (instance.Instance, *instance.HardwareCharacteristics, error)
-	// DestroyContainer stops and destroyes the container identified by Instance.
+
+	// DestroyContainer stops and destroys the container identified by Instance.
 	DestroyContainer(instance.Instance) error
+
+	// StartContainer starts a stopped container
+	StartContainer(instance.Instance) error
+
+	// StopContainer stops a started container
+	StopContainer(instance.Instance) error
+
 	// ListContainers return a list of containers that have been started by
 	// this manager.
 	ListContainers() ([]instance.Instance, error)

=== modified file 'container/kvm/container.go'
--- container/kvm/container.go	2013-12-05 22:32:47 +0000
+++ container/kvm/container.go	2014-03-31 22:42:50 +0000
@@ -10,13 +10,20 @@
 	"launchpad.net/juju-core/log"
 )
 
+// KvmStatus is a cached status of the container
+type KvmStatus string
+
+const (
+	Unknown   KvmStatus = "unknown"
+	Stopped   KvmStatus = "stopped"
+	Running   KvmStatus = "running"
+	Destroyed KvmStatus = "destroyed"
+)
+
 type kvmContainer struct {
-	factory *containerFactory
-	name    string
-	// 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
+	factory      *containerFactory
+	name         string
+	cachedStatus KvmStatus
 }
 
 var _ Container = (*kvmContainer)(nil)
@@ -25,7 +32,11 @@
 	return c.name
 }
 
-func (c *kvmContainer) Start(params StartParams) error {
+func (c *kvmContainer) Create(params StartParams) error {
+	if c.Exists() {
+		logger.Debugf("%s already exists", c.name)
+		return nil
+	}
 	logger.Debugf("Synchronise images for %s %s", params.Series, params.Arch)
 	if err := SyncImages(params.Series, params.Arch); err != nil {
 		return err
@@ -49,34 +60,74 @@
 		CpuCores:      params.CpuCores,
 		RootDisk:      params.RootDisk,
 	}); err != nil {
+		c.cachedStatus = Unknown
 		return err
 	}
-
+	c.cachedStatus = Running
 	logger.Debugf("Set machine %s to autostart", c.name)
 	return AutostartMachine(c.name)
 }
 
+func (c *kvmContainer) Destroy() error {
+	if !c.Exists() {
+		logger.Debugf("%s does not exist", c.name)
+		return nil
+	}
+	logger.Debugf("Destroy %s", c.name)
+	if err := DestroyMachine(c.name); err != nil {
+		c.cachedStatus = Unknown
+		return err
+	}
+	c.cachedStatus = Destroyed
+	return nil
+
+}
+
 func (c *kvmContainer) Stop() error {
-	if !c.IsRunning() {
-		logger.Debugf("%s is already stopped", c.name)
+	if c.Status() == Stopped {
+		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 {
-	if c.started != nil {
-		return *c.started
-	}
+	if err := StopMachine(c.name); err != nil {
+		c.cachedStatus = Unknown
+		return err
+	}
+	c.cachedStatus = Stopped
+	return nil
+}
+
+func (c *kvmContainer) Start() error {
+	if c.Status() == Running {
+		logger.Debugf("%s is already Running", c.name)
+		return nil
+	}
+	logger.Debugf("start %s", c.name)
+	if err := StartMachine(c.name); err != nil {
+		c.cachedStatus = Unknown
+		return err
+	}
+	c.cachedStatus = Running
+	return nil
+}
+
+func (c *kvmContainer) Status() KvmStatus {
+	if c.cachedStatus == Unknown {
+		c.cachedStatus = GetStatusFromState(c.State())
+	}
+	return c.cachedStatus
+}
+
+func (c *kvmContainer) State() KvmState {
 	machines, err := ListMachines()
 	if err != nil {
-		return false
+		return KvmState("")
 	}
-	c.started = isRunning(machines[c.name])
-	return *c.started
+	return KvmState(machines[c.name])
+}
+
+func (c *kvmContainer) Exists() bool {
+	return c.Status() == Running || c.Status() == Stopped
 }
 
 func (c *kvmContainer) String() string {

=== modified file 'container/kvm/containerfactory.go'
--- container/kvm/containerfactory.go	2013-11-18 22:26:36 +0000
+++ container/kvm/containerfactory.go	2014-03-31 22:42:50 +0000
@@ -15,12 +15,38 @@
 	}
 }
 
-func isRunning(value string) *bool {
-	var result *bool = new(bool)
-	if value == "running" {
-		*result = true
-	}
-	return result
+// KvmState is the actual state of the kvm container, as returned by the cmd "uvt-kvm list" or
+// "virsh list".
+type KvmState string
+
+const (
+	KvmRunning     KvmState = "running"
+	KvmShutoff     KvmState = "shut off"
+	KvmStopped     KvmState = "stopped"
+	KvmIdle        KvmState = "idle"
+	KvmPaused      KvmState = "paused"
+	KvmShutdown    KvmState = "shutdown"
+	KvmCrashed     KvmState = "crashed"
+	KvmDying       KvmState = "dying"
+	KvmPmSuspended KvmState = "pmsuspended"
+)
+
+func GetStatusFromState(state KvmState) KvmStatus {
+	stateStatus := map[KvmState]KvmStatus{
+		KvmRunning:     Running,
+		KvmIdle:        Running,
+		KvmPaused:      Running,
+		KvmPmSuspended: Running,
+		KvmStopped:     Stopped,
+		KvmShutdown:    Stopped,
+		KvmShutoff:     Stopped,
+		KvmCrashed:     Stopped,
+		KvmDying:       Stopped,
+	}
+	if _, ok := stateStatus[state]; !ok {
+		return Unknown
+	}
+	return stateStatus[state]
 }
 
 func (factory *containerFactory) List() (result []Container, err error) {
@@ -28,11 +54,11 @@
 	if err != nil {
 		return nil, err
 	}
-	for hostname, status := range machines {
+	for hostname, state := range machines {
 		result = append(result, &kvmContainer{
-			factory: factory,
-			name:    hostname,
-			started: isRunning(status),
+			factory:      factory,
+			name:         hostname,
+			cachedStatus: GetStatusFromState(state),
 		})
 
 	}

=== modified file 'container/kvm/instance.go'
--- container/kvm/instance.go	2013-12-16 09:53:26 +0000
+++ container/kvm/instance.go	2014-03-31 22:42:50 +0000
@@ -23,10 +23,7 @@
 
 // Status implements instance.Instance.Status.
 func (kvm *kvmInstance) Status() string {
-	if kvm.container.IsRunning() {
-		return "running"
-	}
-	return "stopped"
+	return string(kvm.container.Status())
 }
 
 func (*kvmInstance) Refresh() error {

=== modified file 'container/kvm/interface.go'
--- container/kvm/interface.go	2013-12-05 03:38:24 +0000
+++ container/kvm/interface.go	2014-03-31 22:42:50 +0000
@@ -25,14 +25,19 @@
 	// Name returns the name of the container.
 	Name() string
 
-	// Start runs the container as a daemon.
-	Start(params StartParams) error
-
-	// Stop terminates the running container.
+	// Create runs the container as a daemon.
+	Create(params StartParams) error
+
+	// Destroy terminates the container.
+	Destroy() error
+
+	// Start starts the container.
+	Start() error
+
+	// Stop stops the container.
 	Stop() error
 
-	// IsRunning returns wheter or not the container is running and active.
-	IsRunning() bool
+	Status() KvmStatus
 
 	// String returns information about the container, like the name, state,
 	// and process id.

=== modified file 'container/kvm/kvm.go'
--- container/kvm/kvm.go	2014-03-19 21:08:58 +0000
+++ container/kvm/kvm.go	2014-03-31 22:42:50 +0000
@@ -78,6 +78,21 @@
 
 var _ container.Manager = (*containerManager)(nil)
 
+// StartContainer starts a container that exists but is stopped.
+func (manager *containerManager) StartContainer(instance instance.Instance) error {
+	name := string(instance.Id())
+	kvmContainer := KvmObjectFactory.New(name)
+	return kvmContainer.Start()
+}
+
+// StopContainer stops an existing container that is running.
+func (manager *containerManager) StopContainer(instance instance.Instance) error {
+	name := string(instance.Id())
+	kvmContainer := KvmObjectFactory.New(name)
+	return kvmContainer.Stop()
+}
+
+// CreateContainer creates and starts the container.
 func (manager *containerManager) CreateContainer(
 	machineConfig *cloudinit.MachineConfig,
 	series string,
@@ -118,17 +133,18 @@
 	}
 
 	logger.Tracef("create the container, constraints: %v", machineConfig.Constraints)
-	if err := kvmContainer.Start(startParams); err != nil {
+	if err := kvmContainer.Create(startParams); err != nil {
 		return nil, nil, log.LoggedErrorf(logger, "kvm container creation failed: %v", err)
 	}
 	logger.Tracef("kvm container created")
 	return &kvmInstance{kvmContainer, name}, &hardware, nil
 }
 
+// DestroyContainer stops and destroys the container.
 func (manager *containerManager) DestroyContainer(instance instance.Instance) error {
 	name := string(instance.Id())
 	kvmContainer := KvmObjectFactory.New(name)
-	if err := kvmContainer.Stop(); err != nil {
+	if err := kvmContainer.Destroy(); err != nil {
 		logger.Errorf("failed to stop kvm container: %v", err)
 		return err
 	}
@@ -148,7 +164,7 @@
 		if !strings.HasPrefix(name, managerPrefix) {
 			continue
 		}
-		if container.IsRunning() {
+		if container.Status() == Running {
 			result = append(result, &kvmInstance{container, name})
 		}
 	}

=== modified file 'container/kvm/kvm_test.go'
--- container/kvm/kvm_test.go	2014-03-19 21:08:58 +0000
+++ container/kvm/kvm_test.go	2014-03-31 22:42:50 +0000
@@ -58,7 +58,7 @@
 func (s *KVMSuite) createRunningContainer(c *gc.C, name string) kvm.Container {
 	kvmContainer := s.Factory.New(name)
 	network := container.BridgeNetworkConfig("testbr0")
-	c.Assert(kvmContainer.Start(kvm.StartParams{
+	c.Assert(kvmContainer.Create(kvm.StartParams{
 		Series:       "quantal",
 		Arch:         version.Current.Arch,
 		UserDataFile: "userdata.txt",
@@ -108,6 +108,17 @@
 	c.Assert(filepath.Join(s.RemovedDir, name), jc.IsDirectory)
 }
 
+func (s *KVMSuite) TestStartContainer(c *gc.C) {
+	instance := containertesting.CreateContainer(c, s.manager, "1/kvm/0")
+	c.Assert(containertesting.StopContainer(c, s.manager, instance), gc.IsNil)
+	c.Assert(containertesting.StartContainer(c, s.manager, instance), gc.IsNil)
+}
+
+func (s *KVMSuite) TestStopContainer(c *gc.C) {
+	instance := containertesting.CreateContainer(c, s.manager, "1/kvm/0")
+	c.Assert(containertesting.StopContainer(c, s.manager, instance), gc.IsNil)
+}
+
 type ConstraintsSuite struct {
 	testbase.LoggingSuite
 }

=== modified file 'container/kvm/libvirt.go'
--- container/kvm/libvirt.go	2013-12-05 22:32:47 +0000
+++ container/kvm/libvirt.go	2014-03-31 22:42:50 +0000
@@ -104,6 +104,16 @@
 	return err
 }
 
+func StopMachine(hostname string) error {
+	_, err := run("virsh", "destroy", hostname)
+	return err
+}
+
+func StartMachine(hostname string) error {
+	_, err := run("virsh", "start", hostname)
+	return err
+}
+
 // AutostartMachine indicates that the virtual machines should automatically
 // restart when the host restarts.
 func AutostartMachine(hostname string) error {
@@ -113,7 +123,7 @@
 
 // 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) {
+func ListMachines() (map[string]KvmState, error) {
 	output, err := run("virsh", "-q", "list", "--all")
 	if err != nil {
 		return nil, err
@@ -122,11 +132,11 @@
 	// 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)
+	result := make(map[string]KvmState)
 	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]
+		result[parts[0]] = KvmState(parts[1])
 	}
 	return result, nil
 }

=== modified file 'container/kvm/mock/mock-kvm.go'
--- container/kvm/mock/mock-kvm.go	2013-12-05 03:38:24 +0000
+++ container/kvm/mock/mock-kvm.go	2014-03-31 22:42:50 +0000
@@ -15,14 +15,22 @@
 type Action int
 
 const (
+	// A container has been created..
+	Created Action = iota
+	// A container has been destroyed.
+	Destroyed
 	// A container has been started.
-	Started Action = iota
+	Started
 	// A container has been stopped.
 	Stopped
 )
 
 func (action Action) String() string {
 	switch action {
+	case Created:
+		return "Created"
+	case Destroyed:
+		return "Destroyed"
 	case Started:
 		return "Started"
 	case Stopped:
@@ -45,48 +53,77 @@
 }
 
 type mockFactory struct {
-	instances map[string]kvm.Container
+	instances map[string]*mockContainer
 	listeners []chan<- Event
 }
 
-func MockFactory() ContainerFactory {
+func MockFactory() *mockFactory {
 	return &mockFactory{
-		instances: make(map[string]kvm.Container),
+		instances: make(map[string]*mockContainer),
 	}
 }
 
 type mockContainer struct {
-	factory *mockFactory
-	name    string
-	started bool
+	factory      *mockFactory
+	name         string
+	cachedStatus kvm.KvmStatus
+	mockState    kvm.KvmState
 }
 
+var _ kvm.Container = (*mockContainer)(nil)
+var _ ContainerFactory = (*mockFactory)(nil)
+
 // Name returns the name of the container.
 func (mock *mockContainer) Name() string {
 	return mock.name
 }
 
-func (mock *mockContainer) Start(params kvm.StartParams) error {
-	if mock.started {
+func (mock *mockContainer) Create(params kvm.StartParams) error {
+	if mock.Exists() {
+		return fmt.Errorf("container already exists")
+	}
+	mock.factory.notify(Created, mock.name)
+	mock.cachedStatus = kvm.Running
+	return nil
+}
+
+// Destroy terminates the running container.
+func (mock *mockContainer) Destroy() error {
+	if !mock.Exists() {
+		return fmt.Errorf("container does not exist")
+	}
+	mock.factory.notify(Destroyed, mock.name)
+	mock.cachedStatus = kvm.Destroyed
+	return nil
+}
+
+func (mock *mockContainer) Start() error {
+	if mock.Status() == kvm.Running {
 		return fmt.Errorf("container is already running")
 	}
-	mock.started = true
 	mock.factory.notify(Started, mock.name)
+	mock.cachedStatus = kvm.Running
 	return nil
 }
 
-// Stop terminates the running container.
 func (mock *mockContainer) Stop() error {
-	if !mock.started {
+	if mock.Status() != kvm.Running {
 		return fmt.Errorf("container is not running")
 	}
-	mock.started = false
 	mock.factory.notify(Stopped, mock.name)
+	mock.cachedStatus = kvm.Stopped
 	return nil
 }
 
-func (mock *mockContainer) IsRunning() bool {
-	return mock.started
+func (mock *mockContainer) Status() kvm.KvmStatus {
+	if mock.cachedStatus == kvm.Unknown {
+		mock.cachedStatus = kvm.GetStatusFromState(mock.State())
+	}
+	return mock.cachedStatus
+}
+
+func (c *mockContainer) Exists() bool {
+	return c.Status() == kvm.Running || c.Status() == kvm.Stopped
 }
 
 // String returns information about the container.
@@ -103,9 +140,12 @@
 	if ok {
 		return container
 	}
+	state := kvm.KvmState("")
 	container = &mockContainer{
-		factory: mock,
-		name:    name,
+		factory:      mock,
+		name:         name,
+		cachedStatus: kvm.GetStatusFromState(state),
+		mockState:    state,
 	}
 	mock.instances[name] = container
 	return container
@@ -118,6 +158,16 @@
 	return
 }
 
+func (mock *mockContainer) State() kvm.KvmState {
+	factory := MockFactory()
+	for _, container := range factory.instances {
+		if container.Name() == mock.name {
+			return container.mockState
+		}
+	}
+	return kvm.KvmState("")
+}
+
 func (mock *mockFactory) notify(action Action, instanceId string) {
 	event := Event{action, instanceId}
 	for _, c := range mock.listeners {

=== modified file 'container/kvm/mock/mock-kvm_test.go'
--- container/kvm/mock/mock-kvm_test.go	2014-03-13 07:54:56 +0000
+++ container/kvm/mock/mock-kvm_test.go	2014-03-31 22:42:50 +0000
@@ -39,41 +39,83 @@
 	factory := mock.MockFactory()
 	container := factory.New("first")
 	c.Assert(container.Name(), gc.Equals, "first")
-	c.Assert(container.IsRunning(), jc.IsFalse)
+	c.Assert(container.Status(), gc.Not(gc.Equals), kvm.Running)
 }
 
 func (*MockSuite) TestContainerStoppingStoppedErrors(c *gc.C) {
 	factory := mock.MockFactory()
 	container := factory.New("first")
-	err := container.Stop()
+	err := container.Create(kvm.StartParams{})
+	c.Assert(err, gc.IsNil)
+	err = container.Stop()
+	c.Assert(err, gc.IsNil)
+	err = container.Stop()
 	c.Assert(err, gc.ErrorMatches, "container is not running")
 }
 
+func (*MockSuite) TestContainerDestroyDestroyedErrors(c *gc.C) {
+	factory := mock.MockFactory()
+	container := factory.New("first")
+	err := container.Destroy()
+	c.Assert(err, gc.ErrorMatches, "container does not exist")
+}
+
+func (*MockSuite) TestContainerCreateStarts(c *gc.C) {
+	factory := mock.MockFactory()
+	container := factory.New("first")
+	err := container.Create(kvm.StartParams{})
+	c.Assert(err, gc.IsNil)
+	c.Assert(container.Status(), gc.Equals, kvm.Running)
+}
+
 func (*MockSuite) TestContainerStartStarts(c *gc.C) {
 	factory := mock.MockFactory()
 	container := factory.New("first")
-	err := container.Start(kvm.StartParams{})
-	c.Assert(err, gc.IsNil)
-	c.Assert(container.IsRunning(), jc.IsTrue)
+	err := container.Create(kvm.StartParams{})
+	c.Assert(err, gc.IsNil)
+	err = container.Stop()
+	c.Assert(err, gc.IsNil)
+	err = container.Start()
+	c.Assert(err, gc.IsNil)
+	c.Assert(container.Status(), gc.Equals, kvm.Running)
 }
 
 func (*MockSuite) TestContainerStartingRunningErrors(c *gc.C) {
 	factory := mock.MockFactory()
 	container := factory.New("first")
-	err := container.Start(kvm.StartParams{})
+	err := container.Create(kvm.StartParams{})
 	c.Assert(err, gc.IsNil)
-	err = container.Start(kvm.StartParams{})
+	err = container.Start()
 	c.Assert(err, gc.ErrorMatches, "container is already running")
 }
 
+func (*MockSuite) TestContainerCreatingRunningErrors(c *gc.C) {
+	factory := mock.MockFactory()
+	container := factory.New("first")
+	err := container.Create(kvm.StartParams{})
+	c.Assert(err, gc.IsNil)
+	err = container.Create(kvm.StartParams{})
+	c.Assert(err, gc.ErrorMatches, "container already exists")
+}
+
 func (*MockSuite) TestContainerStoppingRunningStops(c *gc.C) {
 	factory := mock.MockFactory()
 	container := factory.New("first")
-	err := container.Start(kvm.StartParams{})
+	err := container.Create(kvm.StartParams{})
 	c.Assert(err, gc.IsNil)
 	err = container.Stop()
 	c.Assert(err, gc.IsNil)
-	c.Assert(container.IsRunning(), jc.IsFalse)
+	c.Assert(container.Status(), gc.Not(gc.Equals), kvm.Running)
+}
+
+func (*MockSuite) TestContainerDestroyingRunningStops(c *gc.C) {
+	factory := mock.MockFactory()
+	container := factory.New("first")
+	err := container.Create(kvm.StartParams{})
+	c.Assert(err, gc.IsNil)
+	err = container.Destroy()
+	c.Assert(err, gc.IsNil)
+	c.Assert(container.Status(), gc.Not(gc.Equals), kvm.Running)
 }
 
 func (*MockSuite) TestAddListener(c *gc.C) {
@@ -127,20 +169,28 @@
 
 func (*MockSuite) TestEvents(c *gc.C) {
 	factory := mock.MockFactory()
-	listener := make(chan mock.Event, 5)
+	listener := make(chan mock.Event, 15)
 	factory.AddListener(listener)
 
 	first := factory.New("first")
 	second := factory.New("second")
-	first.Start(kvm.StartParams{})
-	second.Start(kvm.StartParams{})
+	first.Create(kvm.StartParams{})
+	second.Create(kvm.StartParams{})
+	first.Stop()
 	second.Stop()
-	first.Stop()
+	first.Start()
+	second.Start()
+	second.Destroy()
+	first.Destroy()
 
+	c.Assert(<-listener, gc.Equals, mock.Event{mock.Created, "first"})
+	c.Assert(<-listener, gc.Equals, mock.Event{mock.Created, "second"})
+	c.Assert(<-listener, gc.Equals, mock.Event{mock.Stopped, "first"})
+	c.Assert(<-listener, gc.Equals, mock.Event{mock.Stopped, "second"})
 	c.Assert(<-listener, gc.Equals, mock.Event{mock.Started, "first"})
 	c.Assert(<-listener, gc.Equals, mock.Event{mock.Started, "second"})
-	c.Assert(<-listener, gc.Equals, mock.Event{mock.Stopped, "second"})
-	c.Assert(<-listener, gc.Equals, mock.Event{mock.Stopped, "first"})
+	c.Assert(<-listener, gc.Equals, mock.Event{mock.Destroyed, "second"})
+	c.Assert(<-listener, gc.Equals, mock.Event{mock.Destroyed, "first"})
 }
 
 func (*MockSuite) TestEventsGoToAllListeners(c *gc.C) {
@@ -151,11 +201,17 @@
 	factory.AddListener(second)
 
 	container := factory.New("container")
-	container.Start(kvm.StartParams{})
+	container.Create(kvm.StartParams{})
 	container.Stop()
+	container.Start()
+	container.Destroy()
 
+	c.Assert(<-first, gc.Equals, mock.Event{mock.Created, "container"})
+	c.Assert(<-second, gc.Equals, mock.Event{mock.Created, "container"})
+	c.Assert(<-first, gc.Equals, mock.Event{mock.Stopped, "container"})
+	c.Assert(<-second, gc.Equals, mock.Event{mock.Stopped, "container"})
 	c.Assert(<-first, gc.Equals, mock.Event{mock.Started, "container"})
 	c.Assert(<-second, gc.Equals, mock.Event{mock.Started, "container"})
-	c.Assert(<-first, gc.Equals, mock.Event{mock.Stopped, "container"})
-	c.Assert(<-second, gc.Equals, mock.Event{mock.Stopped, "container"})
+	c.Assert(<-first, gc.Equals, mock.Event{mock.Destroyed, "container"})
+	c.Assert(<-second, gc.Equals, mock.Event{mock.Destroyed, "container"})
 }

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2014-03-19 21:08:58 +0000
+++ container/lxc/lxc.go	2014-03-31 22:42:50 +0000
@@ -112,6 +112,36 @@
 	}, nil
 }
 
+func (manager *containerManager) StartContainer(instance instance.Instance) error {
+	name := string(instance.Id())
+	lxcContainer := LxcObjectFactory.New(name)
+	// Create the cloud-init.
+	directory, err := container.NewDirectory(name)
+	if err != nil {
+		return err
+	}
+	// Start the lxc container with the appropriate settings for grabbing the
+	// console output and a log file.
+	consoleFile := filepath.Join(directory, "console.log")
+	lxcContainer.SetLogFile(filepath.Join(directory, "container.log"), golxc.LogDebug)
+	logger.Tracef("start the container")
+	// We explicitly don't pass through the config file to the container.Start
+	// method as we have passed it through at container creation time.  This
+	// is necessary to get the appropriate rootfs reference without explicitly
+	// setting it ourselves.
+	if err = lxcContainer.Start("", consoleFile); err != nil {
+		logger.Errorf("container failed to start: %v", err)
+		return err
+	}
+	return nil
+}
+
+func (manager *containerManager) StopContainer(instance instance.Instance) error {
+	name := string(instance.Id())
+	lxcContainer := LxcObjectFactory.New(name)
+	return lxcContainer.Stop()
+}
+
 func (manager *containerManager) CreateContainer(
 	machineConfig *cloudinit.MachineConfig,
 	series string,
@@ -204,25 +234,17 @@
 	if err := mountHostLogDir(name, manager.logdir); err != nil {
 		return nil, nil, err
 	}
-	// Start the lxc container with the appropriate settings for grabbing the
-	// console output and a log file.
-	consoleFile := filepath.Join(directory, "console.log")
-	lxcContainer.SetLogFile(filepath.Join(directory, "container.log"), golxc.LogDebug)
-	logger.Tracef("start the container")
-	// We explicitly don't pass through the config file to the container.Start
-	// method as we have passed it through at container creation time.  This
-	// is necessary to get the appropriate rootfs reference without explicitly
-	// setting it ourselves.
-	if err = lxcContainer.Start("", consoleFile); err != nil {
-		logger.Errorf("container failed to start: %v", err)
-		return nil, nil, err
-	}
+
+	container := &lxcInstance{lxcContainer, name}
+
+	manager.StartContainer(container)
+
 	arch := version.Current.Arch
 	hardware := &instance.HardwareCharacteristics{
 		Arch: &arch,
 	}
 	logger.Tracef("container %q started: %v", name, time.Now().Sub(start))
-	return &lxcInstance{lxcContainer, name}, hardware, nil
+	return container, hardware, nil
 }
 
 func appendToContainerConfig(name, line string) error {

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2014-03-19 21:08:58 +0000
+++ container/lxc/lxc_test.go	2014-03-31 22:42:50 +0000
@@ -113,6 +113,29 @@
 	c.Assert(c.GetTestLog(), jc.Contains, `WARNING juju.container unused config option: "shazam" -> "Captain Marvel"`)
 }
 
+func (s *LxcSuite) TestStopContainer(c *gc.C) {
+	manager := s.makeManager(c, "test")
+	instance := containertesting.CreateContainer(c, manager, "1/lxc/0")
+	err := containertesting.StopContainer(c, manager, instance)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *LxcSuite) TestStartStoppedContainer(c *gc.C) {
+	manager := s.makeManager(c, "test")
+	instance := containertesting.CreateContainer(c, manager, "1/lxc/0")
+	err := containertesting.StopContainer(c, manager, instance)
+	c.Assert(err, gc.IsNil)
+	err = containertesting.StartContainer(c, manager, instance)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *LxcSuite) TestStartRunningContainer(c *gc.C) {
+	manager := s.makeManager(c, "test")
+	instance := containertesting.CreateContainer(c, manager, "1/lxc/0")
+	err := containertesting.StartContainer(c, manager, instance)
+	c.Assert(err, gc.ErrorMatches, "container is already running")
+}
+
 func (s *LxcSuite) TestCreateContainer(c *gc.C) {
 	manager := s.makeManager(c, "test")
 	instance := containertesting.CreateContainer(c, manager, "1/lxc/0")

=== modified file 'container/testing/common.go'
--- container/testing/common.go	2014-03-19 20:54:52 +0000
+++ container/testing/common.go	2014-03-31 22:42:50 +0000
@@ -17,6 +17,14 @@
 	"launchpad.net/juju-core/version"
 )
 
+func StartContainer(c *gc.C, manager container.Manager, instance instance.Instance) error {
+	return manager.StartContainer(instance)
+}
+
+func StopContainer(c *gc.C, manager container.Manager, instance instance.Instance) error {
+	return manager.StopContainer(instance)
+}
+
 func CreateContainer(c *gc.C, manager container.Manager, machineId string) instance.Instance {
 	stateInfo := jujutesting.FakeStateInfo(machineId)
 	apiInfo := jujutesting.FakeAPIInfo(machineId)

=== modified file 'worker/provisioner/kvm-broker_test.go'
--- worker/provisioner/kvm-broker_test.go	2014-03-14 02:19:35 +0000
+++ worker/provisioner/kvm-broker_test.go	2014-03-31 22:42:50 +0000
@@ -179,7 +179,7 @@
 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)
+	c.Assert(event.Action, gc.Equals, mock.Created)
 	err := machine.Refresh()
 	c.Assert(err, gc.IsNil)
 	s.waitInstanceId(c, machine, instance.Id(event.InstanceId))
@@ -189,7 +189,7 @@
 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.Action, gc.Equals, mock.Destroyed)
 	c.Assert(event.InstanceId, gc.Equals, instId)
 }