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

[Raphaël Badin]

Do not attempt to stop the VMs inside DestroyDeployment.

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.

[Gavin Panella (allenap)]

I surprised this works, but looks good.

review: Approve

[Julian Edwards (julian-edwards)]

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)]

> 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)]

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.

=== modified file 'example/management/run.go'
--- example/management/run.go	2013-07-02 13:53:21 +0000
+++ example/management/run.go	2013-07-04 20:25:31 +0000
@@ -172,17 +172,10 @@
     fmt.Println("Done adding VM deployment\n")
 
     defer func() {
-        fmt.Println("Deleting VM disk...")
-        err = api.DeleteDisk(diskName)
-        checkError(err)
-        fmt.Println("Done deleting VM disk\n")
-    }()
-
-    defer func() {
-        fmt.Println("Deleting deployment...")
-        err = api.DeleteDeployment(hostServiceName, deployment.Name)
-        checkError(err)
-        fmt.Println("Done deleting deployment\n")
+        fmt.Printf("Destroying deployment %s...\n", machineName)
+        requestDestroyDeployment := &gwacl.DestroyDeploymentRequest{ServiceName: hostServiceName, DeploymentName: machineName}
+        api.DestroyDeployment(requestDestroyDeployment)
+        fmt.Println("Done destroying deployment\n")
     }()
 
     fmt.Println("Starting VM...")
@@ -190,13 +183,6 @@
     checkError(err)
     fmt.Println("Done starting VM\n")
 
-    defer func() {
-        fmt.Println("Stopping VM...")
-        err = api.ShutdownRole(&gwacl.ShutdownRoleRequest{hostServiceName, deployment.Name, role.RoleName})
-        checkError(err)
-        fmt.Println("Done stopping VM\n")
-    }()
-
     fmt.Println("Listing VM...")
     instances, err := api.ListInstances(&gwacl.ListInstancesRequest{hostServiceName})
     checkError(err)
 
=== modified file 'management.go'
--- management.go	2013-07-04 09:53:30 +0000
+++ management.go	2013-07-04 20:25:31 +0000
@@ -102,7 +102,7 @@
 }
 
 // DestroyDeployment brings down all resources within a deployment - running
-// instances, disks, etc. - then deletes the deployment itself.
+// instances, disks, etc. - and deletes the deployment itself.
 func (api *ManagementAPI) DestroyDeployment(request *DestroyDeploymentRequest) error {
     deployment, err := api.GetDeployment(&GetDeploymentRequest{
         ServiceName:    request.ServiceName,
@@ -114,42 +114,32 @@
         }
         return err
     }
-    // 1. Stop VMs.
-    for _, roleInstance := range deployment.RoleInstanceList {
-        err = api.ShutdownRole(&ShutdownRoleRequest{
-            ServiceName:    request.ServiceName,
-            DeploymentName: request.DeploymentName,
-            RoleName:       roleInstance.RoleName,
-        })
-        if err != nil && !IsNotFoundError(err) {
-            return err
-        }
-    }
-    // 2. Delete VM disks.
+    // 1. Get the list of the VM disks.
     diskNameMap := make(map[string]bool)
     for _, role := range deployment.RoleList {
         for _, osVHD := range role.OSVirtualHardDisk {
             diskNameMap[osVHD.DiskName] = true
         }
     }
+    // 2. Delete deployment.  This will delete all the role instances inside
+    // this deployment as a side effect.
+    err = api.DeleteDeployment(request.ServiceName, request.DeploymentName)
+    if err != nil && !IsNotFoundError(err) {
+        return err
+    }
     // Sort the disk names to aid testing.
     diskNames := []string{}
     for diskName := range diskNameMap {
         diskNames = append(diskNames, diskName)
     }
     sort.Strings(diskNames)
-    // Actually delete them.
+    // 3. Delete the disks.
     for _, diskName := range diskNames {
         err = api.DeleteDisk(diskName)
         if err != nil && !IsNotFoundError(err) {
             return err
         }
     }
-    // 3. Delete deployment.
-    err = api.DeleteDeployment(request.ServiceName, request.DeploymentName)
-    if err != nil && !IsNotFoundError(err) {
-        return err
-    }
     // Done.
     return nil
 }
 
=== modified file 'management_test.go'
--- management_test.go	2013-07-04 10:53:40 +0000
+++ management_test.go	2013-07-04 20:25:31 +0000
@@ -250,12 +250,10 @@
     var responses []DispatcherResponse
     // Prepare.
     responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
-    // For shutting down VMs.
-    responses = append(responses, exampleOkayResponse, exampleOkayResponse)
-    // For deleting disks.
-    responses = append(responses, exampleOkayResponse, exampleOkayResponse, exampleOkayResponse)
-    // For other requests.
+    // For deleting the deployment.
     responses = append(responses, exampleOkayResponse)
+    // For deleting disks.
+    responses = append(responses, exampleOkayResponse, exampleOkayResponse, exampleOkayResponse)
     record := []*X509Request{}
     rigRecordingPreparedResponseDispatcher(&record, responses)
     // Exercise DestroyDeployment.
@@ -266,23 +264,14 @@
     }
     err := api.DestroyDeployment(request)
     c.Assert(err, IsNil)
-    c.Check(record, HasLen, 7)
-    // The first request is for the deployment.
+    c.Check(record, HasLen, 5)
     assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
         request.ServiceName, request.DeploymentName}, record[0])
-    // The second is to shutdown RoleInstance "one".
-    assertShutdownRoleRequest(c, api, &ShutdownRoleRequest{
-        request.ServiceName, request.DeploymentName, "one"}, record[1])
-    // The third is to shutdown RoleInstance "two", similar to like the second.
-    assertShutdownRoleRequest(c, api, &ShutdownRoleRequest{
-        request.ServiceName, request.DeploymentName, "two"}, record[2])
-    // The fourth, fifth and sixth requests delete disks.
-    assertDeleteDiskRequest(c, api, "disk1", record[3])
-    assertDeleteDiskRequest(c, api, "disk2", record[4])
-    assertDeleteDiskRequest(c, api, "disk3", record[5])
-    // The fourth request deletes the deployment.
     assertDeleteDeploymentRequest(c, api, request.ServiceName,
-        request.DeploymentName, record[6])
+        request.DeploymentName, record[1])
+    assertDeleteDiskRequest(c, api, "disk1", record[2])
+    assertDeleteDiskRequest(c, api, "disk2", record[3])
+    assertDeleteDiskRequest(c, api, "disk3", record[4])
 }
 
 func (suite *suiteDestroyDeployment) TestOkayWhenDeploymentNotFound(c *C) {
@@ -306,12 +295,10 @@
     var responses []DispatcherResponse
     // Prepare.
     responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
-    // For shutting down VMs.
-    responses = append(responses, exampleNotFoundResponse, exampleNotFoundResponse)
-    // For deleting disks.
+    // For deleting the deployment.
+    responses = append(responses, exampleNotFoundResponse)
+    // For deleting the disks.
     responses = append(responses, exampleNotFoundResponse, exampleNotFoundResponse, exampleNotFoundResponse)
-    // For other requests.
-    responses = append(responses, exampleNotFoundResponse)
     record := []*X509Request{}
     rigRecordingPreparedResponseDispatcher(&record, responses)
     // Exercise DestroyDeployment.
@@ -322,23 +309,14 @@
     }
     err := api.DestroyDeployment(request)
     c.Assert(err, IsNil)
-    c.Check(record, HasLen, 7)
-    // The first request is for the deployment.
+    c.Check(record, HasLen, 5)
     assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
         request.ServiceName, request.DeploymentName}, record[0])
-    // The second is to shutdown RoleInstance "one".
-    assertShutdownRoleRequest(c, api, &ShutdownRoleRequest{
-        request.ServiceName, request.DeploymentName, "one"}, record[1])
-    // The third is to shutdown RoleInstance "two", similar to like the second.
-    assertShutdownRoleRequest(c, api, &ShutdownRoleRequest{
-        request.ServiceName, request.DeploymentName, "two"}, record[2])
-    // The fourth, fifth and sixth requests delete disks.
-    assertDeleteDiskRequest(c, api, "disk1", record[3])
-    assertDeleteDiskRequest(c, api, "disk2", record[4])
-    assertDeleteDiskRequest(c, api, "disk3", record[5])
-    // The fourth request deletes the deployment.
     assertDeleteDeploymentRequest(c, api, request.ServiceName,
-        request.DeploymentName, record[6])
+        request.DeploymentName, record[1])
+    assertDeleteDiskRequest(c, api, "disk1", record[2])
+    assertDeleteDiskRequest(c, api, "disk2", record[3])
+    assertDeleteDiskRequest(c, api, "disk3", record[4])
 }
 
 func (suite *suiteDestroyDeployment) TestFailsGettingDeployment(c *C) {
@@ -359,10 +337,11 @@
     c.Check(record, HasLen, 1)
 }
 
-func (suite *suiteDestroyDeployment) TestFailsShuttingDownRole(c *C) {
+func (suite *suiteDestroyDeployment) TestFailsDeletingDisk(c *C) {
     var responses []DispatcherResponse
     // Prepare.
     responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
+    // For deleting disks.
     responses = append(responses, exampleFailResponse)
     record := []*X509Request{}
     rigRecordingPreparedResponseDispatcher(&record, responses)
@@ -374,38 +353,14 @@
     }
     err := api.DestroyDeployment(request)
     c.Assert(err, NotNil)
-    c.Check(err, ErrorMatches, "POST request failed [(]500: Internal Server Error[)]")
+    c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]")
     c.Check(record, HasLen, 2)
 }
 
-func (suite *suiteDestroyDeployment) TestFailsDeletingDisk(c *C) {
-    var responses []DispatcherResponse
-    // Prepare.
-    responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
-    // For shutting down VMs.
-    responses = append(responses, exampleOkayResponse, exampleOkayResponse)
-    // For deleting disks.
-    responses = append(responses, exampleFailResponse)
-    record := []*X509Request{}
-    rigRecordingPreparedResponseDispatcher(&record, responses)
-    // Exercise DestroyDeployment.
-    api := makeAPI(c)
-    request := &DestroyDeploymentRequest{
-        ServiceName:    "service-name",
-        DeploymentName: "deployment-name",
-    }
-    err := api.DestroyDeployment(request)
-    c.Assert(err, NotNil)
-    c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]")
-    c.Check(record, HasLen, 4)
-}
-
 func (suite *suiteDestroyDeployment) TestFailsDeletingDeployment(c *C) {
     var responses []DispatcherResponse
     // Prepare.
     responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
-    // For shutting down VMs.
-    responses = append(responses, exampleOkayResponse, exampleOkayResponse)
     // For deleting disks.
     responses = append(responses, exampleOkayResponse, exampleOkayResponse, exampleOkayResponse)
     // For other requests.
@@ -421,5 +376,5 @@
     err := api.DestroyDeployment(request)
     c.Assert(err, NotNil)
     c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]")
-    c.Check(record, HasLen, 7)
+    c.Check(record, HasLen, 5)
 }