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

Proposed by Andrew Wilkins on 2014-05-13
Status: Merged
Approved by: Andrew Wilkins on 2014-05-14
Approved revision: 2724
Merged at revision: 2728
Proposed branch: lp:~axwalk/juju-core/lp1260171-stopinstances-ids
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 1305 lines (+232/-257)
38 files modified
container/interface.go (+7/-3)
container/kvm/kvm.go (+2/-2)
container/kvm/kvm_test.go (+1/-1)
container/kvm/live_test.go (+1/-1)
container/lxc/lxc.go (+2/-2)
container/lxc/lxc_test.go (+5/-5)
dependencies.tsv (+1/-1)
environs/broker.go (+3/-2)
environs/jujutest/livetests.go (+6/-6)
environs/jujutest/tests.go (+1/-1)
provider/azure/environ.go (+17/-16)
provider/azure/environ_test.go (+11/-12)
provider/common/bootstrap.go (+1/-1)
provider/common/bootstrap_test.go (+7/-7)
provider/common/destroy.go (+6/-1)
provider/common/destroy_test.go (+11/-11)
provider/common/mock_test.go (+3/-3)
provider/dummy/environs.go (+7/-7)
provider/ec2/ec2.go (+1/-5)
provider/ec2/live_test.go (+5/-5)
provider/joyent/environ_instance.go (+39/-6)
provider/joyent/instance.go (+0/-41)
provider/joyent/local_test.go (+3/-3)
provider/local/environ.go (+5/-5)
provider/maas/environ.go (+11/-25)
provider/maas/environ_whitebox_test.go (+17/-13)
provider/maas/util.go (+3/-3)
provider/maas/util_test.go (+1/-1)
provider/manual/environ.go (+1/-1)
provider/openstack/local_test.go (+8/-8)
provider/openstack/provider.go (+23/-19)
state/apiserver/client/destroy.go (+1/-22)
worker/provisioner/kvm-broker.go (+4/-4)
worker/provisioner/kvm-broker_test.go (+3/-3)
worker/provisioner/lxc-broker.go (+4/-4)
worker/provisioner/lxc-broker_test.go (+3/-3)
worker/provisioner/provisioner_task.go (+6/-2)
worker/provisioner/provisioner_test.go (+2/-2)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1260171-stopinstances-ids
Reviewer Review Type Date Requested Status
Juju Engineering 2014-05-13 Pending
Review via email: mp+219347@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Andrew Wilkins (axwalk) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'container/interface.go'
2--- container/interface.go 2014-03-19 21:08:58 +0000
3+++ container/interface.go 2014-05-14 03:01:15 +0000
4@@ -20,13 +20,17 @@
5 // Manager is responsible for starting containers, and stopping and listing
6 // containers that it has started.
7 type Manager interface {
8- // CreateContainer creates and starts a new container for the specified machine.
9+ // CreateContainer creates and starts a new container for the specified
10+ // machine.
11 CreateContainer(
12 machineConfig *cloudinit.MachineConfig,
13 series string,
14 network *NetworkConfig) (instance.Instance, *instance.HardwareCharacteristics, error)
15- // DestroyContainer stops and destroyes the container identified by Instance.
16- DestroyContainer(instance.Instance) error
17+
18+ // DestroyContainer stops and destroyes the container identified by
19+ // instance id.
20+ DestroyContainer(instance.Id) error
21+
22 // ListContainers return a list of containers that have been started by
23 // this manager.
24 ListContainers() ([]instance.Instance, error)
25
26=== modified file 'container/kvm/kvm.go'
27--- container/kvm/kvm.go 2014-04-17 03:41:32 +0000
28+++ container/kvm/kvm.go 2014-05-14 03:01:15 +0000
29@@ -125,8 +125,8 @@
30 return &kvmInstance{kvmContainer, name}, &hardware, nil
31 }
32
33-func (manager *containerManager) DestroyContainer(instance instance.Instance) error {
34- name := string(instance.Id())
35+func (manager *containerManager) DestroyContainer(id instance.Id) error {
36+ name := string(id)
37 kvmContainer := KvmObjectFactory.New(name)
38 if err := kvmContainer.Stop(); err != nil {
39 logger.Errorf("failed to stop kvm container: %v", err)
40
41=== modified file 'container/kvm/kvm_test.go'
42--- container/kvm/kvm_test.go 2014-04-09 16:36:12 +0000
43+++ container/kvm/kvm_test.go 2014-05-14 03:01:15 +0000
44@@ -98,7 +98,7 @@
45 func (s *KVMSuite) TestDestroyContainer(c *gc.C) {
46 instance := containertesting.CreateContainer(c, s.manager, "1/lxc/0")
47
48- err := s.manager.DestroyContainer(instance)
49+ err := s.manager.DestroyContainer(instance.Id())
50 c.Assert(err, gc.IsNil)
51
52 name := string(instance.Id())
53
54=== modified file 'container/kvm/live_test.go'
55--- container/kvm/live_test.go 2014-04-01 16:27:22 +0000
56+++ container/kvm/live_test.go 2014-05-14 03:01:15 +0000
57@@ -75,7 +75,7 @@
58 instances, err := manager.ListContainers()
59 c.Assert(err, gc.IsNil)
60 for _, instance := range instances {
61- err := manager.DestroyContainer(instance)
62+ err := manager.DestroyContainer(instance.Id())
63 c.Check(err, gc.IsNil)
64 }
65 }
66
67=== modified file 'container/lxc/lxc.go'
68--- container/lxc/lxc.go 2014-03-19 21:08:58 +0000
69+++ container/lxc/lxc.go 2014-05-14 03:01:15 +0000
70@@ -269,9 +269,9 @@
71 return appendToContainerConfig(name, line)
72 }
73
74-func (manager *containerManager) DestroyContainer(instance instance.Instance) error {
75+func (manager *containerManager) DestroyContainer(id instance.Id) error {
76 start := time.Now()
77- name := string(instance.Id())
78+ name := string(id)
79 lxcContainer := LxcObjectFactory.New(name)
80 if useRestartDir() {
81 // Remove the autostart link.
82
83=== modified file 'container/lxc/lxc_test.go'
84--- container/lxc/lxc_test.go 2014-03-19 21:08:58 +0000
85+++ container/lxc/lxc_test.go 2014-05-14 03:01:15 +0000
86@@ -274,7 +274,7 @@
87
88 // DestroyContainer stops and then destroys the container, putting it
89 // into "unknown" state.
90- err := manager.DestroyContainer(instance)
91+ err := manager.DestroyContainer(instance.Id())
92 c.Assert(err, gc.IsNil)
93 c.Assert(instance.Status(), gc.Equals, string(golxc.StateUnknown))
94 }
95@@ -283,7 +283,7 @@
96 manager := s.makeManager(c, "test")
97 instance := containertesting.CreateContainer(c, manager, "1/lxc/0")
98
99- err := manager.DestroyContainer(instance)
100+ err := manager.DestroyContainer(instance.Id())
101 c.Assert(err, gc.IsNil)
102
103 name := string(instance.Id())
104@@ -302,7 +302,7 @@
105 err := os.MkdirAll(targetDir, 0755)
106 c.Assert(err, gc.IsNil)
107
108- err = manager.DestroyContainer(instance)
109+ err = manager.DestroyContainer(instance.Id())
110 c.Assert(err, gc.IsNil)
111
112 // Check that the container dir is no longer in the container dir
113@@ -368,7 +368,7 @@
114 func (s *LxcSuite) TestDestroyContainerRemovesAutostartLink(c *gc.C) {
115 manager := s.makeManager(c, "test")
116 instance := containertesting.CreateContainer(c, manager, "1/lxc/0")
117- err := manager.DestroyContainer(instance)
118+ err := manager.DestroyContainer(instance.Id())
119 c.Assert(err, gc.IsNil)
120 autostartLink := lxc.RestartSymlink(string(instance.Id()))
121 c.Assert(autostartLink, jc.SymlinkDoesNotExist)
122@@ -380,7 +380,7 @@
123
124 manager := s.makeManager(c, "test")
125 instance := containertesting.CreateContainer(c, manager, "1/lxc/0")
126- err = manager.DestroyContainer(instance)
127+ err = manager.DestroyContainer(instance.Id())
128 c.Assert(err, gc.IsNil)
129 }
130
131
132=== modified file 'dependencies.tsv'
133--- dependencies.tsv 2014-04-30 22:17:51 +0000
134+++ dependencies.tsv 2014-05-14 03:01:15 +0000
135@@ -13,7 +13,7 @@
136 launchpad.net/goamz bzr roger.peppe@canonical.com-20131218155244-hbnkvlkkzy3vmlh9 44
137 launchpad.net/gocheck bzr gustavo@niemeyer.net-20140225173054-xu9zlkf9kxhvow02 87
138 launchpad.net/golxc bzr tim.penhey@canonical.com-20140311005930-b14361bwnocu3krh 8
139-launchpad.net/gomaasapi bzr martin.packman@canonical.com-20140410114803-rsv7r2a2mgs0z0q4 49
140+launchpad.net/gomaasapi bzr andrew.wilkins@canonical.com-20140513111813-kstzbs2kx1ujl3m3 50
141 launchpad.net/goose bzr tarmac-20140423072524-vxav71c7ko4lgtcu 119
142 launchpad.net/goyaml bzr gustavo@niemeyer.net-20131114120802-abe042syx64z2m7s 50
143 launchpad.net/gwacl bzr tarmac-20140312041035-ac7gw7kcqjx7db63 234
144
145=== modified file 'environs/broker.go'
146--- environs/broker.go 2014-04-22 09:23:39 +0000
147+++ environs/broker.go 2014-05-14 03:01:15 +0000
148@@ -50,8 +50,9 @@
149 // id.
150 StartInstance(args StartInstanceParams) (instance.Instance, *instance.HardwareCharacteristics, []network.Info, error)
151
152- // StopInstances shuts down the given instances.
153- StopInstances([]instance.Instance) error
154+ // StopInstances shuts down the instances with the specified IDs.
155+ // Unknown instance IDs are ignored, to enable idempotency.
156+ StopInstances(...instance.Id) error
157
158 // AllInstances returns all instances currently known to the broker.
159 AllInstances() ([]instance.Instance, error)
160
161=== modified file 'environs/jujutest/livetests.go'
162--- environs/jujutest/livetests.go 2014-04-24 02:27:38 +0000
163+++ environs/jujutest/livetests.go 2014-05-14 03:01:15 +0000
164@@ -208,7 +208,7 @@
165 c.Check(insts[0].Id(), gc.Equals, id0)
166 c.Check(insts[1], gc.IsNil)
167
168- err = t.Env.StopInstances([]instance.Instance{inst})
169+ err = t.Env.StopInstances(inst.Id())
170 c.Assert(err, gc.IsNil)
171
172 // The machine may not be marked as shutting down
173@@ -229,7 +229,7 @@
174
175 inst1, _ := testing.AssertStartInstance(c, t.Env, "1")
176 c.Assert(inst1, gc.NotNil)
177- defer t.Env.StopInstances([]instance.Instance{inst1})
178+ defer t.Env.StopInstances(inst1.Id())
179 ports, err := inst1.Ports("1")
180 c.Assert(err, gc.IsNil)
181 c.Assert(ports, gc.HasLen, 0)
182@@ -239,7 +239,7 @@
183 ports, err = inst2.Ports("2")
184 c.Assert(err, gc.IsNil)
185 c.Assert(ports, gc.HasLen, 0)
186- defer t.Env.StopInstances([]instance.Instance{inst2})
187+ defer t.Env.StopInstances(inst2.Id())
188
189 // Open some ports and check they're there.
190 err = inst1.OpenPorts("1", []instance.Port{{"udp", 67}, {"tcp", 45}})
191@@ -332,7 +332,7 @@
192
193 // Create instances and check open ports on both instances.
194 inst1, _ := testing.AssertStartInstance(c, t.Env, "1")
195- defer t.Env.StopInstances([]instance.Instance{inst1})
196+ defer t.Env.StopInstances(inst1.Id())
197 ports, err := t.Env.Ports()
198 c.Assert(err, gc.IsNil)
199 c.Assert(ports, gc.HasLen, 0)
200@@ -341,7 +341,7 @@
201 ports, err = t.Env.Ports()
202 c.Assert(err, gc.IsNil)
203 c.Assert(ports, gc.HasLen, 0)
204- defer t.Env.StopInstances([]instance.Instance{inst2})
205+ defer t.Env.StopInstances(inst2.Id())
206
207 err = t.Env.OpenPorts([]instance.Port{{"udp", 67}, {"tcp", 45}, {"tcp", 89}, {"tcp", 99}})
208 c.Assert(err, gc.IsNil)
209@@ -865,7 +865,7 @@
210 MachineConfig: machineConfig,
211 })
212 if inst != nil {
213- err := t.Env.StopInstances([]instance.Instance{inst})
214+ err := t.Env.StopInstances(inst.Id())
215 c.Check(err, gc.IsNil)
216 }
217 c.Assert(inst, gc.IsNil)
218
219=== modified file 'environs/jujutest/tests.go'
220--- environs/jujutest/tests.go 2014-05-06 21:40:29 +0000
221+++ environs/jujutest/tests.go 2014-05-14 03:01:15 +0000
222@@ -114,7 +114,7 @@
223 c.Assert(insts, gc.HasLen, 2)
224 c.Assert(insts[0].Id(), gc.Not(gc.Equals), insts[1].Id())
225
226- err = e.StopInstances([]instance.Instance{inst0})
227+ err = e.StopInstances(inst0.Id())
228 c.Assert(err, gc.IsNil)
229
230 insts, err = e.Instances([]instance.Id{id0, id1})
231
232=== modified file 'provider/azure/environ.go'
233--- provider/azure/environ.go 2014-05-06 16:08:51 +0000
234+++ provider/azure/environ.go 2014-05-14 03:01:15 +0000
235@@ -475,7 +475,7 @@
236 var inst instance.Instance
237 defer func() {
238 if inst != nil && resultErr != nil {
239- if err := env.StopInstances([]instance.Instance{inst}); err != nil {
240+ if err := env.StopInstances(inst.Id()); err != nil {
241 // Failure upon failure. Log it, but return the original error.
242 logger.Errorf("error releasing failed instance: %v", err)
243 }
244@@ -760,7 +760,7 @@
245 }
246
247 // StopInstances is specified in the InstanceBroker interface.
248-func (env *azureEnviron) StopInstances(instances []instance.Instance) error {
249+func (env *azureEnviron) StopInstances(ids ...instance.Id) error {
250 context, err := env.getManagementAPI()
251 if err != nil {
252 return err
253@@ -769,18 +769,18 @@
254
255 // Map services to role names we want to delete.
256 serviceInstances := make(map[string]map[string]bool)
257- for _, instance := range instances {
258- instance, ok := instance.(*azureInstance)
259- if !ok {
260- continue
261- }
262- serviceName := instance.hostedService.ServiceName
263- deleteRoleNames, ok := serviceInstances[serviceName]
264- if !ok {
265- deleteRoleNames = make(map[string]bool)
266- serviceInstances[serviceName] = deleteRoleNames
267- }
268- deleteRoleNames[instance.roleName] = true
269+ for _, id := range ids {
270+ serviceName, roleName := env.splitInstanceId(id)
271+ if roleName == "" {
272+ serviceInstances[serviceName] = nil
273+ } else {
274+ deleteRoleNames, ok := serviceInstances[serviceName]
275+ if !ok {
276+ deleteRoleNames = make(map[string]bool)
277+ serviceInstances[serviceName] = deleteRoleNames
278+ }
279+ deleteRoleNames[roleName] = true
280+ }
281 }
282
283 // Load the properties of each service, so we know whether to
284@@ -806,8 +806,9 @@
285 }
286 }
287 // If we're deleting all the roles, we need to delete the
288- // entire cloud service or we'll get an error.
289- if len(deleteRoleNames) == roleNames.Size() {
290+ // entire cloud service or we'll get an error. deleteRoleNames
291+ // is nil if we're dealing with a legacy deployment.
292+ if deleteRoleNames == nil || len(deleteRoleNames) == roleNames.Size() {
293 if err := context.DeleteHostedService(serviceName); err != nil {
294 return err
295 }
296
297=== modified file 'provider/azure/environ_test.go'
298--- provider/azure/environ_test.go 2014-04-30 23:18:40 +0000
299+++ provider/azure/environ_test.go 2014-05-14 03:01:15 +0000
300@@ -762,10 +762,11 @@
301
302 func (s *environSuite) TestStopInstancesDestroysMachines(c *gc.C) {
303 env := makeEnviron(c)
304+ prefix := env.getEnvPrefix()
305 service1Name := "service1"
306- service1 := makeLegacyDeployment(env, service1Name)
307+ service1 := makeLegacyDeployment(env, prefix+service1Name)
308 service2Name := "service2"
309- service2 := makeDeployment(env, service2Name)
310+ service2 := makeDeployment(env, prefix+service2Name)
311
312 inst1, err := env.getInstance(service1, "")
313 c.Assert(err, gc.IsNil)
314@@ -781,7 +782,7 @@
315 responses = append(responses, buildGetServicePropertiesResponses(c, service2)...)
316 responses = append(responses, buildStatusOKResponses(c, 1)...) // DeleteHostedService
317 requests := gwacl.PatchManagementAPIResponses(responses)
318- err = env.StopInstances([]instance.Instance{inst1, inst2, inst3})
319+ err = env.StopInstances(inst1.Id(), inst2.Id(), inst3.Id())
320 c.Check(err, gc.IsNil)
321
322 // One GET and DELETE per service
323@@ -795,7 +796,7 @@
324
325 func (s *environSuite) TestStopInstancesServiceSubset(c *gc.C) {
326 env := makeEnviron(c)
327- service := makeDeployment(env, "service")
328+ service := makeDeployment(env, env.getEnvPrefix()+"service")
329
330 role1Name := service.Deployments[0].RoleList[0].RoleName
331 inst1, err := env.getInstance(service, role1Name)
332@@ -804,7 +805,7 @@
333 responses := buildGetServicePropertiesResponses(c, service)
334 responses = append(responses, buildStatusOKResponses(c, 1)...) // DeleteRole
335 requests := gwacl.PatchManagementAPIResponses(responses)
336- err = env.StopInstances([]instance.Instance{inst1})
337+ err = env.StopInstances(inst1.Id())
338 c.Check(err, gc.IsNil)
339
340 // One GET for the service, and one DELETE for the role.
341@@ -817,8 +818,9 @@
342
343 func (s *environSuite) TestStopInstancesWhenStoppingMachinesFails(c *gc.C) {
344 env := makeEnviron(c)
345- service1 := makeDeployment(env, "service1")
346- service2 := makeDeployment(env, "service2")
347+ prefix := env.getEnvPrefix()
348+ service1 := makeDeployment(env, prefix+"service1")
349+ service2 := makeDeployment(env, prefix+"service2")
350 service1Role1Name := service1.Deployments[0].RoleList[0].RoleName
351 inst1, err := env.getInstance(service1, service1Role1Name)
352 c.Assert(err, gc.IsNil)
353@@ -831,8 +833,7 @@
354 responses = append(responses, gwacl.NewDispatcherResponse(nil, http.StatusConflict, nil))
355 requests := gwacl.PatchManagementAPIResponses(responses)
356
357- instances := []instance.Instance{inst1, inst2}
358- err = env.StopInstances(instances)
359+ err = env.StopInstances(inst1.Id(), inst2.Id())
360 c.Check(err, gc.ErrorMatches, ".*Conflict.*")
361
362 c.Check(len(*requests), gc.Equals, len(responses))
363@@ -843,9 +844,7 @@
364
365 func (s *environSuite) TestStopInstancesWithZeroInstance(c *gc.C) {
366 env := makeEnviron(c)
367- instances := []instance.Instance{}
368-
369- err := env.StopInstances(instances)
370+ err := env.StopInstances()
371 c.Check(err, gc.IsNil)
372 }
373
374
375=== modified file 'provider/common/bootstrap.go'
376--- provider/common/bootstrap.go 2014-04-30 23:18:40 +0000
377+++ provider/common/bootstrap.go 2014-05-14 03:01:15 +0000
378@@ -128,7 +128,7 @@
379
380 if inst != nil {
381 fmt.Fprintln(ctx.GetStderr(), "Stopping instance...")
382- if stoperr := env.StopInstances([]instance.Instance{inst}); stoperr != nil {
383+ if stoperr := env.StopInstances(inst.Id()); stoperr != nil {
384 logger.Errorf("cannot stop failed bootstrap instance %q: %v", inst.Id(), stoperr)
385 } else {
386 // set to nil so we know we can safely delete the state file
387
388=== modified file 'provider/common/bootstrap_test.go'
389--- provider/common/bootstrap_test.go 2014-04-24 08:15:45 +0000
390+++ provider/common/bootstrap_test.go 2014-05-14 03:01:15 +0000
391@@ -117,9 +117,9 @@
392 return &mockInstance{id: "i-blah"}, nil, nil, nil
393 }
394
395- var stopped []instance.Instance
396- stopInstances := func(instances []instance.Instance) error {
397- stopped = append(stopped, instances...)
398+ var stopped []instance.Id
399+ stopInstances := func(ids []instance.Id) error {
400+ stopped = append(stopped, ids...)
401 return nil
402 }
403
404@@ -134,7 +134,7 @@
405 err := common.Bootstrap(ctx, env, environs.BootstrapParams{})
406 c.Assert(err, gc.ErrorMatches, "cannot save state: suddenly a wild blah")
407 c.Assert(stopped, gc.HasLen, 1)
408- c.Assert(stopped[0].Id(), gc.Equals, instance.Id("i-blah"))
409+ c.Assert(stopped[0], gc.Equals, instance.Id("i-blah"))
410 }
411
412 func (s *BootstrapSuite) TestCannotRecordThenCannotStop(c *gc.C) {
413@@ -150,8 +150,8 @@
414 return &mockInstance{id: "i-blah"}, nil, nil, nil
415 }
416
417- var stopped []instance.Instance
418- stopInstances := func(instances []instance.Instance) error {
419+ var stopped []instance.Id
420+ stopInstances := func(instances []instance.Id) error {
421 stopped = append(stopped, instances...)
422 return fmt.Errorf("bork bork borken")
423 }
424@@ -171,7 +171,7 @@
425 err := common.Bootstrap(ctx, env, environs.BootstrapParams{})
426 c.Assert(err, gc.ErrorMatches, "cannot save state: suddenly a wild blah")
427 c.Assert(stopped, gc.HasLen, 1)
428- c.Assert(stopped[0].Id(), gc.Equals, instance.Id("i-blah"))
429+ c.Assert(stopped[0], gc.Equals, instance.Id("i-blah"))
430 c.Assert(tw.Log, jc.LogMatches, []jc.SimpleMessage{{
431 loggo.ERROR, `cannot stop failed bootstrap instance "i-blah": bork bork borken`,
432 }})
433
434=== modified file 'provider/common/destroy.go'
435--- provider/common/destroy.go 2014-03-14 20:38:20 +0000
436+++ provider/common/destroy.go 2014-05-14 03:01:15 +0000
437@@ -5,6 +5,7 @@
438
439 import (
440 "launchpad.net/juju-core/environs"
441+ "launchpad.net/juju-core/instance"
442 )
443
444 // Destroy is a common implementation of the Destroy method defined on
445@@ -15,7 +16,11 @@
446 instances, err := env.AllInstances()
447 switch err {
448 case nil:
449- if err := env.StopInstances(instances); err != nil {
450+ ids := make([]instance.Id, len(instances))
451+ for i, inst := range instances {
452+ ids[i] = inst.Id()
453+ }
454+ if err := env.StopInstances(ids...); err != nil {
455 return err
456 }
457 fallthrough
458
459=== modified file 'provider/common/destroy_test.go'
460--- provider/common/destroy_test.go 2014-04-14 12:36:13 +0000
461+++ provider/common/destroy_test.go 2014-05-14 03:01:15 +0000
462@@ -41,10 +41,10 @@
463 &mockInstance{id: "another"},
464 }, nil
465 },
466- stopInstances: func(instances []instance.Instance) error {
467- c.Assert(instances, gc.HasLen, 2)
468- c.Assert(instances[0].Id(), gc.Equals, instance.Id("one"))
469- c.Assert(instances[1].Id(), gc.Equals, instance.Id("another"))
470+ stopInstances: func(ids []instance.Id) error {
471+ c.Assert(ids, gc.HasLen, 2)
472+ c.Assert(ids[0], gc.Equals, instance.Id("one"))
473+ c.Assert(ids[1], gc.Equals, instance.Id("another"))
474 return fmt.Errorf("nah")
475 },
476 }
477@@ -61,10 +61,10 @@
478 &mockInstance{id: "another"},
479 }, nil
480 },
481- stopInstances: func(instances []instance.Instance) error {
482- c.Assert(instances, gc.HasLen, 2)
483- c.Assert(instances[0].Id(), gc.Equals, instance.Id("one"))
484- c.Assert(instances[1].Id(), gc.Equals, instance.Id("another"))
485+ stopInstances: func(ids []instance.Id) error {
486+ c.Assert(ids, gc.HasLen, 2)
487+ c.Assert(ids[0], gc.Equals, instance.Id("one"))
488+ c.Assert(ids[1], gc.Equals, instance.Id("another"))
489 return nil
490 },
491 }
492@@ -84,9 +84,9 @@
493 &mockInstance{id: "one"},
494 }, nil
495 },
496- stopInstances: func(instances []instance.Instance) error {
497- c.Assert(instances, gc.HasLen, 1)
498- c.Assert(instances[0].Id(), gc.Equals, instance.Id("one"))
499+ stopInstances: func(ids []instance.Id) error {
500+ c.Assert(ids, gc.HasLen, 1)
501+ c.Assert(ids[0], gc.Equals, instance.Id("one"))
502 return nil
503 },
504 }
505
506=== modified file 'provider/common/mock_test.go'
507--- provider/common/mock_test.go 2014-04-24 02:27:38 +0000
508+++ provider/common/mock_test.go 2014-05-14 03:01:15 +0000
509@@ -19,7 +19,7 @@
510
511 type allInstancesFunc func() ([]instance.Instance, error)
512 type startInstanceFunc func(string, constraints.Value, []string, []string, tools.List, *cloudinit.MachineConfig) (instance.Instance, *instance.HardwareCharacteristics, []network.Info, error)
513-type stopInstancesFunc func([]instance.Instance) error
514+type stopInstancesFunc func([]instance.Id) error
515 type getToolsSourcesFunc func() ([]simplestreams.DataSource, error)
516 type configFunc func() *config.Config
517 type setConfigFunc func(*config.Config) error
518@@ -60,8 +60,8 @@
519 args.MachineConfig)
520 }
521
522-func (env *mockEnviron) StopInstances(instances []instance.Instance) error {
523- return env.stopInstances(instances)
524+func (env *mockEnviron) StopInstances(ids ...instance.Id) error {
525+ return env.stopInstances(ids)
526 }
527
528 func (env *mockEnviron) Config() *config.Config {
529
530=== modified file 'provider/dummy/environs.go'
531--- provider/dummy/environs.go 2014-05-06 16:08:51 +0000
532+++ provider/dummy/environs.go 2014-05-14 03:01:15 +0000
533@@ -130,8 +130,8 @@
534 }
535
536 type OpStopInstances struct {
537- Env string
538- Instances []instance.Instance
539+ Env string
540+ Ids []instance.Id
541 }
542
543 type OpOpenPorts struct {
544@@ -829,7 +829,7 @@
545 return i, hc, networkInfo, nil
546 }
547
548-func (e *environ) StopInstances(is []instance.Instance) error {
549+func (e *environ) StopInstances(ids ...instance.Id) error {
550 defer delay()
551 if err := e.checkBroken("StopInstance"); err != nil {
552 return err
553@@ -840,12 +840,12 @@
554 }
555 estate.mu.Lock()
556 defer estate.mu.Unlock()
557- for _, i := range is {
558- delete(estate.insts, i.(*dummyInstance).id)
559+ for _, id := range ids {
560+ delete(estate.insts, id)
561 }
562 estate.ops <- OpStopInstances{
563- Env: e.name,
564- Instances: is,
565+ Env: e.name,
566+ Ids: ids,
567 }
568 return nil
569 }
570
571=== modified file 'provider/ec2/ec2.go'
572--- provider/ec2/ec2.go 2014-05-06 16:08:51 +0000
573+++ provider/ec2/ec2.go 2014-05-14 03:01:15 +0000
574@@ -537,11 +537,7 @@
575 return inst, &hc, nil, nil
576 }
577
578-func (e *environ) StopInstances(insts []instance.Instance) error {
579- ids := make([]instance.Id, len(insts))
580- for i, inst := range insts {
581- ids[i] = inst.(*ec2Instance).Id()
582- }
583+func (e *environ) StopInstances(ids ...instance.Id) error {
584 return e.terminateInstances(ids)
585 }
586
587
588=== modified file 'provider/ec2/live_test.go'
589--- provider/ec2/live_test.go 2014-04-09 16:36:12 +0000
590+++ provider/ec2/live_test.go 2014-05-14 03:01:15 +0000
591@@ -116,7 +116,7 @@
592
593 func (t *LiveTests) TestInstanceAttributes(c *gc.C) {
594 inst, hc := testing.AssertStartInstance(c, t.Env, "30")
595- defer t.Env.StopInstances([]instance.Instance{inst})
596+ defer t.Env.StopInstances(inst.Id())
597 // Sanity check for hardware characteristics.
598 c.Assert(hc.Arch, gc.NotNil)
599 c.Assert(hc.Mem, gc.NotNil)
600@@ -140,7 +140,7 @@
601 func (t *LiveTests) TestStartInstanceConstraints(c *gc.C) {
602 cons := constraints.MustParse("mem=2G")
603 inst, hc := testing.AssertStartInstanceWithConstraints(c, t.Env, "30", cons)
604- defer t.Env.StopInstances([]instance.Instance{inst})
605+ defer t.Env.StopInstances(inst.Id())
606 ec2inst := ec2.InstanceEC2(inst)
607 c.Assert(ec2inst.InstanceType, gc.Equals, "m1.medium")
608 c.Assert(*hc.Arch, gc.Equals, "amd64")
609@@ -187,14 +187,14 @@
610 c.Assert(err, gc.IsNil)
611
612 inst0, _ := testing.AssertStartInstance(c, t.Env, "98")
613- defer t.Env.StopInstances([]instance.Instance{inst0})
614+ defer t.Env.StopInstances(inst0.Id())
615
616 // Create a same-named group for the second instance
617 // before starting it, to check that it's reused correctly.
618 oldMachineGroup := createGroup(c, ec2conn, groups[2].Name, "old machine group")
619
620 inst1, _ := testing.AssertStartInstance(c, t.Env, "99")
621- defer t.Env.StopInstances([]instance.Instance{inst1})
622+ defer t.Env.StopInstances(inst1.Id())
623
624 groupsResp, err := ec2conn.SecurityGroups(groups, nil)
625 c.Assert(err, gc.IsNil)
626@@ -343,7 +343,7 @@
627 inst1 := ec2.FabricateInstance(inst0, "i-aaaaaaaa")
628 inst2, _ := testing.AssertStartInstance(c, t.Env, "41")
629
630- err := t.Env.StopInstances([]instance.Instance{inst0, inst1, inst2})
631+ err := t.Env.StopInstances(inst0.Id(), inst1.Id(), inst2.Id())
632 c.Check(err, gc.IsNil)
633
634 var insts []instance.Instance
635
636=== modified file 'provider/joyent/environ_instance.go'
637--- provider/joyent/environ_instance.go 2014-05-06 16:08:51 +0000
638+++ provider/joyent/environ_instance.go 2014-05-14 03:01:15 +0000
639@@ -232,17 +232,17 @@
640 return instance.Address{}, errors.NotImplementedf("AllocateAddress")
641 }
642
643-func (env *joyentEnviron) StopInstances(instances []instance.Instance) error {
644+func (env *joyentEnviron) StopInstances(ids ...instance.Id) error {
645 // Remove all the instances in parallel so that we incur less round-trips.
646 var wg sync.WaitGroup
647 //var err error
648- wg.Add(len(instances))
649- errc := make(chan error, len(instances))
650- for _, inst := range instances {
651- inst := inst.(*joyentInstance)
652+ wg.Add(len(ids))
653+ errc := make(chan error, len(ids))
654+ for _, id := range ids {
655+ id := id // copy to new free var for closure
656 go func() {
657 defer wg.Done()
658- if err := inst.Stop(); err != nil {
659+ if err := env.stopInstance(string(id)); err != nil {
660 errc <- err
661 }
662 }()
663@@ -257,6 +257,39 @@
664 return nil
665 }
666
667+func (env *joyentEnviron) stopInstance(id string) error {
668+ // wait for machine to be running
669+ // if machine is still provisioning stop will fail
670+ for !env.pollMachineState(id, "running") {
671+ time.Sleep(1 * time.Second)
672+ }
673+
674+ err := env.compute.cloudapi.StopMachine(id)
675+ if err != nil {
676+ return fmt.Errorf("cannot stop instance %s: %v", id, err)
677+ }
678+
679+ // wait for machine to be stopped
680+ for !env.pollMachineState(id, "stopped") {
681+ time.Sleep(1 * time.Second)
682+ }
683+
684+ err = env.compute.cloudapi.DeleteMachine(id)
685+ if err != nil {
686+ return fmt.Errorf("cannot delete instance %s: %v", id, err)
687+ }
688+
689+ return nil
690+}
691+
692+func (env *joyentEnviron) pollMachineState(machineId, state string) bool {
693+ machineConfig, err := env.compute.cloudapi.GetMachine(machineId)
694+ if err != nil {
695+ return false
696+ }
697+ return strings.EqualFold(machineConfig.State, state)
698+}
699+
700 func (env *joyentEnviron) listInstanceTypes() ([]instances.InstanceType, error) {
701 packages, err := env.compute.cloudapi.ListPackages(nil)
702 if err != nil {
703
704=== modified file 'provider/joyent/instance.go'
705--- provider/joyent/instance.go 2014-04-02 11:35:49 +0000
706+++ provider/joyent/instance.go 2014-05-14 03:01:15 +0000
707@@ -4,10 +4,6 @@
708 package joyent
709
710 import (
711- "fmt"
712- "strings"
713- "time"
714-
715 "github.com/joyent/gosdc/cloudapi"
716
717 "launchpad.net/juju-core/instance"
718@@ -63,40 +59,3 @@
719 func (inst *joyentInstance) WaitDNSName() (string, error) {
720 return common.WaitDNSName(inst)
721 }
722-
723-// Stop will stop and delete the machine
724-// Stopped machines are still billed for in the Joyent Public Cloud
725-func (inst *joyentInstance) Stop() error {
726- id := string(inst.Id())
727-
728- // wait for machine to be running
729- // if machine is still provisioning stop will fail
730- for !inst.pollMachineState(id, "running") {
731- time.Sleep(1 * time.Second)
732- }
733-
734- err := inst.env.compute.cloudapi.StopMachine(id)
735- if err != nil {
736- return fmt.Errorf("cannot stop instance %s: %v", id, err)
737- }
738-
739- // wait for machine to be stopped
740- for !inst.pollMachineState(id, "stopped") {
741- time.Sleep(1 * time.Second)
742- }
743-
744- err = inst.env.compute.cloudapi.DeleteMachine(id)
745- if err != nil {
746- return fmt.Errorf("cannot delete instance %s: %v", id, err)
747- }
748-
749- return nil
750-}
751-
752-func (inst *joyentInstance) pollMachineState(machineId, state string) bool {
753- machineConfig, err := inst.env.compute.cloudapi.GetMachine(machineId)
754- if err != nil {
755- return false
756- }
757- return strings.EqualFold(machineConfig.State, state)
758-}
759
760=== modified file 'provider/joyent/local_test.go'
761--- provider/joyent/local_test.go 2014-04-24 12:33:19 +0000
762+++ provider/joyent/local_test.go 2014-05-14 03:01:15 +0000
763@@ -193,7 +193,7 @@
764 err := bootstrap.Bootstrap(bootstrapContext(c), env, environs.BootstrapParams{})
765 c.Assert(err, gc.IsNil)
766 inst, _ := testing.AssertStartInstance(c, env, "100")
767- err = env.StopInstances([]instance.Instance{inst})
768+ err = env.StopInstances(inst.Id())
769 c.Assert(err, gc.IsNil)
770 }
771
772@@ -261,7 +261,7 @@
773 envtesting.UploadFakeTools(c, env.Storage())
774 inst, _ := testing.AssertStartInstance(c, env, "100")
775 c.Assert(inst.Status(), gc.Equals, "running")
776- err := env.StopInstances([]instance.Instance{inst})
777+ err := env.StopInstances(inst.Id())
778 c.Assert(err, gc.IsNil)
779 }
780
781@@ -274,7 +274,7 @@
782 id1 := inst1.Id()
783 c.Logf("id0: %s, id1: %s", id0, id1)
784 defer func() {
785- err := env.StopInstances([]instance.Instance{inst0, inst1})
786+ err := env.StopInstances(inst0.Id(), inst1.Id())
787 c.Assert(err, gc.IsNil)
788 }()
789
790
791=== modified file 'provider/local/environ.go'
792--- provider/local/environ.go 2014-05-06 16:08:51 +0000
793+++ provider/local/environ.go 2014-05-14 03:01:15 +0000
794@@ -354,12 +354,12 @@
795 }
796
797 // StopInstances is specified in the InstanceBroker interface.
798-func (env *localEnviron) StopInstances(instances []instance.Instance) error {
799- for _, inst := range instances {
800- if inst.Id() == bootstrapInstanceId {
801+func (env *localEnviron) StopInstances(ids ...instance.Id) error {
802+ for _, id := range ids {
803+ if id == bootstrapInstanceId {
804 return fmt.Errorf("cannot stop the bootstrap instance")
805 }
806- if err := env.containerManager.DestroyContainer(inst); err != nil {
807+ if err := env.containerManager.DestroyContainer(id); err != nil {
808 return err
809 }
810 }
811@@ -462,7 +462,7 @@
812 return err
813 }
814 for _, inst := range containers {
815- if err := env.containerManager.DestroyContainer(inst); err != nil {
816+ if err := env.containerManager.DestroyContainer(inst.Id()); err != nil {
817 return err
818 }
819 }
820
821=== modified file 'provider/maas/environ.go'
822--- provider/maas/environ.go 2014-05-07 19:09:23 +0000
823+++ provider/maas/environ.go 2014-05-14 03:01:15 +0000
824@@ -434,7 +434,7 @@
825 }
826 defer func() {
827 if err != nil {
828- if err := environ.releaseInstance(inst); err != nil {
829+ if err := environ.StopInstances(inst.Id()); err != nil {
830 logger.Errorf("error releasing failed instance: %v", err)
831 }
832 }
833@@ -546,32 +546,18 @@
834 }
835
836 // StopInstances is specified in the InstanceBroker interface.
837-func (environ *maasEnviron) StopInstances(instances []instance.Instance) error {
838+func (environ *maasEnviron) StopInstances(ids ...instance.Id) error {
839 // Shortcut to exit quickly if 'instances' is an empty slice or nil.
840- if len(instances) == 0 {
841+ if len(ids) == 0 {
842 return nil
843 }
844- // Tell MAAS to release each of the instances. If there are errors,
845- // return only the first one (but release all instances regardless).
846- // Note that releasing instances also turns them off.
847- var firstErr error
848- for _, instance := range instances {
849- err := environ.releaseInstance(instance)
850- if firstErr == nil {
851- firstErr = err
852- }
853- }
854- return firstErr
855-}
856-
857-// releaseInstance releases a single instance.
858-func (environ *maasEnviron) releaseInstance(inst instance.Instance) error {
859- maasInst := inst.(*maasInstance)
860- maasObj := maasInst.maasObject
861- _, err := maasObj.CallPost("release", nil)
862- if err != nil {
863- logger.Debugf("error releasing instance %v", maasInst)
864- }
865+ // TODO(axw) 2014-05-13 #1319016
866+ // Nodes that have been removed out of band will cause
867+ // the release call to fail. We should parse the error
868+ // returned from MAAS and retry, or otherwise request
869+ // an enhancement to MAAS to ignore unknown node IDs.
870+ nodes := environ.getMAASClient().GetSubObject("nodes")
871+ _, err := nodes.CallPost("release", getSystemIdValues("nodes", ids))
872 return err
873 }
874
875@@ -580,7 +566,7 @@
876 // "ids" matches all instances (not none as you might expect).
877 func (environ *maasEnviron) instances(ids []instance.Id) ([]instance.Instance, error) {
878 nodeListing := environ.getMAASClient().GetSubObject("nodes")
879- filter := getSystemIdValues(ids)
880+ filter := getSystemIdValues("id", ids)
881 filter.Add("agent_name", environ.ecfg().maasAgentName())
882 listNodeObjects, err := nodeListing.CallGet("list", filter)
883 if err != nil {
884
885=== modified file 'provider/maas/environ_whitebox_test.go'
886--- provider/maas/environ_whitebox_test.go 2014-04-24 12:33:19 +0000
887+++ provider/maas/environ_whitebox_test.go 2014-05-14 03:01:15 +0000
888@@ -420,24 +420,27 @@
889 func (suite *environSuite) TestStopInstancesReturnsIfParameterEmpty(c *gc.C) {
890 suite.getInstance("test1")
891
892- err := suite.makeEnviron().StopInstances([]instance.Instance{})
893+ err := suite.makeEnviron().StopInstances()
894 c.Check(err, gc.IsNil)
895 operations := suite.testMAASObject.TestServer.NodeOperations()
896 c.Check(operations, gc.DeepEquals, map[string][]string{})
897 }
898
899 func (suite *environSuite) TestStopInstancesStopsAndReleasesInstances(c *gc.C) {
900- instance1 := suite.getInstance("test1")
901- instance2 := suite.getInstance("test2")
902+ suite.getInstance("test1")
903+ suite.getInstance("test2")
904 suite.getInstance("test3")
905- instances := []instance.Instance{instance1, instance2}
906-
907- err := suite.makeEnviron().StopInstances(instances)
908-
909+ // mark test1 and test2 as being allocated, but not test3.
910+ // The release operation will ignore test3.
911+ suite.testMAASObject.TestServer.OwnedNodes()["test1"] = true
912+ suite.testMAASObject.TestServer.OwnedNodes()["test2"] = true
913+
914+ err := suite.makeEnviron().StopInstances("test1", "test2", "test3")
915 c.Check(err, gc.IsNil)
916- operations := suite.testMAASObject.TestServer.NodeOperations()
917- expectedOperations := map[string][]string{"test1": {"release"}, "test2": {"release"}}
918- c.Check(operations, gc.DeepEquals, expectedOperations)
919+ operations := suite.testMAASObject.TestServer.NodesOperations()
920+ c.Check(operations, gc.DeepEquals, []string{"release"})
921+ c.Assert(suite.testMAASObject.TestServer.OwnedNodes()["test1"], jc.IsFalse)
922+ c.Assert(suite.testMAASObject.TestServer.OwnedNodes()["test2"], jc.IsFalse)
923 }
924
925 func (suite *environSuite) TestStateInfo(c *gc.C) {
926@@ -472,6 +475,7 @@
927 func (suite *environSuite) TestDestroy(c *gc.C) {
928 env := suite.makeEnviron()
929 suite.getInstance("test1")
930+ suite.testMAASObject.TestServer.OwnedNodes()["test1"] = true // simulate acquire
931 data := makeRandomBytes(10)
932 suite.testMAASObject.TestServer.NewFile("filename", data)
933 stor := env.Storage()
934@@ -480,9 +484,9 @@
935 c.Check(err, gc.IsNil)
936
937 // Instances have been stopped.
938- operations := suite.testMAASObject.TestServer.NodeOperations()
939- expectedOperations := map[string][]string{"test1": {"release"}}
940- c.Check(operations, gc.DeepEquals, expectedOperations)
941+ operations := suite.testMAASObject.TestServer.NodesOperations()
942+ c.Check(operations, gc.DeepEquals, []string{"release"})
943+ c.Check(suite.testMAASObject.TestServer.OwnedNodes()["test1"], jc.IsFalse)
944 // Files have been cleaned up.
945 listing, err := storage.List(stor, "")
946 c.Assert(err, gc.IsNil)
947
948=== modified file 'provider/maas/util.go'
949--- provider/maas/util.go 2013-08-02 05:13:41 +0000
950+++ provider/maas/util.go 2014-05-14 03:01:15 +0000
951@@ -24,12 +24,12 @@
952 }
953
954 // getSystemIdValues returns a url.Values object with all the 'system_ids'
955-// from the given instanceIds stored under the key 'id'. This is used
956+// from the given instanceIds stored under the specified key. This is used
957 // to filter out instances when listing the nodes objects.
958-func getSystemIdValues(instanceIds []instance.Id) url.Values {
959+func getSystemIdValues(key string, instanceIds []instance.Id) url.Values {
960 values := url.Values{}
961 for _, instanceId := range instanceIds {
962- values.Add("id", extractSystemId(instanceId))
963+ values.Add(key, extractSystemId(instanceId))
964 }
965 return values
966 }
967
968=== modified file 'provider/maas/util_test.go'
969--- provider/maas/util_test.go 2013-08-09 04:58:34 +0000
970+++ provider/maas/util_test.go 2014-05-14 03:01:15 +0000
971@@ -30,7 +30,7 @@
972 instanceId2 := instance.Id("/MAAS/api/1.0/nodes/system_id2/")
973 instanceIds := []instance.Id{instanceId1, instanceId2}
974
975- values := getSystemIdValues(instanceIds)
976+ values := getSystemIdValues("id", instanceIds)
977
978 c.Check(values["id"], gc.DeepEquals, []string{"system_id1", "system_id2"})
979 }
980
981=== modified file 'provider/manual/environ.go'
982--- provider/manual/environ.go 2014-05-06 16:08:51 +0000
983+++ provider/manual/environ.go 2014-05-14 03:01:15 +0000
984@@ -70,7 +70,7 @@
985 return nil, nil, nil, errNoStartInstance
986 }
987
988-func (*manualEnviron) StopInstances([]instance.Instance) error {
989+func (*manualEnviron) StopInstances(...instance.Id) error {
990 return errNoStopInstance
991 }
992
993
994=== modified file 'provider/openstack/local_test.go'
995--- provider/openstack/local_test.go 2014-04-24 12:33:19 +0000
996+++ provider/openstack/local_test.go 2014-05-14 03:01:15 +0000
997@@ -264,7 +264,7 @@
998 err = bootstrap.Bootstrap(coretesting.Context(c), env, environs.BootstrapParams{})
999 c.Assert(err, gc.IsNil)
1000 inst, _ := testing.AssertStartInstance(c, env, "100")
1001- err = env.StopInstances([]instance.Instance{inst})
1002+ err = env.StopInstances(inst.Id())
1003 c.Assert(err, gc.IsNil)
1004 }
1005
1006@@ -288,7 +288,7 @@
1007 env, err := environs.New(cfg)
1008 c.Assert(err, gc.IsNil)
1009 inst, _ := testing.AssertStartInstance(c, env, "100")
1010- err = env.StopInstances([]instance.Instance{inst})
1011+ err = env.StopInstances(inst.Id())
1012 c.Assert(err, gc.IsNil)
1013 }
1014
1015@@ -363,7 +363,7 @@
1016 // group, one group for the entire environment, and another for the
1017 // new instance.
1018 assertSecurityGroups(c, env, []string{"default", fmt.Sprintf("juju-%v", env.Name()), fmt.Sprintf("juju-%v-%v", env.Name(), instanceName)})
1019- err = env.StopInstances([]instance.Instance{inst})
1020+ err = env.StopInstances(inst.Id())
1021 c.Assert(err, gc.IsNil)
1022 // The security group for this instance is now removed.
1023 assertSecurityGroups(c, env, []string{"default", fmt.Sprintf("juju-%v", env.Name())})
1024@@ -391,7 +391,7 @@
1025 inst, _ := testing.AssertStartInstance(c, env, instanceName)
1026 allSecurityGroups := []string{"default", fmt.Sprintf("juju-%v", env.Name()), fmt.Sprintf("juju-%v-%v", env.Name(), instanceName)}
1027 assertSecurityGroups(c, env, allSecurityGroups)
1028- err = env.StopInstances([]instance.Instance{inst})
1029+ err = env.StopInstances(inst.Id())
1030 c.Assert(err, gc.IsNil)
1031 assertSecurityGroups(c, env, allSecurityGroups)
1032 }
1033@@ -478,7 +478,7 @@
1034 // goose's test service always returns ACTIVE state.
1035 inst, _ := testing.AssertStartInstance(c, env, "100")
1036 c.Assert(inst.Status(), gc.Equals, nova.StatusActive)
1037- err := env.StopInstances([]instance.Instance{inst})
1038+ err := env.StopInstances(inst.Id())
1039 c.Assert(err, gc.IsNil)
1040 }
1041
1042@@ -489,7 +489,7 @@
1043 inst1, _ := testing.AssertStartInstance(c, env, "101")
1044 id1 := inst1.Id()
1045 defer func() {
1046- err := env.StopInstances([]instance.Instance{inst0, inst1})
1047+ err := env.StopInstances(inst0.Id(), inst1.Id())
1048 c.Assert(err, gc.IsNil)
1049 }()
1050
1051@@ -534,7 +534,7 @@
1052 defer cleanup()
1053 stateInst, _ := testing.AssertStartInstance(c, env, "100")
1054 defer func() {
1055- err := env.StopInstances([]instance.Instance{stateInst})
1056+ err := env.StopInstances(stateInst.Id())
1057 c.Assert(err, gc.IsNil)
1058 }()
1059 found := make(map[instance.Id]instance.Instance)
1060@@ -559,7 +559,7 @@
1061 defer cleanup()
1062 stateInst, _ := testing.AssertStartInstance(c, env, "100")
1063 defer func() {
1064- err := env.StopInstances([]instance.Instance{stateInst})
1065+ err := env.StopInstances(stateInst.Id())
1066 c.Assert(err, gc.IsNil)
1067 }()
1068
1069
1070=== modified file 'provider/openstack/provider.go'
1071--- provider/openstack/provider.go 2014-05-06 16:08:51 +0000
1072+++ provider/openstack/provider.go 2014-05-14 03:01:15 +0000
1073@@ -6,7 +6,6 @@
1074 package openstack
1075
1076 import (
1077- "errors"
1078 "fmt"
1079 "io/ioutil"
1080 "net/http"
1081@@ -902,28 +901,33 @@
1082 return inst, inst.hardwareCharacteristics(), nil, nil
1083 }
1084
1085-func (e *environ) StopInstances(insts []instance.Instance) error {
1086- ids := make([]instance.Id, len(insts))
1087- securityGroupNames := make([]string, len(insts))
1088- for i, inst := range insts {
1089- instanceValue, ok := inst.(*openstackInstance)
1090- if !ok {
1091- return errors.New("Incompatible instance.Instance supplied")
1092- }
1093- ids[i] = instanceValue.Id()
1094- openstackName := instanceValue.getServerDetail().Name
1095- lastDashPos := strings.LastIndex(openstackName, "-")
1096- if lastDashPos == -1 {
1097- return fmt.Errorf("cannot identify instance ID in openstack server name %q", openstackName)
1098- }
1099- securityGroupNames[i] = e.machineGroupName(openstackName[lastDashPos+1:])
1100+func (e *environ) StopInstances(ids ...instance.Id) error {
1101+ // If in instance firewall mode, gather the security group names.
1102+ var securityGroupNames []string
1103+ if e.Config().FirewallMode() == config.FwInstance {
1104+ instances, err := e.Instances(ids)
1105+ if err == environs.ErrNoInstances {
1106+ return nil
1107+ }
1108+ securityGroupNames = make([]string, 0, len(ids))
1109+ for _, inst := range instances {
1110+ if inst == nil {
1111+ continue
1112+ }
1113+ openstackName := inst.(*openstackInstance).getServerDetail().Name
1114+ lastDashPos := strings.LastIndex(openstackName, "-")
1115+ if lastDashPos == -1 {
1116+ return fmt.Errorf("cannot identify machine ID in openstack server name %q", openstackName)
1117+ }
1118+ securityGroupName := e.machineGroupName(openstackName[lastDashPos+1:])
1119+ securityGroupNames = append(securityGroupNames, securityGroupName)
1120+ }
1121 }
1122 logger.Debugf("terminating instances %v", ids)
1123- err := e.terminateInstances(ids)
1124- if err != nil {
1125+ if err := e.terminateInstances(ids); err != nil {
1126 return err
1127 }
1128- if e.Config().FirewallMode() == config.FwInstance {
1129+ if securityGroupNames != nil {
1130 return e.deleteSecurityGroups(securityGroupNames)
1131 }
1132 return nil
1133
1134=== modified file 'state/apiserver/client/destroy.go'
1135--- state/apiserver/client/destroy.go 2014-01-17 16:41:32 +0000
1136+++ state/apiserver/client/destroy.go 2014-05-14 03:01:15 +0000
1137@@ -104,28 +104,7 @@
1138 if err != nil {
1139 return err
1140 }
1141- // TODO(axw) 2013-12-12 #1260171
1142- // Modify InstanceBroker.StopInstances to take
1143- // a slice of IDs rather than Instances.
1144- instances, err := env.Instances(ids)
1145- switch err {
1146- case nil:
1147- default:
1148- return err
1149- case environs.ErrNoInstances:
1150- return nil
1151- case environs.ErrPartialInstances:
1152- var nonNilInstances []instance.Instance
1153- for i, inst := range instances {
1154- if inst == nil {
1155- logger.Warningf("unknown instance ID: %v", ids[i])
1156- continue
1157- }
1158- nonNilInstances = append(nonNilInstances, inst)
1159- }
1160- instances = nonNilInstances
1161- }
1162- return env.StopInstances(instances)
1163+ return env.StopInstances(ids...)
1164 }
1165
1166 // checkManualMachines checks if any of the machines in the slice were
1167
1168=== modified file 'worker/provisioner/kvm-broker.go'
1169--- worker/provisioner/kvm-broker.go 2014-05-09 13:24:50 +0000
1170+++ worker/provisioner/kvm-broker.go 2014-05-14 03:01:15 +0000
1171@@ -105,11 +105,11 @@
1172 }
1173
1174 // StopInstances shuts down the given instances.
1175-func (broker *kvmBroker) StopInstances(instances []instance.Instance) error {
1176+func (broker *kvmBroker) StopInstances(ids ...instance.Id) error {
1177 // TODO: potentially parallelise.
1178- for _, instance := range instances {
1179- kvmLogger.Infof("stopping kvm container for instance: %s", instance.Id())
1180- if err := broker.manager.DestroyContainer(instance); err != nil {
1181+ for _, id := range ids {
1182+ kvmLogger.Infof("stopping kvm container for instance: %s", id)
1183+ if err := broker.manager.DestroyContainer(id); err != nil {
1184 kvmLogger.Errorf("container did not stop: %v", err)
1185 return err
1186 }
1187
1188=== modified file 'worker/provisioner/kvm-broker_test.go'
1189--- worker/provisioner/kvm-broker_test.go 2014-05-09 13:24:50 +0000
1190+++ worker/provisioner/kvm-broker_test.go 2014-05-14 03:01:15 +0000
1191@@ -102,13 +102,13 @@
1192 kvm1 := s.startInstance(c, "1/kvm/1")
1193 kvm2 := s.startInstance(c, "1/kvm/2")
1194
1195- err := s.broker.StopInstances([]instance.Instance{kvm0})
1196+ err := s.broker.StopInstances(kvm0.Id())
1197 c.Assert(err, gc.IsNil)
1198 s.assertInstances(c, kvm1, kvm2)
1199 c.Assert(s.kvmContainerDir(kvm0), jc.DoesNotExist)
1200 c.Assert(s.kvmRemovedContainerDir(kvm0), jc.IsDirectory)
1201
1202- err = s.broker.StopInstances([]instance.Instance{kvm1, kvm2})
1203+ err = s.broker.StopInstances(kvm1.Id(), kvm2.Id())
1204 c.Assert(err, gc.IsNil)
1205 s.assertInstances(c)
1206 }
1207@@ -118,7 +118,7 @@
1208 kvm1 := s.startInstance(c, "1/kvm/1")
1209 s.assertInstances(c, kvm0, kvm1)
1210
1211- err := s.broker.StopInstances([]instance.Instance{kvm1})
1212+ err := s.broker.StopInstances(kvm1.Id())
1213 c.Assert(err, gc.IsNil)
1214 kvm2 := s.startInstance(c, "1/kvm/2")
1215 s.assertInstances(c, kvm0, kvm2)
1216
1217=== modified file 'worker/provisioner/lxc-broker.go'
1218--- worker/provisioner/lxc-broker.go 2014-05-09 13:24:50 +0000
1219+++ worker/provisioner/lxc-broker.go 2014-05-14 03:01:15 +0000
1220@@ -102,11 +102,11 @@
1221 }
1222
1223 // StopInstances shuts down the given instances.
1224-func (broker *lxcBroker) StopInstances(instances []instance.Instance) error {
1225+func (broker *lxcBroker) StopInstances(ids ...instance.Id) error {
1226 // TODO: potentially parallelise.
1227- for _, instance := range instances {
1228- lxcLogger.Infof("stopping lxc container for instance: %s", instance.Id())
1229- if err := broker.manager.DestroyContainer(instance); err != nil {
1230+ for _, id := range ids {
1231+ lxcLogger.Infof("stopping lxc container for instance: %s", id)
1232+ if err := broker.manager.DestroyContainer(id); err != nil {
1233 lxcLogger.Errorf("container did not stop: %v", err)
1234 return err
1235 }
1236
1237=== modified file 'worker/provisioner/lxc-broker_test.go'
1238--- worker/provisioner/lxc-broker_test.go 2014-05-09 13:24:50 +0000
1239+++ worker/provisioner/lxc-broker_test.go 2014-05-14 03:01:15 +0000
1240@@ -131,13 +131,13 @@
1241 lxc1 := s.startInstance(c, "1/lxc/1")
1242 lxc2 := s.startInstance(c, "1/lxc/2")
1243
1244- err := s.broker.StopInstances([]instance.Instance{lxc0})
1245+ err := s.broker.StopInstances(lxc0.Id())
1246 c.Assert(err, gc.IsNil)
1247 s.assertInstances(c, lxc1, lxc2)
1248 c.Assert(s.lxcContainerDir(lxc0), jc.DoesNotExist)
1249 c.Assert(s.lxcRemovedContainerDir(lxc0), jc.IsDirectory)
1250
1251- err = s.broker.StopInstances([]instance.Instance{lxc1, lxc2})
1252+ err = s.broker.StopInstances(lxc1.Id(), lxc2.Id())
1253 c.Assert(err, gc.IsNil)
1254 s.assertInstances(c)
1255 }
1256@@ -147,7 +147,7 @@
1257 lxc1 := s.startInstance(c, "1/lxc/1")
1258 s.assertInstances(c, lxc0, lxc1)
1259
1260- err := s.broker.StopInstances([]instance.Instance{lxc1})
1261+ err := s.broker.StopInstances(lxc1.Id())
1262 c.Assert(err, gc.IsNil)
1263 lxc2 := s.startInstance(c, "1/lxc/2")
1264 s.assertInstances(c, lxc0, lxc2)
1265
1266=== modified file 'worker/provisioner/provisioner_task.go'
1267--- worker/provisioner/provisioner_task.go 2014-05-08 06:58:42 +0000
1268+++ worker/provisioner/provisioner_task.go 2014-05-14 03:01:15 +0000
1269@@ -397,7 +397,11 @@
1270 if len(instances) == 0 {
1271 return nil
1272 }
1273- if err := task.broker.StopInstances(instances); err != nil {
1274+ ids := make([]instance.Id, len(instances))
1275+ for i, inst := range instances {
1276+ ids[i] = inst.Id()
1277+ }
1278+ if err := task.broker.StopInstances(ids...); err != nil {
1279 logger.Errorf("broker failed to stop instances: %v", err)
1280 return err
1281 }
1282@@ -484,7 +488,7 @@
1283 }
1284 // We need to stop the instance right away here, set error status and go on.
1285 task.setErrorStatus("cannot register instance for machine %v: %v", machine, err)
1286- if err := task.broker.StopInstances([]instance.Instance{inst}); err != nil {
1287+ if err := task.broker.StopInstances(inst.Id()); err != nil {
1288 // We cannot even stop the instance, log the error and quit.
1289 logger.Errorf("cannot stop instance %q for machine %v: %v", inst.Id(), machine, err)
1290 return err
1291
1292=== modified file 'worker/provisioner/provisioner_test.go'
1293--- worker/provisioner/provisioner_test.go 2014-04-30 23:18:40 +0000
1294+++ worker/provisioner/provisioner_test.go 2014-05-14 03:01:15 +0000
1295@@ -254,8 +254,8 @@
1296 case o := <-s.op:
1297 switch o := o.(type) {
1298 case dummy.OpStopInstances:
1299- for _, stoppedInstance := range o.Instances {
1300- instId := string(stoppedInstance.Id())
1301+ for _, id := range o.Ids {
1302+ instId := string(id)
1303 instanceIdsToStop.Remove(instId)
1304 if instanceIdsToKeep.Contains(instId) {
1305 c.Errorf("provisioner unexpectedly stopped instance %s", instId)

Subscribers

People subscribed via source and target branches

to status/vote changes: