Merge lp:~fwereade/juju-core/jujud-integrate-cleaner-resumer into lp:~go-bot/juju-core/trunk

[William Reade]

jujud: JobManageState runs cleaner, resumer

Nothing to see there... but I also got angry and tweaked Agent so I could
just test the damn tasks themselves and not worry about the side effects.
And then I needed to test RunAgentLoop as well to verify that it does the
right thing with the tasks it gets from a mocked Agent.

https://codereview.appspot.com/10439046/

[William Reade (fwereade)]

Reviewers: mp+170907_code.launchpad.net,

Message:
Please take a look.

Description:
jujud: JobManageState runs cleaner, resumer

Nothing to see there... but I also got angry and tweaked Agent so I
could
just test the damn tasks themselves and not worry about the side
effects.
And then I needed to test RunAgentLoop as well to verify that it does
the
right thing with the tasks it gets from a mocked Agent.

https://code.launchpad.net/~fwereade/juju-core/jujud-integrate-cleaner-resumer/+merge/170907

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/jujud/agent.go
   M cmd/jujud/agent_test.go
   M cmd/jujud/machine.go
   M cmd/jujud/machine_test.go
   M cmd/jujud/unit.go
   M environs/testing/storage.go

[John A Meinel (jameinel)]

Some questions about the testing infrastructure, but overall
LGTM

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent.go
File cmd/jujud/agent.go (left):

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent.go#oldcode130
cmd/jujud/agent.go:130: RunOnce(st *state.State, entity AgentState)
error
I do really like the rename of RunOnce to StartTasks.
As RunOnce can be interpreted as "only ever run this once", which
confuses it a bit with why you are calling it in a loop.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go
File cmd/jujud/agent_test.go (right):

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode275
cmd/jujud/agent_test.go:275: conf.MachineNonce = state.BootstrapNonce
I thought all machines that aren't machine/0 were meant to have their
own Nonce's. Is this just to have *something*? (As in, it could just as
easily be a random string, as long as you set it and preserve it?)

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode430
cmd/jujud/agent_test.go:430: s.retryDelay, retryDelay = retryDelay,
time.Millisecond
I appreciate not having delays in the test suite.
I wonder about using a more obvious naming convention for package-global
variables that will be mutated in separate files.

Certainly when I read this, it looks like a function-local 'retryDelay'
variable. I realize there is no ':', and I can do the 'grep' to figure
out where 'retryDelay' is defined (somewhere in the cmd/jujud package).

Nothing to do with your proposed changes, just something about "where is
this variable coming from" that I find hard to read with our current
idioms. (In python you at least have to say 'global retryDelay' so you
know to go look for it somewhere else. And even then it would still be
in the same file.)

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode448
cmd/jujud/agent_test.go:448: }
Not specifically related to your patch, but "type Agent interface" has
no doc strings for me to understand what it is actually trying to do.
And AgentState is defined in 'upgrade.go' while 'Agent' is defined in
agent.go.

Anyway, the point I wanted to make was to think about Mock and what we
are trying to do. The concern is always that a Mock gets out of sync
with the real thing, and your tests just always keep passing because
Mock doesn't migrate.

I'm guessing go's "interface" helps us enough here, in that if Agent
changes its interface mockAgent will be updated to match (at least the
function signatures). Like in this case, you changed from RunOnce to
StartTasks, you do have to update the mock or the compiler will
complain.

In general, I love to see decoupling of the various bits of Juju and
make it easier to test just one layer. Though I generally prefer tested
Doubles (they act like the thing they are doubling) rather than
ultralightweight Mocks (they record what they were called with).

It also seems odd that you are able to mock out running 'heavyweight
tasks', but you still have to have a State object that connects to a
real mongodb running. It feels like if you are going to mock anything,
you would want to remove the State conn...

[William Reade (fwereade)]

Conflicts irreparably with API changes to jujud

[William Reade (fwereade)]

Thanks for the review; dropping this due to collision.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go
File cmd/jujud/agent_test.go (right):

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode275
cmd/jujud/agent_test.go:275: conf.MachineNonce = state.BootstrapNonce
On 2013/06/23 12:53:07, jameinel wrote:
> I thought all machines that aren't machine/0 were meant to have their
own
> Nonce's. Is this just to have *something*? (As in, it could just as
easily be a
> random string, as long as you set it and preserve it?)

Ha, yeah, I moved that method without reading it. Not sure why that's
there at all.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode430
cmd/jujud/agent_test.go:430: s.retryDelay, retryDelay = retryDelay,
time.Millisecond
On 2013/06/23 12:53:07, jameinel wrote:
> I appreciate not having delays in the test suite.
> I wonder about using a more obvious naming convention for
package-global
> variables that will be mutated in separate files.

> Certainly when I read this, it looks like a function-local
'retryDelay'
> variable. I realize there is no ':', and I can do the 'grep' to figure
out where
> 'retryDelay' is defined (somewhere in the cmd/jujud package).

> Nothing to do with your proposed changes, just something about "where
is this
> variable coming from" that I find hard to read with our current
idioms. (In
> python you at least have to say 'global retryDelay' so you know to go
look for
> it somewhere else. And even then it would still be in the same file.)

I think we've got a bit of a general problem with code being pretty
randomly scattered across files in a package... that's not for want of
bikeshedding, though. It's worse here because we're testing in-package
and the usual restrictions don't apply.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode448
cmd/jujud/agent_test.go:448: }
On 2013/06/23 12:53:07, jameinel wrote:
> Not specifically related to your patch, but "type Agent interface" has
no doc
> strings for me to understand what it is actually trying to do. And
AgentState is
> defined in 'upgrade.go' while 'Agent' is defined in agent.go.

I hate spelunking through this package for exactly these reasons :).

> Anyway, the point I wanted to make was to think about Mock and what we
are
> trying to do. The concern is always that a Mock gets out of sync with
the real
> thing, and your tests just always keep passing because Mock doesn't
migrate.

> I'm guessing go's "interface" helps us enough here, in that if Agent
changes its
> interface mockAgent will be updated to match (at least the function
signatures).
> Like in this case, you changed from RunOnce to StartTasks, you do have
to update
> the mock or the compiler will complain.

Yeah; doesn't help with semantic changes, but it's handy all the same.

> In general, I love to see decoupling of the various bits of Juju and
make it
> easier to test just one layer. Though I generally prefer tested
Doubles (they
> act like the thing they are doubling) rather than ultralightweight
Mocks (they
> record what they were called with).

No theoretical argument, but I'm not...

Unmerged revisions

1318. By William Reade on 2013-06-21

go fmt :/

1317. By William Reade on 2013-06-21

merge parent

1316. By William Reade on 2013-06-21

machine agent with JobManageState now runs Cleaner, Resumer; also, more unity testing for machine agent

=== modified file 'cmd/jujud/agent.go'
--- cmd/jujud/agent.go	2013-05-31 08:33:34 +0000
+++ cmd/jujud/agent.go	2013-06-21 20:42:27 +0000
@@ -126,10 +126,10 @@
 }
 
 type Agent interface {
+	Tag() string
 	Tomb() *tomb.Tomb
-	RunOnce(st *state.State, entity AgentState) error
 	Entity(st *state.State) (AgentState, error)
-	Tag() string
+	StartTasks(st *state.State, entity AgentState) []task
 }
 
 // runLoop repeatedly calls runOnce until it returns worker.ErrTerminateAgent
@@ -187,17 +187,19 @@
 }
 
 // RunAgentLoop repeatedly connects to the state server
-// and calls the agent's RunOnce method.
+// and runs the agent's tasks.
 func RunAgentLoop(c *agent.Conf, a Agent) error {
-	return runLoop(func() error {
+	dying := a.Tomb().Dying()
+	body := func() error {
 		st, entity, err := openState(c, a)
 		if err != nil {
 			return err
 		}
 		defer st.Close()
 		// TODO(rog) connect to API.
-		return a.RunOnce(st, entity)
-	}, a.Tomb().Dying())
+		return runTasks(dying, a.StartTasks(st, entity)...)
+	}
+	return runLoop(body, dying)
 }
 
 // This mutex ensures that we can have two concurrent workers
 
=== modified file 'cmd/jujud/agent_test.go'
--- cmd/jujud/agent_test.go	2013-05-31 08:33:34 +0000
+++ cmd/jujud/agent_test.go	2013-06-21 20:42:27 +0000
@@ -5,9 +5,16 @@
 
 import (
 	"bytes"
+	stderrors "errors"
 	"fmt"
+	"net/http"
+	"os"
+	"path/filepath"
+	"time"
+
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs/agent"
 	"launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/errors"
@@ -17,10 +24,6 @@
 	"launchpad.net/juju-core/version"
 	"launchpad.net/juju-core/worker"
 	"launchpad.net/tomb"
-	"net/http"
-	"os"
-	"path/filepath"
-	"time"
 )
 
 var _ = Suite(&toolSuite{})
@@ -258,6 +261,23 @@
 	testing.JujuConnSuite
 }
 
+// primeMachineAgent adds a new Machine to run the given jobs, and sets up the
+// machine agent's directory.  It returns the new machine, the
+// agent's configuration and the tools currently running.
+func (s *agentSuite) primeMachineAgent(c *C, jobs ...state.MachineJob) (*state.Machine, *agent.Conf, *state.Tools) {
+	m, err := s.State.InjectMachine("series", constraints.Value{}, "ardbeg-0", jobs...)
+	c.Assert(err, IsNil)
+	err = m.SetMongoPassword("machine-password")
+	c.Assert(err, IsNil)
+	err = m.SetPassword("machine-api-password")
+	c.Assert(err, IsNil)
+	conf, tools := s.primeAgent(c, state.MachineTag(m.Id()), "machine-password")
+	conf.MachineNonce = state.BootstrapNonce
+	conf.Write()
+	c.Assert(err, IsNil)
+	return m, conf, tools
+}
+
 // primeAgent writes the configuration file and tools
 // for an agent with the given entity name.
 // It returns the agent's configuration and the current tools.
@@ -399,3 +419,168 @@
 	*c = *nc
 	return nil
 }
+
+type RunAgentLoopSuite struct {
+	agentSuite
+	retryDelay time.Duration
+}
+
+func (s *RunAgentLoopSuite) SetUpTest(c *C) {
+	s.agentSuite.SetUpTest(c)
+	s.retryDelay, retryDelay = retryDelay, time.Millisecond
+}
+
+func (s *RunAgentLoopSuite) TearDownTest(c *C) {
+	retryDelay = s.retryDelay
+	s.agentSuite.TearDownTest(c)
+}
+
+var _ = Suite(&RunAgentLoopSuite{})
+
+// mockAgent allows us to inject our own task list into RunAgentLoop and thus
+// verify its behaviour without running real, heavyweight, tasks.
+type mockAgent struct {
+	c      *C
+	conf   *agent.Conf
+	tomb   tomb.Tomb
+	entity AgentState
+	tasks  func() []task
+}
+
+func (m *mockAgent) Tag() string {
+	return m.entity.Tag()
+}
+
+func (m *mockAgent) Tomb() *tomb.Tomb {
+	return &m.tomb
+}
+
+func (m *mockAgent) Entity(_ *state.State) (AgentState, error) {
+	return m.entity, nil
+}
+
+func (m *mockAgent) StartTasks(_ *state.State, entity AgentState) []task {
+	m.c.Assert(entity, Equals, m.entity)
+	return m.tasks()
+}
+
+func (s *RunAgentLoopSuite) startLoop(c *C, tasks func() []task) (*tomb.Tomb, <-chan error) {
+	machine, conf, _ := s.primeMachineAgent(c, state.JobHostUnits)
+	agent := &mockAgent{
+		conf:   conf,
+		entity: machine,
+		tasks:  tasks,
+	}
+	done := make(chan error)
+	go func() {
+		done <- RunAgentLoop(conf, agent)
+	}()
+	return agent.Tomb(), done
+}
+
+// errorTask returns a new error each time it's run (and then panics).
+type errorTask struct {
+	c         *C
+	waitCount int
+	errs      []error
+}
+
+func (t *errorTask) Stop() error {
+	err := t.errs[t.waitCount]
+	t.waitCount++
+	return err
+}
+
+func (t *errorTask) Wait() error {
+	return t.errs[t.waitCount]
+}
+
+func (t *errorTask) String() string {
+	return fmt.Sprintf("%#v", t)
+}
+
+func (s *RunAgentLoopSuite) TestRetryUntilErrTerminateAgent(c *C) {
+	errTask := &errorTask{c: c, errs: []error{
+		stderrors.New("abritrary"),
+		stderrors.New("different"),
+		nil,
+		stderrors.New("another"),
+		worker.ErrTerminateAgent,
+	}}
+	_, done := s.startLoop(c, func() []task {
+		return []task{errTask}
+	})
+	select {
+	case <-time.After(500 * time.Millisecond):
+		c.Fatalf("tasks not stopped")
+	case err := <-done:
+		c.Assert(err, IsNil)
+	}
+}
+
+func (s *RunAgentLoopSuite) TestRetryUntilUpgradedError(c *C) {
+	upgradeErr := &UpgradeReadyError{}
+	errTask := &errorTask{c: c, errs: []error{
+		stderrors.New("abritrary"),
+		stderrors.New("different"),
+		nil,
+		stderrors.New("another"),
+		upgradeErr,
+	}}
+	_, done := s.startLoop(c, func() []task {
+		return []task{errTask}
+	})
+	select {
+	case <-time.After(500 * time.Millisecond):
+		c.Fatalf("tasks not stopped")
+	case err := <-done:
+		c.Assert(err, Equals, upgradeErr)
+	}
+}
+
+// waitTask is a task that "runs" until stopped and returns its error.
+type waitTask struct {
+	tomb tomb.Tomb
+	err  error
+}
+
+func (t *waitTask) Stop() error {
+	t.tomb.Kill(t.err)
+	t.tomb.Done()
+	return t.Wait()
+}
+
+func (t *waitTask) Wait() error {
+	return t.tomb.Wait()
+}
+
+func (t *waitTask) String() string {
+	return fmt.Sprintf("%#v", t)
+}
+
+func (s *RunAgentLoopSuite) TestRunUntilStopped(c *C) {
+	tomb, done := s.startLoop(c, func() []task {
+		return []task{&waitTask{}}
+	})
+	tomb.Kill(stderrors.New("external"))
+	select {
+	case <-time.After(500 * time.Millisecond):
+		c.Fatalf("task not stopped")
+	case err := <-done:
+		c.Assert(err, IsNil)
+	}
+}
+
+func (s *RunAgentLoopSuite) TestRunUntilStoppedError(c *C) {
+	tomb, done := s.startLoop(c, func() []task {
+		return []task{&waitTask{err: stderrors.New("don't stop me now")}}
+	})
+	tomb.Kill(stderrors.New("external"))
+	select {
+	case <-time.After(500 * time.Millisecond):
+		c.Fatalf("task not stopped")
+	case err := <-done:
+		// It's OK, it'll be logged.
+		c.Assert(err, IsNil)
+	}
+}
 
=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go	2013-06-18 11:04:41 +0000
+++ cmd/jujud/machine.go	2013-06-21 20:42:27 +0000
@@ -5,6 +5,9 @@
 
 import (
 	"fmt"
+	"path/filepath"
+	"time"
+
 	"launchpad.net/gnuflag"
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/cmd"
@@ -13,12 +16,12 @@
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/apiserver"
 	"launchpad.net/juju-core/worker"
+	"launchpad.net/juju-core/worker/cleaner"
 	"launchpad.net/juju-core/worker/firewaller"
 	"launchpad.net/juju-core/worker/machiner"
 	"launchpad.net/juju-core/worker/provisioner"
+	"launchpad.net/juju-core/worker/resumer"
 	"launchpad.net/tomb"
-	"path/filepath"
-	"time"
 )
 
 var retryDelay = 3 * time.Second
@@ -105,7 +108,8 @@
 	return err
 }
 
-func (a *MachineAgent) RunOnce(st *state.State, e AgentState) error {
+// StartTasks returns all tasks the machine agent is expected to perform.
+func (a *MachineAgent) StartTasks(st *state.State, e AgentState) []task {
 	m := e.(*state.Machine)
 	log.Infof("jobs for machine agent: %v", m.Jobs())
 	dataDir := a.Conf.DataDir
@@ -123,13 +127,14 @@
 				provisioner.NewProvisioner(st, a.MachineId),
 				firewaller.NewFirewaller(st))
 		case state.JobManageState:
-			// Ignore because it's started independently.
-			continue
+			tasks = append(tasks,
+				cleaner.NewCleaner(st),
+				resumer.NewResumer(st))
 		default:
 			log.Warningf("ignoring unknown job %q", j)
 		}
 	}
-	return runTasks(a.tomb.Dying(), tasks...)
+	return tasks
 }
 
 func (a *MachineAgent) Entity(st *state.State) (AgentState, error) {
 
=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2013-06-21 13:33:32 +0000
+++ cmd/jujud/machine_test.go	2013-06-21 20:42:27 +0000
@@ -5,23 +5,21 @@
 
 import (
 	"fmt"
+	"path/filepath"
+	"reflect"
+	"sort"
+	"sync"
+	"time"
+
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/cmd"
-	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs/agent"
 	"launchpad.net/juju-core/environs/dummy"
-	envtesting "launchpad.net/juju-core/environs/testing"
-	"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/testing"
-	"launchpad.net/juju-core/version"
-	"path/filepath"
-	"reflect"
-	"time"
 )
 
 type MachineSuite struct {
@@ -41,23 +39,6 @@
 	s.agentSuite.TearDownSuite(c)
 }
 
-// primeAgent adds a new Machine to run the given jobs, and sets up the
-// machine agent's directory.  It returns the new machine, the
-// agent's configuration and the tools currently running.
-func (s *MachineSuite) primeAgent(c *C, jobs ...state.MachineJob) (*state.Machine, *agent.Conf, *state.Tools) {
-	m, err := s.State.InjectMachine("series", constraints.Value{}, "ardbeg-0", jobs...)
-	c.Assert(err, IsNil)
-	err = m.SetMongoPassword("machine-password")
-	c.Assert(err, IsNil)
-	err = m.SetPassword("machine-api-password")
-	c.Assert(err, IsNil)
-	conf, tools := s.agentSuite.primeAgent(c, state.MachineTag(m.Id()), "machine-password")
-	conf.MachineNonce = state.BootstrapNonce
-	conf.Write()
-	c.Assert(err, IsNil)
-	return m, conf, tools
-}
-
 // newAgent returns a new MachineAgent instance
 func (s *MachineSuite) newAgent(c *C, m *state.Machine) *MachineAgent {
 	a := &MachineAgent{}
@@ -92,13 +73,13 @@
 
 func (s *MachineSuite) TestRunInvalidMachineId(c *C) {
 	c.Skip("agents don't yet distinguish between temporary and permanent errors")
-	m, _, _ := s.primeAgent(c, state.JobHostUnits)
+	m, _, _ := s.primeMachineAgent(c, state.JobHostUnits)
 	err := s.newAgent(c, m).Run(nil)
 	c.Assert(err, ErrorMatches, "some error")
 }
 
 func (s *MachineSuite) TestRunStop(c *C) {
-	m, ac, _ := s.primeAgent(c, state.JobHostUnits)
+	m, ac, _ := s.primeMachineAgent(c, state.JobHostUnits)
 	a := s.newAgent(c, m)
 	done := make(chan error)
 	go func() {
@@ -111,7 +92,7 @@
 }
 
 func (s *MachineSuite) TestWithDeadMachine(c *C) {
-	m, _, _ := s.primeAgent(c, state.JobHostUnits, state.JobManageState)
+	m, _, _ := s.primeMachineAgent(c, state.JobHostUnits, state.JobManageState)
 	err := m.EnsureDead()
 	c.Assert(err, IsNil)
 	a := s.newAgent(c, m)
@@ -126,153 +107,6 @@
 	c.Assert(err, IsNil)
 }
 
-func (s *MachineSuite) TestDyingMachine(c *C) {
-	m, _, _ := s.primeAgent(c, state.JobHostUnits)
-	a := s.newAgent(c, m)
-	done := make(chan error)
-	go func() {
-		done <- a.Run(nil)
-	}()
-	defer func() {
-		c.Check(a.Stop(), IsNil)
-	}()
-	time.Sleep(1 * time.Second)
-	err := m.Destroy()
-	c.Assert(err, IsNil)
-	select {
-	case err := <-done:
-		c.Assert(err, IsNil)
-	case <-time.After(watcher.Period * 5 / 4):
-		// TODO(rog) Fix this so it doesn't wait for so long.
-		// https://bugs.launchpad.net/juju-core/+bug/1163983
-		c.Fatalf("timed out waiting for agent to terminate")
-	}
-	err = m.Refresh()
-	c.Assert(err, IsNil)
-	c.Assert(m.Life(), Equals, state.Dead)
-}
-
-func (s *MachineSuite) TestHostUnits(c *C) {
-	m, conf, _ := s.primeAgent(c, state.JobHostUnits)
-	a := s.newAgent(c, m)
-	ctx, reset := patchDeployContext(c, conf.StateInfo, conf.DataDir)
-	defer reset()
-	go func() { c.Check(a.Run(nil), IsNil) }()
-	defer func() { c.Check(a.Stop(), IsNil) }()
-
-	// check that unassigned units don't trigger any deployments.
-	svc, err := s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))
-	c.Assert(err, IsNil)
-	u0, err := svc.AddUnit()
-	c.Assert(err, IsNil)
-	u1, err := svc.AddUnit()
-	c.Assert(err, IsNil)
-	ctx.waitDeployed(c)
-
-	// assign u0, check it's deployed.
-	err = u0.AssignToMachine(m)
-	c.Assert(err, IsNil)
-	ctx.waitDeployed(c, u0.Name())
-
-	// "start the agent" for u0 to prevent short-circuited remove-on-destroy;
-	// check that it's kept deployed despite being Dying.
-	err = u0.SetStatus(params.StatusStarted, "")
-	c.Assert(err, IsNil)
-	err = u0.Destroy()
-	c.Assert(err, IsNil)
-	ctx.waitDeployed(c, u0.Name())
-
-	// add u1 to the machine, check it's deployed.
-	err = u1.AssignToMachine(m)
-	c.Assert(err, IsNil)
-	ctx.waitDeployed(c, u0.Name(), u1.Name())
-
-	// make u0 dead; check the deployer recalls the unit and removes it from
-	// state.
-	err = u0.EnsureDead()
-	c.Assert(err, IsNil)
-	ctx.waitDeployed(c, u1.Name())
-	err = u0.Refresh()
-	c.Assert(errors.IsNotFoundError(err), Equals, true)
-
-	// short-circuit-remove u1 after it's been deployed; check it's recalled
-	// and removed from state.
-	err = u1.Destroy()
-	c.Assert(err, IsNil)
-	err = u1.Refresh()
-	c.Assert(errors.IsNotFoundError(err), Equals, true)
-	ctx.waitDeployed(c)
-}
-
-func (s *MachineSuite) TestManageEnviron(c *C) {
-	usefulVersion := version.Current
-	usefulVersion.Series = "series" // to match the charm created below
-	envtesting.UploadFakeToolsVersion(c, s.Conn.Environ.Storage(), usefulVersion)
-	m, _, _ := s.primeAgent(c, state.JobManageEnviron)
-	op := make(chan dummy.Operation, 200)
-	dummy.Listen(op)
-
-	a := s.newAgent(c, m)
-	// Make sure the agent is stopped even if the test fails.
-	defer a.Stop()
-	done := make(chan error)
-	go func() {
-		done <- a.Run(nil)
-	}()
-
-	// Check that the provisioner and firewaller are alive by doing
-	// a rudimentary check that it responds to state changes.
-
-	// Add one unit to a service; it should get allocated a machine
-	// and then its ports should be opened.
-	charm := s.AddTestingCharm(c, "dummy")
-	svc, err := s.State.AddService("test-service", charm)
-	c.Assert(err, IsNil)
-	err = svc.SetExposed()
-	c.Assert(err, IsNil)
-	units, err := s.Conn.AddUnits(svc, 1, "")
-	c.Assert(err, IsNil)
-	c.Check(opRecvTimeout(c, s.State, op, dummy.OpStartInstance{}), NotNil)
-
-	// Wait for the instance id to show up in the state.
-	id1, err := units[0].AssignedMachineId()
-	c.Assert(err, IsNil)
-	m1, err := s.State.Machine(id1)
-	c.Assert(err, IsNil)
-	w := m1.Watch()
-	defer w.Stop()
-	for _ = range w.Changes() {
-		err = m1.Refresh()
-		c.Assert(err, IsNil)
-		if _, ok := m1.InstanceId(); ok {
-			break
-		}
-	}
-	err = units[0].OpenPort("tcp", 999)
-	c.Assert(err, IsNil)
-
-	c.Check(opRecvTimeout(c, s.State, op, dummy.OpOpenPorts{}), NotNil)
-
-	err = a.Stop()
-	c.Assert(err, IsNil)
-
-	select {
-	case err := <-done:
-		c.Assert(err, IsNil)
-	case <-time.After(5 * time.Second):
-		c.Fatalf("timed out waiting for agent to terminate")
-	}
-}
-
-func (s *MachineSuite) TestUpgrade(c *C) {
-	m, conf, currentTools := s.primeAgent(c, state.JobManageState, state.JobManageEnviron, state.JobHostUnits)
-	addAPIInfo(conf, m)
-	err := conf.Write()
-	c.Assert(err, IsNil)
-	a := s.newAgent(c, m)
-	s.testUpgrade(c, a, currentTools)
-}
-
 func addAPIInfo(conf *agent.Conf, m *state.Machine) {
 	port := testing.FindTCPPort()
 	conf.APIInfo = &api.Info{
@@ -292,7 +126,7 @@
 }
 
 func (s *MachineSuite) TestServeAPI(c *C) {
-	stm, conf, _ := s.primeAgent(c, state.JobManageState)
+	stm, conf, _ := s.primeMachineAgent(c, state.JobManageState)
 	addAPIInfo(conf, stm)
 	err := conf.Write()
 	c.Assert(err, IsNil)
@@ -338,7 +172,7 @@
 }}
 
 func (s *MachineSuite) TestServeAPIWithBadConf(c *C) {
-	m, conf, _ := s.primeAgent(c, state.JobManageState)
+	m, conf, _ := s.primeMachineAgent(c, state.JobManageState)
 	addAPIInfo(conf, m)
 	for i, t := range serveAPIWithBadConfTests {
 		c.Logf("test %d: %q", i, t.err)
@@ -375,9 +209,64 @@
 }
 
 func (s *MachineSuite) TestChangePasswordChanging(c *C) {
-	m, _, _ := s.primeAgent(c, state.JobHostUnits)
+	m, _, _ := s.primeMachineAgent(c, state.JobHostUnits)
 	newAgent := func() runner {
 		return s.newAgent(c, m)
 	}
 	s.testAgentPasswordChanging(c, m, newAgent)
 }
+
+type jobs []state.MachineJob
+
+func (s *MachineSuite) TestStateTasks(c *C) {
+	for i, test := range []struct {
+		info   string
+		jobs   []state.MachineJob
+		expect []string
+	}{{
+		jobs:   jobs{state.JobHostUnits},
+		expect: []string{"upgrader", "machiner", "deployer"},
+	}, {
+		jobs:   jobs{state.JobManageEnviron},
+		expect: []string{"upgrader", "machiner", "provisioning worker", "firewaller"},
+	}, {
+		jobs:   jobs{state.JobManageState},
+		expect: []string{"upgrader", "machiner", "cleaner", "resumer"},
+	}, {
+		jobs:   jobs{state.JobManageState, state.JobHostUnits},
+		expect: []string{"upgrader", "machiner", "cleaner", "resumer", "deployer"},
+	}} {
+		c.Logf("test %d: %v", i, test.jobs)
+		machine, conf, _ := s.primeMachineAgent(c, test.jobs...)
+		expect := make([]string, len(test.expect))
+		for i, kind := range test.expect {
+			switch kind {
+			case "machiner":
+				kind = fmt.Sprintf("machiner %s", machine.Id())
+			case "deployer":
+				kind = fmt.Sprintf("deployer for machine-%s", machine.Id())
+			}
+			expect[i] = kind
+		}
+		sort.Strings(expect)
+
+		// This is horrible. But it makes this functionality somewhat testable...
+		agent := s.newAgent(c, machine)
+		agent.Conf.Conf = conf
+
+		var wg = sync.WaitGroup{}
+		var actual []string
+		for _, t := range agent.StartTasks(s.State, machine) {
+			actual = append(actual, t.String())
+			wg.Add(1)
+			go func(t task) {
+				c.Check(t.Stop(), IsNil)
+				wg.Done()
+			}(t)
+		}
+		wg.Wait()
+		sort.Strings(actual)
+		c.Logf("%#v", actual)
+		c.Check(actual, DeepEquals, expect)
+	}
+}
 
=== modified file 'cmd/jujud/unit.go'
--- cmd/jujud/unit.go	2013-05-28 14:22:24 +0000
+++ cmd/jujud/unit.go	2013-06-21 20:42:27 +0000
@@ -66,8 +66,8 @@
 	return err
 }
 
-// RunOnce runs a unit agent once.
-func (a *UnitAgent) RunOnce(st *state.State, e AgentState) error {
+// StartTasks returns all tasks the unit agent is expected to perform.
+func (a *UnitAgent) StartTasks(st *state.State, e AgentState) []task {
 	unit := e.(*state.Unit)
 	dataDir := a.Conf.DataDir
 	tasks := []task{
@@ -78,7 +78,7 @@
 		tasks = append(tasks,
 			newDeployer(st, unit.WatchSubordinateUnits(), dataDir))
 	}
-	return runTasks(a.tomb.Dying(), tasks...)
+	return tasks
 }
 
 func (a *UnitAgent) Entity(st *state.State) (AgentState, error) {
 
=== modified file 'environs/testing/storage.go'
--- environs/testing/storage.go	2013-06-20 13:27:50 +0000
+++ environs/testing/storage.go	2013-06-21 20:42:27 +0000
@@ -46,7 +46,7 @@
 	listener net.Listener
 }
 
-// NewEC2HTTPTestStorage creates a storage server for tests 
+// NewEC2HTTPTestStorage creates a storage server for tests
 // with the HTTPStorageReader.
 func NewEC2HTTPTestStorage(ip string) (*EC2HTTPTestStorage, error) {
 	var err error