Merge lp:~thumper/juju-core/provisioner-step-1 into lp:~juju/juju-core/trunk

[Tim Penhey]

First part of the provisioner refactoring.

This branch breaks up the current provisioner and defines some interfaces that
we'll be using for containers.

The broker interface is what we have starting, stopping instances, and listing
instances and related machines.

An environment broker is written that defers the actual calls to the environ.

The provisioner now creates a provisioning task for the environment provider
using a machine watcher and the environment broker. The common provisioning
methods are now in provisioner_task.go.

I refactored some of the methods as we were duplicating a lot of calls, like
get all instances, and then getting instances for individual ids. Also,
looking up specific machines, and also getting all machines. Now we just do
the "all" gets, and keep a map around for the duration of the checks.

https://codereview.appspot.com/9937046/

[Tim Penhey (thumper)]

Reviewers: mp+167684_code.launchpad.net,

Message:
Please take a look.

Description:
First part of the provisioner refactoring.

This branch breaks up the current provisioner and defines some
interfaces that
we'll be using for containers.

The broker interface is what we have starting, stopping instances, and
listing
instances and related machines.

An environment broker is written that defers the actual calls to the
environ
or state.

The provisioner now creates a provisioning task for the environment
provider
using a machine watcher and the environment broker. The common
provisioning
methods are now in provisioner_task.go.

I refactored some of the methods as we were duplicating a lot of calls,
like
get all instances, and then getting instances for individual ids. Also,
looking up specific machines, and also getting all machines. Now we
just do
the "all" gets, and keep a map around for the duration of the checks.

There are a couple of open questions around what to do if the
environment
provider task stops, and I'm a little unclear on the providing dying
bit, the
tests pass, but I feel that something is missing here.

https://code.launchpad.net/~thumper/juju-core/provisioner-step-1/+merge/167684

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   A worker/provisioner/broker.go
   A worker/provisioner/environ_broker.go
   M worker/provisioner/provisioner.go
   A worker/provisioner/provisioner_task.go

[Ian Booth (wallyworld)]

I think you have essentially copied a bunch of methods from provisioner
to provisioner_task, so mainly a mechanical change. LGTM

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go
File worker/provisioner/broker.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode23
worker/provisioner/broker.go:23: StopInstances([]environs.Instance)
error
I think the signature here was done to match the existing Environs
StopInstances but I'd rather see StopInstances([]string) ie pass in the
ids od the instances to stop. There's no need to pass in more info than
is necessary and callers typically should be designed to pass in just
the ids. Both ec2 and openstack just extract the ids anyway and mass can
be tweaked as needed.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_broker.go
File worker/provisioner/environ_broker.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_broker.go#newcode11
worker/provisioner/environ_broker.go:11: func
newEnvironmentBroker(environ environs.Environ, state *state.State)
Broker {
newEnvironBroker I think is more idiomatic

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.go#newcode84
worker/provisioner/provisioner.go:84: p.environmentProvider =
newProvisionerTask(
environBroker perhaps, expecially if newEnvironmentBroker is renamed

https://codereview.appspot.com/9937046/

[Frank Mueller (themue)]

Still interested in how the provisioning of containers on possibly each
machine and there without an environBroker will look like. Or is this
provisioner only intended as environment provisioner?

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go
File worker/provisioner/broker.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode23
worker/provisioner/broker.go:23: StopInstances([]environs.Instance)
error
On 2013/06/06 04:12:43, wallyworld wrote:
> I think the signature here was done to match the existing Environs
StopInstances
> but I'd rather see StopInstances([]string) ie pass in the ids od the
instances
> to stop. There's no need to pass in more info than is necessary and
callers
> typically should be designed to pass in just the ids. Both ec2 and
openstack
> just extract the ids anyway and mass can be tweaked as needed.

+1

https://codereview.appspot.com/9937046/

[William Reade (fwereade)]

Only serious issue is the AllMachines stuff that I don't think we need.
I'll try to swing by tonight for a chat; if not, I'm fine calling
progress-not-perfection and LGTM, but I'd be happiest if we could
resolve it first.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go
File worker/provisioner/broker.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode20
worker/provisioner/broker.go:20: StartInstance(machineId, machineNonce
string, series string, cons constraints.Value, info *state.Info, apiInfo
*api.Info) (environs.Instance, error)
maybe that'll be an instance.Instance one day... but not today,
certainly.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode23
worker/provisioner/broker.go:23: StopInstances([]environs.Instance)
error
On 2013/06/06 07:30:30, mue wrote:
> On 2013/06/06 04:12:43, wallyworld wrote:
> > I think the signature here was done to match the existing Environs
> StopInstances
> > but I'd rather see StopInstances([]string) ie pass in the ids od the
instances
> > to stop. There's no need to pass in more info than is necessary and
callers
> > typically should be designed to pass in just the ids. Both ec2 and
openstack
> > just extract the ids anyway and mass can be tweaked as needed.

> +1

+0.5, but I'd like to keep this CL focused.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode29
worker/provisioner/broker.go:29: AllMachines() ([]*state.Machine, error)
Not quite sure about this.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_broker.go
File worker/provisioner/environ_broker.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_broker.go#newcode17
worker/provisioner/environ_broker.go:17: *state.State
I think we can keep state out of here.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.go#newcode91
worker/provisioner/provisioner.go:91: // we should perhaps watch in the
loop?
yeah, select on its Dying()?

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.go#newcode98
worker/provisioner/provisioner.go:98: p.environmentProvider.Wait()
defer a Stop just after creation, surely?

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_task.go
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_task.go#newcode74
worker/provisioner/provisioner_task.go:74: // Call processMachines to
stop any unknown instances before watching machines.
I *think* this whole block is redundant. The first event out of the
watcher will contain all relevant machine ids, and if we pass that
through to populateMachineMaps we can just use that... right?

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_task.go#newcode125
worker/provisioner/provisioner_task.go:125: // instance for the same
machine ID.
I think this comment is wrong, but I'm still in driveby mod...

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go
File worker/provisioner/broker.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode23
worker/provisioner/broker.go:23: StopInstances([]environs.Instance)
error
On 2013/06/06 04:12:43, wallyworld wrote:
> I think the signature here was done to match the existing Environs
StopInstances
> but I'd rather see StopInstances([]string) ie pass in the ids od the
instances
> to stop. There's no need to pass in more info than is necessary and
callers
> typically should be designed to pass in just the ids. Both ec2 and
openstack
> just extract the ids anyway and mass can be tweaked as needed.

the reason the StopInstances call was designed the way it was is so you
cannot stop an instance that is not part of the current environment
(you have to call Environ.Instances to get the instance first, which can
make sure that the instance id corresponds to one in the environment).

perhaps it's not a problem to relax that restriction, but i think it's
worth pointing out.

https://codereview.appspot.com/9937046/

[Tim Penhey (thumper)]

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go
File worker/provisioner/broker.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode20
worker/provisioner/broker.go:20: StartInstance(machineId, machineNonce
string, series string, cons constraints.Value, info *state.Info, apiInfo
*api.Info) (environs.Instance, error)
On 2013/06/06 14:50:55, fwereade wrote:
> maybe that'll be an instance.Instance one day... but not today,
certainly.

Perhaps *state.Machine? instance.Instance is for started instances
isn't it?

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode23
worker/provisioner/broker.go:23: StopInstances([]environs.Instance)
error
On 2013/06/06 16:07:35, rog wrote:
> On 2013/06/06 04:12:43, wallyworld wrote:
> > I think the signature here was done to match the existing Environs
> StopInstances
> > but I'd rather see StopInstances([]string) ie pass in the ids od the
instances
> > to stop. There's no need to pass in more info than is necessary and
callers
> > typically should be designed to pass in just the ids. Both ec2 and
openstack
> > just extract the ids anyway and mass can be tweaked as needed.

> the reason the StopInstances call was designed the way it was is so
you
> cannot stop an instance that is not part of the current environment
> (you have to call Environ.Instances to get the instance first, which
can make
> sure that the instance id corresponds to one in the environment).

> perhaps it's not a problem to relax that restriction, but i think it's
worth
> pointing out.

Perhaps []state.InstanceId? this is a string type under the covers, but
better matches intent.

The provisioner has done the "lookup the instances" for the machine ids
it gets, so we know, at least from the provisioner side, that all of the
values being passed through should be valid.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode29
worker/provisioner/broker.go:29: AllMachines() ([]*state.Machine, error)
On 2013/06/06 14:50:55, fwereade wrote:
> Not quite sure about this.

I thought quite a bit about this, and ended up putting it here. The
provisioner task uses it when trying to identify the unknown instances.
It made sense to me that the broker would supply this.

Otherwise it is a global call to State, and we certainly don't care
about "AllMachines" when dealing with a container on a machine 4.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_broker.go
File worker/provisioner/environ_broker.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_broker.go#newcode11
worker/provisioner/environ_broker.go:11: func
newEnvironmentBroker(environ environs.Environ, state *state.State)
Broker {
On 2013/06/06 04:12:43, wallyworld wrote:
> newEnvironBroker I think is more idiomatic

probably

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_broker.go#newcode17
worker/provisioner/environ_broker.go:17: *state.State
On 2013/06/06 14:50:55, fwereade wrote:
> I think we can keep state out of here.

It is used only for the AllMachines call, which I commented on th...

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/9937046/

[Tim Penhey (thumper)]

On 2013/06/07 03:44:19, thumper wrote:
> Please take a look.

FYI - bootstrapped EC2 with this code, after the status below, I
destroyed the environment and was able to see all the instances shutting
down from the AWS console.

$ juju status
machines:
   "0":
     agent-state: started
     agent-version: 1.11.1.1
     dns-name: ec2-23-20-239-165.compute-1.amazonaws.com
     instance-id: i-83b05aec
     series: precise
   "1":
     agent-state: started
     agent-version: 1.11.1.1
     dns-name: ec2-54-235-31-114.compute-1.amazonaws.com
     instance-id: i-55f5b63a
     series: precise
   "2":
     agent-state: started
     agent-version: 1.11.1.1
     dns-name: ec2-54-242-136-39.compute-1.amazonaws.com
     instance-id: i-d9f4b7b6
     series: precise
services:
   mysql:
     charm: cs:precise/mysql-23
     exposed: false
     relations:
       cluster:
       - mysql
     units:
       mysql/0:
         agent-state: installed
         agent-version: 1.11.1.1
         machine: "2"
         public-address: ec2-54-242-136-39.compute-1.amazonaws.com
   wordpress:
     charm: cs:precise/wordpress-15
     exposed: false
     relations:
       loadbalancer:
       - wordpress
     units:
       wordpress/0:
         agent-state: pending
         agent-version: 1.11.1.1
         machine: "1"
         public-address: ec2-54-235-31-114.compute-1.amazonaws.com

https://codereview.appspot.com/9937046/

[William Reade (fwereade)]

Huuuge pile of comments; the majority are not conveniently addressable
this CL, but I would appreciate consideration of those that are. I'm
happy to LGTM on the understanding that you will give them due
consideration before merging.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go
File worker/provisioner/broker.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode20
worker/provisioner/broker.go:20: StartInstance(machineId, machineNonce
string, series string, cons constraints.Value, info *state.Info, apiInfo
*api.Info) (environs.Instance, error)
On 2013/06/06 22:14:53, thumper wrote:
> On 2013/06/06 14:50:55, fwereade wrote:
> > maybe that'll be an instance.Instance one day... but not today,
certainly.

> Perhaps *state.Machine? instance.Instance is for started instances
isn't it?

There's a firm distinction between machines and instances: a machine
exists purely in state, and an instance is completely independent of
state. What I'm getting at is that Instance is going to stop being an
environs-specific concept, and should probably move to its own package
at some point.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode23
worker/provisioner/broker.go:23: StopInstances([]environs.Instance)
error
On 2013/06/06 22:14:53, thumper wrote:
> On 2013/06/06 16:07:35, rog wrote:
> > On 2013/06/06 04:12:43, wallyworld wrote:
> > > I think the signature here was done to match the existing Environs
> > StopInstances
> > > but I'd rather see StopInstances([]string) ie pass in the ids od
the
> instances
> > > to stop. There's no need to pass in more info than is necessary
and callers
> > > typically should be designed to pass in just the ids. Both ec2 and
openstack
> > > just extract the ids anyway and mass can be tweaked as needed.
> >
> > the reason the StopInstances call was designed the way it was is so
you
> > cannot stop an instance that is not part of the current environment
> > (you have to call Environ.Instances to get the instance first, which
can make
> > sure that the instance id corresponds to one in the environment).
> >
> > perhaps it's not a problem to relax that restriction, but i think
it's worth
> > pointing out.

> Perhaps []state.InstanceId? this is a string type under the covers,
but better
> matches intent.

> The provisioner has done the "lookup the instances" for the machine
ids it gets,
> so we know, at least from the provisioner side, that all of the values
being
> passed through should be valid.

I'd be ok with InstanceId fwiw.

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_task.go
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_task.go#newcode147
worker/provisioner/provisioner_task.go:147: machines, err :=
task.broker.AllMachines()
On 2013/06/06 22:14:53, thumper wrote:
> On 2013/06/06 14:50:55, fwereade wrote:
> > Yeah, there's no call for AllMachines() at all. We just need to pass
the list
> of
> > changed machine ids down from the watcher, get st.Machine() for any
that not
> are
> > already known (inefficient, s...

[Tim Penhey (thumper)]

*** Submitted:

First part of the provisioner refactoring.

This branch breaks up the current provisioner and defines some
interfaces that
we'll be using for containers.

The broker interface is what we have starting, stopping instances, and
listing
instances and related machines.

An environment broker is written that defers the actual calls to the
environ.

The provisioner now creates a provisioning task for the environment
provider
using a machine watcher and the environment broker. The common
provisioning
methods are now in provisioner_task.go.

I refactored some of the methods as we were duplicating a lot of calls,
like
get all instances, and then getting instances for individual ids. Also,
looking up specific machines, and also getting all machines. Now we
just do
the "all" gets, and keep a map around for the duration of the checks.

R=wallyworld, mue, fwereade, rog
CC=
https://codereview.appspot.com/9937046

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provisioner.go#newcode26
worker/provisioner/provisioner.go:26: environmentProvisioner
ProvisionerTask
On 2013/06/07 22:40:45, fwereade wrote:
> Only used in one method, no need to put it on the type.

Done.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provisioner.go#newcode73
worker/provisioner/provisioner.go:73: stateInfo, apiInfo, err :=
p.environ.StateInfo()
On 2013/06/07 22:40:45, fwereade wrote:
> Hmm. This should probably come from state instead (one day the API).
It has
> Addresses() and APIAddresses() methods, and a CACert() method required
by both,
> and you can build perfectly good *Infos from scratch inside the task
so long as
> they have access to that information.

> And, it should definitely not be done just once in the lifetime of the
> provisioner: the set of addresses is theoretically subject to change
and I'd
> prefer to get "fresh" data at least once for every group of machines
we start.
> But for now it's moot wrt observable behaviour, so leave them out of
this CL.

Should have a follow up to do this.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provisioner.go#newcode91
worker/provisioner/provisioner.go:91: defer
p.environmentProvisioner.Stop()
On 2013/06/07 22:40:45, fwereade wrote:
> `defer watcher.Stop(p.environmentProvisioner, &p.tomb)` to pass
shutdown errors
> into this type's tomb.

I had a look at this. I think we should move the method. I inferred by
the name that it expects a watcher, but it doesn't, just a Stopper.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provisioner.go#newcode97
worker/provisioner/provisioner.go:97: case cfg, ok :=
<-environWatcher.Changes():
On 2013/06/07 22:40:45, fwereade wrote:
> Soon (ie before we write an API that poos this whole thing over the
wire every
> time) I'd like to make this an EntityWatcher, and to get EnvironConfig
directly
> and explicitly in response to the empty notifications. Not this CL
though.

poos this whole thing? Sounds meesy.

https://codereview.appspot.com/9937046/diff/8002/wo...

=== added file 'worker/provisioner/broker.go'
--- worker/provisioner/broker.go	1970-01-01 00:00:00 +0000
+++ worker/provisioner/broker.go	2013-06-07 03:44:25 +0000
@@ -0,0 +1,27 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package provisioner
+
+import (
+	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api"
+)
+
+type Broker interface {
+	// StartInstance asks for a new instance to be created, associated with
+	// the provided machine identifier. The given info describes the juju
+	// state for the new instance to connect to. The nonce, which must be
+	// unique within an environment, is used by juju to protect against the
+	// consequences of multiple instances being started with the same machine
+	// id.
+	StartInstance(machineId, machineNonce string, series string, cons constraints.Value, info *state.Info, apiInfo *api.Info) (environs.Instance, error)
+
+	// StopInstances shuts down the given instances.
+	StopInstances([]environs.Instance) error
+
+	// AllInstances returns all instances currently known to the broker.
+	AllInstances() ([]environs.Instance, error)
+}

=== added file 'worker/provisioner/environ_broker.go'
--- worker/provisioner/environ_broker.go	1970-01-01 00:00:00 +0000
+++ worker/provisioner/environ_broker.go	2013-06-07 03:44:25 +0000
@@ -0,0 +1,21 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package provisioner
+
+import (
+	"launchpad.net/juju-core/environs"
+)
+
+func newEnvironBroker(environ environs.Environ) Broker {
+	return &environBroker{environ}
+}
+
+type environBroker struct {
+	environs.Environ
+}
+
+// Defer to the Environ for:
+//   StartInstance
+//   StopInstances
+//   AllInstances

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2013-05-31 01:02:53 +0000
+++ worker/provisioner/provisioner.go	2013-06-07 03:44:25 +0000
@@ -4,35 +4,26 @@
 package provisioner
 
 import (
-	stderrors "errors"
-	"fmt"
+	"sync"
+
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
-	"launchpad.net/juju-core/errors"
-	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/state"
-	"launchpad.net/juju-core/state/api"
-	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/watcher"
-	"launchpad.net/juju-core/utils"
 	"launchpad.net/juju-core/worker"
+	"launchpad.net/loggo"
 	"launchpad.net/tomb"
-	"sync"
 )
 
+var logger = loggo.GetLogger("juju.provisioner")
+
 // Provisioner represents a running provisioning worker.
 type Provisioner struct {
-	st        *state.State
-	machineId string // Which machine runs the provisioner.
-	stateInfo *state.Info
-	apiInfo   *api.Info
-	environ   environs.Environ
-	tomb      tomb.Tomb
-
-	// machine.Id => environs.Instance
-	instances map[string]environs.Instance
-	// instance.Id => machine id
-	machines map[state.InstanceId]string
+	st                     *state.State
+	machineId              string // Which machine runs the provisioner.
+	environ                environs.Environ
+	tomb                   tomb.Tomb
+	environmentProvisioner ProvisionerTask
 
 	configObserver
 }
@@ -58,8 +49,6 @@
 	p := &Provisioner{
 		st:        st,
 		machineId: machineId,
-		instances: make(map[string]environs.Instance),
-		machines:  make(map[state.InstanceId]string),
 	}
 	go func() {
 		defer p.tomb.Done()
@@ -81,19 +70,26 @@
 	// Get a new StateInfo from the environment: the one used to
 	// launch the agent may refer to localhost, which will be
 	// unhelpful when attempting to run an agent on a new machine.
-	if p.stateInfo, p.apiInfo, err = p.environ.StateInfo(); err != nil {
+	stateInfo, apiInfo, err := p.environ.StateInfo()
+	if err != nil {
 		return err
 	}
 
-	// Call processMachines to stop any unknown instances before watching machines.
-	if err := p.processMachines(nil); err != nil {
-		return err
-	}
+	// Start a new worker for the environment provider.
 
 	// Start responding to changes in machines, and to any further updates
 	// to the environment config.
 	machinesWatcher := p.st.WatchMachines()
-	defer watcher.Stop(machinesWatcher, &p.tomb)
+	environmentBroker := newEnvironBroker(p.environ)
+	p.environmentProvisioner = newProvisionerTask(
+		p.machineId,
+		p.st,
+		machinesWatcher,
+		environmentBroker,
+		stateInfo,
+		apiInfo)
+	defer p.environmentProvisioner.Stop()
+
 	for {
 		select {
 		case <-p.tomb.Dying():
@@ -103,17 +99,12 @@
 				return watcher.MustErr(environWatcher)
 			}
 			if err := p.setConfig(cfg); err != nil {
-				log.Errorf("worker/provisioner: loaded invalid environment configuration: %v", err)
-			}
-		case ids, ok := <-machinesWatcher.Changes():
-			if !ok {
-				return watcher.MustErr(machinesWatcher)
-			}
-			// TODO(dfc; lp:1042717) fire process machines periodically to shut down unknown
-			// instances.
-			if err := p.processMachines(ids); err != nil {
-				return err
-			}
+				logger.Error("loaded invalid environment configuration: %v", err)
+			}
+		case <-p.environmentProvisioner.Dying():
+			err := p.environmentProvisioner.Err()
+			logger.Error("environment provisioner died: %v", err)
+			return err
 		}
 	}
 	panic("not reached")
@@ -155,251 +146,3 @@
 	p.tomb.Kill(nil)
 	return p.tomb.Wait()
 }
-
-func (p *Provisioner) processMachines(ids []string) error {
-	// Find machines without an instance id or that are dead
-	pending, dead, err := p.pendingOrDead(ids)
-	if err != nil {
-		return err
-	}
-
-	// Find running instances that have no machines associated
-	unknown, err := p.findUnknownInstances()
-	if err != nil {
-		return err
-	}
-
-	// Stop all machines that are dead
-	stopping, err := p.instancesForMachines(dead)
-	if err != nil {
-		return err
-	}
-
-	// It's important that we stop unknown instances before starting
-	// pending ones, because if we start an instance and then fail to
-	// set its InstanceId on the machine we don't want to start a new
-	// instance for the same machine ID.
-	if err := p.stopInstances(append(stopping, unknown...)); err != nil {
-		return err
-	}
-
-	// Start an instance for the pending ones
-	return p.startMachines(pending)
-}
-
-// findUnknownInstances finds instances which are not associated with a machine.
-func (p *Provisioner) findUnknownInstances() ([]environs.Instance, error) {
-	all, err := p.environ.AllInstances()
-	if err != nil {
-		return nil, err
-	}
-	instances := make(map[state.InstanceId]environs.Instance)
-	for _, i := range all {
-		instances[i.Id()] = i
-	}
-	// TODO(dfc) this is very inefficient.
-	machines, err := p.st.AllMachines()
-	if err != nil {
-		return nil, err
-	}
-	for _, m := range machines {
-		if instId, ok := m.InstanceId(); ok {
-			delete(instances, instId)
-		}
-	}
-	var unknown []environs.Instance
-	for _, i := range instances {
-		unknown = append(unknown, i)
-	}
-	return unknown, nil
-}
-
-// pendingOrDead looks up machines with ids and retuns those that do not
-// have an instance id assigned yet, and also those that are dead.
-func (p *Provisioner) pendingOrDead(ids []string) (pending, dead []*state.Machine, err error) {
-	// TODO(niemeyer): ms, err := st.Machines(alive)
-	for _, id := range ids {
-		m, err := p.st.Machine(id)
-		if errors.IsNotFoundError(err) {
-			log.Infof("worker/provisioner: machine %q not found in state", m)
-			continue
-		}
-		if err != nil {
-			return nil, nil, err
-		}
-		switch m.Life() {
-		case state.Dying:
-			if _, ok := m.InstanceId(); ok {
-				continue
-			}
-			log.Infof("worker/provisioner: killing dying, unprovisioned machine %q", m)
-			if err := m.EnsureDead(); err != nil {
-				return nil, nil, err
-			}
-			fallthrough
-		case state.Dead:
-			dead = append(dead, m)
-			log.Infof("worker/provisioner: removing dead machine %q", m)
-			if err := m.Remove(); err != nil {
-				return nil, nil, err
-			}
-			continue
-		}
-		if instId, hasInstId := m.InstanceId(); !hasInstId {
-			status, _, err := m.Status()
-			if err != nil {
-				log.Infof("worker/provisioner: cannot get machine %q status: %v", m, err)
-				continue
-			}
-			if status == params.StatusPending {
-				pending = append(pending, m)
-				log.Infof("worker/provisioner: found machine %q pending provisioning", m)
-				continue
-			}
-		} else {
-			log.Infof("worker/provisioner: machine %v already started as instance %q", m, instId)
-		}
-	}
-	return
-}
-
-func (p *Provisioner) startMachines(machines []*state.Machine) error {
-	for _, m := range machines {
-		if err := p.startMachine(m); err != nil {
-			return fmt.Errorf("cannot start machine %v: %v", m, err)
-		}
-	}
-	return nil
-}
-
-func (p *Provisioner) startMachine(m *state.Machine) error {
-	// TODO(dfc) the state.Info passed to environ.StartInstance remains contentious
-	// however as the PA only knows one state.Info, and that info is used by MAs and
-	// UAs to locate the state for this environment, it is logical to use the same
-	// state.Info as the PA.
-	stateInfo, apiInfo, err := p.setupAuthentication(m)
-	if err != nil {
-		return err
-	}
-	cons, err := m.Constraints()
-	if err != nil {
-		return err
-	}
-	// Generate a unique nonce for the new instance.
-	uuid, err := utils.NewUUID()
-	if err != nil {
-		return err
-	}
-	// Generated nonce has the format: "machine-#:UUID". The first
-	// part is a badge, specifying the tag of the machine the provisioner
-	// is running on, while the second part is a random UUID.
-	nonce := fmt.Sprintf("%s:%s", state.MachineTag(p.machineId), uuid.String())
-	inst, err := p.environ.StartInstance(m.Id(), nonce, m.Series(), cons, stateInfo, apiInfo)
-	if err != nil {
-		// Set the state to error, so the machine will be skipped next
-		// time until the error is resolved, but don't return an
-		// error; just keep going with the other machines.
-		log.Errorf("worker/provisioner: cannot start instance for machine %q: %v", m, err)
-		if err1 := m.SetStatus(params.StatusError, err.Error()); err1 != nil {
-			// Something is wrong with this machine, better report it back.
-			log.Errorf("worker/provisioner: cannot set error status for machine %q: %v", m, err1)
-			return err1
-		}
-		return nil
-	}
-	if err := m.SetProvisioned(inst.Id(), nonce); err != nil {
-		// The machine is started, but we can't record the mapping in
-		// state. It'll keep running while we fail out and restart,
-		// but will then be detected by findUnknownInstances and
-		// killed again.
-		//
-		// TODO(dimitern) Stop the instance right away here.
-		//
-		// Multiple instantiations of a given machine (with the same
-		// machine ID) cannot coexist, because findUnknownInstances is
-		// called before startMachines. However, if the first machine
-		// had started to do work before being replaced, we may
-		// encounter surprising problems.
-		return err
-	}
-	// populate the local cache
-	p.instances[m.Id()] = inst
-	p.machines[inst.Id()] = m.Id()
-	log.Noticef("worker/provisioner: started machine %s as instance %s", m, inst.Id())
-	return nil
-}
-
-func (p *Provisioner) setupAuthentication(m *state.Machine) (*state.Info, *api.Info, error) {
-	password, err := utils.RandomPassword()
-	if err != nil {
-		return nil, nil, fmt.Errorf("cannot make password for machine %v: %v", m, err)
-	}
-	if err := m.SetMongoPassword(password); err != nil {
-		return nil, nil, fmt.Errorf("cannot set password for machine %v: %v", m, err)
-	}
-	stateInfo := *p.stateInfo
-	stateInfo.Tag = m.Tag()
-	stateInfo.Password = password
-	apiInfo := *p.apiInfo
-	apiInfo.Tag = m.Tag()
-	apiInfo.Password = password
-	return &stateInfo, &apiInfo, nil
-}
-
-func (p *Provisioner) stopInstances(instances []environs.Instance) error {
-	// Although calling StopInstance with an empty slice should produce no change in the
-	// provider, environs like dummy do not consider this a noop.
-	if len(instances) == 0 {
-		return nil
-	}
-	if err := p.environ.StopInstances(instances); err != nil {
-		return err
-	}
-
-	// cleanup cache
-	for _, i := range instances {
-		if id, ok := p.machines[i.Id()]; ok {
-			delete(p.machines, i.Id())
-			delete(p.instances, id)
-		}
-	}
-	return nil
-}
-
-var errNotProvisioned = stderrors.New("machine has no instance id set")
-
-// instanceForMachine returns the environs.Instance that represents this machine's instance.
-func (p *Provisioner) instanceForMachine(m *state.Machine) (environs.Instance, error) {
-	inst, ok := p.instances[m.Id()]
-	if ok {
-		return inst, nil
-	}
-	instId, ok := m.InstanceId()
-	if !ok {
-		return nil, errNotProvisioned
-	}
-	// TODO(dfc): Ask for all instances at once.
-	insts, err := p.environ.Instances([]state.InstanceId{instId})
-	if err != nil {
-		return nil, err
-	}
-	inst = insts[0]
-	return inst, nil
-}
-
-// instancesForMachines returns a list of environs.Instance that represent
-// the list of machines running in the provider. Missing machines are
-// omitted from the list.
-func (p *Provisioner) instancesForMachines(ms []*state.Machine) ([]environs.Instance, error) {
-	var insts []environs.Instance
-	for _, m := range ms {
-		switch inst, err := p.instanceForMachine(m); err {
-		case nil:
-			insts = append(insts, inst)
-		case errNotProvisioned, environs.ErrNoInstances:
-		default:
-			return nil, err
-		}
-	}
-	return insts, nil
-}

=== added file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go	1970-01-01 00:00:00 +0000
+++ worker/provisioner/provisioner_task.go	2013-06-07 03:44:25 +0000
@@ -0,0 +1,373 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package provisioner
+
+import (
+	"fmt"
+
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/errors"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api"
+	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/watcher"
+	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/worker"
+	"launchpad.net/tomb"
+)
+
+type ProvisionerTask interface {
+	worker.Worker
+	Stop() error
+	Dying() <-chan struct{}
+	Err() error
+}
+
+type Watcher interface {
+	watcher.Errer
+	watcher.Stopper
+	Changes() <-chan []string
+}
+
+type MachineGetter interface {
+	Machine(id string) (*state.Machine, error)
+}
+
+func newProvisionerTask(
+	machineId string,
+	machineGetter MachineGetter,
+	watcher Watcher,
+	broker Broker,
+	stateInfo *state.Info,
+	apiInfo *api.Info,
+) ProvisionerTask {
+	task := &provisionerTask{
+		machineId:      machineId,
+		machineGetter:  machineGetter,
+		machineWatcher: watcher,
+		broker:         broker,
+		stateInfo:      stateInfo,
+		apiInfo:        apiInfo,
+		machines:       make(map[string]*state.Machine),
+	}
+	go func() {
+		defer task.tomb.Done()
+		task.tomb.Kill(task.loop())
+	}()
+	return task
+}
+
+type provisionerTask struct {
+	machineId      string
+	machineGetter  MachineGetter
+	machineWatcher Watcher
+	broker         Broker
+	tomb           tomb.Tomb
+	stateInfo      *state.Info
+	apiInfo        *api.Info
+
+	// instance id -> instance
+	instances map[state.InstanceId]environs.Instance
+	// machine id -> machine
+	machines map[string]*state.Machine
+}
+
+// Kill implements worker.Worker.Kill.
+func (task *provisionerTask) Kill() {
+	task.tomb.Kill(nil)
+}
+
+// Wait implements worker.Worker.Wait.
+func (task *provisionerTask) Wait() error {
+	return task.tomb.Wait()
+}
+
+func (task *provisionerTask) Stop() error {
+	task.Kill()
+	return task.Wait()
+}
+
+func (task *provisionerTask) Dying() <-chan struct{} {
+	return task.tomb.Dying()
+}
+
+func (task *provisionerTask) Err() error {
+	return task.tomb.Err()
+}
+
+func (task *provisionerTask) loop() error {
+	logger.Info("Starting up provisioner task %s", task.machineId)
+	defer watcher.Stop(task.machineWatcher, &task.tomb)
+
+	// When the watcher is started, it will have the initial changes be all
+	// the machines that are relevant. Also, since this is available straight
+	// away, we know there will be some changes right off the bat.
+	for {
+		select {
+		case <-task.tomb.Dying():
+			logger.Info("Shutting down provisioner task %s", task.machineId)
+			return tomb.ErrDying
+		case ids, ok := <-task.machineWatcher.Changes():
+			if !ok {
+				return watcher.MustErr(task.machineWatcher)
+			}
+			// TODO(dfc; lp:1042717) fire process machines periodically to shut down unknown
+			// instances.
+			if err := task.processMachines(ids); err != nil {
+				logger.Error("Process machines failed: %v", err)
+				return err
+			}
+		}
+	}
+	panic("not reached")
+}
+
+func (task *provisionerTask) processMachines(ids []string) error {
+	logger.Trace("processMachines(%v)", ids)
+	// Populate the tasks maps of current instances and machines.
+	err := task.populateMachineMaps(ids)
+	if err != nil {
+		return err
+	}
+
+	// Find machines without an instance id or that are dead
+	pending, dead, err := task.pendingOrDead(ids)
+	if err != nil {
+		return err
+	}
+
+	// Find running instances that have no machines associated
+	unknown, err := task.findUnknownInstances()
+	if err != nil {
+		return err
+	}
+
+	// Stop all machines that are dead
+	stopping := task.instancesForMachines(dead)
+
+	// It's important that we stop unknown instances before starting
+	// pending ones, because if we start an instance and then fail to
+	// set its InstanceId on the machine we don't want to start a new
+	// instance for the same machine ID.
+	if err := task.stopInstances(append(stopping, unknown...)); err != nil {
+		return err
+	}
+
+	// Start an instance for the pending ones
+	return task.startMachines(pending)
+}
+
+func (task *provisionerTask) populateMachineMaps(ids []string) error {
+	task.instances = make(map[state.InstanceId]environs.Instance)
+
+	instances, err := task.broker.AllInstances()
+	if err != nil {
+		logger.Error("failed to get all instances from broker: %v", err)
+		return err
+	}
+	for _, i := range instances {
+		task.instances[i.Id()] = i
+	}
+
+	// Update the machines map with new data for each of the machines in the
+	// change list.
+	// TODO(thumper): update for API server later to get all machines in one go.
+	for _, id := range ids {
+		machine, err := task.machineGetter.Machine(id)
+		switch {
+		case errors.IsNotFoundError(err):
+			logger.Debug("machine %q not found in state", id)
+			delete(task.machines, id)
+		case err == nil:
+			task.machines[id] = machine
+		default:
+			logger.Error("failed to get machine: %v", err)
+		}
+	}
+	return nil
+}
+
+// pendingOrDead looks up machines with ids and retuns those that do not
+// have an instance id assigned yet, and also those that are dead.
+func (task *provisionerTask) pendingOrDead(ids []string) (pending, dead []*state.Machine, err error) {
+	for _, id := range ids {
+		machine, found := task.machines[id]
+		if !found {
+			logger.Info("machine %q not found", id)
+			continue
+		}
+		switch machine.Life() {
+		case state.Dying:
+			if _, ok := machine.InstanceId(); ok {
+				continue
+			}
+			logger.Info("killing dying, unprovisioned machine %q", machine)
+			if err := machine.EnsureDead(); err != nil {
+				logger.Error("failed to ensure machine dead %q: %v", machine, err)
+				return nil, nil, err
+			}
+			fallthrough
+		case state.Dead:
+			dead = append(dead, machine)
+			logger.Info("removing dead machine %q", machine)
+			if err := machine.Remove(); err != nil {
+				logger.Error("failed to remove dead machine %q", machine)
+				return nil, nil, err
+			}
+			continue
+		}
+		if instId, hasInstId := machine.InstanceId(); !hasInstId {
+			status, _, err := machine.Status()
+			if err != nil {
+				logger.Info("cannot get machine %q status: %v", machine, err)
+				continue
+			}
+			if status == params.StatusPending {
+				pending = append(pending, machine)
+				logger.Info("found machine %q pending provisioning", machine)
+				continue
+			}
+		} else {
+			logger.Info("machine %v already started as instance %q", machine, instId)
+		}
+	}
+	return
+}
+
+// findUnknownInstances finds instances which are not associated with a machine.
+func (task *provisionerTask) findUnknownInstances() ([]environs.Instance, error) {
+	// Make a copy of the instances we know about.
+	instances := make(map[state.InstanceId]environs.Instance)
+	for k, v := range task.instances {
+		instances[k] = v
+	}
+
+	for _, m := range task.machines {
+		if instId, ok := m.InstanceId(); ok {
+			delete(instances, instId)
+		}
+	}
+	var unknown []environs.Instance
+	for _, i := range instances {
+		unknown = append(unknown, i)
+	}
+	logger.Trace("unknown: %v", unknown)
+	return unknown, nil
+}
+
+// instancesForMachines returns a list of environs.Instance that represent
+// the list of machines running in the provider. Missing machines are
+// omitted from the list.
+func (task *provisionerTask) instancesForMachines(machines []*state.Machine) []environs.Instance {
+	var instances []environs.Instance
+	for _, machine := range machines {
+		instId, ok := machine.InstanceId()
+		if ok {
+			instance, found := task.instances[instId]
+			// If the instance is not found, it means that the underlying
+			// instance is already dead, and we don't need to stop it.
+			if found {
+				instances = append(instances, instance)
+				// now remove it from the machines map
+				delete(task.machines, machine.Id())
+			}
+		}
+	}
+	return instances
+}
+
+func (task *provisionerTask) stopInstances(instances []environs.Instance) error {
+	// Although calling StopInstance with an empty slice should produce no change in the
+	// provider, environs like dummy do not consider this a noop.
+	if len(instances) == 0 {
+		return nil
+	}
+	logger.Debug("Stopping instances: %v", instances)
+	if err := task.broker.StopInstances(instances); err != nil {
+		logger.Error("broker failed to stop instances: %v", err)
+		return err
+	}
+	return nil
+}
+
+func (task *provisionerTask) startMachines(machines []*state.Machine) error {
+	for _, m := range machines {
+		if err := task.startMachine(m); err != nil {
+			return fmt.Errorf("cannot start machine %v: %v", m, err)
+		}
+	}
+	return nil
+}
+
+func (task *provisionerTask) startMachine(machine *state.Machine) error {
+	// TODO(dfc) the state.Info passed to environ.StartInstance remains contentious
+	// however as the PA only knows one state.Info, and that info is used by MAs and
+	// UAs to locate the state for this environment, it is logical to use the same
+	// state.Info as the PA.
+	stateInfo, apiInfo, err := task.setupAuthentication(machine)
+	if err != nil {
+		logger.Error("failed to setup authentication: %v", err)
+		return err
+	}
+	cons, err := machine.Constraints()
+	if err != nil {
+		return err
+	}
+	// Generate a unique nonce for the new instance.
+	uuid, err := utils.NewUUID()
+	if err != nil {
+		return err
+	}
+	// Generated nonce has the format: "machine-#:UUID". The first
+	// part is a badge, specifying the tag of the machine the provisioner
+	// is running on, while the second part is a random UUID.
+	nonce := fmt.Sprintf("%s:%s", state.MachineTag(task.machineId), uuid.String())
+	inst, err := task.broker.StartInstance(machine.Id(), nonce, machine.Series(), cons, stateInfo, apiInfo)
+	if err != nil {
+		// Set the state to error, so the machine will be skipped next
+		// time until the error is resolved, but don't return an
+		// error; just keep going with the other machines.
+		logger.Error("cannot start instance for machine %q: %v", machine, err)
+		if err1 := machine.SetStatus(params.StatusError, err.Error()); err1 != nil {
+			// Something is wrong with this machine, better report it back.
+			logger.Error("cannot set error status for machine %q: %v", machine, err1)
+			return err1
+		}
+		return nil
+	}
+	if err := machine.SetProvisioned(inst.Id(), nonce); err != nil {
+		// The machine is started, but we can't record the mapping in
+		// state. It'll keep running while we fail out and restart,
+		// but will then be detected by findUnknownInstances and
+		// killed again.
+		//
+		// TODO(dimitern) Stop the instance right away here.
+		//
+		// Multiple instantiations of a given machine (with the same
+		// machine ID) cannot coexist, because findUnknownInstances is
+		// called before startMachines. However, if the first machine
+		// had started to do work before being replaced, we may
+		// encounter surprising problems.
+		return err
+	}
+	logger.Info("started machine %s as instance %s", machine, inst.Id())
+	return nil
+}
+
+func (task *provisionerTask) setupAuthentication(machine *state.Machine) (*state.Info, *api.Info, error) {
+	password, err := utils.RandomPassword()
+	if err != nil {
+		return nil, nil, fmt.Errorf("cannot make password for machine %v: %v", machine, err)
+	}
+	if err := machine.SetMongoPassword(password); err != nil {
+		return nil, nil, fmt.Errorf("cannot set password for machine %v: %v", machine, err)
+	}
+	stateInfo := *task.stateInfo
+	stateInfo.Tag = machine.Tag()
+	stateInfo.Password = password
+	apiInfo := *task.apiInfo
+	apiInfo.Tag = machine.Tag()
+	apiInfo.Password = password
+	return &stateInfo, &apiInfo, nil
+}

=== modified file 'worker/provisioner/provisioner_test.go'
--- worker/provisioner/provisioner_test.go	2013-05-31 00:16:39 +0000
+++ worker/provisioner/provisioner_test.go	2013-06-07 03:44:25 +0000
@@ -5,9 +5,14 @@
 
 import (
 	"fmt"
+	"strings"
+	stdtesting "testing"
+	"time"
+
 	"labix.org/v2/mgo/bson"
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/dummy"
 	"launchpad.net/juju-core/errors"
@@ -16,11 +21,9 @@
 	"launchpad.net/juju-core/state/api/params"
 	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/utils/set"
 	"launchpad.net/juju-core/worker"
 	"launchpad.net/juju-core/worker/provisioner"
-	"strings"
-	stdtesting "testing"
-	"time"
 )
 
 func TestPackage(t *stdtesting.T) {
@@ -97,18 +100,19 @@
 	c.Assert(s.Stop(), IsNil)
 }
 
-func (s *ProvisionerSuite) checkStartInstance(c *C, m *state.Machine) {
-	s.checkStartInstanceCustom(c, m, "pork", constraints.Value{})
+func (s *ProvisionerSuite) checkStartInstance(c *C, m *state.Machine) environs.Instance {
+	return s.checkStartInstanceCustom(c, m, "pork", constraints.Value{})
 }
 
-func (s *ProvisionerSuite) checkStartInstanceCustom(c *C, m *state.Machine, secret string, cons constraints.Value) {
+func (s *ProvisionerSuite) checkStartInstanceCustom(c *C, m *state.Machine, secret string, cons constraints.Value) (instance environs.Instance) {
 	s.State.StartSync()
 	for {
 		select {
 		case o := <-s.op:
 			switch o := o.(type) {
 			case dummy.OpStartInstance:
-				s.waitInstanceId(c, m, o.Instance.Id())
+				instance = o.Instance
+				s.waitInstanceId(c, m, instance.Id())
 
 				// Check the instance was started with the expected params.
 				c.Assert(o.MachineId, Equals, m.Id())
@@ -141,6 +145,7 @@
 			return
 		}
 	}
+	return
 }
 
 // checkNoOperations checks that the environ was not operated upon.
@@ -154,19 +159,31 @@
 	}
 }
 
-// checkStopInstance checks that an instance has been stopped.
-func (s *ProvisionerSuite) checkStopInstance(c *C) {
+// checkStopInstances checks that an instance has been stopped.
+func (s *ProvisionerSuite) checkStopInstances(c *C, instances ...environs.Instance) {
 	s.State.StartSync()
-	select {
-	case o := <-s.op:
-		switch o.(type) {
-		case dummy.OpStopInstances:
-		default:
-			c.Fatalf("unexpected operation %#v", o)
+	instanceIds := set.NewStrings()
+	for _, instance := range instances {
+		instanceIds.Add(string(instance.Id()))
+	}
+	// Continue checking for stop instance calls until all the instances we
+	// are waiting on to finish, actually finish, or we time out.
+	for !instanceIds.IsEmpty() {
+		select {
+		case o := <-s.op:
+			switch o := o.(type) {
+			case dummy.OpStopInstances:
+				for _, stoppedInstance := range o.Instances {
+					instanceIds.Remove(string(stoppedInstance.Id()))
+				}
+			default:
+				c.Fatalf("unexpected operation %#v", o)
+				return
+			}
+		case <-time.After(2 * time.Second):
+			c.Fatalf("provisioner did not stop an instance")
+			return
 		}
-	case <-time.After(2 * time.Second):
-		c.Fatalf("provisioner did not stop an instance")
-		return
 	}
 }
 
@@ -230,11 +247,11 @@
 	// Check that an instance is provisioned when the machine is created...
 	m, err := s.State.AddMachine(config.DefaultSeries, state.JobHostUnits)
 	c.Assert(err, IsNil)
-	s.checkStartInstance(c, m)
+	instance := s.checkStartInstance(c, m)
 
 	// ...and removed, along with the machine, when the machine is Dead.
 	c.Assert(m.EnsureDead(), IsNil)
-	s.checkStopInstance(c)
+	s.checkStopInstances(c, instance)
 	s.waitRemoved(c, m)
 }
 
@@ -366,12 +383,12 @@
 	// create a machine
 	m0, err := s.State.AddMachine(config.DefaultSeries, state.JobHostUnits)
 	c.Assert(err, IsNil)
-	s.checkStartInstance(c, m0)
+	i0 := s.checkStartInstance(c, m0)
 
 	// create a second machine
 	m1, err := s.State.AddMachine(config.DefaultSeries, state.JobHostUnits)
 	c.Assert(err, IsNil)
-	s.checkStartInstance(c, m1)
+	i1 := s.checkStartInstance(c, m1)
 	stop(c, p)
 
 	// mark the first machine as dead
@@ -384,8 +401,7 @@
 	// start a new provisioner to shut them both down
 	p = provisioner.NewProvisioner(s.State, "0")
 	defer stop(c, p)
-	s.checkStopInstance(c)
-	s.checkStopInstance(c)
+	s.checkStopInstances(c, i0, i1)
 	s.waitRemoved(c, m0)
 }