Merge lp:~allenap/gwacl/destroy-hosted-service into lp:gwacl

[Gavin Panella]

New method to destroy a hosted service.

[Raphaël Badin (rvb)]

Please wait a bit to review and land this… we've got a problem with the underlying DestroyDeployment() method (I've tested it using example/management/run.go and it does not work [blows up trying to stop the VMs]… I thought the only problem is that we should ignore the error we get when stopping the VMs but it turns out it's a bit more involved).

[Raphaël Badin (rvb)]

I think we should fix DestroyDeployment by doing this first: https://code.launchpad.net/~rvb/gwacl/fix-destroydeploy2/+merge/173095

Since the deployment used in the tests does not contain VMs, this branch will probably work as is, even with that change.

[Raphaël Badin (rvb)]

Confirmed, this can be reviewed (and landed) independently from my "fix-destroydeploy2" branch.

[Raphaël Badin (rvb)]

Looks good but please consider my suggestions below:

[0]

We've had trouble lately with using these DestroyDeployment() so I'd advice to do this: now that my "fix-destroydeploy2" branch has landed, merge trunk and then update the example/management/run.go file to call DestroyHostedService() (instead of DestroyDeployment() and the defered which cleans up the hosted service) from there and make sure this works fine when talking to the Azure server.

[1]

126 +var exampleHostedService = &HostedService{
127 + Deployments: []Deployment{
128 + {Name: "one"}, {Name: "two"},
129 + },
130 +}

Having that as a global variable is, I think, an anti-pattern. This saves a bit of duplicate to have that as a global variable and not repeat it in each and every test. But a side effect is that the tests are not "self-contained" anymore and it makes it them much more difficult to understand and reason with.

For instance, the reasoning behind these checks:
159 + assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
160 + request.ServiceName, "one"}, record[1])
161 + assertDeleteDeploymentRequest(c, api, request.ServiceName, "one",
162 + record[2])
163 + // The fourth and fifth requests are a repaat of the previous two, but for
164 + // deployment "two".
165 + assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
166 + request.ServiceName, "two"}, record[3])
167 + assertDeleteDeploymentRequest(c, api, request.ServiceName, "two",
168 + record[4])

is inherently linked to the structure of the tested object… and one has to navigate in the file to find the exampleHostedService's definition to understand why these assertions are meaningful.

I'd suggest:
- creating a tiny makeHostedService method that would take a slice of Deployment objects and return a *HostedService
- calling that method from every test so that the initialization of the tested object is not far from the checks.

Even better, maybe use variables instead of duplicating the strings/objects on which the checks are performed:
deployName1 := "one"
deployName2 := "two"
exampleHostedService = makeHostedService([]Deployment{{Name: deployName1}, {Name: deployName2}})

then the checks can be along the lines of:
assertDeleteDeploymentRequest(c, api, request.ServiceName, deployName2, record[4])

review: Approve

[Gavin Panella (allenap)]

Thanks for the review.

> [0]
>
> We've had trouble lately with using these DestroyDeployment() so I'd advice to
> do this: now that my "fix-destroydeploy2"  branch has landed, merge trunk and
> then update the example/management/run.go file to call DestroyHostedService()
> (instead of DestroyDeployment() and the defered which cleans up the hosted
> service) from there and make sure this works fine when talking to the Azure
> server.

Done.

>
> [1]
>
> 126     +var exampleHostedService = &HostedService{
> 127     +    Deployments: []Deployment{
> 128     +        {Name: "one"}, {Name: "two"},
> 129     +    },
> 130     +}
>
> Having that as a global variable is, I think, an anti-pattern.  This saves a
> bit of duplicate to have that as a global variable and not repeat it in each
> and every test.  But a side effect is that the tests are not "self-contained"
> anymore and it makes it them much more difficult to understand and reason
> with.
...

I've changed some of these example bits to be functions instead, and
I'm not sure it makes it easier to reason about. I've noticed with
some of the test refactoring going on - extracting things into
functions and whatnot - that:

- The loss of locality of what I'm looking at is more harmful than
  having a few extra lines of code in my test.

- The indirection of having all test data constructed at runtime from
  parameters is more harmful than having static test data.

- Using strings like "disk1" is easier to read and grok than either
  deployments[0].RoleList[1].OSVirtualHardDisk[0].DiskName, or storing
  the names of everything in lots of local variables.

- Using functions for creating things in Go is painful because of the
  very limited calling conventions available. We are going to end up
  with 100s of factory functions for creating objects when it would
  simpler and more expressive to create them near the point of use.

Even so, I'll land the few changes that I did make.

[Julian Edwards (julian-edwards)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'example/management/run.go'
--- example/management/run.go	2013-07-04 17:25:26 +0000
+++ example/management/run.go	2013-07-05 10:51:26 +0000
@@ -108,9 +108,10 @@
     fmt.Println("Done creating a hosted service\n")
 
     defer func() {
-        fmt.Println("Deleting hosted service...")
-        api.DeleteHostedService(hostServiceName)
-        fmt.Println("Done deleting hosted service\n")
+        fmt.Println("Destroying hosted service...")
+        api.DestroyHostedService(&gwacl.DestroyHostedServiceRequest{
+            ServiceName: hostServiceName})
+        fmt.Println("Done destroying hosted service\n")
     }()
 
     fmt.Println("Listing hosted services...")
@@ -171,13 +172,6 @@
     checkError(err)
     fmt.Println("Done adding VM deployment\n")
 
-    defer func() {
-        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...")
     err = api.StartRole(&gwacl.StartRoleRequest{hostServiceName, deployment.Name, role.RoleName})
     checkError(err)

=== modified file 'management.go'
--- management.go	2013-07-04 20:23:56 +0000
+++ management.go	2013-07-05 10:51:26 +0000
@@ -143,3 +143,37 @@
     // Done.
     return nil
 }
+
+type DestroyHostedServiceRequest struct {
+    ServiceName string
+}
+
+// DestroyHostedService destroys all of the hosted service's contained
+// deployments then deletes the hosted service itself.
+func (api *ManagementAPI) DestroyHostedService(request *DestroyHostedServiceRequest) error {
+    // 1. Get properties.
+    properties, err := api.GetHostedServiceProperties(request.ServiceName, true)
+    if err != nil {
+        if IsNotFoundError(err) {
+            return nil
+        }
+        return err
+    }
+    // 2. Delete deployments.
+    for _, deployment := range properties.Deployments {
+        err := api.DestroyDeployment(&DestroyDeploymentRequest{
+            ServiceName:    request.ServiceName,
+            DeploymentName: deployment.Name,
+        })
+        if err != nil {
+            return err
+        }
+    }
+    // 3. Delete service.
+    err = api.DeleteHostedService(request.ServiceName)
+    if err != nil && !IsNotFoundError(err) {
+        return err
+    }
+    // Done.
+    return nil
+}

=== modified file 'management_base_test.go'
--- management_base_test.go	2013-07-04 07:11:22 +0000
+++ management_base_test.go	2013-07-05 10:51:26 +0000
@@ -399,6 +399,18 @@
     checkOneRequest(c, &recordedRequests, expectedURL, requestPayload, "PUT")
 }
 
+func assertGetHostedServicePropertiesRequest(c *C, api *ManagementAPI, serviceName string, embedDetail bool, httpRequest *X509Request) {
+    var query string
+    if embedDetail {
+        query = "embed-detail=true"
+    } else {
+        query = "embed-detail=false"
+    }
+    expectedURL := fmt.Sprintf("%s%s/services/hostedservices/%s?%s", AZURE_URL,
+        api.session.subscriptionId, serviceName, query)
+    checkRequest(c, httpRequest, expectedURL, nil, "GET")
+}
+
 func (suite *managementBaseAPISuite) TestGetHostedServiceProperties_withoutDetails(c *C) {
     api := makeAPI(c)
     body := `
@@ -435,9 +447,8 @@
 
     properties, err := api.GetHostedServiceProperties(serviceName, false)
     c.Assert(err, IsNil)
-
-    expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + serviceName + "?embed-detail=false"
-    checkOneRequest(c, &recordedRequests, expectedURL, []byte{}, "GET")
+    c.Check(recordedRequests, HasLen, 1)
+    assertGetHostedServicePropertiesRequest(c, api, serviceName, false, recordedRequests[0])
 
     c.Check(properties.URL, Equals, "hosted-service-url")
     c.Check(properties.ServiceName, Equals, "hosted-service-name")
@@ -487,9 +498,8 @@
 
     properties, err := api.GetHostedServiceProperties(serviceName, true)
     c.Assert(err, IsNil)
-
-    expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + serviceName + "?embed-detail=true"
-    checkOneRequest(c, &recordedRequests, expectedURL, []byte{}, "GET")
+    c.Check(recordedRequests, HasLen, 1)
+    assertGetHostedServicePropertiesRequest(c, api, serviceName, true, recordedRequests[0])
 
     c.Check(properties.URL, Equals, "hosted-service-url")
     c.Check(properties.ServiceName, Equals, "hosted-service-name")
@@ -510,15 +520,20 @@
     checkOneRequest(c, recordedRequests, expectedURL, expectedPayload, "POST")
 }
 
+func assertDeleteHostedServiceRequest(c *C, api *ManagementAPI, serviceName string, httpRequest *X509Request) {
+    expectedURL := fmt.Sprintf("%s%s/services/hostedservices/%s", AZURE_URL,
+        api.session.subscriptionId, serviceName)
+    checkRequest(c, httpRequest, expectedURL, nil, "DELETE")
+}
+
 func (suite *managementBaseAPISuite) TestDeleteHostedService(c *C) {
     api := makeAPI(c)
     recordedRequests := setUpDispatcher("operationID")
     hostedServiceName := "testName"
     err := api.DeleteHostedService(hostedServiceName)
-
     c.Assert(err, IsNil)
-    expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + hostedServiceName
-    checkOneRequest(c, recordedRequests, expectedURL, []byte{}, "DELETE")
+    c.Check(*recordedRequests, HasLen, 1)
+    assertDeleteHostedServiceRequest(c, api, hostedServiceName, (*recordedRequests)[0])
 }
 
 func (suite *managementBaseAPISuite) TestAddDeployment(c *C) {

=== modified file 'management_test.go'
--- management_test.go	2013-07-04 20:23:56 +0000
+++ management_test.go	2013-07-05 10:51:26 +0000
@@ -220,20 +220,6 @@
     c.Assert(recordedRequests, HasLen, 1)
 }
 
-type suiteDestroyDeployment struct{}
-
-var _ = Suite(&suiteDestroyDeployment{})
-
-var exampleDeployment = &Deployment{
-    RoleInstanceList: makeNamedRoleInstances("one", "two"),
-    RoleList: []Role{
-        {OSVirtualHardDisk: []OSVirtualHardDisk{
-            {DiskName: "disk1"}, {DiskName: "disk2"}}},
-        {OSVirtualHardDisk: []OSVirtualHardDisk{
-            {DiskName: "disk1"}, {DiskName: "disk3"}}},
-    },
-}
-
 var exampleOkayResponse = DispatcherResponse{
     response: &x509Response{StatusCode: http.StatusOK},
 }
@@ -246,9 +232,26 @@
     response: &x509Response{StatusCode: http.StatusNotFound},
 }
 
+type suiteDestroyDeployment struct{}
+
+var _ = Suite(&suiteDestroyDeployment{})
+
+func (suite *suiteDestroyDeployment) makeExampleDeployment() *Deployment {
+    return &Deployment{
+        RoleInstanceList: makeNamedRoleInstances("one", "two"),
+        RoleList: []Role{
+            {OSVirtualHardDisk: []OSVirtualHardDisk{
+                {DiskName: "disk1"}, {DiskName: "disk2"}}},
+            {OSVirtualHardDisk: []OSVirtualHardDisk{
+                {DiskName: "disk1"}, {DiskName: "disk3"}}},
+        },
+    }
+}
+
 func (suite *suiteDestroyDeployment) TestHappyPath(c *C) {
     var responses []DispatcherResponse
     // Prepare.
+    exampleDeployment := suite.makeExampleDeployment()
     responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
     // For deleting the deployment.
     responses = append(responses, exampleOkayResponse)
@@ -294,6 +297,7 @@
 func (suite *suiteDestroyDeployment) TestOkayWhenAssetsNotFound(c *C) {
     var responses []DispatcherResponse
     // Prepare.
+    exampleDeployment := suite.makeExampleDeployment()
     responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
     // For deleting the deployment.
     responses = append(responses, exampleNotFoundResponse)
@@ -340,6 +344,7 @@
 func (suite *suiteDestroyDeployment) TestFailsDeletingDisk(c *C) {
     var responses []DispatcherResponse
     // Prepare.
+    exampleDeployment := suite.makeExampleDeployment()
     responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
     // For deleting disks.
     responses = append(responses, exampleFailResponse)
@@ -360,6 +365,7 @@
 func (suite *suiteDestroyDeployment) TestFailsDeletingDeployment(c *C) {
     var responses []DispatcherResponse
     // Prepare.
+    exampleDeployment := suite.makeExampleDeployment()
     responses = append(responses, makeOKXMLResponse(c, exampleDeployment)...)
     // For deleting disks.
     responses = append(responses, exampleOkayResponse, exampleOkayResponse, exampleOkayResponse)
@@ -378,3 +384,173 @@
     c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]")
     c.Check(record, HasLen, 5)
 }
+
+type suiteDestroyHostedService struct{}
+
+var _ = Suite(&suiteDestroyHostedService{})
+
+func (suite *suiteDestroyHostedService) makeExampleHostedService(deploymentNames ...string) *HostedService {
+    var exampleHostedService = &HostedService{}
+    for _, deploymentName := range deploymentNames {
+        exampleHostedService.Deployments = append(
+            exampleHostedService.Deployments,
+            Deployment{Name: deploymentName})
+    }
+    return exampleHostedService
+}
+
+func (suite *suiteDestroyHostedService) TestHappyPath(c *C) {
+    var responses []DispatcherResponse
+    // DestroyHostedService first gets the hosted service proerties.
+    exampleHostedService := suite.makeExampleHostedService("one", "two")
+    responses = append(responses, makeOKXMLResponse(c, exampleHostedService)...)
+    // It calls DestroyDeployment, which first gets the deployment's
+    // properties, deletes any assets contained therein (none in this case)
+    // then deletes the deployment.
+    for _, deployment := range exampleHostedService.Deployments {
+        responses = append(responses, makeOKXMLResponse(c, &deployment)...)
+        responses = append(responses, exampleOkayResponse)
+    }
+    // For deleting the service itself.
+    responses = append(responses, exampleOkayResponse)
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    // Exercise DestroyHostedService.
+    api := makeAPI(c)
+    request := &DestroyHostedServiceRequest{
+        ServiceName: "service-name",
+    }
+    err := api.DestroyHostedService(request)
+    c.Assert(err, IsNil)
+    c.Check(record, HasLen, 6)
+    // The first request is for the hosted service.
+    assertGetHostedServicePropertiesRequest(c, api, request.ServiceName, true, record[0])
+    // The second and third requests fetch then delete deployment "one"; see
+    // DestroyDeployment for an explanation of this behaviour.
+    assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
+        request.ServiceName, "one"}, record[1])
+    assertDeleteDeploymentRequest(c, api, request.ServiceName, "one",
+        record[2])
+    // The fourth and fifth requests are a repaat of the previous two, but for
+    // deployment "two".
+    assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
+        request.ServiceName, "two"}, record[3])
+    assertDeleteDeploymentRequest(c, api, request.ServiceName, "two",
+        record[4])
+    // The last request deletes the hosted service.
+    assertDeleteHostedServiceRequest(c, api, request.ServiceName, record[5])
+}
+
+func (suite *suiteDestroyHostedService) TestOkayWhenHostedServiceNotFound(c *C) {
+    var responses []DispatcherResponse
+    // Prepare.
+    responses = []DispatcherResponse{exampleNotFoundResponse}
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    // Exercise DestroyHostedService.
+    api := makeAPI(c)
+    request := &DestroyHostedServiceRequest{ServiceName: "service-name"}
+    err := api.DestroyHostedService(request)
+    c.Assert(err, IsNil)
+    c.Check(record, HasLen, 1)
+}
+
+func (suite *suiteDestroyHostedService) TestOkayWhenDeploymentsNotFound(c *C) {
+    var responses []DispatcherResponse
+    // Prepare.
+    exampleHostedService := suite.makeExampleHostedService("one", "two")
+    responses = append(responses, makeOKXMLResponse(c, exampleHostedService)...)
+    // Someone else has destroyed the deployments in the meantime.
+    responses = append(responses, exampleNotFoundResponse, exampleNotFoundResponse)
+    // Success deleting the hosted service.
+    responses = append(responses, exampleOkayResponse)
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    // Exercise DestroyHostedService.
+    api := makeAPI(c)
+    request := &DestroyHostedServiceRequest{ServiceName: "service-name"}
+    err := api.DestroyHostedService(request)
+    c.Assert(err, IsNil)
+    c.Check(record, HasLen, 4)
+    assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
+        request.ServiceName, "one"}, record[1])
+    assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
+        request.ServiceName, "two"}, record[2])
+    assertDeleteHostedServiceRequest(c, api, request.ServiceName, record[3])
+}
+
+func (suite *suiteDestroyHostedService) TestOkayWhenHostedServiceNotFoundWhenDeleting(c *C) {
+    var responses []DispatcherResponse
+    // Prepare.
+    exampleHostedService := suite.makeExampleHostedService("one", "two")
+    responses = append(responses, makeOKXMLResponse(c, exampleHostedService)...)
+    // Someone else has destroyed the deployments in the meantime.
+    responses = append(responses, exampleNotFoundResponse, exampleNotFoundResponse)
+    // Someone else has destroyed the hosted service in the meantime.
+    responses = append(responses, exampleNotFoundResponse)
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    // Exercise DestroyHostedService.
+    api := makeAPI(c)
+    request := &DestroyHostedServiceRequest{ServiceName: "service-name"}
+    err := api.DestroyHostedService(request)
+    c.Assert(err, IsNil)
+    c.Check(record, HasLen, 4)
+    assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
+        request.ServiceName, "one"}, record[1])
+    assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
+        request.ServiceName, "two"}, record[2])
+    assertDeleteHostedServiceRequest(c, api, request.ServiceName, record[3])
+}
+
+func (suite *suiteDestroyHostedService) TestFailsGettingHostedService(c *C) {
+    var responses []DispatcherResponse
+    // Prepare.
+    responses = []DispatcherResponse{exampleFailResponse}
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    // Exercise DestroyHostedService.
+    api := makeAPI(c)
+    request := &DestroyHostedServiceRequest{ServiceName: "service-name"}
+    err := api.DestroyHostedService(request)
+    c.Assert(err, NotNil)
+    c.Check(err, ErrorMatches, "GET request failed [(]500: Internal Server Error[)]")
+    c.Check(record, HasLen, 1)
+}
+
+func (suite *suiteDestroyHostedService) TestFailsDestroyingDeployment(c *C) {
+    var responses []DispatcherResponse
+    // Prepare.
+    exampleHostedService := suite.makeExampleHostedService("one", "two")
+    responses = append(responses, makeOKXMLResponse(c, exampleHostedService)...)
+    responses = append(responses, exampleFailResponse)
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    // Exercise DestroyHostedService.
+    api := makeAPI(c)
+    request := &DestroyHostedServiceRequest{ServiceName: "service-name"}
+    err := api.DestroyHostedService(request)
+    c.Assert(err, NotNil)
+    c.Check(err, ErrorMatches, "GET request failed [(]500: Internal Server Error[)]")
+    c.Check(record, HasLen, 2)
+}
+
+func (suite *suiteDestroyHostedService) TestFailsDeletingHostedService(c *C) {
+    var responses []DispatcherResponse
+    // Prepare.
+    exampleHostedService := suite.makeExampleHostedService("one", "two")
+    responses = append(responses, makeOKXMLResponse(c, exampleHostedService)...)
+    // Deployments not found, but that's okay.
+    responses = append(responses, exampleNotFoundResponse, exampleNotFoundResponse)
+    // When deleting hosted service.
+    responses = append(responses, exampleFailResponse)
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    // Exercise DestroyHostedService.
+    api := makeAPI(c)
+    request := &DestroyHostedServiceRequest{ServiceName: "service-name"}
+    err := api.DestroyHostedService(request)
+    c.Assert(err, NotNil)
+    c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]")
+    c.Check(record, HasLen, 4)
+}