Merge lp:~axwalk/juju-core/lp1260171-stopinstances-ids into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Change StopInstances to take []instance.Id

StopInstances previously took []instance.Instance,
which is unnecessary for most providers, and imposes
an unnecessary cost for callers that only have IDs
initially. We change StopInstances to

The openstack provider's StopInstances is now slightly
more expensive than before, requiring an additional call
to Instances. This cost will disappear when we fix the
security groups/open-port implementation.

Drive-by: the maas provider now uses a bulk "release"
in StopInstances.

Live tested on ec2, local, virtual MAAS, canonistack, azure.
I lack a Joyent account, but I think it should be fine.

Fixes lp:1260171
Fixes lp:1316272

https://codereview.appspot.com/97400043/

Change StopInstances to take []instance.Id

StopInstances previously took []instance.Instance,
which is unnecessary for most providers, and imposes
an unnecessary cost for callers that only have IDs
initially. We change StopInstances to

The openstack provider's StopInstances is now slightly
more expensive than before, requiring an additional call
to Instances. This cost will disappear when we fix the
security groups/open-port implementation.

Drive-by: the maas provider now uses a bulk "release"
in StopInstances.

Live tested on ec2, local, virtual MAAS, canonistack, azure.
I lack a Joyent account, but I think it should be fine.

Fixes lp:1260171
Fixes lp:1316272

https://codereview.appspot.com/97400043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+219347_code.launchpad.net,

Message:
Please take a look.

Description:
Change StopInstances to take []instance.Id

StopInstances previously took []instance.Instance,
which is unnecessary for most providers, and imposes
an unnecessary cost for callers that only have IDs
initially. We change StopInstances to

The openstack provider's StopInstances is now slightly
more expensive than before, requiring an additional call
to Instances. This cost will disappear when we fix the
security groups/open-port implementation.

Drive-by: the maas provider now uses a bulk "release"
in StopInstances.

Live tested on ec2, local, virtual MAAS, canonistack, azure.
I lack a Joyent account, but I think it should be fine.

Fixes lp:1260171
Fixes lp:1316272

https://code.launchpad.net/~axwalk/juju-core/lp1260171-stopinstances-ids/+merge/219347

(do not edit description out of merge proposal)

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

Affected files (+234, -257 lines):
   A [revision details]
   M container/interface.go
   M container/kvm/kvm.go
   M container/kvm/kvm_test.go
   M container/kvm/live_test.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go
   M dependencies.tsv
   M environs/broker.go
   M environs/jujutest/livetests.go
   M environs/jujutest/tests.go
   M provider/azure/environ.go
   M provider/azure/environ_test.go
   M provider/common/bootstrap.go
   M provider/common/bootstrap_test.go
   M provider/common/destroy.go
   M provider/common/destroy_test.go
   M provider/common/mock_test.go
   M provider/dummy/environs.go
   M provider/ec2/ec2.go
   M provider/ec2/live_test.go
   M provider/joyent/environ_instance.go
   M provider/joyent/instance.go
   M provider/joyent/local_test.go
   M provider/local/environ.go
   M provider/maas/environ.go
   M provider/maas/environ_whitebox_test.go
   M provider/maas/util.go
   M provider/maas/util_test.go
   M provider/manual/environ.go
   M provider/openstack/local_test.go
   M provider/openstack/provider.go
   M state/apiserver/client/destroy.go
   M worker/provisioner/kvm-broker.go
   M worker/provisioner/kvm-broker_test.go
   M worker/provisioner/lxc-broker.go
   M worker/provisioner/lxc-broker_test.go
   M worker/provisioner/provisioner_task.go
   M worker/provisioner/provisioner_test.go

[Dave Cheney (dave-cheney)]

On 2014/05/13 11:58:10, axw wrote:
> Please take a look.

SGTM. How do you feel about making StopInstance variadic ?

https://codereview.appspot.com/97400043/

[Ian Booth (wallyworld)]

LGTM, agree with Dave's comment so would be good to do that before
landing

https://codereview.appspot.com/97400043/diff/1/container/interface.go
File container/interface.go (right):

https://codereview.appspot.com/97400043/diff/1/container/interface.go#newcode31
container/interface.go:31: // Instance.
comment tweak - "by instance id"

https://codereview.appspot.com/97400043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/97400043/diff/1/container/interface.go
File container/interface.go (right):

https://codereview.appspot.com/97400043/diff/1/container/interface.go#newcode31
container/interface.go:31: // Instance.
On 2014/05/14 02:37:16, wallyworld wrote:
> comment tweak - "by instance id"

Done.

https://codereview.appspot.com/97400043/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/lp1260171-stopinstances-ids into lp:juju-core failed. Below is the output from the failed tests.

godeps: cannot update "/home/tarmac/trees/src/launchpad.net/gomaasapi": bzr: ERROR: branch has no revision <email address hidden>
bzr update --revision only works for a revision in the branch history
mongod: no process found

[Dave Cheney (dave-cheney)]

On Wed, May 14, 2014 at 12:37 PM, <email address hidden> wrote:
> LGTM, agree with Dave's comment so would be good to do that before
> landing

Making StopInstance variadic would make some callers initially more verbose, ie

var i []instance.Instance
p.StopInstances(i...) // note variadic expansion to pass the contents
of the slice, not the slice itself.

but, as I found with the change that I made to Addresses recently,
most of the callers that are creating those slices are doing so to
pass one item, so they can be refactored to be variadic themselves and
everything becomes a lot simpler to read without so much tedious
boxing of single values into slices.

=== modified file 'container/interface.go'
--- container/interface.go	2014-03-19 21:08:58 +0000
+++ container/interface.go	2014-05-14 03:01:15 +0000
@@ -20,13 +20,17 @@
 // Manager is responsible for starting containers, and stopping and listing
 // containers that it has started.
 type Manager interface {
-	// CreateContainer creates and starts a new container for the specified machine.
+	// CreateContainer creates and starts a new container for the specified
+	// machine.
 	CreateContainer(
 		machineConfig *cloudinit.MachineConfig,
 		series string,
 		network *NetworkConfig) (instance.Instance, *instance.HardwareCharacteristics, error)
-	// DestroyContainer stops and destroyes the container identified by Instance.
-	DestroyContainer(instance.Instance) error
+
+	// DestroyContainer stops and destroyes the container identified by
+	// instance id.
+	DestroyContainer(instance.Id) error
+
 	// ListContainers return a list of containers that have been started by
 	// this manager.
 	ListContainers() ([]instance.Instance, error)

=== modified file 'container/kvm/kvm.go'
--- container/kvm/kvm.go	2014-04-17 03:41:32 +0000
+++ container/kvm/kvm.go	2014-05-14 03:01:15 +0000
@@ -125,8 +125,8 @@
 	return &kvmInstance{kvmContainer, name}, &hardware, nil
 }
 
-func (manager *containerManager) DestroyContainer(instance instance.Instance) error {
-	name := string(instance.Id())
+func (manager *containerManager) DestroyContainer(id instance.Id) error {
+	name := string(id)
 	kvmContainer := KvmObjectFactory.New(name)
 	if err := kvmContainer.Stop(); err != nil {
 		logger.Errorf("failed to stop kvm container: %v", err)

=== modified file 'container/kvm/kvm_test.go'
--- container/kvm/kvm_test.go	2014-04-09 16:36:12 +0000
+++ container/kvm/kvm_test.go	2014-05-14 03:01:15 +0000
@@ -98,7 +98,7 @@
 func (s *KVMSuite) TestDestroyContainer(c *gc.C) {
 	instance := containertesting.CreateContainer(c, s.manager, "1/lxc/0")
 
-	err := s.manager.DestroyContainer(instance)
+	err := s.manager.DestroyContainer(instance.Id())
 	c.Assert(err, gc.IsNil)
 
 	name := string(instance.Id())

=== modified file 'container/kvm/live_test.go'
--- container/kvm/live_test.go	2014-04-01 16:27:22 +0000
+++ container/kvm/live_test.go	2014-05-14 03:01:15 +0000
@@ -75,7 +75,7 @@
 		instances, err := manager.ListContainers()
 		c.Assert(err, gc.IsNil)
 		for _, instance := range instances {
-			err := manager.DestroyContainer(instance)
+			err := manager.DestroyContainer(instance.Id())
 			c.Check(err, gc.IsNil)
 		}
 	}

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2014-03-19 21:08:58 +0000
+++ container/lxc/lxc.go	2014-05-14 03:01:15 +0000
@@ -269,9 +269,9 @@
 	return appendToContainerConfig(name, line)
 }
 
-func (manager *containerManager) DestroyContainer(instance instance.Instance) error {
+func (manager *containerManager) DestroyContainer(id instance.Id) error {
 	start := time.Now()
-	name := string(instance.Id())
+	name := string(id)
 	lxcContainer := LxcObjectFactory.New(name)
 	if useRestartDir() {
 		// Remove the autostart link.

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2014-03-19 21:08:58 +0000
+++ container/lxc/lxc_test.go	2014-05-14 03:01:15 +0000
@@ -274,7 +274,7 @@
 
 	// DestroyContainer stops and then destroys the container, putting it
 	// into "unknown" state.
-	err := manager.DestroyContainer(instance)
+	err := manager.DestroyContainer(instance.Id())
 	c.Assert(err, gc.IsNil)
 	c.Assert(instance.Status(), gc.Equals, string(golxc.StateUnknown))
 }
@@ -283,7 +283,7 @@
 	manager := s.makeManager(c, "test")
 	instance := containertesting.CreateContainer(c, manager, "1/lxc/0")
 
-	err := manager.DestroyContainer(instance)
+	err := manager.DestroyContainer(instance.Id())
 	c.Assert(err, gc.IsNil)
 
 	name := string(instance.Id())
@@ -302,7 +302,7 @@
 	err := os.MkdirAll(targetDir, 0755)
 	c.Assert(err, gc.IsNil)
 
-	err = manager.DestroyContainer(instance)
+	err = manager.DestroyContainer(instance.Id())
 	c.Assert(err, gc.IsNil)
 
 	// Check that the container dir is no longer in the container dir
@@ -368,7 +368,7 @@
 func (s *LxcSuite) TestDestroyContainerRemovesAutostartLink(c *gc.C) {
 	manager := s.makeManager(c, "test")
 	instance := containertesting.CreateContainer(c, manager, "1/lxc/0")
-	err := manager.DestroyContainer(instance)
+	err := manager.DestroyContainer(instance.Id())
 	c.Assert(err, gc.IsNil)
 	autostartLink := lxc.RestartSymlink(string(instance.Id()))
 	c.Assert(autostartLink, jc.SymlinkDoesNotExist)
@@ -380,7 +380,7 @@
 
 	manager := s.makeManager(c, "test")
 	instance := containertesting.CreateContainer(c, manager, "1/lxc/0")
-	err = manager.DestroyContainer(instance)
+	err = manager.DestroyContainer(instance.Id())
 	c.Assert(err, gc.IsNil)
 }
 

=== modified file 'dependencies.tsv'
--- dependencies.tsv	2014-04-30 22:17:51 +0000
+++ dependencies.tsv	2014-05-14 03:01:15 +0000
@@ -13,7 +13,7 @@
 launchpad.net/goamz	bzr	[e-mail]-20131218155244-hbnkvlkkzy3vmlh9	44
 launchpad.net/gocheck	bzr	[e-mail]-20140225173054-xu9zlkf9kxhvow02	87
 launchpad.net/golxc	bzr	[e-mail]-20140311005930-b14361bwnocu3krh	8
-launchpad.net/gomaasapi	bzr	[e-mail]-20140410114803-rsv7r2a2mgs0z0q4	49
+launchpad.net/gomaasapi	bzr	[e-mail]-20140513111813-kstzbs2kx1ujl3m3	50
 launchpad.net/goose	bzr	tarmac-20140423072524-vxav71c7ko4lgtcu	119
 launchpad.net/goyaml	bzr	[e-mail]-20131114120802-abe042syx64z2m7s	50
 launchpad.net/gwacl	bzr	tarmac-20140312041035-ac7gw7kcqjx7db63	234

=== modified file 'environs/broker.go'
--- environs/broker.go	2014-04-22 09:23:39 +0000
+++ environs/broker.go	2014-05-14 03:01:15 +0000
@@ -50,8 +50,9 @@
 	// id.
 	StartInstance(args StartInstanceParams) (instance.Instance, *instance.HardwareCharacteristics, []network.Info, error)
 
-	// StopInstances shuts down the given instances.
-	StopInstances([]instance.Instance) error
+	// StopInstances shuts down the instances with the specified IDs.
+	// Unknown instance IDs are ignored, to enable idempotency.
+	StopInstances(...instance.Id) error
 
 	// AllInstances returns all instances currently known to the broker.
 	AllInstances() ([]instance.Instance, error)

=== modified file 'environs/jujutest/livetests.go'
--- environs/jujutest/livetests.go	2014-04-24 02:27:38 +0000
+++ environs/jujutest/livetests.go	2014-05-14 03:01:15 +0000
@@ -208,7 +208,7 @@
 	c.Check(insts[0].Id(), gc.Equals, id0)
 	c.Check(insts[1], gc.IsNil)
 
-	err = t.Env.StopInstances([]instance.Instance{inst})
+	err = t.Env.StopInstances(inst.Id())
 	c.Assert(err, gc.IsNil)
 
 	// The machine may not be marked as shutting down
@@ -229,7 +229,7 @@
 
 	inst1, _ := testing.AssertStartInstance(c, t.Env, "1")
 	c.Assert(inst1, gc.NotNil)
-	defer t.Env.StopInstances([]instance.Instance{inst1})
+	defer t.Env.StopInstances(inst1.Id())
 	ports, err := inst1.Ports("1")
 	c.Assert(err, gc.IsNil)
 	c.Assert(ports, gc.HasLen, 0)
@@ -239,7 +239,7 @@
 	ports, err = inst2.Ports("2")
 	c.Assert(err, gc.IsNil)
 	c.Assert(ports, gc.HasLen, 0)
-	defer t.Env.StopInstances([]instance.Instance{inst2})
+	defer t.Env.StopInstances(inst2.Id())
 
 	// Open some ports and check they're there.
 	err = inst1.OpenPorts("1", []instance.Port{{"udp", 67}, {"tcp", 45}})
@@ -332,7 +332,7 @@
 
 	// Create instances and check open ports on both instances.
 	inst1, _ := testing.AssertStartInstance(c, t.Env, "1")
-	defer t.Env.StopInstances([]instance.Instance{inst1})
+	defer t.Env.StopInstances(inst1.Id())
 	ports, err := t.Env.Ports()
 	c.Assert(err, gc.IsNil)
 	c.Assert(ports, gc.HasLen, 0)
@@ -341,7 +341,7 @@
 	ports, err = t.Env.Ports()
 	c.Assert(err, gc.IsNil)
 	c.Assert(ports, gc.HasLen, 0)
-	defer t.Env.StopInstances([]instance.Instance{inst2})
+	defer t.Env.StopInstances(inst2.Id())
 
 	err = t.Env.OpenPorts([]instance.Port{{"udp", 67}, {"tcp", 45}, {"tcp", 89}, {"tcp", 99}})
 	c.Assert(err, gc.IsNil)
@@ -865,7 +865,7 @@
 		MachineConfig: machineConfig,
 	})
 	if inst != nil {
-		err := t.Env.StopInstances([]instance.Instance{inst})
+		err := t.Env.StopInstances(inst.Id())
 		c.Check(err, gc.IsNil)
 	}
 	c.Assert(inst, gc.IsNil)

=== modified file 'environs/jujutest/tests.go'
--- environs/jujutest/tests.go	2014-05-06 21:40:29 +0000
+++ environs/jujutest/tests.go	2014-05-14 03:01:15 +0000
@@ -114,7 +114,7 @@
 	c.Assert(insts, gc.HasLen, 2)
 	c.Assert(insts[0].Id(), gc.Not(gc.Equals), insts[1].Id())
 
-	err = e.StopInstances([]instance.Instance{inst0})
+	err = e.StopInstances(inst0.Id())
 	c.Assert(err, gc.IsNil)
 
 	insts, err = e.Instances([]instance.Id{id0, id1})

=== modified file 'provider/azure/environ.go'
--- provider/azure/environ.go	2014-05-06 16:08:51 +0000
+++ provider/azure/environ.go	2014-05-14 03:01:15 +0000
@@ -475,7 +475,7 @@
 	var inst instance.Instance
 	defer func() {
 		if inst != nil && resultErr != nil {
-			if err := env.StopInstances([]instance.Instance{inst}); err != nil {
+			if err := env.StopInstances(inst.Id()); err != nil {
 				// Failure upon failure. Log it, but return the original error.
 				logger.Errorf("error releasing failed instance: %v", err)
 			}
@@ -760,7 +760,7 @@
 }
 
 // StopInstances is specified in the InstanceBroker interface.
-func (env *azureEnviron) StopInstances(instances []instance.Instance) error {
+func (env *azureEnviron) StopInstances(ids ...instance.Id) error {
 	context, err := env.getManagementAPI()
 	if err != nil {
 		return err
@@ -769,18 +769,18 @@
 
 	// Map services to role names we want to delete.
 	serviceInstances := make(map[string]map[string]bool)
-	for _, instance := range instances {
-		instance, ok := instance.(*azureInstance)
-		if !ok {
-			continue
-		}
-		serviceName := instance.hostedService.ServiceName
-		deleteRoleNames, ok := serviceInstances[serviceName]
-		if !ok {
-			deleteRoleNames = make(map[string]bool)
-			serviceInstances[serviceName] = deleteRoleNames
-		}
-		deleteRoleNames[instance.roleName] = true
+	for _, id := range ids {
+		serviceName, roleName := env.splitInstanceId(id)
+		if roleName == "" {
+			serviceInstances[serviceName] = nil
+		} else {
+			deleteRoleNames, ok := serviceInstances[serviceName]
+			if !ok {
+				deleteRoleNames = make(map[string]bool)
+				serviceInstances[serviceName] = deleteRoleNames
+			}
+			deleteRoleNames[roleName] = true
+		}
 	}
 
 	// Load the properties of each service, so we know whether to
@@ -806,8 +806,9 @@
 			}
 		}
 		// If we're deleting all the roles, we need to delete the
-		// entire cloud service or we'll get an error.
-		if len(deleteRoleNames) == roleNames.Size() {
+		// entire cloud service or we'll get an error. deleteRoleNames
+		// is nil if we're dealing with a legacy deployment.
+		if deleteRoleNames == nil || len(deleteRoleNames) == roleNames.Size() {
 			if err := context.DeleteHostedService(serviceName); err != nil {
 				return err
 			}

=== modified file 'provider/azure/environ_test.go'
--- provider/azure/environ_test.go	2014-04-30 23:18:40 +0000
+++ provider/azure/environ_test.go	2014-05-14 03:01:15 +0000
@@ -762,10 +762,11 @@
 
 func (s *environSuite) TestStopInstancesDestroysMachines(c *gc.C) {
 	env := makeEnviron(c)
+	prefix := env.getEnvPrefix()
 	service1Name := "service1"
-	service1 := makeLegacyDeployment(env, service1Name)
+	service1 := makeLegacyDeployment(env, prefix+service1Name)
 	service2Name := "service2"
-	service2 := makeDeployment(env, service2Name)
+	service2 := makeDeployment(env, prefix+service2Name)
 
 	inst1, err := env.getInstance(service1, "")
 	c.Assert(err, gc.IsNil)
@@ -781,7 +782,7 @@
 	responses = append(responses, buildGetServicePropertiesResponses(c, service2)...)
 	responses = append(responses, buildStatusOKResponses(c, 1)...) // DeleteHostedService
 	requests := gwacl.PatchManagementAPIResponses(responses)
-	err = env.StopInstances([]instance.Instance{inst1, inst2, inst3})
+	err = env.StopInstances(inst1.Id(), inst2.Id(), inst3.Id())
 	c.Check(err, gc.IsNil)
 
 	// One GET and DELETE per service
@@ -795,7 +796,7 @@
 
 func (s *environSuite) TestStopInstancesServiceSubset(c *gc.C) {
 	env := makeEnviron(c)
-	service := makeDeployment(env, "service")
+	service := makeDeployment(env, env.getEnvPrefix()+"service")
 
 	role1Name := service.Deployments[0].RoleList[0].RoleName
 	inst1, err := env.getInstance(service, role1Name)
@@ -804,7 +805,7 @@
 	responses := buildGetServicePropertiesResponses(c, service)
 	responses = append(responses, buildStatusOKResponses(c, 1)...) // DeleteRole
 	requests := gwacl.PatchManagementAPIResponses(responses)
-	err = env.StopInstances([]instance.Instance{inst1})
+	err = env.StopInstances(inst1.Id())
 	c.Check(err, gc.IsNil)
 
 	// One GET for the service, and one DELETE for the role.
@@ -817,8 +818,9 @@
 
 func (s *environSuite) TestStopInstancesWhenStoppingMachinesFails(c *gc.C) {
 	env := makeEnviron(c)
-	service1 := makeDeployment(env, "service1")
-	service2 := makeDeployment(env, "service2")
+	prefix := env.getEnvPrefix()
+	service1 := makeDeployment(env, prefix+"service1")
+	service2 := makeDeployment(env, prefix+"service2")
 	service1Role1Name := service1.Deployments[0].RoleList[0].RoleName
 	inst1, err := env.getInstance(service1, service1Role1Name)
 	c.Assert(err, gc.IsNil)
@@ -831,8 +833,7 @@
 	responses = append(responses, gwacl.NewDispatcherResponse(nil, http.StatusConflict, nil))
 	requests := gwacl.PatchManagementAPIResponses(responses)
 
-	instances := []instance.Instance{inst1, inst2}
-	err = env.StopInstances(instances)
+	err = env.StopInstances(inst1.Id(), inst2.Id())
 	c.Check(err, gc.ErrorMatches, ".*Conflict.*")
 
 	c.Check(len(*requests), gc.Equals, len(responses))
@@ -843,9 +844,7 @@
 
 func (s *environSuite) TestStopInstancesWithZeroInstance(c *gc.C) {
 	env := makeEnviron(c)
-	instances := []instance.Instance{}
-
-	err := env.StopInstances(instances)
+	err := env.StopInstances()
 	c.Check(err, gc.IsNil)
 }
 

=== modified file 'provider/common/bootstrap.go'
--- provider/common/bootstrap.go	2014-04-30 23:18:40 +0000
+++ provider/common/bootstrap.go	2014-05-14 03:01:15 +0000
@@ -128,7 +128,7 @@
 
 	if inst != nil {
 		fmt.Fprintln(ctx.GetStderr(), "Stopping instance...")
-		if stoperr := env.StopInstances([]instance.Instance{inst}); stoperr != nil {
+		if stoperr := env.StopInstances(inst.Id()); stoperr != nil {
 			logger.Errorf("cannot stop failed bootstrap instance %q: %v", inst.Id(), stoperr)
 		} else {
 			// set to nil so we know we can safely delete the state file

=== modified file 'provider/common/bootstrap_test.go'
--- provider/common/bootstrap_test.go	2014-04-24 08:15:45 +0000
+++ provider/common/bootstrap_test.go	2014-05-14 03:01:15 +0000
@@ -117,9 +117,9 @@
 		return &mockInstance{id: "i-blah"}, nil, nil, nil
 	}
 
-	var stopped []instance.Instance
-	stopInstances := func(instances []instance.Instance) error {
-		stopped = append(stopped, instances...)
+	var stopped []instance.Id
+	stopInstances := func(ids []instance.Id) error {
+		stopped = append(stopped, ids...)
 		return nil
 	}
 
@@ -134,7 +134,7 @@
 	err := common.Bootstrap(ctx, env, environs.BootstrapParams{})
 	c.Assert(err, gc.ErrorMatches, "cannot save state: suddenly a wild blah")
 	c.Assert(stopped, gc.HasLen, 1)
-	c.Assert(stopped[0].Id(), gc.Equals, instance.Id("i-blah"))
+	c.Assert(stopped[0], gc.Equals, instance.Id("i-blah"))
 }
 
 func (s *BootstrapSuite) TestCannotRecordThenCannotStop(c *gc.C) {
@@ -150,8 +150,8 @@
 		return &mockInstance{id: "i-blah"}, nil, nil, nil
 	}
 
-	var stopped []instance.Instance
-	stopInstances := func(instances []instance.Instance) error {
+	var stopped []instance.Id
+	stopInstances := func(instances []instance.Id) error {
 		stopped = append(stopped, instances...)
 		return fmt.Errorf("bork bork borken")
 	}
@@ -171,7 +171,7 @@
 	err := common.Bootstrap(ctx, env, environs.BootstrapParams{})
 	c.Assert(err, gc.ErrorMatches, "cannot save state: suddenly a wild blah")
 	c.Assert(stopped, gc.HasLen, 1)
-	c.Assert(stopped[0].Id(), gc.Equals, instance.Id("i-blah"))
+	c.Assert(stopped[0], gc.Equals, instance.Id("i-blah"))
 	c.Assert(tw.Log, jc.LogMatches, []jc.SimpleMessage{{
 		loggo.ERROR, `cannot stop failed bootstrap instance "i-blah": bork bork borken`,
 	}})

=== modified file 'provider/common/destroy.go'
--- provider/common/destroy.go	2014-03-14 20:38:20 +0000
+++ provider/common/destroy.go	2014-05-14 03:01:15 +0000
@@ -5,6 +5,7 @@
 
 import (
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/instance"
 )
 
 // Destroy is a common implementation of the Destroy method defined on
@@ -15,7 +16,11 @@
 	instances, err := env.AllInstances()
 	switch err {
 	case nil:
-		if err := env.StopInstances(instances); err != nil {
+		ids := make([]instance.Id, len(instances))
+		for i, inst := range instances {
+			ids[i] = inst.Id()
+		}
+		if err := env.StopInstances(ids...); err != nil {
 			return err
 		}
 		fallthrough

=== modified file 'provider/common/destroy_test.go'
--- provider/common/destroy_test.go	2014-04-14 12:36:13 +0000
+++ provider/common/destroy_test.go	2014-05-14 03:01:15 +0000
@@ -41,10 +41,10 @@
 				&mockInstance{id: "another"},
 			}, nil
 		},
-		stopInstances: func(instances []instance.Instance) error {
-			c.Assert(instances, gc.HasLen, 2)
-			c.Assert(instances[0].Id(), gc.Equals, instance.Id("one"))
-			c.Assert(instances[1].Id(), gc.Equals, instance.Id("another"))
+		stopInstances: func(ids []instance.Id) error {
+			c.Assert(ids, gc.HasLen, 2)
+			c.Assert(ids[0], gc.Equals, instance.Id("one"))
+			c.Assert(ids[1], gc.Equals, instance.Id("another"))
 			return fmt.Errorf("nah")
 		},
 	}
@@ -61,10 +61,10 @@
 				&mockInstance{id: "another"},
 			}, nil
 		},
-		stopInstances: func(instances []instance.Instance) error {
-			c.Assert(instances, gc.HasLen, 2)
-			c.Assert(instances[0].Id(), gc.Equals, instance.Id("one"))
-			c.Assert(instances[1].Id(), gc.Equals, instance.Id("another"))
+		stopInstances: func(ids []instance.Id) error {
+			c.Assert(ids, gc.HasLen, 2)
+			c.Assert(ids[0], gc.Equals, instance.Id("one"))
+			c.Assert(ids[1], gc.Equals, instance.Id("another"))
 			return nil
 		},
 	}
@@ -84,9 +84,9 @@
 				&mockInstance{id: "one"},
 			}, nil
 		},
-		stopInstances: func(instances []instance.Instance) error {
-			c.Assert(instances, gc.HasLen, 1)
-			c.Assert(instances[0].Id(), gc.Equals, instance.Id("one"))
+		stopInstances: func(ids []instance.Id) error {
+			c.Assert(ids, gc.HasLen, 1)
+			c.Assert(ids[0], gc.Equals, instance.Id("one"))
 			return nil
 		},
 	}

=== modified file 'provider/common/mock_test.go'
--- provider/common/mock_test.go	2014-04-24 02:27:38 +0000
+++ provider/common/mock_test.go	2014-05-14 03:01:15 +0000
@@ -19,7 +19,7 @@
 
 type allInstancesFunc func() ([]instance.Instance, error)
 type startInstanceFunc func(string, constraints.Value, []string, []string, tools.List, *cloudinit.MachineConfig) (instance.Instance, *instance.HardwareCharacteristics, []network.Info, error)
-type stopInstancesFunc func([]instance.Instance) error
+type stopInstancesFunc func([]instance.Id) error
 type getToolsSourcesFunc func() ([]simplestreams.DataSource, error)
 type configFunc func() *config.Config
 type setConfigFunc func(*config.Config) error
@@ -60,8 +60,8 @@
 		args.MachineConfig)
 }
 
-func (env *mockEnviron) StopInstances(instances []instance.Instance) error {
-	return env.stopInstances(instances)
+func (env *mockEnviron) StopInstances(ids ...instance.Id) error {
+	return env.stopInstances(ids)
 }
 
 func (env *mockEnviron) Config() *config.Config {

=== modified file 'provider/dummy/environs.go'
--- provider/dummy/environs.go	2014-05-06 16:08:51 +0000
+++ provider/dummy/environs.go	2014-05-14 03:01:15 +0000
@@ -130,8 +130,8 @@
 }
 
 type OpStopInstances struct {
-	Env       string
-	Instances []instance.Instance
+	Env string
+	Ids []instance.Id
 }
 
 type OpOpenPorts struct {
@@ -829,7 +829,7 @@
 	return i, hc, networkInfo, nil
 }
 
-func (e *environ) StopInstances(is []instance.Instance) error {
+func (e *environ) StopInstances(ids ...instance.Id) error {
 	defer delay()
 	if err := e.checkBroken("StopInstance"); err != nil {
 		return err
@@ -840,12 +840,12 @@
 	}
 	estate.mu.Lock()
 	defer estate.mu.Unlock()
-	for _, i := range is {
-		delete(estate.insts, i.(*dummyInstance).id)
+	for _, id := range ids {
+		delete(estate.insts, id)
 	}
 	estate.ops <- OpStopInstances{
-		Env:       e.name,
-		Instances: is,
+		Env: e.name,
+		Ids: ids,
 	}
 	return nil
 }

=== modified file 'provider/ec2/ec2.go'
--- provider/ec2/ec2.go	2014-05-06 16:08:51 +0000
+++ provider/ec2/ec2.go	2014-05-14 03:01:15 +0000
@@ -537,11 +537,7 @@
 	return inst, &hc, nil, nil
 }
 
-func (e *environ) StopInstances(insts []instance.Instance) error {
-	ids := make([]instance.Id, len(insts))
-	for i, inst := range insts {
-		ids[i] = inst.(*ec2Instance).Id()
-	}
+func (e *environ) StopInstances(ids ...instance.Id) error {
 	return e.terminateInstances(ids)
 }
 

=== modified file 'provider/ec2/live_test.go'
--- provider/ec2/live_test.go	2014-04-09 16:36:12 +0000
+++ provider/ec2/live_test.go	2014-05-14 03:01:15 +0000
@@ -116,7 +116,7 @@
 
 func (t *LiveTests) TestInstanceAttributes(c *gc.C) {
 	inst, hc := testing.AssertStartInstance(c, t.Env, "30")
-	defer t.Env.StopInstances([]instance.Instance{inst})
+	defer t.Env.StopInstances(inst.Id())
 	// Sanity check for hardware characteristics.
 	c.Assert(hc.Arch, gc.NotNil)
 	c.Assert(hc.Mem, gc.NotNil)
@@ -140,7 +140,7 @@
 func (t *LiveTests) TestStartInstanceConstraints(c *gc.C) {
 	cons := constraints.MustParse("mem=2G")
 	inst, hc := testing.AssertStartInstanceWithConstraints(c, t.Env, "30", cons)
-	defer t.Env.StopInstances([]instance.Instance{inst})
+	defer t.Env.StopInstances(inst.Id())
 	ec2inst := ec2.InstanceEC2(inst)
 	c.Assert(ec2inst.InstanceType, gc.Equals, "m1.medium")
 	c.Assert(*hc.Arch, gc.Equals, "amd64")
@@ -187,14 +187,14 @@
 	c.Assert(err, gc.IsNil)
 
 	inst0, _ := testing.AssertStartInstance(c, t.Env, "98")
-	defer t.Env.StopInstances([]instance.Instance{inst0})
+	defer t.Env.StopInstances(inst0.Id())
 
 	// Create a same-named group for the second instance
 	// before starting it, to check that it's reused correctly.
 	oldMachineGroup := createGroup(c, ec2conn, groups[2].Name, "old machine group")
 
 	inst1, _ := testing.AssertStartInstance(c, t.Env, "99")
-	defer t.Env.StopInstances([]instance.Instance{inst1})
+	defer t.Env.StopInstances(inst1.Id())
 
 	groupsResp, err := ec2conn.SecurityGroups(groups, nil)
 	c.Assert(err, gc.IsNil)
@@ -343,7 +343,7 @@
 	inst1 := ec2.FabricateInstance(inst0, "i-aaaaaaaa")
 	inst2, _ := testing.AssertStartInstance(c, t.Env, "41")
 
-	err := t.Env.StopInstances([]instance.Instance{inst0, inst1, inst2})
+	err := t.Env.StopInstances(inst0.Id(), inst1.Id(), inst2.Id())
 	c.Check(err, gc.IsNil)
 
 	var insts []instance.Instance

=== modified file 'provider/joyent/environ_instance.go'
--- provider/joyent/environ_instance.go	2014-05-06 16:08:51 +0000
+++ provider/joyent/environ_instance.go	2014-05-14 03:01:15 +0000
@@ -232,17 +232,17 @@
 	return instance.Address{}, errors.NotImplementedf("AllocateAddress")
 }
 
-func (env *joyentEnviron) StopInstances(instances []instance.Instance) error {
+func (env *joyentEnviron) StopInstances(ids ...instance.Id) error {
 	// Remove all the instances in parallel so that we incur less round-trips.
 	var wg sync.WaitGroup
 	//var err error
-	wg.Add(len(instances))
-	errc := make(chan error, len(instances))
-	for _, inst := range instances {
-		inst := inst.(*joyentInstance)
+	wg.Add(len(ids))
+	errc := make(chan error, len(ids))
+	for _, id := range ids {
+		id := id // copy to new free var for closure
 		go func() {
 			defer wg.Done()
-			if err := inst.Stop(); err != nil {
+			if err := env.stopInstance(string(id)); err != nil {
 				errc <- err
 			}
 		}()
@@ -257,6 +257,39 @@
 	return nil
 }
 
+func (env *joyentEnviron) stopInstance(id string) error {
+	// wait for machine to be running
+	// if machine is still provisioning stop will fail
+	for !env.pollMachineState(id, "running") {
+		time.Sleep(1 * time.Second)
+	}
+
+	err := env.compute.cloudapi.StopMachine(id)
+	if err != nil {
+		return fmt.Errorf("cannot stop instance %s: %v", id, err)
+	}
+
+	// wait for machine to be stopped
+	for !env.pollMachineState(id, "stopped") {
+		time.Sleep(1 * time.Second)
+	}
+
+	err = env.compute.cloudapi.DeleteMachine(id)
+	if err != nil {
+		return fmt.Errorf("cannot delete instance %s: %v", id, err)
+	}
+
+	return nil
+}
+
+func (env *joyentEnviron) pollMachineState(machineId, state string) bool {
+	machineConfig, err := env.compute.cloudapi.GetMachine(machineId)
+	if err != nil {
+		return false
+	}
+	return strings.EqualFold(machineConfig.State, state)
+}
+
 func (env *joyentEnviron) listInstanceTypes() ([]instances.InstanceType, error) {
 	packages, err := env.compute.cloudapi.ListPackages(nil)
 	if err != nil {

=== modified file 'provider/joyent/instance.go'
--- provider/joyent/instance.go	2014-04-02 11:35:49 +0000
+++ provider/joyent/instance.go	2014-05-14 03:01:15 +0000
@@ -4,10 +4,6 @@
 package joyent
 
 import (
-	"fmt"
-	"strings"
-	"time"
-
 	"github.com/joyent/gosdc/cloudapi"
 
 	"launchpad.net/juju-core/instance"
@@ -63,40 +59,3 @@
 func (inst *joyentInstance) WaitDNSName() (string, error) {
 	return common.WaitDNSName(inst)
 }
-
-// Stop will stop and delete the machine
-// Stopped machines are still billed for in the Joyent Public Cloud
-func (inst *joyentInstance) Stop() error {
-	id := string(inst.Id())
-
-	// wait for machine to be running
-	// if machine is still provisioning stop will fail
-	for !inst.pollMachineState(id, "running") {
-		time.Sleep(1 * time.Second)
-	}
-
-	err := inst.env.compute.cloudapi.StopMachine(id)
-	if err != nil {
-		return fmt.Errorf("cannot stop instance %s: %v", id, err)
-	}
-
-	// wait for machine to be stopped
-	for !inst.pollMachineState(id, "stopped") {
-		time.Sleep(1 * time.Second)
-	}
-
-	err = inst.env.compute.cloudapi.DeleteMachine(id)
-	if err != nil {
-		return fmt.Errorf("cannot delete instance %s: %v", id, err)
-	}
-
-	return nil
-}
-
-func (inst *joyentInstance) pollMachineState(machineId, state string) bool {
-	machineConfig, err := inst.env.compute.cloudapi.GetMachine(machineId)
-	if err != nil {
-		return false
-	}
-	return strings.EqualFold(machineConfig.State, state)
-}

=== modified file 'provider/joyent/local_test.go'
--- provider/joyent/local_test.go	2014-04-24 12:33:19 +0000
+++ provider/joyent/local_test.go	2014-05-14 03:01:15 +0000
@@ -193,7 +193,7 @@
 	err := bootstrap.Bootstrap(bootstrapContext(c), env, environs.BootstrapParams{})
 	c.Assert(err, gc.IsNil)
 	inst, _ := testing.AssertStartInstance(c, env, "100")
-	err = env.StopInstances([]instance.Instance{inst})
+	err = env.StopInstances(inst.Id())
 	c.Assert(err, gc.IsNil)
 }
 
@@ -261,7 +261,7 @@
 	envtesting.UploadFakeTools(c, env.Storage())
 	inst, _ := testing.AssertStartInstance(c, env, "100")
 	c.Assert(inst.Status(), gc.Equals, "running")
-	err := env.StopInstances([]instance.Instance{inst})
+	err := env.StopInstances(inst.Id())
 	c.Assert(err, gc.IsNil)
 }
 
@@ -274,7 +274,7 @@
 	id1 := inst1.Id()
 	c.Logf("id0: %s, id1: %s", id0, id1)
 	defer func() {
-		err := env.StopInstances([]instance.Instance{inst0, inst1})
+		err := env.StopInstances(inst0.Id(), inst1.Id())
 		c.Assert(err, gc.IsNil)
 	}()
 

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2014-05-06 16:08:51 +0000
+++ provider/local/environ.go	2014-05-14 03:01:15 +0000
@@ -354,12 +354,12 @@
 }
 
 // StopInstances is specified in the InstanceBroker interface.
-func (env *localEnviron) StopInstances(instances []instance.Instance) error {
-	for _, inst := range instances {
-		if inst.Id() == bootstrapInstanceId {
+func (env *localEnviron) StopInstances(ids ...instance.Id) error {
+	for _, id := range ids {
+		if id == bootstrapInstanceId {
 			return fmt.Errorf("cannot stop the bootstrap instance")
 		}
-		if err := env.containerManager.DestroyContainer(inst); err != nil {
+		if err := env.containerManager.DestroyContainer(id); err != nil {
 			return err
 		}
 	}
@@ -462,7 +462,7 @@
 		return err
 	}
 	for _, inst := range containers {
-		if err := env.containerManager.DestroyContainer(inst); err != nil {
+		if err := env.containerManager.DestroyContainer(inst.Id()); err != nil {
 			return err
 		}
 	}

=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go	2014-05-07 19:09:23 +0000
+++ provider/maas/environ.go	2014-05-14 03:01:15 +0000
@@ -434,7 +434,7 @@
 	}
 	defer func() {
 		if err != nil {
-			if err := environ.releaseInstance(inst); err != nil {
+			if err := environ.StopInstances(inst.Id()); err != nil {
 				logger.Errorf("error releasing failed instance: %v", err)
 			}
 		}
@@ -546,32 +546,18 @@
 }
 
 // StopInstances is specified in the InstanceBroker interface.
-func (environ *maasEnviron) StopInstances(instances []instance.Instance) error {
+func (environ *maasEnviron) StopInstances(ids ...instance.Id) error {
 	// Shortcut to exit quickly if 'instances' is an empty slice or nil.
-	if len(instances) == 0 {
+	if len(ids) == 0 {
 		return nil
 	}
-	// Tell MAAS to release each of the instances.  If there are errors,
-	// return only the first one (but release all instances regardless).
-	// Note that releasing instances also turns them off.
-	var firstErr error
-	for _, instance := range instances {
-		err := environ.releaseInstance(instance)
-		if firstErr == nil {
-			firstErr = err
-		}
-	}
-	return firstErr
-}
-
-// releaseInstance releases a single instance.
-func (environ *maasEnviron) releaseInstance(inst instance.Instance) error {
-	maasInst := inst.(*maasInstance)
-	maasObj := maasInst.maasObject
-	_, err := maasObj.CallPost("release", nil)
-	if err != nil {
-		logger.Debugf("error releasing instance %v", maasInst)
-	}
+	// TODO(axw) 2014-05-13 #1319016
+	// Nodes that have been removed out of band will cause
+	// the release call to fail. We should parse the error
+	// returned from MAAS and retry, or otherwise request
+	// an enhancement to MAAS to ignore unknown node IDs.
+	nodes := environ.getMAASClient().GetSubObject("nodes")
+	_, err := nodes.CallPost("release", getSystemIdValues("nodes", ids))
 	return err
 }
 
@@ -580,7 +566,7 @@
 // "ids" matches all instances (not none as you might expect).
 func (environ *maasEnviron) instances(ids []instance.Id) ([]instance.Instance, error) {
 	nodeListing := environ.getMAASClient().GetSubObject("nodes")
-	filter := getSystemIdValues(ids)
+	filter := getSystemIdValues("id", ids)
 	filter.Add("agent_name", environ.ecfg().maasAgentName())
 	listNodeObjects, err := nodeListing.CallGet("list", filter)
 	if err != nil {

=== modified file 'provider/maas/environ_whitebox_test.go'
--- provider/maas/environ_whitebox_test.go	2014-04-24 12:33:19 +0000
+++ provider/maas/environ_whitebox_test.go	2014-05-14 03:01:15 +0000
@@ -420,24 +420,27 @@
 func (suite *environSuite) TestStopInstancesReturnsIfParameterEmpty(c *gc.C) {
 	suite.getInstance("test1")
 
-	err := suite.makeEnviron().StopInstances([]instance.Instance{})
+	err := suite.makeEnviron().StopInstances()
 	c.Check(err, gc.IsNil)
 	operations := suite.testMAASObject.TestServer.NodeOperations()
 	c.Check(operations, gc.DeepEquals, map[string][]string{})
 }
 
 func (suite *environSuite) TestStopInstancesStopsAndReleasesInstances(c *gc.C) {
-	instance1 := suite.getInstance("test1")
-	instance2 := suite.getInstance("test2")
+	suite.getInstance("test1")
+	suite.getInstance("test2")
 	suite.getInstance("test3")
-	instances := []instance.Instance{instance1, instance2}
-
-	err := suite.makeEnviron().StopInstances(instances)
-
+	// mark test1 and test2 as being allocated, but not test3.
+	// The release operation will ignore test3.
+	suite.testMAASObject.TestServer.OwnedNodes()["test1"] = true
+	suite.testMAASObject.TestServer.OwnedNodes()["test2"] = true
+
+	err := suite.makeEnviron().StopInstances("test1", "test2", "test3")
 	c.Check(err, gc.IsNil)
-	operations := suite.testMAASObject.TestServer.NodeOperations()
-	expectedOperations := map[string][]string{"test1": {"release"}, "test2": {"release"}}
-	c.Check(operations, gc.DeepEquals, expectedOperations)
+	operations := suite.testMAASObject.TestServer.NodesOperations()
+	c.Check(operations, gc.DeepEquals, []string{"release"})
+	c.Assert(suite.testMAASObject.TestServer.OwnedNodes()["test1"], jc.IsFalse)
+	c.Assert(suite.testMAASObject.TestServer.OwnedNodes()["test2"], jc.IsFalse)
 }
 
 func (suite *environSuite) TestStateInfo(c *gc.C) {
@@ -472,6 +475,7 @@
 func (suite *environSuite) TestDestroy(c *gc.C) {
 	env := suite.makeEnviron()
 	suite.getInstance("test1")
+	suite.testMAASObject.TestServer.OwnedNodes()["test1"] = true // simulate acquire
 	data := makeRandomBytes(10)
 	suite.testMAASObject.TestServer.NewFile("filename", data)
 	stor := env.Storage()
@@ -480,9 +484,9 @@
 	c.Check(err, gc.IsNil)
 
 	// Instances have been stopped.
-	operations := suite.testMAASObject.TestServer.NodeOperations()
-	expectedOperations := map[string][]string{"test1": {"release"}}
-	c.Check(operations, gc.DeepEquals, expectedOperations)
+	operations := suite.testMAASObject.TestServer.NodesOperations()
+	c.Check(operations, gc.DeepEquals, []string{"release"})
+	c.Check(suite.testMAASObject.TestServer.OwnedNodes()["test1"], jc.IsFalse)
 	// Files have been cleaned up.
 	listing, err := storage.List(stor, "")
 	c.Assert(err, gc.IsNil)

=== modified file 'provider/maas/util.go'
--- provider/maas/util.go	2013-08-02 05:13:41 +0000
+++ provider/maas/util.go	2014-05-14 03:01:15 +0000
@@ -24,12 +24,12 @@
 }
 
 // getSystemIdValues returns a url.Values object with all the 'system_ids'
-// from the given instanceIds stored under the key 'id'.  This is used
+// from the given instanceIds stored under the specified key.  This is used
 // to filter out instances when listing the nodes objects.
-func getSystemIdValues(instanceIds []instance.Id) url.Values {
+func getSystemIdValues(key string, instanceIds []instance.Id) url.Values {
 	values := url.Values{}
 	for _, instanceId := range instanceIds {
-		values.Add("id", extractSystemId(instanceId))
+		values.Add(key, extractSystemId(instanceId))
 	}
 	return values
 }

=== modified file 'provider/maas/util_test.go'
--- provider/maas/util_test.go	2013-08-09 04:58:34 +0000
+++ provider/maas/util_test.go	2014-05-14 03:01:15 +0000
@@ -30,7 +30,7 @@
 	instanceId2 := instance.Id("/MAAS/api/1.0/nodes/system_id2/")
 	instanceIds := []instance.Id{instanceId1, instanceId2}
 
-	values := getSystemIdValues(instanceIds)
+	values := getSystemIdValues("id", instanceIds)
 
 	c.Check(values["id"], gc.DeepEquals, []string{"system_id1", "system_id2"})
 }

=== modified file 'provider/manual/environ.go'
--- provider/manual/environ.go	2014-05-06 16:08:51 +0000
+++ provider/manual/environ.go	2014-05-14 03:01:15 +0000
@@ -70,7 +70,7 @@
 	return nil, nil, nil, errNoStartInstance
 }
 
-func (*manualEnviron) StopInstances([]instance.Instance) error {
+func (*manualEnviron) StopInstances(...instance.Id) error {
 	return errNoStopInstance
 }
 

=== modified file 'provider/openstack/local_test.go'
--- provider/openstack/local_test.go	2014-04-24 12:33:19 +0000
+++ provider/openstack/local_test.go	2014-05-14 03:01:15 +0000
@@ -264,7 +264,7 @@
 	err = bootstrap.Bootstrap(coretesting.Context(c), env, environs.BootstrapParams{})
 	c.Assert(err, gc.IsNil)
 	inst, _ := testing.AssertStartInstance(c, env, "100")
-	err = env.StopInstances([]instance.Instance{inst})
+	err = env.StopInstances(inst.Id())
 	c.Assert(err, gc.IsNil)
 }
 
@@ -288,7 +288,7 @@
 	env, err := environs.New(cfg)
 	c.Assert(err, gc.IsNil)
 	inst, _ := testing.AssertStartInstance(c, env, "100")
-	err = env.StopInstances([]instance.Instance{inst})
+	err = env.StopInstances(inst.Id())
 	c.Assert(err, gc.IsNil)
 }
 
@@ -363,7 +363,7 @@
 	// group, one group for the entire environment, and another for the
 	// new instance.
 	assertSecurityGroups(c, env, []string{"default", fmt.Sprintf("juju-%v", env.Name()), fmt.Sprintf("juju-%v-%v", env.Name(), instanceName)})
-	err = env.StopInstances([]instance.Instance{inst})
+	err = env.StopInstances(inst.Id())
 	c.Assert(err, gc.IsNil)
 	// The security group for this instance is now removed.
 	assertSecurityGroups(c, env, []string{"default", fmt.Sprintf("juju-%v", env.Name())})
@@ -391,7 +391,7 @@
 	inst, _ := testing.AssertStartInstance(c, env, instanceName)
 	allSecurityGroups := []string{"default", fmt.Sprintf("juju-%v", env.Name()), fmt.Sprintf("juju-%v-%v", env.Name(), instanceName)}
 	assertSecurityGroups(c, env, allSecurityGroups)
-	err = env.StopInstances([]instance.Instance{inst})
+	err = env.StopInstances(inst.Id())
 	c.Assert(err, gc.IsNil)
 	assertSecurityGroups(c, env, allSecurityGroups)
 }
@@ -478,7 +478,7 @@
 	// goose's test service always returns ACTIVE state.
 	inst, _ := testing.AssertStartInstance(c, env, "100")
 	c.Assert(inst.Status(), gc.Equals, nova.StatusActive)
-	err := env.StopInstances([]instance.Instance{inst})
+	err := env.StopInstances(inst.Id())
 	c.Assert(err, gc.IsNil)
 }
 
@@ -489,7 +489,7 @@
 	inst1, _ := testing.AssertStartInstance(c, env, "101")
 	id1 := inst1.Id()
 	defer func() {
-		err := env.StopInstances([]instance.Instance{inst0, inst1})
+		err := env.StopInstances(inst0.Id(), inst1.Id())
 		c.Assert(err, gc.IsNil)
 	}()
 
@@ -534,7 +534,7 @@
 	defer cleanup()
 	stateInst, _ := testing.AssertStartInstance(c, env, "100")
 	defer func() {
-		err := env.StopInstances([]instance.Instance{stateInst})
+		err := env.StopInstances(stateInst.Id())
 		c.Assert(err, gc.IsNil)
 	}()
 	found := make(map[instance.Id]instance.Instance)
@@ -559,7 +559,7 @@
 	defer cleanup()
 	stateInst, _ := testing.AssertStartInstance(c, env, "100")
 	defer func() {
-		err := env.StopInstances([]instance.Instance{stateInst})
+		err := env.StopInstances(stateInst.Id())
 		c.Assert(err, gc.IsNil)
 	}()
 

=== modified file 'provider/openstack/provider.go'
--- provider/openstack/provider.go	2014-05-06 16:08:51 +0000
+++ provider/openstack/provider.go	2014-05-14 03:01:15 +0000
@@ -6,7 +6,6 @@
 package openstack
 
 import (
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -902,28 +901,33 @@
 	return inst, inst.hardwareCharacteristics(), nil, nil
 }
 
-func (e *environ) StopInstances(insts []instance.Instance) error {
-	ids := make([]instance.Id, len(insts))
-	securityGroupNames := make([]string, len(insts))
-	for i, inst := range insts {
-		instanceValue, ok := inst.(*openstackInstance)
-		if !ok {
-			return errors.New("Incompatible instance.Instance supplied")
-		}
-		ids[i] = instanceValue.Id()
-		openstackName := instanceValue.getServerDetail().Name
-		lastDashPos := strings.LastIndex(openstackName, "-")
-		if lastDashPos == -1 {
-			return fmt.Errorf("cannot identify instance ID in openstack server name %q", openstackName)
-		}
-		securityGroupNames[i] = e.machineGroupName(openstackName[lastDashPos+1:])
+func (e *environ) StopInstances(ids ...instance.Id) error {
+	// If in instance firewall mode, gather the security group names.
+	var securityGroupNames []string
+	if e.Config().FirewallMode() == config.FwInstance {
+		instances, err := e.Instances(ids)
+		if err == environs.ErrNoInstances {
+			return nil
+		}
+		securityGroupNames = make([]string, 0, len(ids))
+		for _, inst := range instances {
+			if inst == nil {
+				continue
+			}
+			openstackName := inst.(*openstackInstance).getServerDetail().Name
+			lastDashPos := strings.LastIndex(openstackName, "-")
+			if lastDashPos == -1 {
+				return fmt.Errorf("cannot identify machine ID in openstack server name %q", openstackName)
+			}
+			securityGroupName := e.machineGroupName(openstackName[lastDashPos+1:])
+			securityGroupNames = append(securityGroupNames, securityGroupName)
+		}
 	}
 	logger.Debugf("terminating instances %v", ids)
-	err := e.terminateInstances(ids)
-	if err != nil {
+	if err := e.terminateInstances(ids); err != nil {
 		return err
 	}
-	if e.Config().FirewallMode() == config.FwInstance {
+	if securityGroupNames != nil {
 		return e.deleteSecurityGroups(securityGroupNames)
 	}
 	return nil

=== modified file 'state/apiserver/client/destroy.go'
--- state/apiserver/client/destroy.go	2014-01-17 16:41:32 +0000
+++ state/apiserver/client/destroy.go	2014-05-14 03:01:15 +0000
@@ -104,28 +104,7 @@
 	if err != nil {
 		return err
 	}
-	// TODO(axw) 2013-12-12 #1260171
-	// Modify InstanceBroker.StopInstances to take
-	// a slice of IDs rather than Instances.
-	instances, err := env.Instances(ids)
-	switch err {
-	case nil:
-	default:
-		return err
-	case environs.ErrNoInstances:
-		return nil
-	case environs.ErrPartialInstances:
-		var nonNilInstances []instance.Instance
-		for i, inst := range instances {
-			if inst == nil {
-				logger.Warningf("unknown instance ID: %v", ids[i])
-				continue
-			}
-			nonNilInstances = append(nonNilInstances, inst)
-		}
-		instances = nonNilInstances
-	}
-	return env.StopInstances(instances)
+	return env.StopInstances(ids...)
 }
 
 // checkManualMachines checks if any of the machines in the slice were

=== modified file 'worker/provisioner/kvm-broker.go'
--- worker/provisioner/kvm-broker.go	2014-05-09 13:24:50 +0000
+++ worker/provisioner/kvm-broker.go	2014-05-14 03:01:15 +0000
@@ -105,11 +105,11 @@
 }
 
 // StopInstances shuts down the given instances.
-func (broker *kvmBroker) StopInstances(instances []instance.Instance) error {
+func (broker *kvmBroker) StopInstances(ids ...instance.Id) error {
 	// TODO: potentially parallelise.
-	for _, instance := range instances {
-		kvmLogger.Infof("stopping kvm container for instance: %s", instance.Id())
-		if err := broker.manager.DestroyContainer(instance); err != nil {
+	for _, id := range ids {
+		kvmLogger.Infof("stopping kvm container for instance: %s", id)
+		if err := broker.manager.DestroyContainer(id); err != nil {
 			kvmLogger.Errorf("container did not stop: %v", err)
 			return err
 		}

=== modified file 'worker/provisioner/kvm-broker_test.go'
--- worker/provisioner/kvm-broker_test.go	2014-05-09 13:24:50 +0000
+++ worker/provisioner/kvm-broker_test.go	2014-05-14 03:01:15 +0000
@@ -102,13 +102,13 @@
 	kvm1 := s.startInstance(c, "1/kvm/1")
 	kvm2 := s.startInstance(c, "1/kvm/2")
 
-	err := s.broker.StopInstances([]instance.Instance{kvm0})
+	err := s.broker.StopInstances(kvm0.Id())
 	c.Assert(err, gc.IsNil)
 	s.assertInstances(c, kvm1, kvm2)
 	c.Assert(s.kvmContainerDir(kvm0), jc.DoesNotExist)
 	c.Assert(s.kvmRemovedContainerDir(kvm0), jc.IsDirectory)
 
-	err = s.broker.StopInstances([]instance.Instance{kvm1, kvm2})
+	err = s.broker.StopInstances(kvm1.Id(), kvm2.Id())
 	c.Assert(err, gc.IsNil)
 	s.assertInstances(c)
 }
@@ -118,7 +118,7 @@
 	kvm1 := s.startInstance(c, "1/kvm/1")
 	s.assertInstances(c, kvm0, kvm1)
 
-	err := s.broker.StopInstances([]instance.Instance{kvm1})
+	err := s.broker.StopInstances(kvm1.Id())
 	c.Assert(err, gc.IsNil)
 	kvm2 := s.startInstance(c, "1/kvm/2")
 	s.assertInstances(c, kvm0, kvm2)

=== modified file 'worker/provisioner/lxc-broker.go'
--- worker/provisioner/lxc-broker.go	2014-05-09 13:24:50 +0000
+++ worker/provisioner/lxc-broker.go	2014-05-14 03:01:15 +0000
@@ -102,11 +102,11 @@
 }
 
 // StopInstances shuts down the given instances.
-func (broker *lxcBroker) StopInstances(instances []instance.Instance) error {
+func (broker *lxcBroker) StopInstances(ids ...instance.Id) error {
 	// TODO: potentially parallelise.
-	for _, instance := range instances {
-		lxcLogger.Infof("stopping lxc container for instance: %s", instance.Id())
-		if err := broker.manager.DestroyContainer(instance); err != nil {
+	for _, id := range ids {
+		lxcLogger.Infof("stopping lxc container for instance: %s", id)
+		if err := broker.manager.DestroyContainer(id); err != nil {
 			lxcLogger.Errorf("container did not stop: %v", err)
 			return err
 		}

=== modified file 'worker/provisioner/lxc-broker_test.go'
--- worker/provisioner/lxc-broker_test.go	2014-05-09 13:24:50 +0000
+++ worker/provisioner/lxc-broker_test.go	2014-05-14 03:01:15 +0000
@@ -131,13 +131,13 @@
 	lxc1 := s.startInstance(c, "1/lxc/1")
 	lxc2 := s.startInstance(c, "1/lxc/2")
 
-	err := s.broker.StopInstances([]instance.Instance{lxc0})
+	err := s.broker.StopInstances(lxc0.Id())
 	c.Assert(err, gc.IsNil)
 	s.assertInstances(c, lxc1, lxc2)
 	c.Assert(s.lxcContainerDir(lxc0), jc.DoesNotExist)
 	c.Assert(s.lxcRemovedContainerDir(lxc0), jc.IsDirectory)
 
-	err = s.broker.StopInstances([]instance.Instance{lxc1, lxc2})
+	err = s.broker.StopInstances(lxc1.Id(), lxc2.Id())
 	c.Assert(err, gc.IsNil)
 	s.assertInstances(c)
 }
@@ -147,7 +147,7 @@
 	lxc1 := s.startInstance(c, "1/lxc/1")
 	s.assertInstances(c, lxc0, lxc1)
 
-	err := s.broker.StopInstances([]instance.Instance{lxc1})
+	err := s.broker.StopInstances(lxc1.Id())
 	c.Assert(err, gc.IsNil)
 	lxc2 := s.startInstance(c, "1/lxc/2")
 	s.assertInstances(c, lxc0, lxc2)

=== modified file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go	2014-05-08 06:58:42 +0000
+++ worker/provisioner/provisioner_task.go	2014-05-14 03:01:15 +0000
@@ -397,7 +397,11 @@
 	if len(instances) == 0 {
 		return nil
 	}
-	if err := task.broker.StopInstances(instances); err != nil {
+	ids := make([]instance.Id, len(instances))
+	for i, inst := range instances {
+		ids[i] = inst.Id()
+	}
+	if err := task.broker.StopInstances(ids...); err != nil {
 		logger.Errorf("broker failed to stop instances: %v", err)
 		return err
 	}
@@ -484,7 +488,7 @@
 	}
 	// We need to stop the instance right away here, set error status and go on.
 	task.setErrorStatus("cannot register instance for machine %v: %v", machine, err)
-	if err := task.broker.StopInstances([]instance.Instance{inst}); err != nil {
+	if err := task.broker.StopInstances(inst.Id()); err != nil {
 		// We cannot even stop the instance, log the error and quit.
 		logger.Errorf("cannot stop instance %q for machine %v: %v", inst.Id(), machine, err)
 		return err

=== modified file 'worker/provisioner/provisioner_test.go'
--- worker/provisioner/provisioner_test.go	2014-04-30 23:18:40 +0000
+++ worker/provisioner/provisioner_test.go	2014-05-14 03:01:15 +0000
@@ -254,8 +254,8 @@
 		case o := <-s.op:
 			switch o := o.(type) {
 			case dummy.OpStopInstances:
-				for _, stoppedInstance := range o.Instances {
-					instId := string(stoppedInstance.Id())
+				for _, id := range o.Ids {
+					instId := string(id)
 					instanceIdsToStop.Remove(instId)
 					if instanceIdsToKeep.Contains(instId) {
 						c.Errorf("provisioner unexpectedly stopped instance %s", instId)