Merge lp:~rvb/juju-core/az-destroy into lp:~go-bot/juju-core/trunk

[Raphaël Badin]

Azure provider: Destroy() and StopInstances().

Azure provider: Destroy() and StopInstances().

This branch implements environ's Destroy() and StopInstances() in the Azure provider. As always with the provider code, most of the non-obvious code is in the tests. Since the testing utilities provided by gwacl return a slice of the fake requests made, it's tempting to make an extensive usage of that information and basically duplicate the unit-tests for the called gwacl methods. Instead of doing that, I chose a middle ground and checked that the requests look right without going too deep; this should be sufficient to test that the provider methods have the expected behavior and get tests to fail if the gwacl methods change significantly.

[Raphaël Badin (rvb)]

Reviewers: mp+173657_code.launchpad.net,

Message:
Please take a look.

Description:
Azure provider: Destroy() and StopInstances().

This branch implements environ's Destroy() and StopInstances() in the
Azure provider. As always with the provider code, most of the
non-obvious code is in the tests. Since the testing utilities provided
by gwacl return a slice of the fake requests made, it's tempting to make
an extensive usage of that information and basically duplicate the
unit-tests for the called gwacl methods. Instead of doing that, I chose
a middle ground and checked that the requests look right without going
too deep; this should be sufficient to test that the provider methods
have the expected behavior and get tests to fail if the gwacl methods
change significantly.

https://code.launchpad.net/~rvb/juju-core/az-destroy/+merge/173657

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/11034044/

Affected files:
   A [revision details]
   M environs/azure/environ.go
   M environs/azure/environ_test.go

[John A Meinel (jameinel)]

I don't think you have to change this, but there are a couple bits that
might make it easier to support.

'makeService' is a bit confusing because Juju has a 'service' concept.
So clarifying what you are creating here would be nice.

Just asserting that we call GET seems a bit weak, vs at least asserting
what *type* of thing you are GETing.

I do still prefer the double approach to the mocking-HTTP requests.
Mostly because when you do change gwacl to fix things, it can easily
break the juju-core test suite without the juju-core code itself
actually being incorrect. (eg you discover that you know enough that you
can just DELETE the services without needing to GET their details
first.)

If it isn't really possible to assert more about the types of URLs being
accessed, then LGTM.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode351
environs/azure/environ_test.go:351: // When destroying a hosted service,
gwacl first issues a Get reques
typo: request

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode390
environs/azure/environ_test.go:390: responses :=
buildDestroyServiceResponses(c, services)
How does this match up with the 1 instance == 1 service model that JTV
has brought up might be changing?

Given that Juju has its own notion of what a service is, maybe the
helper could be "makeAzureService" to distinguish and avoid confusion?

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode403
environs/azure/environ_test.go:403: c.Check((*requests)[3].Method,
Equals, "DELETE")
It feels like this could use slightly more assertion if we are going to
do it this way.
Specifically that we are GET and or DELETE a service (somehow the
"service1" is in the content, or something). To handle the "I wanted to
delete a service, but I actually just deleted an instance" or something
along those lines.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode456
environs/azure/environ_test.go:456:
c.Check(strings.HasSuffix(transport.Exchanges[2].Request.URL.String(),
"blob-2"), IsTrue)
Like this one is actually asserting that you are DELETEing something
"blob-1" related. (Otherwise this test could have passed if you just
DELETE services.)

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode496
environs/azure/environ_test.go:496: c.Check((*requests), HasLen,
1+len(services)*2)
So I think what I'd like to see is not testing of the formatting of the
requests (that belongs in lp:gwacl), but slightly more sanity checking
than just GET. So something that shows it is a GET of all instances, and
then a GET + DELETE of a given service, etc.

https://codereview.appspot.com/11034044/

[Jeroen T. Vermeulen (jtv)]

LGTM, thoroughly tested, but as always a few notes still.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode134
environs/azure/environ.go:134: // Shortcut to exit quickly if
'instances' is an empty slice or nil.

I'd prefer it if you didn't do this unless the comment could cite a very
good reason, such as breakage or a specific, unacceptable performance
overhead. Otherwise it's pointless cyclomatic complexity, which means
reduced test coverage.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode144
environs/azure/environ.go:144: // Shutdown all the instances; If there
are errors, return only the

Typo: "if" after semicolon, not "If"!

Also, "shutdown" is the noun and "shut down" is the verb. (Same in the
line below).

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode247
environs/azure/environ.go:247: logger.Debugf("environs/azure: destroying
environment %q", env.name)

No need to prefix the message with "environs/azure." That's built into
the logger.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode261
environs/azure/environ.go:261: // Stop all instances.

You're trying to delete all blobs and *then* stopping the instances.
Has Azure fixed the problems with deleting VHDs yet? Because otherwise
I imagine you'd just get errors about VHD files being still in use by
the VMs that have them mounted.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode271
environs/azure/environ.go:271: // Add any instances we've been told
about but haven't yet shown

I think I recognize this code from the ec2/openstack providers... What
an ugly piece of code it is.

 From a Python background, I would say chuck both sources of instances
into the same set, then pass that set as an iterable. Unfortunately in
Go, that comes out as:

combinedInstances := make(map[string]bool)
for _, inst := range env.AllInstances() {
 combinedInstances[inst] = true
}
for _, inst := range ensureInstances {
 combinedInstances[inst] = true
}
instances := make([]string, 0)
for inst := range combinedInstances {
 instances = append(instances, inst)
}

More regular perhaps, but still an insane amount of work for what it is.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode281
environs/azure/environ.go:281: if err != nil {

Maybe just "return env.StopInstances(insts)"? I just can't take the
mindless repetition of error propagation any more!

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode9
environs/azure/environ_test.go:9: "io/ioutil"

By the way, there's a new convention for formatting our imports — but as
far as I can tell, no cleanup drive. So we're expected to fix this
ourselves as we go along, I guess.

The convention is: still one import statement, b...

[Raphaël Badin (rvb)]

On 2013/07/09 09:05:34, jtv.canonical wrote:
> LGTM, thoroughly tested, but as always a few notes still.

Thanks for the review!

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go
> File environs/azure/environ.go (right):

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode134
> environs/azure/environ.go:134: // Shortcut to exit quickly if
'instances' is an
> empty slice or nil.

> I'd prefer it if you didn't do this unless the comment could cite a
very good
> reason, such as breakage or a specific, unacceptable performance
overhead.
> Otherwise it's pointless cyclomatic complexity, which means reduced
test
> coverage.

Fixed.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode144
> environs/azure/environ.go:144: // Shutdown all the instances; If there
are
> errors, return only the

> Typo: "if" after semicolon, not "If"!

> Also, "shutdown" is the noun and "shut down" is the verb. (Same in
the line
> below).

Fixed.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode247
> environs/azure/environ.go:247: logger.Debugf("environs/azure:
destroying
> environment %q", env.name)

> No need to prefix the message with "environs/azure." That's built
into the
> logger.

Well spotted, fixed.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode261
> environs/azure/environ.go:261: // Stop all instances.

> You're trying to delete all blobs and *then* stopping the instances.
Has Azure
> fixed the problems with deleting VHDs yet? Because otherwise I
imagine you'd
> just get errors about VHD files being still in use by the VMs that
have them
> mounted.

I think you're mixing up two different things here: the objects that are
in the storage (the tools, the state-info file, etc) and the VHD.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode271
> environs/azure/environ.go:271: // Add any instances we've been told
about but
> haven't yet shown

> I think I recognize this code from the ec2/openstack providers...
What an ugly
> piece of code it is.

It is ugly indeed, this should not be part of the provider's code but
until this gets refactored, I'd prefer to keep that an exact copy of the
code the other providers use so that this can be easily refactored
later.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newcode281
> environs/azure/environ.go:281: if err != nil {

> Maybe just "return env.StopInstances(insts)"? I just can't take the
mindless
> repetition of error propagation any more!

Done.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go
> File environs/azure/environ_test.go (right):

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode9
> environs/azure/environ_test.go:9: "io/ioutil"

> By the way, there's a new convention for formatting our imports — but
as far as
> I can tell, no cleanup drive. So we're expected to fix this ourselves
as we go
> along, I guess.

> The convention ...

[Jeroen T. Vermeulen (jtv)]

On 2013/07/09 10:12:41, rvb wrote:

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode351
> > environs/azure/environ_test.go:351: // When destroying a hosted
service, gwacl
> > first issues a Get reques
> >
> > This makes it sound as if gwacl.DeleteHostedService is doing all
that, when
> > actually apart from the one mention of a GET request, you're
describing what
> the
> > provider code does. My first reaction was: "why isn't this
knowledge kept
> > inside gwacl then?"

> No, that's what DestroyHostedService does.

In that case, it's painful to have this detailed knowledge of gwacl's
implementation embedded here instead of in gwacl. Can you think of a
useful way to move it?

> > I think you can test this at a higher level, with less hassle and
less code.
> > Look for setDummyStorage; that will set up your azureEnviron with a
fake
> > storage. Then you can test directly for things like "my file gets
deleted
> from
> > storage" without having to muck about with requests and responses.
Leave that
> > stuff to the storage tests.

> I don't think that's possible because:
> a) I need to test the destroying the environment actually deletes all
the files.
> b) The casting statement inside Destroy() prevents me from using
> setDummyStorage.

It's a shame about b) because a) is not actually an obstacle — it's
exactly the sort of thing I just described you could test more easily
this way!

This suggests to me that there's a DeleteAll method missing on
StorageWriter...

> I indented the XML strings better… I don't really see why I would use
dedent,
> XML does not really care about white spaces between tags.

I assumed you did it the ugly way for some good reason that was going to
become obvious. :-P

https://codereview.appspot.com/11034044/

[Raphaël Badin (rvb)]

Thanks for the review!

On 2013/07/09 08:00:44, jameinel wrote:
> I don't think you have to change this, but there are a couple bits
that might
> make it easier to support.

> 'makeService' is a bit confusing because Juju has a 'service' concept.
So
> clarifying what you are creating here would be nice.

Good point, I've renamed all the helpers (for instance
s/makeService/makeAzureService).

> Just asserting that we call GET seems a bit weak, vs at least
asserting what
> *type* of thing you are GETing.

You're right, I've improved the tests to check that they target the
right objects.

> I do still prefer the double approach to the mocking-HTTP requests.
Mostly
> because when you do change gwacl to fix things, it can easily break
the
> juju-core test suite without the juju-core code itself actually being
incorrect.
> (eg you discover that you know enough that you can just DELETE the
services
> without needing to GET their details first.)

This is a bit late for this and we deliberately choose not to do a test
double in gwacl. This saves a lot of code in gwacl but the drawback is
that some of gwacl's internal leak into the tests of third party
applications, such as the juju provider, which use gwacl. Again, this
is a tradeoff and the main goal is to allow us to move quickly. It
maintenance is easier this way, once this is all stabilized (I'm talking
about gwacl here and how we map Juju services onto gwacl objects), it
might be good to move to a model where all the tests will be based on a
test double.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode351
> environs/azure/environ_test.go:351: // When destroying a hosted
service, gwacl
> first issues a Get reques
> typo: request

Fixed.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode390
> environs/azure/environ_test.go:390: responses :=
buildDestroyServiceResponses(c,
> services)
> How does this match up with the 1 instance == 1 service model that JTV
has
> brought up might be changing?

This works under that assumption, you're right… I've added a comment in
the code (the actual code for StopInstances, not the test code).

> Given that Juju has its own notion of what a service is, maybe the
helper could
> be "makeAzureService" to distinguish and avoid confusion?

Done.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode403
> environs/azure/environ_test.go:403: c.Check((*requests)[3].Method,
Equals,
> "DELETE")
> It feels like this could use slightly more assertion if we are going
to do it
> this way.
> Specifically that we are GET and or DELETE a service (somehow the
"service1" is
> in the content, or something). To handle the "I wanted to delete a
service, but
> I actually just deleted an instance" or something along those lines.

https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go#newcode456
> environs/azure/environ_test.go:456:
> c.Check(strings.HasSuffix(transport.Exchanges[2].Request.URL.String(),
> "blob-2"), IsTrue)
> Like this one is actually asserting that ...

=== modified file 'environs/azure/environ.go'
--- environs/azure/environ.go	2013-07-08 08:34:47 +0000
+++ environs/azure/environ.go	2013-07-09 13:50:30 +0000
@@ -5,6 +5,8 @@
 
 import (
 	"fmt"
+	"sync"
+
 	"launchpad.net/gwacl"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
@@ -12,7 +14,6 @@
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
-	"sync"
 )
 
 type azureEnviron struct {
@@ -130,19 +131,32 @@
 }
 
 // StopInstances is specified in the Environ interface.
-func (env *azureEnviron) StopInstances([]instance.Instance) error {
-	panic("unimplemented")
+func (env *azureEnviron) StopInstances(instances []instance.Instance) error {
+	// Each Juju instance is an Azure Service (instance==service), destroy
+	// all the Azure services.
+	// Acquire management API object.
+	context, err := env.getManagementAPI()
+	if err != nil {
+		return err
+	}
+	defer env.releaseManagementAPI(context)
+	// Shut down all the instances; if there are errors, return only the
+	// first one (but try to shut down all instances regardless).
+	var firstErr error
+	for _, instance := range instances {
+		request := &gwacl.DestroyHostedServiceRequest{ServiceName: string(instance.Id())}
+		err := context.DestroyHostedService(request)
+		if err != nil && firstErr == nil {
+			firstErr = err
+		}
+	}
+	return firstErr
 }
 
 // Instances is specified in the Environ interface.
 func (env *azureEnviron) Instances(ids []instance.Id) ([]instance.Instance, error) {
 	// The instance list is built using the list of all the relevant
 	// Azure Services (instance==service).
-	// If the list of ids is empty, return nil as specified by the
-	// interface
-	if len(ids) == 0 {
-		return nil, environs.ErrNoInstances
-	}
 	// Acquire management API object.
 	context, err := env.getManagementAPI()
 	if err != nil {
@@ -223,8 +237,41 @@
 }
 
 // Destroy is specified in the Environ interface.
-func (env *azureEnviron) Destroy(insts []instance.Instance) error {
-	panic("unimplemented")
+func (env *azureEnviron) Destroy(ensureInsts []instance.Instance) error {
+	logger.Debugf("destroying environment %q", env.name)
+
+	// Delete storage.
+	st := env.Storage().(*azureStorage)
+	context, err := st.getStorageContext()
+	if err != nil {
+		return err
+	}
+	request := &gwacl.DeleteAllBlobsRequest{Container: st.getContainer()}
+	err = context.DeleteAllBlobs(request)
+	if err != nil {
+		return fmt.Errorf("cannot clean up storage: %v", err)
+	}
+
+	// Stop all instances.
+	insts, err := env.AllInstances()
+	if err != nil {
+		return fmt.Errorf("cannot get instances: %v", err)
+	}
+	found := make(map[instance.Id]bool)
+	for _, inst := range insts {
+		found[inst.Id()] = true
+	}
+
+	// Add any instances we've been told about but haven't yet shown
+	// up in the instance list.
+	for _, inst := range ensureInsts {
+		id := inst.Id()
+		if !found[id] {
+			insts = append(insts, inst)
+			found[id] = true
+		}
+	}
+	return env.StopInstances(insts)
 }
 
 // OpenPorts is specified in the Environ interface.

=== modified file 'environs/azure/environ_test.go'
--- environs/azure/environ_test.go	2013-07-08 08:34:47 +0000
+++ environs/azure/environ_test.go	2013-07-09 13:50:30 +0000
@@ -6,6 +6,11 @@
 import (
 	"encoding/base64"
 	"fmt"
+	"io/ioutil"
+	"net/http"
+	"strings"
+	"sync"
+
 	. "launchpad.net/gocheck"
 	"launchpad.net/gwacl"
 	"launchpad.net/juju-core/environs"
@@ -15,9 +20,6 @@
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/testing"
 	. "launchpad.net/juju-core/testing/checkers"
-	"net/http"
-	"strings"
-	"sync"
 )
 
 type EnvironSuite struct {
@@ -112,7 +114,7 @@
 	// The real test is that this does not panic.
 }
 
-func patchWithServiceListResponse(c *C, services []gwacl.HostedServiceDescriptor) *[]*gwacl.X509Request {
+func buildAzureServiceListResponse(c *C, services []gwacl.HostedServiceDescriptor) []gwacl.DispatcherResponse {
 	list := gwacl.HostedServiceDescriptorList{HostedServices: services}
 	listXML, err := list.Serialize()
 	c.Assert(err, IsNil)
@@ -121,8 +123,12 @@
 		http.StatusOK,
 		nil,
 	)}
-	requests := gwacl.PatchManagementAPIResponses(responses)
-	return requests
+	return responses
+}
+
+func patchWithServiceListResponse(c *C, services []gwacl.HostedServiceDescriptor) *[]*gwacl.X509Request {
+	responses := buildAzureServiceListResponse(c, services)
+	return gwacl.PatchManagementAPIResponses(responses)
 }
 
 func (suite EnvironSuite) TestGetEnvPrefixContainsEnvName(c *C) {
@@ -339,3 +345,168 @@
 	c.Check(stateInfo.Addrs, DeepEquals, []string{expectedDNSName + statePortSuffix})
 	c.Check(apiInfo.Addrs, DeepEquals, []string{expectedDNSName + apiPortSuffix})
 }
+
+// buildDestroyAzureServiceResponses returns a slice containing the responses that a fake Azure server
+// can use to simulate the deletion of the given list of services.
+func buildDestroyAzureServiceResponses(c *C, services []*gwacl.HostedService) []gwacl.DispatcherResponse {
+	responses := []gwacl.DispatcherResponse{}
+	for _, service := range services {
+		// When destroying a hosted service, gwacl first issues a Get request
+		// to fetch the properties of the services.  Then it destroys all the
+		// deployments found in this service (none in this case, we make sure
+		// the service does not contain deployments to keep the testing simple)
+		// And it finally deletes the service itself.
+		if len(service.Deployments) != 0 {
+			panic("buildDestroyAzureServiceResponses does not support services with deployments!")
+		}
+		serviceXML, err := service.Serialize()
+		c.Assert(err, IsNil)
+		serviceGetResponse := gwacl.NewDispatcherResponse(
+			[]byte(serviceXML),
+			http.StatusOK,
+			nil,
+		)
+		responses = append(responses, serviceGetResponse)
+		serviceDeleteResponse := gwacl.NewDispatcherResponse(
+			nil,
+			http.StatusOK,
+			nil,
+		)
+		responses = append(responses, serviceDeleteResponse)
+	}
+	return responses
+}
+
+func makeAzureService(name string) (*gwacl.HostedService, *gwacl.HostedServiceDescriptor) {
+	service1 := &gwacl.HostedService{ServiceName: name}
+	service1Desc := &gwacl.HostedServiceDescriptor{ServiceName: name}
+	return service1, service1Desc
+}
+
+func (EnvironSuite) TestStopInstancesDestroysMachines(c *C) {
+	service1Name := "service1"
+	service1, service1Desc := makeAzureService(service1Name)
+	service2Name := "service2"
+	service2, service2Desc := makeAzureService(service2Name)
+	services := []*gwacl.HostedService{service1, service2}
+	responses := buildDestroyAzureServiceResponses(c, services)
+	requests := gwacl.PatchManagementAPIResponses(responses)
+	env := makeEnviron(c)
+	instances := convertToInstances([]gwacl.HostedServiceDescriptor{*service1Desc, *service2Desc})
+
+	err := env.StopInstances(instances)
+	c.Check(err, IsNil)
+
+	// It takes 2 API calls to delete each service:
+	// - one GET request to fetch the properties about the service;
+	// - one DELETE request to delete the service.
+	c.Check(len(*requests), Equals, len(services)*2)
+	c.Check((*requests)[0].Method, Equals, "GET")
+	c.Check((*requests)[1].Method, Equals, "DELETE")
+	c.Check((*requests)[2].Method, Equals, "GET")
+	c.Check((*requests)[3].Method, Equals, "DELETE")
+}
+
+// makeAzureStorageMocking creates a test azureStorage object that will talk to a
+// fake http server set up to always return the given http.Response object.
+func makeAzureStorageMocking(transport http.RoundTripper, container string, account string) azureStorage {
+	client := &http.Client{Transport: transport}
+	storageContext := gwacl.NewTestStorageContext(client)
+	storageContext.Account = account
+	context := &testStorageContext{container: container, storageContext: storageContext}
+	azStorage := azureStorage{context}
+	return azStorage
+}
+
+var blobListWith2BlobsResponse = `
+  <?xml version="1.0" encoding="utf-8"?>
+  <EnumerationResults ContainerName="http://myaccount.blob.core.windows.net/mycontainer">
+    <Prefix>prefix</Prefix>
+    <Marker>marker</Marker>
+    <MaxResults>maxresults</MaxResults>
+    <Delimiter>delimiter</Delimiter>
+    <Blobs>
+      <Blob>
+        <Name>blob-1</Name>
+        <Url>blob-url1</Url>
+      </Blob>
+      <Blob>
+        <Name>blob-2</Name>
+        <Url>blob-url2</Url>
+     </Blob>
+    </Blobs>
+    <NextMarker />
+  </EnumerationResults>`
+
+func (EnvironSuite) TestDestroyCleansUpStorage(c *C) {
+	env := makeEnviron(c)
+	container := "container"
+	transport := &gwacl.MockingTransport{}
+	transport.AddExchange(&http.Response{StatusCode: http.StatusOK, Body: ioutil.NopCloser(strings.NewReader(blobListWith2BlobsResponse))}, nil)
+	transport.AddExchange(&http.Response{StatusCode: http.StatusAccepted, Body: nil}, nil)
+	transport.AddExchange(&http.Response{StatusCode: http.StatusAccepted, Body: nil}, nil)
+	azStorage := makeAzureStorageMocking(transport, container, "account")
+	env.storage = &azStorage
+	services := []gwacl.HostedServiceDescriptor{}
+	patchWithServiceListResponse(c, services)
+	instances := convertToInstances([]gwacl.HostedServiceDescriptor{})
+
+	err := env.Destroy(instances)
+
+	c.Check(err, IsNil)
+	c.Check(transport.Exchanges[1].Request.Method, Equals, "DELETE")
+	c.Check(strings.HasSuffix(transport.Exchanges[1].Request.URL.String(), "blob-1"), IsTrue)
+	c.Check(transport.Exchanges[2].Request.Method, Equals, "DELETE")
+	c.Check(strings.HasSuffix(transport.Exchanges[2].Request.URL.String(), "blob-2"), IsTrue)
+}
+
+var emptyListResponse = `
+  <?xml version="1.0" encoding="utf-8"?>
+  <EnumerationResults ContainerName="http://myaccount.blob.core.windows.net/mycontainer">
+    <Prefix>prefix</Prefix>
+    <Marker>marker</Marker>
+    <MaxResults>maxresults</MaxResults>
+    <Delimiter>delimiter</Delimiter>
+    <Blobs></Blobs>
+    <NextMarker />
+  </EnumerationResults>`
+
+func (EnvironSuite) TestDestroyStopsAllInstances(c *C) {
+	env := makeEnviron(c)
+	// Setup an empty storage.
+	container := "container"
+	response := makeResponse(emptyListResponse, http.StatusOK)
+	azStorage, _ := makeAzureStorage(response, container, "account")
+	env.storage = &azStorage
+	// Simulate 2 nodes corresponding to two Azure services.
+	prefix := env.getEnvPrefix()
+	service1Name := prefix + "service1"
+	service2Name := prefix + "service2"
+	service1, service1Desc := makeAzureService(service1Name)
+	service2, service2Desc := makeAzureService(service2Name)
+	services := []*gwacl.HostedService{service1, service2}
+	// The call to AllInstances() will return only one service (service1).
+	listInstancesResponses := buildAzureServiceListResponse(c, []gwacl.HostedServiceDescriptor{*service1Desc})
+	destroyResponses := buildDestroyAzureServiceResponses(c, services)
+	responses := append(listInstancesResponses, destroyResponses...)
+	requests := gwacl.PatchManagementAPIResponses(responses)
+
+	// Call Destroy with service1 and service2.
+	instances := convertToInstances([]gwacl.HostedServiceDescriptor{*service1Desc, *service2Desc})
+	err := env.Destroy(instances)
+	c.Check(err, IsNil)
+
+	// One request to get the list of all the environment's instances.
+	// Then two requests per destroyed machine (one to fetch the
+	// service's information, one to delete it).
+	c.Check((*requests), HasLen, 1+len(services)*2)
+	c.Check((*requests)[0].Method, Equals, "GET")
+	c.Check((*requests)[1].Method, Equals, "GET")
+	c.Check(strings.Contains((*requests)[1].URL, service1Name), IsTrue)
+	c.Check((*requests)[2].Method, Equals, "DELETE")
+	c.Check(strings.Contains((*requests)[2].URL, service1Name), IsTrue)
+	c.Check((*requests)[3].Method, Equals, "GET")
+	c.Check(strings.Contains((*requests)[3].URL, service2Name), IsTrue)
+	c.Check((*requests)[4].Method, Equals, "DELETE")
+	c.Check(strings.Contains((*requests)[4].URL, service2Name), IsTrue)
+}