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

Proposed by Raphaël Badin on 2013-07-04
Status: Rejected
Rejected by: Raphaël Badin on 2013-07-04
Proposed branch: lp:~rvb/gwacl/fix-destroydeploy
Merge into: lp:gwacl
Diff against target: 84 lines (+17/-21)
3 files modified
example/management/run.go (+4/-18)
management.go (+8/-3)
management_test.go (+5/-0)
To merge this branch: bzr merge lp:~rvb/gwacl/fix-destroydeploy
Reviewer Review Type Date Requested Status
Gavin Panella 2013-07-04 Approve on 2013-07-04
Review via email: mp+173034@code.launchpad.net

Commit message

Do not check the error we get from stopping the VM inside DestroyDeployment.

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

Looks good.

[1]

-        if err != nil && !IsNotFoundError(err) {
-            return err
-        }
+        // Even when the operation itself works (i.e. the VM is stopped) we
+        // seem to alway get a 500 error back.
+        // We chose to ignore the error.
+        // If the operation errors for real and the VM is still running then
+        // the next operation (deleting the disks) will fail.
+        // if err != nil && !IsNotFoundError(err) {
+        //    return err
+        // }

I suggest only ignoring 500 errors. A new IsInternalServerError()
helper would be good here.

[2]

+/*
+// This test is currently disabled as we chose to ignore the errors returned
+// when shutting down the VM.  See the comment in DestroyDeployment's code
+// for more information.
 func (suite *suiteDestroyDeployment) TestFailsShuttingDownRole(c *C) {
     var responses []DispatcherResponse
     // Prepare.
@@ -377,6 +381,7 @@
     c.Check(err, ErrorMatches, "POST request failed [(]500: Internal Server Error[)]")
     c.Check(record, HasLen, 2)
 }
+*/

You can use c.ExpectFailure(reason string) here; see
http://godoc.org/launchpad.net/gocheck#C.ExpectFailure.

review: Approve
lp:~rvb/gwacl/fix-destroydeploy updated on 2013-07-04
164. By Raphaël Badin on 2013-07-04

Remove the storage account.

165. By Raphaël Badin on 2013-07-04

Remove the storage account.

Raphaël Badin (rvb) wrote :

Unmerged revisions

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 17:27:26 +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 17:27:26 +0000
44@@ -121,9 +121,14 @@
45 DeploymentName: request.DeploymentName,
46 RoleName: roleInstance.RoleName,
47 })
48- if err != nil && !IsNotFoundError(err) {
49- return err
50- }
51+ // Even when the operation itself works (i.e. the VM is stopped) we
52+ // seem to alway get a 500 error back.
53+ // We chose to ignore the error.
54+ // If the operation errors for real and the VM is still running then
55+ // the next operation (deleting the disks) will fail.
56+ // if err != nil && !IsNotFoundError(err) {
57+ // return err
58+ // }
59 }
60 // 2. Delete VM disks.
61 diskNameMap := make(map[string]bool)
62
63=== modified file 'management_test.go'
64--- management_test.go 2013-07-04 10:53:40 +0000
65+++ management_test.go 2013-07-04 17:27:26 +0000
66@@ -359,6 +359,10 @@
67 c.Check(record, HasLen, 1)
68 }
69
70+/*
71+// This test is currently disabled as we chose to ignore the errors returned
72+// when shutting down the VM. See the comment in DestroyDeployment's code
73+// for more information.
74 func (suite *suiteDestroyDeployment) TestFailsShuttingDownRole(c *C) {
75 var responses []DispatcherResponse
76 // Prepare.
77@@ -377,6 +381,7 @@
78 c.Check(err, ErrorMatches, "POST request failed [(]500: Internal Server Error[)]")
79 c.Check(record, HasLen, 2)
80 }
81+*/
82
83 func (suite *suiteDestroyDeployment) TestFailsDeletingDisk(c *C) {
84 var responses []DispatcherResponse

Subscribers

People subscribed via source and target branches

to all changes: