Merge lp:~fwereade/juju-core/api-force-destroy-machines into lp:~go-bot/juju-core/trunk

[William Reade]

fix lp:1089291

...which involved a lot more churn than I wanted, mainly because those
DestroyWhatevers methods really shouldn't ever have been on state in the
first place, and moving them turned out to be a ridiculous hassle.

Despite the mess, this branch really just involves:

1) moving multi-Destroy methods off state and onto the api
2) fixing/moving/renaming test things
3) hooking up a --force flag to destroy-machine
4) oh, and making the relevant api.Client methods a teeny tiny bit
   consistent.

https://codereview.appspot.com/24790044/

fix lp:1089291

...which involved a lot more churn than I wanted, mainly because those
DestroyWhatevers methods really shouldn't ever have been on state in the
first place, and moving them turned out to be a ridiculous hassle.

Despite the mess, this branch really just involves:

1) moving multi-Destroy methods off state and onto the api
2) fixing/moving/renaming test things
3) hooking up a --force flag to destroy-machine
4) oh, and making the relevant api.Client methods a teeny tiny bit
   consistent.

https://codereview.appspot.com/24790044/

[William Reade (fwereade)]

Reviewers: mp+194764_code.launchpad.net,

Message:
Please take a look.

Description:
fix lp:1089291

...which involved a lot more churn than I wanted, mainly because those
DestroyWhatevers methods really shouldn't ever have been on state in the
first place, and moving them turned out to be a ridiculous hassle.

Despite the mess, this branch really just involves:

1) moving multi-Destroy methods off state and onto the api
2) fixing/moving/renaming test things
3) hooking up a --force flag to destroy-machine
4) oh, and making the relevant api.Client methods a teeny tiny bit
    consistent.

https://code.launchpad.net/~fwereade/juju-core/api-force-destroy-machines/+merge/194764

(do not edit description out of merge proposal)

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

Affected files (+363, -284 lines):
   A [revision details]
   M cmd/juju/destroymachine.go
   M cmd/juju/destroymachine_test.go
   M cmd/juju/destroyunit.go
   M cmd/jujud/machine_test.go
   M state/api/client.go
   M state/api/params/params.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go
   M state/apiserver/client/perm_test.go
   M state/cleanup_test.go
   M state/life.go
   M state/life_test.go
   M state/machine.go
   M state/machine_test.go
   M state/service_test.go
   M state/state.go
   M state/unit_test.go

[Ian Booth (wallyworld)]

Some nice refactoring. It does make me a little sad that we are putting
service methods (ie verbs like Destroy) on domain objects. IMO we want
to be heading towards a SOA and domain objects should only be
responsible for their own internal state, not interacting with services
and jobs etc. So it is great that the methods have come off State but
putting them on Machine is wrong to me.

My other main issue is that forcibly destroying a machine leaves its
life as Alive. That is very confusing to me.

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine.go
File cmd/juju/destroymachine.go (right):

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine.go#newcode34
cmd/juju/destroymachine.go:34: f.BoolVar(&c.Force, "force", false,
"completely remove machine and all dependencies")
Perhaps expand a bit:

"completely remove machine and all dependencies, even if Juju thinks the
machine has running units or containers"

I think we need to be verbose as to what the implications are. I would
even go so far as to say we need a [y/N] prompt like for
destroy-environment as a typo in the machine id could be catastrophic.

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine_test.go
File cmd/juju/destroymachine_test.go (right):

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine_test.go#newcode48
cmd/juju/destroymachine_test.go:48: // is warned about.
The above comment needs rewording now the test has been split up

https://codereview.appspot.com/24790044/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/24790044/diff/1/state/api/client.go#newcode154
state/api/client.go:154: func (c *Client) ForceDestroyMachines(machines
...string) error {
It makes me sad that we need to add a whole new endpoint because Go
doesn't support default parameter values :-(

https://codereview.appspot.com/24790044/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/24790044/diff/1/state/apiserver/client/client_test.go#newcode549
state/apiserver/client/client_test.go:549: assertLife(c, m0,
state.Alive)
Why doesn't force destroy cause the life to be set to Dying? I find it
particularly unintuitive that a status would show the machine as alive
when in fact it is on death row and waiting for the clean up job to
remove it.

https://codereview.appspot.com/24790044/diff/1/state/cleanup_test.go
File state/cleanup_test.go (right):

https://codereview.appspot.com/24790044/diff/1/state/cleanup_test.go#newcode93
state/cleanup_test.go:93: func (s *CleanupSuite)
TestForceDestroyMachineErrors(c *gc.C) {
The code in this test can definitely be extracted out into a helper

func (s *CleanupSuite) assertForceDestroyMachineError(c *gc.C, job
state.MachineJob) {
     m, err := s.State.AddMachine("quantal", job)
     c.Assert(err, gc.IsNil)
     err = environManager.ForceDestroy()
     c.Assert(err, gc.ErrorMatches, "machine [0-9] is required by the
environment")
     s.assertDoesNotNeedCleanup(c)
     assertLife(c, m, state.Alive)
}

https://codereview.appspot.com/24790044/diff/1/state/machine.go
File state/machin...

[John A Meinel (jameinel)]

just some of my thoughts, not a full review.

https://codereview.appspot.com/24790044/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/24790044/diff/1/state/api/client.go#newcode154
state/api/client.go:154: func (c *Client) ForceDestroyMachines(machines
...string) error {
On 2013/11/12 00:43:42, wallyworld wrote:
> It makes me sad that we need to add a whole new endpoint because Go
doesn't
> support default parameter values :-(

It isn't really an endpoint, but it is a new function.

I only make the distinction because I was about to say the next change:
-func (c *Client) DestroyServiceUnits(unitNames []string) error {
+func (c *Client) DestroyServiceUnits(unitNames ...string) error {

was an API break, but this is all just within the client process.

so we *could* spell it:

DestroyMachines(force bool, machines ...string)

I'm guessing it was a bit ackward and William wanted to avoid bool in
APIs especially for a Force sort of function.

https://codereview.appspot.com/24790044/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/24790044/diff/1/state/apiserver/client/client_test.go#newcode549
state/apiserver/client/client_test.go:549: assertLife(c, m0,
state.Alive)
On 2013/11/12 00:43:42, wallyworld wrote:
> Why doesn't force destroy cause the life to be set to Dying? I find it
> particularly unintuitive that a status would show the machine as alive
when in
> fact it is on death row and waiting for the clean up job to remove it.

In earlier discussions, this is a limitation of the Machine lifecycle
model. A Machine can't be set to dying until all Units are no longer on
the machine.

https://codereview.appspot.com/24790044/

[William Reade (fwereade)]

Machine lifecycle is out of scope; SOA concepts demand more discussion
in a separate forum; I think I've addressed the rest.

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine.go
File cmd/juju/destroymachine.go (right):

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine.go#newcode34
cmd/juju/destroymachine.go:34: f.BoolVar(&c.Force, "force", false,
"completely remove machine and all dependencies")
On 2013/11/12 00:43:42, wallyworld wrote:
> Perhaps expand a bit:

> "completely remove machine and all dependencies, even if Juju thinks
the machine
> has running units or containers"

> I think we need to be verbose as to what the implications are. I would
even go
> so far as to say we need a [y/N] prompt like for destroy-environment
as a typo
> in the machine id could be catastrophic.

Expanded the doc string -- I'm kinda -1 on an additional gate beyond
specifying "--force" though.

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine_test.go
File cmd/juju/destroymachine_test.go (right):

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine_test.go#newcode48
cmd/juju/destroymachine_test.go:48: // is warned about.
On 2013/11/12 00:43:42, wallyworld wrote:
> The above comment needs rewording now the test has been split up

Whoops, thanks.

https://codereview.appspot.com/24790044/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/24790044/diff/1/state/api/client.go#newcode154
state/api/client.go:154: func (c *Client) ForceDestroyMachines(machines
...string) error {
On 2013/11/12 02:58:37, jameinel wrote:
> On 2013/11/12 00:43:42, wallyworld wrote:
> > It makes me sad that we need to add a whole new endpoint because Go
doesn't
> > support default parameter values :-(

> It isn't really an endpoint, but it is a new function.

> I only make the distinction because I was about to say the next
change:
> -func (c *Client) DestroyServiceUnits(unitNames []string) error {
> +func (c *Client) DestroyServiceUnits(unitNames ...string) error {

> was an API break, but this is all just within the client process.

> so we *could* spell it:

> DestroyMachines(force bool, machines ...string)

> I'm guessing it was a bit ackward and William wanted to avoid bool in
APIs
> especially for a Force sort of function.

Yeah, it's just a new function, and the bool option felt horrible. Re
DestroyServiceUnits I'm willing to impose that minimal pain on anyone
who's using api.Client in an external library, because I think the
global pain of inconsistency is worse.

https://codereview.appspot.com/24790044/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/24790044/diff/1/state/apiserver/client/client_test.go#newcode549
state/apiserver/client/client_test.go:549: assertLife(c, m0,
state.Alive)
On 2013/11/12 02:58:37, jameinel wrote:
> On 2013/11/12 00:43:42, wallyworld wrote:
> > Why doesn't force destroy cause the life to be set to Dying? I find
it
> > particularly unintuitive that a status would show the machine as
alive when in
> > fact it is on death row and waiting for the clean ...

[William Reade (fwereade)]

Please take a look.

https://codereview.appspot.com/24790044/

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/24790044/

[John A Meinel (jameinel)]

LGTM as well

https://codereview.appspot.com/24790044/

=== modified file 'cmd/juju/destroymachine.go'
--- cmd/juju/destroymachine.go	2013-11-01 06:53:54 +0000
+++ cmd/juju/destroymachine.go	2013-11-12 10:22:12 +0000
@@ -6,6 +6,8 @@
 import (
 	"fmt"
 
+	"launchpad.net/gnuflag"
+
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/juju"
 	"launchpad.net/juju-core/names"
@@ -15,18 +17,30 @@
 type DestroyMachineCommand struct {
 	cmd.EnvCommandBase
 	MachineIds []string
+	Force      bool
 }
 
+const destroyMachineDoc = `
+Machines that are responsible for the environment cannot be destroyed. Machines
+running units or containers can only be destroyed with the --force flag; doing
+so will also destroy all those units and containers without giving them any
+opportunity to shut down cleanly.
+`
+
 func (c *DestroyMachineCommand) Info() *cmd.Info {
 	return &cmd.Info{
 		Name:    "destroy-machine",
 		Args:    "<machine> ...",
 		Purpose: "destroy machines",
-		Doc:     "Machines that have assigned units, or are responsible for the environment, cannot be destroyed.",
+		Doc:     "",
 		Aliases: []string{"terminate-machine"},
 	}
 }
 
+func (c *DestroyMachineCommand) SetFlags(f *gnuflag.FlagSet) {
+	f.BoolVar(&c.Force, "force", false, "completely remove machine and all dependencies")
+}
+
 func (c *DestroyMachineCommand) Init(args []string) error {
 	if len(args) == 0 {
 		return fmt.Errorf("no machines specified")
@@ -46,5 +60,8 @@
 		return err
 	}
 	defer apiclient.Close()
+	if c.Force {
+		return apiclient.ForceDestroyMachines(c.MachineIds...)
+	}
 	return apiclient.DestroyMachines(c.MachineIds...)
 }
 
=== modified file 'cmd/juju/destroymachine_test.go'
--- cmd/juju/destroymachine_test.go	2013-09-27 07:05:45 +0000
+++ cmd/juju/destroymachine_test.go	2013-11-12 10:22:12 +0000
@@ -6,9 +6,11 @@
 import (
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/errors"
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
 )
 
 type DestroyMachineSuite struct {
@@ -22,7 +24,7 @@
 	return err
 }
 
-func (s *DestroyMachineSuite) TestDestroyMachine(c *gc.C) {
+func (s *DestroyMachineSuite) TestDestroyMachineWithUnit(c *gc.C) {
 	// Create a machine running a unit.
 	testing.Charms.BundlePath(s.SeriesPath, "riak")
 	err := runDeploy(c, "local:riak", "riak")
@@ -38,19 +40,16 @@
 	// Try to destroy the machine and fail.
 	err = runDestroyMachine(c, "0")
 	c.Assert(err, gc.ErrorMatches, `no machines were destroyed: machine 0 has unit "riak/0" assigned`)
+}
 
-	// Remove the unit, and try to destroy the machine along with another that
-	// doesn't exist; check that the machine is destroyed, but the missing one
-	// is warned about.
-	err = u.Destroy()
-	c.Assert(err, gc.IsNil)
-	err = u.EnsureDead()
-	c.Assert(err, gc.IsNil)
-	err = u.Remove()
+func (s *DestroyMachineSuite) TestDestroyEmptyMachine(c *gc.C) {
+	// Destroy an empty machine alongside a state server; only the empty machine
+	// gets destroyed.
+	m0, err := s.State.AddMachine("quantal", state.JobHostUnits)
 	c.Assert(err, gc.IsNil)
 	err = runDestroyMachine(c, "0", "1")
 	c.Assert(err, gc.ErrorMatches, `some machines were not destroyed: machine 1 does not exist`)
-	m0, err := s.State.Machine("0")
+	err = m0.Refresh()
 	c.Assert(err, gc.IsNil)
 	c.Assert(m0.Life(), gc.Equals, state.Dying)
 
@@ -60,9 +59,13 @@
 	err = m0.Refresh()
 	c.Assert(err, gc.IsNil)
 	c.Assert(m0.Life(), gc.Equals, state.Dying)
+}
 
-	// As is destroying a Dead machine; destroying it alongside a JobManageEnviron
-	// machine complains only about the JMA machine.
+func (s *DestroyMachineSuite) TestDestroyDeadMachine(c *gc.C) {
+	// Destroying a Dead machine is a no-op; destroying it alongside a JobManageEnviron
+	// machine complains only about the JME machine.
+	m0, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
 	err = m0.EnsureDead()
 	c.Assert(err, gc.IsNil)
 	m1, err := s.State.AddMachine("quantal", state.JobManageEnviron)
@@ -75,9 +78,43 @@
 	err = m1.Refresh()
 	c.Assert(err, gc.IsNil)
 	c.Assert(m1.Life(), gc.Equals, state.Alive)
-
+}
+
+func (s *DestroyMachineSuite) TestForce(c *gc.C) {
+	// Create a machine running a unit.
+	testing.Charms.BundlePath(s.SeriesPath, "riak")
+	err := runDeploy(c, "local:riak", "riak")
+	c.Assert(err, gc.IsNil)
+
+	// Get the state entities to allow sane testing.
+	u, err := s.State.Unit("riak/0")
+	c.Assert(err, gc.IsNil)
+	m0, err := s.State.Machine("0")
+	c.Assert(err, gc.IsNil)
+
+	// Create a manager machine.
+	m1, err := s.State.AddMachine("quantal", state.JobManageEnviron)
+	c.Assert(err, gc.IsNil)
+
+	// Try to force-destroy the machines.
+	err = runDestroyMachine(c, "0", "1", "--force")
+	c.Assert(err, gc.ErrorMatches, `some machines were not destroyed: machine 1 is required by the environment`)
+
+	// Clean up, check state.
+	err = s.State.Cleanup()
+	c.Assert(err, gc.IsNil)
+	err = m0.Refresh()
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+	err = u.Refresh()
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+	err = m1.Refresh()
+	c.Assert(err, gc.IsNil)
+	c.Assert(m1.Life(), gc.Equals, state.Alive)
+}
+
+func (s *DestroyMachineSuite) TestBadArgs(c *gc.C) {
 	// Check invalid args.
-	err = runDestroyMachine(c)
+	err := runDestroyMachine(c)
 	c.Assert(err, gc.ErrorMatches, `no machines specified`)
 	err = runDestroyMachine(c, "1", "2", "nonsense", "rubbish")
 	c.Assert(err, gc.ErrorMatches, `invalid machine id "nonsense"`)
 
=== modified file 'cmd/juju/destroyunit.go'
--- cmd/juju/destroyunit.go	2013-11-05 01:22:35 +0000
+++ cmd/juju/destroyunit.go	2013-11-12 10:22:12 +0000
@@ -48,5 +48,5 @@
 		return err
 	}
 	defer client.Close()
-	return client.DestroyServiceUnits(c.UnitNames)
+	return client.DestroyServiceUnits(c.UnitNames...)
 }
 
=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2013-11-11 10:57:06 +0000
+++ cmd/jujud/machine_test.go	2013-11-12 10:22:12 +0000
@@ -78,7 +78,7 @@
 	err = m.SetPassword(initialMachinePassword)
 	c.Assert(err, gc.IsNil)
 	tag := names.MachineTag(m.Id())
-	if m.IsStateServer() {
+	if m.IsManager() {
 		err = m.SetMongoPassword(initialMachinePassword)
 		c.Assert(err, gc.IsNil)
 		config, tools = s.agentSuite.primeStateAgent(c, tag, initialMachinePassword)
 
=== modified file 'state/api/client.go'
--- state/api/client.go	2013-11-08 06:36:55 +0000
+++ state/api/client.go	2013-11-12 10:22:12 +0000
@@ -150,6 +150,12 @@
 	return c.st.Call("Client", "", "DestroyMachines", params, nil)
 }
 
+// ForceDestroyMachines removes a given set of machines and all associated units.
+func (c *Client) ForceDestroyMachines(machines ...string) error {
+	params := params.DestroyMachines{Force: true, MachineNames: machines}
+	return c.st.Call("Client", "", "DestroyMachines", params, nil)
+}
+
 // ServiceExpose changes the juju-managed firewall to expose any ports that
 // were also explicitly marked by units as open.
 func (c *Client) ServiceExpose(service string) error {
@@ -207,7 +213,7 @@
 }
 
 // DestroyServiceUnits decreases the number of units dedicated to a service.
-func (c *Client) DestroyServiceUnits(unitNames []string) error {
+func (c *Client) DestroyServiceUnits(unitNames ...string) error {
 	params := params.DestroyServiceUnits{unitNames}
 	return c.st.Call("Client", "", "DestroyServiceUnits", params, nil)
 }
 
=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2013-11-08 06:36:55 +0000
+++ state/api/params/params.go	2013-11-12 10:22:12 +0000
@@ -93,6 +93,7 @@
 // DestroyMachines holds parameters for the DestroyMachines call.
 type DestroyMachines struct {
 	MachineNames []string
+	Force        bool
 }
 
 // ServiceDeploy holds the parameters for making the ServiceDeploy call.
 
=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2013-11-08 06:36:55 +0000
+++ state/apiserver/client/client.go	2013-11-12 10:22:12 +0000
@@ -6,11 +6,13 @@
 import (
 	"errors"
 	"fmt"
+	"strings"
 
 	"launchpad.net/loggo"
 
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/environs"
+	coreerrors "launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju"
 	"launchpad.net/juju-core/names"
@@ -413,7 +415,25 @@
 
 // DestroyServiceUnits removes a given set of service units.
 func (c *Client) DestroyServiceUnits(args params.DestroyServiceUnits) error {
-	return c.api.state.DestroyUnits(args.UnitNames...)
+	var errs []string
+	for _, name := range args.UnitNames {
+		unit, err := c.api.state.Unit(name)
+		switch {
+		case coreerrors.IsNotFoundError(err):
+			err = fmt.Errorf("unit %q does not exist", name)
+		case err != nil:
+		case unit.Life() != state.Alive:
+			continue
+		case unit.IsPrincipal():
+			err = unit.Destroy()
+		default:
+			err = fmt.Errorf("unit %q is a subordinate", name)
+		}
+		if err != nil {
+			errs = append(errs, err.Error())
+		}
+	}
+	return destroyErr("units", args.UnitNames, errs)
 }
 
 // ServiceDestroy destroys a given service.
@@ -625,7 +645,25 @@
 
 // DestroyMachines removes a given set of machines.
 func (c *Client) DestroyMachines(args params.DestroyMachines) error {
-	return c.api.state.DestroyMachines(args.MachineNames...)
+	var errs []string
+	for _, id := range args.MachineNames {
+		machine, err := c.api.state.Machine(id)
+		switch {
+		case coreerrors.IsNotFoundError(err):
+			err = fmt.Errorf("machine %s does not exist", id)
+		case err != nil:
+		case args.Force:
+			err = machine.ForceDestroy()
+		case machine.Life() != state.Alive:
+			continue
+		default:
+			err = machine.Destroy()
+		}
+		if err != nil {
+			errs = append(errs, err.Error())
+		}
+	}
+	return destroyErr("machines", args.MachineNames, errs)
 }
 
 // CharmInfo returns information about the requested charm.
@@ -786,3 +824,15 @@
 	// Now try to apply the new validated config.
 	return c.api.state.SetEnvironConfig(newProviderConfig)
 }
+
+func destroyErr(desc string, ids, errs []string) error {
+	if len(errs) == 0 {
+		return nil
+	}
+	msg := "some %s were not destroyed"
+	if len(errs) == len(ids) {
+		msg = "no %s were destroyed"
+	}
+	msg = fmt.Sprintf(msg, desc)
+	return fmt.Errorf("%s: %s", msg, strings.Join(errs, "; "))
+}
 
=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2013-11-08 06:36:55 +0000
+++ state/apiserver/client/client_test.go	2013-11-12 10:22:12 +0000
@@ -493,38 +493,143 @@
 	c.Assert(service.Life(), gc.Not(gc.Equals), state.Alive)
 }
 
-func (s *clientSuite) TestClientDestroyMachines(c *gc.C) {
-	s.setUpScenario(c)
-	err := s.APIState.Client().DestroyMachines("1")
-	c.Assert(err, gc.ErrorMatches, `no machines were destroyed: machine 1 has unit "wordpress/0" assigned`)
-	m, err := s.State.AddMachine("trusty", state.JobHostUnits)
-	c.Assert(err, gc.IsNil)
-	err = s.APIState.Client().DestroyMachines(m.Id())
-	c.Assert(err, gc.IsNil)
-	err = m.Refresh()
-	c.Assert(err, gc.IsNil)
-	c.Assert(m.Life(), gc.Not(gc.Equals), state.Alive)
-}
-
-func (s *clientSuite) TestClientDestroyUnits(c *gc.C) {
-	// Setup:
-	s.setUpScenario(c)
-	service, err := s.State.Service("wordpress")
-	c.Assert(err, gc.IsNil)
-	_, err = service.AddUnit()
-	c.Assert(err, gc.IsNil)
-	// Destroy some units and check the result.
-	err = s.APIState.Client().DestroyServiceUnits([]string{"wordpress/1", "wordpress/2"})
-	c.Assert(err, gc.IsNil)
-	units, err := service.AllUnits()
-	c.Assert(err, gc.IsNil)
-	for i, u := range units {
-		if i == 0 {
-			c.Assert(u.Life(), gc.Equals, state.Alive)
-		} else {
-			c.Assert(u.Life(), gc.Equals, state.Dying)
-		}
+func assertLife(c *gc.C, entity state.Living, life state.Life) {
+	err := entity.Refresh()
+	c.Assert(err, gc.IsNil)
+	c.Assert(entity.Life(), gc.Equals, life)
+}
+
+func assertRemoved(c *gc.C, entity state.Living) {
+	err := entity.Refresh()
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+}
+
+func (s *clientSuite) setupDestroyMachinesTest(c *gc.C) (*state.Machine, *state.Machine, *state.Machine, *state.Unit) {
+	m0, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+	m1, err := s.State.AddMachine("quantal", state.JobManageEnviron)
+	c.Assert(err, gc.IsNil)
+	m2, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+
+	sch := s.AddTestingCharm(c, "wordpress")
+	wordpress, err := s.State.AddService("wordpress", sch)
+	c.Assert(err, gc.IsNil)
+	u, err := wordpress.AddUnit()
+	c.Assert(err, gc.IsNil)
+	err = u.AssignToMachine(m0)
+	c.Assert(err, gc.IsNil)
+
+	return m0, m1, m2, u
+}
+
+func (s *clientSuite) TestDestroyMachines(c *gc.C) {
+	m0, m1, m2, u := s.setupDestroyMachinesTest(c)
+
+	err := s.APIState.Client().DestroyMachines("0", "1", "2")
+	c.Assert(err, gc.ErrorMatches, `some machines were not destroyed: machine 0 has unit "wordpress/0" assigned; machine 1 is required by the environment`)
+	assertLife(c, m0, state.Alive)
+	assertLife(c, m1, state.Alive)
+	assertLife(c, m2, state.Dying)
+
+	err = u.UnassignFromMachine()
+	c.Assert(err, gc.IsNil)
+	err = s.APIState.Client().DestroyMachines("0", "1", "2")
+	c.Assert(err, gc.ErrorMatches, `some machines were not destroyed: machine 1 is required by the environment`)
+	assertLife(c, m0, state.Dying)
+	assertLife(c, m1, state.Alive)
+	assertLife(c, m2, state.Dying)
+}
+
+func (s *clientSuite) TestForceDestroyMachines(c *gc.C) {
+	m0, m1, m2, u := s.setupDestroyMachinesTest(c)
+
+	err := s.APIState.Client().ForceDestroyMachines("0", "1", "2")
+	c.Assert(err, gc.ErrorMatches, `some machines were not destroyed: machine 1 is required by the environment`)
+	assertLife(c, m0, state.Alive)
+	assertLife(c, m1, state.Alive)
+	assertLife(c, m2, state.Alive)
+	assertLife(c, u, state.Alive)
+
+	err = s.State.Cleanup()
+	c.Assert(err, gc.IsNil)
+	assertRemoved(c, m0)
+	assertLife(c, m1, state.Alive)
+	assertRemoved(c, m2)
+	assertRemoved(c, u)
+}
+
+func (s *clientSuite) TestDestroyPrincipalUnits(c *gc.C) {
+	wordpress, err := s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))
+	c.Assert(err, gc.IsNil)
+	units := make([]*state.Unit, 5)
+	for i := range units {
+		unit, err := wordpress.AddUnit()
+		c.Assert(err, gc.IsNil)
+		err = unit.SetStatus(params.StatusStarted, "", nil)
+		c.Assert(err, gc.IsNil)
+		units[i] = unit
 	}
+
+	// Destroy 2 of them; check they become Dying.
+	err = s.APIState.Client().DestroyServiceUnits("wordpress/0", "wordpress/1")
+	c.Assert(err, gc.IsNil)
+	assertLife(c, units[0], state.Dying)
+	assertLife(c, units[1], state.Dying)
+
+	// Try to destroy an Alive one and a Dying one; check
+	// it destroys the Alive one and ignores the Dying one.
+	err = s.APIState.Client().DestroyServiceUnits("wordpress/2", "wordpress/0")
+	c.Assert(err, gc.IsNil)
+	assertLife(c, units[2], state.Dying)
+
+	// Try to destroy an Alive one along with a nonexistent one; check that
+	// the valid instruction is followed but the invalid one is warned about.
+	err = s.APIState.Client().DestroyServiceUnits("boojum/123", "wordpress/3")
+	c.Assert(err, gc.ErrorMatches, `some units were not destroyed: unit "boojum/123" does not exist`)
+	assertLife(c, units[3], state.Dying)
+
+	// Make one Dead, and destroy an Alive one alongside it; check no errors.
+	wp0, err := s.State.Unit("wordpress/0")
+	c.Assert(err, gc.IsNil)
+	err = wp0.EnsureDead()
+	c.Assert(err, gc.IsNil)
+	err = s.APIState.Client().DestroyServiceUnits("wordpress/0", "wordpress/4")
+	c.Assert(err, gc.IsNil)
+	assertLife(c, units[0], state.Dead)
+	assertLife(c, units[4], state.Dying)
+}
+
+func (s *clientSuite) TestDestroySubordinateUnits(c *gc.C) {
+	wordpress, err := s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))
+	c.Assert(err, gc.IsNil)
+	wordpress0, err := wordpress.AddUnit()
+	c.Assert(err, gc.IsNil)
+	_, err = s.State.AddService("logging", s.AddTestingCharm(c, "logging"))
+	c.Assert(err, gc.IsNil)
+	eps, err := s.State.InferEndpoints([]string{"logging", "wordpress"})
+	c.Assert(err, gc.IsNil)
+	rel, err := s.State.AddRelation(eps...)
+	c.Assert(err, gc.IsNil)
+	ru, err := rel.Unit(wordpress0)
+	c.Assert(err, gc.IsNil)
+	err = ru.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+	logging0, err := s.State.Unit("logging/0")
+	c.Assert(err, gc.IsNil)
+
+	// Try to destroy the subordinate alone; check it fails.
+	err = s.APIState.Client().DestroyServiceUnits("logging/0")
+	c.Assert(err, gc.ErrorMatches, `no units were destroyed: unit "logging/0" is a subordinate`)
+	assertLife(c, logging0, state.Alive)
+
+	// Try to destroy the principal and the subordinate together; check it warns
+	// about the subordinate, but destroys the one it can. (The principal unit
+	// agent will be resposible for destroying the subordinate.)
+	err = s.APIState.Client().DestroyServiceUnits("wordpress/0", "logging/0")
+	c.Assert(err, gc.ErrorMatches, `some units were not destroyed: unit "logging/0" is a subordinate`)
+	assertLife(c, wordpress0, state.Dying)
+	assertLife(c, logging0, state.Alive)
 }
 
 func (s *clientSuite) testClientUnitResolved(c *gc.C, retry bool, expectedResolvedMode state.ResolvedMode) {
 
=== modified file 'state/apiserver/client/perm_test.go'
--- state/apiserver/client/perm_test.go	2013-11-06 10:51:34 +0000
+++ state/apiserver/client/perm_test.go	2013-11-12 10:22:12 +0000
@@ -348,7 +348,7 @@
 }
 
 func opClientDestroyServiceUnits(c *gc.C, st *api.State, mst *state.State) (func(), error) {
-	err := st.Client().DestroyServiceUnits([]string{"wordpress/99"})
+	err := st.Client().DestroyServiceUnits("wordpress/99")
 	if err != nil && strings.HasPrefix(err.Error(), "no units were destroyed") {
 		err = nil
 	}
 
=== modified file 'state/cleanup_test.go'
--- state/cleanup_test.go	2013-11-11 15:45:06 +0000
+++ state/cleanup_test.go	2013-11-12 10:22:12 +0000
@@ -1,6 +1,8 @@
 package state_test
 
 import (
+	"fmt"
+
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/charm"
@@ -90,6 +92,22 @@
 	c.Assert(err, gc.ErrorMatches, `cannot read settings for unit "riak/0" in relation "riak:ring": settings not found`)
 }
 
+func (s *CleanupSuite) testForceDestroyManagerError(c *gc.C, job state.MachineJob) {
+	manager, err := s.State.AddMachine("quantal", job)
+	c.Assert(err, gc.IsNil)
+	s.assertDoesNotNeedCleanup(c)
+	err = manager.ForceDestroy()
+	expect := fmt.Sprintf("machine %s is required by the environment", manager.Id())
+	c.Assert(err, gc.ErrorMatches, expect)
+	s.assertDoesNotNeedCleanup(c)
+	assertLife(c, manager, state.Alive)
+}
+
+func (s *CleanupSuite) TestForceDestroyMachineErrors(c *gc.C) {
+	s.testForceDestroyManagerError(c, state.JobManageState)
+	s.testForceDestroyManagerError(c, state.JobManageEnviron)
+}
+
 func (s *CleanupSuite) TestCleanupForceDestroyedMachineUnit(c *gc.C) {
 	s.assertDoesNotNeedCleanup(c)
 
@@ -106,7 +124,7 @@
 	s.assertDoesNotNeedCleanup(c)
 
 	// Force machine destruction, check cleanup queued.
-	err = s.State.ForceDestroyMachines(machine.Id())
+	err = machine.ForceDestroy()
 	c.Assert(err, gc.IsNil)
 	s.assertNeedsCleanup(c)
 
@@ -117,7 +135,7 @@
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 
 	// ...and so has the unit...
-	assertUnitRemoved(c, pr.u0)
+	assertRemoved(c, pr.u0)
 
 	// ...and the unit has departed relation scope.
 	assertNotInScope(c, pr.ru0)
@@ -156,13 +174,13 @@
 	s.assertDoesNotNeedCleanup(c)
 
 	// Force removal of the top-level machine.
-	err = s.State.ForceDestroyMachines(machine.Id())
+	err = machine.ForceDestroy()
 	c.Assert(err, gc.IsNil)
 	s.assertNeedsCleanup(c)
 
 	// And do it again, just to check that the second cleanup doc for the same
 	// machine doesn't cause problems down the line.
-	err = s.State.ForceDestroyMachines(machine.Id())
+	err = machine.ForceDestroy()
 	c.Assert(err, gc.IsNil)
 	s.assertNeedsCleanup(c)
 
@@ -175,10 +193,10 @@
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 
 	// ...and so have all the units...
-	assertUnitRemoved(c, prr.pu0)
-	assertUnitRemoved(c, prr.pu1)
-	assertUnitRemoved(c, prr.ru0)
-	assertUnitRemoved(c, prr.ru1)
+	assertRemoved(c, prr.pu0)
+	assertRemoved(c, prr.pu1)
+	assertRemoved(c, prr.ru0)
+	assertRemoved(c, prr.ru1)
 
 	// ...and none of the units have left relation scopes occupied.
 	assertNotInScope(c, prr.pru0)
 
=== modified file 'state/life.go'
--- state/life.go	2013-07-09 10:32:23 +0000
+++ state/life.go	2013-11-12 10:22:12 +0000
@@ -38,8 +38,15 @@
 type Living interface {
 	Life() Life
 	Destroy() error
+	Refresh() error
+}
+
+// AgentLiving describes state entities with a lifecycle and an agent that
+// manages it.
+type AgentLiving interface {
+	Living
 	EnsureDead() error
-	Refresh() error
+	Remove() error
 }
 
 func isAlive(coll *mgo.Collection, id interface{}) (bool, error) {
 
=== modified file 'state/life_test.go'
--- state/life_test.go	2013-09-27 07:05:45 +0000
+++ state/life_test.go	2013-11-12 10:22:12 +0000
@@ -81,8 +81,7 @@
 
 type lifeFixture interface {
 	id() (coll string, id interface{})
-	setup(s *LifeSuite, c *gc.C) state.Living
-	teardown(s *LifeSuite, c *gc.C)
+	setup(s *LifeSuite, c *gc.C) state.AgentLiving
 }
 
 type unitLife struct {
@@ -93,7 +92,7 @@
 	return "units", l.unit.Name()
 }
 
-func (l *unitLife) setup(s *LifeSuite, c *gc.C) state.Living {
+func (l *unitLife) setup(s *LifeSuite, c *gc.C) state.AgentLiving {
 	unit, err := s.svc.AddUnit()
 	c.Assert(err, gc.IsNil)
 	preventUnitDestroyRemove(c, unit)
@@ -101,11 +100,6 @@
 	return l.unit
 }
 
-func (l *unitLife) teardown(s *LifeSuite, c *gc.C) {
-	err := l.unit.Remove()
-	c.Assert(err, gc.IsNil)
-}
-
 type machineLife struct {
 	machine *state.Machine
 }
@@ -114,18 +108,13 @@
 	return "machines", l.machine.Id()
 }
 
-func (l *machineLife) setup(s *LifeSuite, c *gc.C) state.Living {
+func (l *machineLife) setup(s *LifeSuite, c *gc.C) state.AgentLiving {
 	var err error
 	l.machine, err = s.State.AddMachine("quantal", state.JobHostUnits)
 	c.Assert(err, gc.IsNil)
 	return l.machine
 }
 
-func (l *machineLife) teardown(s *LifeSuite, c *gc.C) {
-	err := l.machine.Remove()
-	c.Assert(err, gc.IsNil)
-}
-
 func (s *LifeSuite) prepareFixture(living state.Living, lfix lifeFixture, cached, dbinitial state.Life, c *gc.C) {
 	collName, id := lfix.id()
 	coll := s.MgoSuite.Session.DB("juju").C(collName)
@@ -165,7 +154,8 @@
 			c.Assert(living.Life(), gc.Equals, v.dbfinal)
 			err = living.EnsureDead()
 			c.Assert(err, gc.IsNil)
-			lfix.teardown(s, c)
+			err = living.Remove()
+			c.Assert(err, gc.IsNil)
 		}
 	}
 }
 
=== modified file 'state/machine.go'
--- state/machine.go	2013-11-11 23:42:49 +0000
+++ state/machine.go	2013-11-12 10:22:12 +0000
@@ -195,10 +195,11 @@
 	return m.doc.Jobs
 }
 
-// IsStateServer returns true if the machine has a JobManageState job.
-func (m *Machine) IsStateServer() bool {
+// IsManager returns true if the machine has JobManageState or JobManageEnviron.
+func (m *Machine) IsManager() bool {
 	for _, job := range m.doc.Jobs {
-		if job == JobManageState {
+		switch job {
+		case JobManageEnviron, JobManageState:
 			return true
 		}
 	}
@@ -314,6 +315,22 @@
 	return m.advanceLifecycle(Dying)
 }
 
+// ForceDestroy queues the machine for complete removal, including the
+// destruction of all units and containers on the machine.
+func (m *Machine) ForceDestroy() error {
+	if !m.IsManager() {
+		ops := []txn.Op{{
+			C:      m.st.machines.Name,
+			Id:     m.doc.Id,
+			Assert: D{{"jobs", D{{"$nin", []MachineJob{JobManageState}}}}},
+		}, m.st.newCleanupOp("machine", m.doc.Id)}
+		if err := m.st.runTransaction(ops); err != txn.ErrAborted {
+			return err
+		}
+	}
+	return fmt.Errorf("machine %s is required by the environment", m.doc.Id)
+}
+
 // EnsureDead sets the machine lifecycle to Dead if it is Alive or Dying.
 // It does nothing otherwise. EnsureDead will fail if the machine has
 // principal units assigned, or if the machine has JobManageEnviron.
 
=== modified file 'state/machine_test.go'
--- state/machine_test.go	2013-11-12 01:59:52 +0000
+++ state/machine_test.go	2013-11-12 10:22:12 +0000
@@ -67,15 +67,17 @@
 	c.Assert(ok, gc.Equals, true)
 }
 
-func (s *MachineSuite) TestMachineIsStateServer(c *gc.C) {
+func (s *MachineSuite) TestMachineIsManager(c *gc.C) {
 	tests := []struct {
 		isStateServer bool
 		jobs          []state.MachineJob
 	}{
 		{false, []state.MachineJob{state.JobHostUnits}},
-		{false, []state.MachineJob{state.JobHostUnits, state.JobManageEnviron}},
+		{true, []state.MachineJob{state.JobManageState}},
+		{true, []state.MachineJob{state.JobManageEnviron}},
+		{true, []state.MachineJob{state.JobHostUnits, state.JobManageState}},
+		{true, []state.MachineJob{state.JobHostUnits, state.JobManageEnviron}},
 		{true, []state.MachineJob{state.JobHostUnits, state.JobManageState, state.JobManageEnviron}},
-		{true, []state.MachineJob{state.JobManageState}},
 	}
 	for _, test := range tests {
 		params := state.AddMachineParams{
@@ -84,7 +86,7 @@
 		}
 		m, err := s.State.AddMachineWithConstraints(&params)
 		c.Assert(err, gc.IsNil)
-		c.Assert(m.IsStateServer(), gc.Equals, test.isStateServer)
+		c.Assert(m.IsManager(), gc.Equals, test.isStateServer)
 	}
 }
 
@@ -94,6 +96,8 @@
 	c.Assert(err, gc.IsNil)
 	err = m.Destroy()
 	c.Assert(err, gc.ErrorMatches, "machine 1 is required by the environment")
+	err = m.ForceDestroy()
+	c.Assert(err, gc.ErrorMatches, "machine 1 is required by the environment")
 	err = m.EnsureDead()
 	c.Assert(err, gc.ErrorMatches, "machine 1 is required by the environment")
 }
@@ -218,41 +222,6 @@
 	c.Assert(err, gc.IsNil)
 }
 
-func (s *MachineSuite) TestDestroyMachines(c *gc.C) {
-	m0 := s.machine
-	m1, err := s.State.AddMachine("quantal", state.JobManageEnviron)
-	c.Assert(err, gc.IsNil)
-	m2, err := s.State.AddMachine("quantal", state.JobHostUnits)
-	c.Assert(err, gc.IsNil)
-
-	sch := s.AddTestingCharm(c, "wordpress")
-	wordpress, err := s.State.AddService("wordpress", sch)
-	c.Assert(err, gc.IsNil)
-	u, err := wordpress.AddUnit()
-	c.Assert(err, gc.IsNil)
-	err = u.AssignToMachine(m0)
-	c.Assert(err, gc.IsNil)
-
-	err = s.State.DestroyMachines("0", "1", "2")
-	c.Assert(err, gc.ErrorMatches, `some machines were not destroyed: machine 0 has unit "wordpress/0" assigned; machine 1 is required by the environment`)
-	assertLife := func(m *state.Machine, life state.Life) {
-		err := m.Refresh()
-		c.Assert(err, gc.IsNil)
-		c.Assert(m.Life(), gc.Equals, life)
-	}
-	assertLife(m0, state.Alive)
-	assertLife(m1, state.Alive)
-	assertLife(m2, state.Dying)
-
-	err = u.UnassignFromMachine()
-	c.Assert(err, gc.IsNil)
-	err = s.State.DestroyMachines("0", "1", "2")
-	c.Assert(err, gc.ErrorMatches, `some machines were not destroyed: machine 1 is required by the environment`)
-	assertLife(m0, state.Dying)
-	assertLife(m1, state.Alive)
-	assertLife(m2, state.Dying)
-}
-
 func (s *MachineSuite) TestMachineSetAgentAlive(c *gc.C) {
 	alive, err := s.machine.AgentAlive()
 	c.Assert(err, gc.IsNil)
@@ -627,11 +596,11 @@
 func (s *MachineSuite) TestWatchDiesOnStateClose(c *gc.C) {
 	// This test is testing logic in watcher.entityWatcher, which
 	// is also used by:
-	//	Machine.WatchHardwareCharacteristics
-	//	Service.Watch
-	//	Unit.Watch
-	//	State.WatchForEnvironConfigChanges
-	//	Unit.WatchConfigSettings
+	//  Machine.WatchHardwareCharacteristics
+	//  Service.Watch
+	//  Unit.Watch
+	//  State.WatchForEnvironConfigChanges
+	//  Unit.WatchConfigSettings
 	testWatcherDiesWhenStateCloses(c, func(c *gc.C, st *state.State) waiter {
 		m, err := st.Machine(s.machine.Id())
 		c.Assert(err, gc.IsNil)
 
=== modified file 'state/service_test.go'
--- state/service_test.go	2013-09-27 19:20:20 +0000
+++ state/service_test.go	2013-11-12 10:22:12 +0000
@@ -1090,7 +1090,7 @@
 	err = s.mysql.Destroy()
 	c.Assert(err, gc.IsNil)
 	for _, unit := range units {
-		assertUnitLife(c, unit, state.Alive)
+		assertLife(c, unit, state.Alive)
 	}
 
 	// Check a cleanup doc was added.
@@ -1103,9 +1103,9 @@
 	c.Assert(err, gc.IsNil)
 	for i, unit := range units {
 		if i%2 != 0 {
-			assertUnitLife(c, unit, state.Dying)
+			assertLife(c, unit, state.Dying)
 		} else {
-			assertUnitRemoved(c, unit)
+			assertRemoved(c, unit)
 		}
 	}
 
 
=== modified file 'state/state.go'
--- state/state.go	2013-11-08 16:15:10 +0000
+++ state/state.go	2013-11-12 10:22:12 +0000
@@ -1051,73 +1051,6 @@
 	return newUnit(st, &doc), nil
 }
 
-// DestroyUnits destroys the units with the specified names.
-func (st *State) DestroyUnits(names ...string) (err error) {
-	// TODO(rog) make this a transaction?
-	var errs []string
-	for _, name := range names {
-		unit, err := st.Unit(name)
-		switch {
-		case errors.IsNotFoundError(err):
-			err = fmt.Errorf("unit %q does not exist", name)
-		case err != nil:
-		case unit.Life() != Alive:
-			continue
-		case unit.IsPrincipal():
-			err = unit.Destroy()
-		default:
-			err = fmt.Errorf("unit %q is a subordinate", name)
-		}
-		if err != nil {
-			errs = append(errs, err.Error())
-		}
-	}
-	return destroyErr("units", names, errs)
-}
-
-// DestroyMachines destroys the machines with the specified ids.
-func (st *State) DestroyMachines(ids ...string) (err error) {
-	var errs []string
-	for _, id := range ids {
-		machine, err := st.Machine(id)
-		switch {
-		case errors.IsNotFoundError(err):
-			err = fmt.Errorf("machine %s does not exist", id)
-		case err != nil:
-		case machine.Life() != Alive:
-			continue
-		default:
-			err = machine.Destroy()
-		}
-		if err != nil {
-			errs = append(errs, err.Error())
-		}
-	}
-	return destroyErr("machines", ids, errs)
-}
-
-func destroyErr(desc string, ids, errs []string) error {
-	if len(errs) == 0 {
-		return nil
-	}
-	msg := "some %s were not destroyed"
-	if len(errs) == len(ids) {
-		msg = "no %s were destroyed"
-	}
-	msg = fmt.Sprintf(msg, desc)
-	return fmt.Errorf("%s: %s", msg, strings.Join(errs, "; "))
-}
-
-// ForceDestroyMachines queues the machines with the specified ids for
-// complete removal, including the destruction of all units on the machine.
-func (st *State) ForceDestroyMachines(ids ...string) error {
-	var ops []txn.Op
-	for _, id := range ids {
-		ops = append(ops, st.newCleanupOp("machine", id))
-	}
-	return st.runTransaction(ops)
-}
-
 // AssignUnit places the unit on a machine. Depending on the policy, and the
 // state of the environment, this may lead to new instances being launched
 // within the environment.
 
=== modified file 'state/unit_test.go'
--- state/unit_test.go	2013-11-07 17:30:38 +0000
+++ state/unit_test.go	2013-11-12 10:22:12 +0000
@@ -435,49 +435,12 @@
 	c.Assert(err, gc.ErrorMatches, `unit "wordpress/0" is dead`)
 }
 
-func (s *UnitSuite) TestDestroyPrincipalUnits(c *gc.C) {
-	preventUnitDestroyRemove(c, s.unit)
-	for i := 0; i < 4; i++ {
-		unit, err := s.service.AddUnit()
-		c.Assert(err, gc.IsNil)
-		preventUnitDestroyRemove(c, unit)
-	}
-
-	// Destroy 2 of them; check they become Dying.
-	err := s.State.DestroyUnits("wordpress/0", "wordpress/1")
-	c.Assert(err, gc.IsNil)
-	s.assertUnitLife(c, "wordpress/0", state.Dying)
-	s.assertUnitLife(c, "wordpress/1", state.Dying)
-
-	// Try to destroy an Alive one and a Dying one; check
-	// it destroys the Alive one and ignores the Dying one.
-	err = s.State.DestroyUnits("wordpress/2", "wordpress/0")
-	c.Assert(err, gc.IsNil)
-	s.assertUnitLife(c, "wordpress/2", state.Dying)
-
-	// Try to destroy an Alive one along with a nonexistent one; check that
-	// the valid instruction is followed but the invalid one is warned about.
-	err = s.State.DestroyUnits("boojum/123", "wordpress/3")
-	c.Assert(err, gc.ErrorMatches, `some units were not destroyed: unit "boojum/123" does not exist`)
-	s.assertUnitLife(c, "wordpress/3", state.Dying)
-
-	// Make one Dead, and destroy an Alive one alongside it; check no errors.
-	wp0, err := s.State.Unit("wordpress/0")
-	c.Assert(err, gc.IsNil)
-	err = wp0.EnsureDead()
-	c.Assert(err, gc.IsNil)
-	err = s.State.DestroyUnits("wordpress/0", "wordpress/4")
-	c.Assert(err, gc.IsNil)
-	s.assertUnitLife(c, "wordpress/0", state.Dead)
-	s.assertUnitLife(c, "wordpress/4", state.Dying)
-}
-
 func (s *UnitSuite) TestDestroySetStatusRetry(c *gc.C) {
 	defer state.SetRetryHooks(c, s.State, func() {
 		err := s.unit.SetStatus(params.StatusStarted, "", nil)
 		c.Assert(err, gc.IsNil)
 	}, func() {
-		assertUnitLife(c, s.unit, state.Dying)
+		assertLife(c, s.unit, state.Dying)
 	}).Check()
 	err := s.unit.Destroy()
 	c.Assert(err, gc.IsNil)
@@ -488,7 +451,7 @@
 		err := s.unit.SetCharmURL(s.charm.URL())
 		c.Assert(err, gc.IsNil)
 	}, func() {
-		assertUnitRemoved(c, s.unit)
+		assertRemoved(c, s.unit)
 	}).Check()
 	err := s.unit.Destroy()
 	c.Assert(err, gc.IsNil)
@@ -505,7 +468,7 @@
 		err := s.unit.SetCharmURL(newCharm.URL())
 		c.Assert(err, gc.IsNil)
 	}, func() {
-		assertUnitRemoved(c, s.unit)
+		assertRemoved(c, s.unit)
 	}).Check()
 	err = s.unit.Destroy()
 	c.Assert(err, gc.IsNil)
@@ -519,7 +482,7 @@
 		err := s.unit.AssignToMachine(machine)
 		c.Assert(err, gc.IsNil)
 	}, func() {
-		assertUnitRemoved(c, s.unit)
+		assertRemoved(c, s.unit)
 		// Also check the unit ref was properly removed from the machine doc --
 		// if it weren't, we wouldn't be able to make the machine Dead.
 		err := machine.EnsureDead()
@@ -539,7 +502,7 @@
 		err := s.unit.UnassignFromMachine()
 		c.Assert(err, gc.IsNil)
 	}, func() {
-		assertUnitRemoved(c, s.unit)
+		assertRemoved(c, s.unit)
 	}).Check()
 	err = s.unit.Destroy()
 	c.Assert(err, gc.IsNil)
@@ -550,7 +513,7 @@
 	err := s.unit.Destroy()
 	c.Assert(err, gc.IsNil)
 	c.Assert(s.unit.Life(), gc.Equals, state.Dying)
-	assertUnitRemoved(c, s.unit)
+	assertRemoved(c, s.unit)
 }
 
 func (s *UnitSuite) TestCannotShortCircuitDestroyWithSubordinates(c *gc.C) {
@@ -568,7 +531,7 @@
 	err = s.unit.Destroy()
 	c.Assert(err, gc.IsNil)
 	c.Assert(s.unit.Life(), gc.Equals, state.Dying)
-	assertUnitLife(c, s.unit, state.Dying)
+	assertLife(c, s.unit, state.Dying)
 }
 
 func (s *UnitSuite) TestCannotShortCircuitDestroyWithStatus(c *gc.C) {
@@ -592,7 +555,7 @@
 		err = unit.Destroy()
 		c.Assert(err, gc.IsNil)
 		c.Assert(unit.Life(), gc.Equals, state.Dying)
-		assertUnitLife(c, unit, state.Dying)
+		assertLife(c, unit, state.Dying)
 	}
 }
 
@@ -610,56 +573,25 @@
 	err = s.unit.Destroy()
 	c.Assert(err, gc.IsNil)
 	c.Assert(s.unit.Life(), gc.Equals, state.Dying)
-	assertUnitRemoved(c, s.unit)
-}
-
-func (s *UnitSuite) TestDestroySubordinateUnits(c *gc.C) {
-	lgsch := s.AddTestingCharm(c, "logging")
-	_, err := s.State.AddService("logging", lgsch)
-	c.Assert(err, gc.IsNil)
-	eps, err := s.State.InferEndpoints([]string{"logging", "wordpress"})
-	c.Assert(err, gc.IsNil)
-	rel, err := s.State.AddRelation(eps...)
-	c.Assert(err, gc.IsNil)
-	ru, err := rel.Unit(s.unit)
-	c.Assert(err, gc.IsNil)
-	err = ru.EnterScope(nil)
-	c.Assert(err, gc.IsNil)
-
-	// Try to destroy the subordinate alone; check it fails.
-	err = s.State.DestroyUnits("logging/0")
-	c.Assert(err, gc.ErrorMatches, `no units were destroyed: unit "logging/0" is a subordinate`)
-	s.assertUnitLife(c, "logging/0", state.Alive)
-
-	// Try to destroy the principal and the subordinate together; check it warns
-	// about the subordinate, but destroys the one it can. (The principal unit
-	// agent will be resposible for destroying the subordinate.)
-	err = s.State.DestroyUnits("wordpress/0", "logging/0")
-	c.Assert(err, gc.ErrorMatches, `some units were not destroyed: unit "logging/0" is a subordinate`)
-	s.assertUnitLife(c, "wordpress/0", state.Dying)
-	s.assertUnitLife(c, "logging/0", state.Alive)
-}
-
-func (s *UnitSuite) assertUnitLife(c *gc.C, name string, life state.Life) {
-	unit, err := s.State.Unit(name)
-	c.Assert(err, gc.IsNil)
-	assertUnitLife(c, unit, life)
-}
-
-func assertUnitLife(c *gc.C, unit *state.Unit, life state.Life) {
-	c.Assert(unit.Refresh(), gc.IsNil)
-	c.Assert(unit.Life(), gc.Equals, life)
-}
-
-func assertUnitRemoved(c *gc.C, unit *state.Unit) {
-	err := unit.Refresh()
+	assertRemoved(c, s.unit)
+}
+
+func assertLife(c *gc.C, entity state.Living, life state.Life) {
+	c.Assert(entity.Refresh(), gc.IsNil)
+	c.Assert(entity.Life(), gc.Equals, life)
+}
+
+func assertRemoved(c *gc.C, entity state.Living) {
+	err := entity.Refresh()
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
-	err = unit.Destroy()
-	c.Assert(err, gc.IsNil)
-	err = unit.EnsureDead()
-	c.Assert(err, gc.IsNil)
-	err = unit.Remove()
-	c.Assert(err, gc.IsNil)
+	err = entity.Destroy()
+	c.Assert(err, gc.IsNil)
+	if entity, ok := entity.(state.AgentLiving); ok {
+		err = entity.EnsureDead()
+		c.Assert(err, gc.IsNil)
+		err = entity.Remove()
+		c.Assert(err, gc.IsNil)
+	}
 }
 
 func (s *UnitSuite) TestTag(c *gc.C) {