Merge lp:~rvb/gwacl/fix-destroydeploy2 into lp:gwacl

Proposed by Raphaël Badin on 2013-07-04
Status: Merged
Approved by: Raphaël Badin on 2013-07-05
Approved revision: 166
Merged at revision: 163
Proposed branch: lp:~rvb/gwacl/fix-destroydeploy2
Merge into: lp:gwacl
Diff against target: 257 lines (+33/-102)
3 files modified
example/management/run.go (+4/-18)
management.go (+9/-19)
management_test.go (+20/-65)
To merge this branch: bzr merge lp:~rvb/gwacl/fix-destroydeploy2
Reviewer Review Type Date Requested Status
Gavin Panella 2013-07-04 Approve on 2013-07-04
Review via email: mp+173095@code.launchpad.net

Commit message

Do not attempt to stop the VMs inside DestroyDeployment.

Description of the change

I found that it's quicker a more reliable to just delete the deployment instead of trying to shutdown the VMs when destroying a deployment. When we delete the deployment object, the VMs are shut down are removed. The only thing left are the disks.

Added bonus: the testing becomes simpler as we issue less API requests.

To post a comment you must log in.
Gavin Panella (allenap) wrote :

I surprised this works, but looks good.

review: Approve
Julian Edwards (julian-edwards) wrote :

On Thursday 04 Jul 2013 22:17:24 you wrote:
> Review: Approve
>
> I surprised this works, but looks good.

Me too, it doesn't work in the UI!

Raphaël Badin (rvb) wrote :

> On Thursday 04 Jul 2013 22:17:24 you wrote:
> > Review: Approve
> >
> > I surprised this works, but looks good.

Surprisingly, it seems to be very reliable and the only long operation is removing the disk(s) (takes ~10 minutes for one disk). Can you confirm that it works fine for you too?

> Me too, it doesn't work in the UI!

Hum, AFAIK deployments are not exposed as first-class objects in the UI so how can you say that it does not work in the UI?

In the UI, when you delete a cloud service with a deployment in it, you get a popup to ask you if you want to delete the deployment as well. That's how I got the idea for this branch (this uses the API to mimic this behavior — plus the removal of the disks).

Also, the doc page for "delete hosted service" mentions that you have to delete the deployments inside the service before you can get rid of the service (http://msdn.microsoft.com/en-us/library/windowsazure/gg441305.aspx) but the doc page for "delete deployment" doesn't say you have to delete the VMs inside it (http://msdn.microsoft.com/en-us/library/windowsazure/ee460815.aspx).

Julian Edwards (julian-edwards) wrote :

On Friday 05 Jul 2013 05:57:25 you wrote:
> > On Thursday 04 Jul 2013 22:17:24 you wrote:
> > > Review: Approve
> > >
> > > I surprised this works, but looks good.
>
> Surprisingly, it seems to be very reliable and the only long operation is
> removing the disk(s) (takes ~10 minutes for one disk). Can you confirm
> that it works fine for you too?
> > Me too, it doesn't work in the UI!
>
> Hum, AFAIK deployments are not exposed as first-class objects in the UI so
> how can you say that it does not work in the UI?

I'm talking about deleting the service.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'example/management/run.go'
2--- example/management/run.go 2013-07-02 13:53:21 +0000
3+++ example/management/run.go 2013-07-04 20:25:31 +0000
4@@ -172,17 +172,10 @@
5 fmt.Println("Done adding VM deployment\n")
6
7 defer func() {
8- fmt.Println("Deleting VM disk...")
9- err = api.DeleteDisk(diskName)
10- checkError(err)
11- fmt.Println("Done deleting VM disk\n")
12- }()
13-
14- defer func() {
15- fmt.Println("Deleting deployment...")
16- err = api.DeleteDeployment(hostServiceName, deployment.Name)
17- checkError(err)
18- fmt.Println("Done deleting deployment\n")
19+ fmt.Printf("Destroying deployment %s...\n", machineName)
20+ requestDestroyDeployment := &gwacl.DestroyDeploymentRequest{ServiceName: hostServiceName, DeploymentName: machineName}
21+ api.DestroyDeployment(requestDestroyDeployment)
22+ fmt.Println("Done destroying deployment\n")
23 }()
24
25 fmt.Println("Starting VM...")
26@@ -190,13 +183,6 @@
27 checkError(err)
28 fmt.Println("Done starting VM\n")
29
30- defer func() {
31- fmt.Println("Stopping VM...")
32- err = api.ShutdownRole(&gwacl.ShutdownRoleRequest{hostServiceName, deployment.Name, role.RoleName})
33- checkError(err)
34- fmt.Println("Done stopping VM\n")
35- }()
36-
37 fmt.Println("Listing VM...")
38 instances, err := api.ListInstances(&gwacl.ListInstancesRequest{hostServiceName})
39 checkError(err)
40
41=== modified file 'management.go'
42--- management.go 2013-07-04 09:53:30 +0000
43+++ management.go 2013-07-04 20:25:31 +0000
44@@ -102,7 +102,7 @@
45 }
46
47 // DestroyDeployment brings down all resources within a deployment - running
48-// instances, disks, etc. - then deletes the deployment itself.
49+// instances, disks, etc. - and deletes the deployment itself.
50 func (api *ManagementAPI) DestroyDeployment(request *DestroyDeploymentRequest) error {
51 deployment, err := api.GetDeployment(&GetDeploymentRequest{
52 ServiceName: request.ServiceName,
53@@ -114,42 +114,32 @@
54 }
55 return err
56 }
57- // 1. Stop VMs.
58- for _, roleInstance := range deployment.RoleInstanceList {
59- err = api.ShutdownRole(&ShutdownRoleRequest{
60- ServiceName: request.ServiceName,
61- DeploymentName: request.DeploymentName,
62- RoleName: roleInstance.RoleName,
63- })
64- if err != nil && !IsNotFoundError(err) {
65- return err
66- }
67- }
68- // 2. Delete VM disks.
69+ // 1. Get the list of the VM disks.
70 diskNameMap := make(map[string]bool)
71 for _, role := range deployment.RoleList {
72 for _, osVHD := range role.OSVirtualHardDisk {
73 diskNameMap[osVHD.DiskName] = true
74 }
75 }
76+ // 2. Delete deployment. This will delete all the role instances inside
77+ // this deployment as a side effect.
78+ err = api.DeleteDeployment(request.ServiceName, request.DeploymentName)
79+ if err != nil && !IsNotFoundError(err) {
80+ return err
81+ }
82 // Sort the disk names to aid testing.
83 diskNames := []string{}
84 for diskName := range diskNameMap {
85 diskNames = append(diskNames, diskName)
86 }
87 sort.Strings(diskNames)
88- // Actually delete them.
89+ // 3. Delete the disks.
90 for _, diskName := range diskNames {
91 err = api.DeleteDisk(diskName)
92 if err != nil && !IsNotFoundError(err) {
93 return err
94 }
95 }
96- // 3. Delete deployment.
97- err = api.DeleteDeployment(request.ServiceName, request.DeploymentName)
98- if err != nil && !IsNotFoundError(err) {
99- return err
100- }
101 // Done.
102 return nil
103 }
104
105=== modified file 'management_test.go'
106--- management_test.go 2013-07-04 10:53:40 +0000
107+++ management_test.go 2013-07-04 20:25:31 +0000
108@@ -250,12 +250,10 @@
109 var responses []DispatcherResponse
110 // Prepare.
111 responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
112- // For shutting down VMs.
113- responses = append(responses, exampleOkayResponse, exampleOkayResponse)
114- // For deleting disks.
115- responses = append(responses, exampleOkayResponse, exampleOkayResponse, exampleOkayResponse)
116- // For other requests.
117+ // For deleting the deployment.
118 responses = append(responses, exampleOkayResponse)
119+ // For deleting disks.
120+ responses = append(responses, exampleOkayResponse, exampleOkayResponse, exampleOkayResponse)
121 record := []*X509Request{}
122 rigRecordingPreparedResponseDispatcher(&record, responses)
123 // Exercise DestroyDeployment.
124@@ -266,23 +264,14 @@
125 }
126 err := api.DestroyDeployment(request)
127 c.Assert(err, IsNil)
128- c.Check(record, HasLen, 7)
129- // The first request is for the deployment.
130+ c.Check(record, HasLen, 5)
131 assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
132 request.ServiceName, request.DeploymentName}, record[0])
133- // The second is to shutdown RoleInstance "one".
134- assertShutdownRoleRequest(c, api, &ShutdownRoleRequest{
135- request.ServiceName, request.DeploymentName, "one"}, record[1])
136- // The third is to shutdown RoleInstance "two", similar to like the second.
137- assertShutdownRoleRequest(c, api, &ShutdownRoleRequest{
138- request.ServiceName, request.DeploymentName, "two"}, record[2])
139- // The fourth, fifth and sixth requests delete disks.
140- assertDeleteDiskRequest(c, api, "disk1", record[3])
141- assertDeleteDiskRequest(c, api, "disk2", record[4])
142- assertDeleteDiskRequest(c, api, "disk3", record[5])
143- // The fourth request deletes the deployment.
144 assertDeleteDeploymentRequest(c, api, request.ServiceName,
145- request.DeploymentName, record[6])
146+ request.DeploymentName, record[1])
147+ assertDeleteDiskRequest(c, api, "disk1", record[2])
148+ assertDeleteDiskRequest(c, api, "disk2", record[3])
149+ assertDeleteDiskRequest(c, api, "disk3", record[4])
150 }
151
152 func (suite *suiteDestroyDeployment) TestOkayWhenDeploymentNotFound(c *C) {
153@@ -306,12 +295,10 @@
154 var responses []DispatcherResponse
155 // Prepare.
156 responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
157- // For shutting down VMs.
158- responses = append(responses, exampleNotFoundResponse, exampleNotFoundResponse)
159- // For deleting disks.
160+ // For deleting the deployment.
161+ responses = append(responses, exampleNotFoundResponse)
162+ // For deleting the disks.
163 responses = append(responses, exampleNotFoundResponse, exampleNotFoundResponse, exampleNotFoundResponse)
164- // For other requests.
165- responses = append(responses, exampleNotFoundResponse)
166 record := []*X509Request{}
167 rigRecordingPreparedResponseDispatcher(&record, responses)
168 // Exercise DestroyDeployment.
169@@ -322,23 +309,14 @@
170 }
171 err := api.DestroyDeployment(request)
172 c.Assert(err, IsNil)
173- c.Check(record, HasLen, 7)
174- // The first request is for the deployment.
175+ c.Check(record, HasLen, 5)
176 assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
177 request.ServiceName, request.DeploymentName}, record[0])
178- // The second is to shutdown RoleInstance "one".
179- assertShutdownRoleRequest(c, api, &ShutdownRoleRequest{
180- request.ServiceName, request.DeploymentName, "one"}, record[1])
181- // The third is to shutdown RoleInstance "two", similar to like the second.
182- assertShutdownRoleRequest(c, api, &ShutdownRoleRequest{
183- request.ServiceName, request.DeploymentName, "two"}, record[2])
184- // The fourth, fifth and sixth requests delete disks.
185- assertDeleteDiskRequest(c, api, "disk1", record[3])
186- assertDeleteDiskRequest(c, api, "disk2", record[4])
187- assertDeleteDiskRequest(c, api, "disk3", record[5])
188- // The fourth request deletes the deployment.
189 assertDeleteDeploymentRequest(c, api, request.ServiceName,
190- request.DeploymentName, record[6])
191+ request.DeploymentName, record[1])
192+ assertDeleteDiskRequest(c, api, "disk1", record[2])
193+ assertDeleteDiskRequest(c, api, "disk2", record[3])
194+ assertDeleteDiskRequest(c, api, "disk3", record[4])
195 }
196
197 func (suite *suiteDestroyDeployment) TestFailsGettingDeployment(c *C) {
198@@ -359,10 +337,11 @@
199 c.Check(record, HasLen, 1)
200 }
201
202-func (suite *suiteDestroyDeployment) TestFailsShuttingDownRole(c *C) {
203+func (suite *suiteDestroyDeployment) TestFailsDeletingDisk(c *C) {
204 var responses []DispatcherResponse
205 // Prepare.
206 responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
207+ // For deleting disks.
208 responses = append(responses, exampleFailResponse)
209 record := []*X509Request{}
210 rigRecordingPreparedResponseDispatcher(&record, responses)
211@@ -374,38 +353,14 @@
212 }
213 err := api.DestroyDeployment(request)
214 c.Assert(err, NotNil)
215- c.Check(err, ErrorMatches, "POST request failed [(]500: Internal Server Error[)]")
216+ c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]")
217 c.Check(record, HasLen, 2)
218 }
219
220-func (suite *suiteDestroyDeployment) TestFailsDeletingDisk(c *C) {
221- var responses []DispatcherResponse
222- // Prepare.
223- responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
224- // For shutting down VMs.
225- responses = append(responses, exampleOkayResponse, exampleOkayResponse)
226- // For deleting disks.
227- responses = append(responses, exampleFailResponse)
228- record := []*X509Request{}
229- rigRecordingPreparedResponseDispatcher(&record, responses)
230- // Exercise DestroyDeployment.
231- api := makeAPI(c)
232- request := &DestroyDeploymentRequest{
233- ServiceName: "service-name",
234- DeploymentName: "deployment-name",
235- }
236- err := api.DestroyDeployment(request)
237- c.Assert(err, NotNil)
238- c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]")
239- c.Check(record, HasLen, 4)
240-}
241-
242 func (suite *suiteDestroyDeployment) TestFailsDeletingDeployment(c *C) {
243 var responses []DispatcherResponse
244 // Prepare.
245 responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
246- // For shutting down VMs.
247- responses = append(responses, exampleOkayResponse, exampleOkayResponse)
248 // For deleting disks.
249 responses = append(responses, exampleOkayResponse, exampleOkayResponse, exampleOkayResponse)
250 // For other requests.
251@@ -421,5 +376,5 @@
252 err := api.DestroyDeployment(request)
253 c.Assert(err, NotNil)
254 c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]")
255- c.Check(record, HasLen, 7)
256+ c.Check(record, HasLen, 5)
257 }

Subscribers

People subscribed via source and target branches

to all changes: