Merge lp:~julian-edwards/gwacl/removeall into lp:gwacl

[Julian Edwards]

New storage function DeleteAllBlobs to delete everything in a container.

I got fed up with looking up the right incantations and elf runes, so I also added a new Makefile target to generate debug builds.

[Raphaël Badin (rvb)]

45 + err := context.DeleteBlob(request.Container, blob.Name)
46 + if err != nil {
47 + return err
48 + }

Don't we want to continue trying to remove the other blobs even if one deletion fail? I mean shouldn't we continue iterating over the blobs and try deleting them all and then, at the end of the loop, return the first error encountered (if any)?

[Gavin Panella (allenap)]

> Don't we want to continue trying to remove the other blobs even if
> one deletion fail? I mean shouldn't we continue iterating over the
> blobs and try deleting them all and then, at the end of the loop,
> return the first error encountered (if any)?

I think we should keep it simple.

Three reasons:

- Continuing in spite of an error has a bad failure mode. if there
  are, say, 10k blobs and none of them can be deleted - because of a
  lease or an internal server error, for example - a long time would
  be spent doing useless work before giving the user the chance to
  retry or do something else.

- One common reason to call DeleteAllBlobs is so that you can delete
  the container straight afterwards. If even one blob fails to delete
  it's the same as all the blobs not deleting.

- The code is simpler.

[Raphaël Badin (rvb)]

> - Continuing in spite of an error has a bad failure mode. if there
> are, say, 10k blobs and none of them can be deleted - because of a
> lease or an internal server error, for example - a long time would
> be spent doing useless work before giving the user the chance to
> retry or do something else.

True, that's why I think we should use goroutines to remove the blobs in parallel rather than in sequence (same as what we've done in the MAAS provider in juju-core http://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/environs/maas/storage.go#L1).

> - One common reason to call DeleteAllBlobs is so that you can delete
> the container straight afterwards. If even one blob fails to delete
> it's the same as all the blobs not deleting.

Well, that's definitely not the case when using DeleteAllBlobs() from the provider when destroying an environment (which is DeleteAllBlobs's raison d'être). The goal here is to be able to cleanup an existing container so that it can be re-used to re-deploy an new environment.

>
> - The code is simpler.

That one is undisputed :)

[Gavin Panella (allenap)]

>> - Continuing in spite of an error has a bad failure mode. if there
>> are, say, 10k blobs and none of them can be deleted - because of a
>> lease or an internal server error, for example - a long time would
>> be spent doing useless work before giving the user the chance to
>> retry or do something else.
>
> True, that's why I think we should use goroutines to remove the
> blobs in parallel rather than in sequence

In the situation above it just means that more useless work will be
done, but concurrently.

What if the server is busy? It's going to get several concurrent
requests for blob deletion, instead of one at a time, and this may
make it overall *slower*. It may also serialise those requests anyway.

Keep it simple until we can prove it's better. Which we don't really
have time for now.

> (same as what we've done in the MAAS provider in juju-core
> http://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/environs/maas/storage.go#L1).

There's a bug in that code: if stor.Remove() panics, no error will be
returned, but deleteAll will complete and return nil.

Istm that Go, by having two competing paradigms for signalling errors,
actually seems to make concurrency harder.

>> - One common reason to call DeleteAllBlobs is so that you can delete
>> the container straight afterwards. If even one blob fails to delete
>> it's the same as all the blobs not deleting.
>
> Well, that's definitely not the case when using DeleteAllBlobs()
> from the provider when destroying an environment (which is
> DeleteAllBlobs's raison d'être). The goal here is to be able to
> cleanup an existing container so that it can be re-used to re-deploy
> an new environment.

It may be DeleteAllBlobs's raison d'être for us, but GWACL is meant to
be a general purpose Azure library in Go too, which is why I said
"*One* common reason".

>>
>> - The code is simpler.
>
> That one is undisputed :)

Excellent :) I think this is really important, especially right now. I
don't want to be chasing concurrency bugs before a deadline.

[Julian Edwards (julian-edwards)]

On Monday 01 Jul 2013 08:47:22 you wrote:
> > - Continuing in spite of an error has a bad failure mode. if there
> >
> > are, say, 10k blobs and none of them can be deleted - because of a
> > lease or an internal server error, for example - a long time would
> > be spent doing useless work before giving the user the chance to
> > retry or do something else.

I thought this might come up as I wrote that code. My reasoning is as Gavin's
here - most of the time you want this to be atomic, and the error reporting
for partial failures gets tricky.

> True, that's why I think we should use goroutines to remove the blobs in
> parallel rather than in sequence (same as what we've done in the MAAS
> provider in juju-core
> http://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/environs/maa
> s/storage.go#L1).

Premature optimisation. There is no use case to do that. If and when there
is, we can improve it.

> > - One common reason to call DeleteAllBlobs is so that you can delete
> >
> > the container straight afterwards. If even one blob fails to delete
> > it's the same as all the blobs not deleting.
>
> Well, that's definitely not the case when using DeleteAllBlobs() from the
> provider when destroying an environment (which is DeleteAllBlobs's raison
> d'être). The goal here is to be able to cleanup an existing container so
> that it can be re-used to re-deploy an new environment.

And if you leave undeleted things behind then it's still not cleaned
regardless of how many there are.

> > - The code is simpler.
>
> That one is undisputed :)

And that is the best reason :)

[Julian Edwards (julian-edwards)]

On Monday 01 Jul 2013 09:25:18 you wrote:
> Excellent :) I think this is really important, especially right now. I
> don't want to be chasing concurrency bugs before a deadline.

So if anyone wants to +1 this, that'd be great :)

[Gavin Panella (allenap)]

Looks good. Some minor points, that's all.

[1]

+debug_check:

Fwiw, make is fine with hyphens, so to safe an extra press of Shift
you could make this debug-check.</bikeshed>

[2]

+// RemoveAllBlobs requests a deletion of all the blobs in a container.
+// Azure only marks the blobs for deletion; they are garbage collected later.
+func (context *StorageContext) DeleteAllBlobs(request *DeleteAllBlobsRequest) error {

Fwiw, is the line about garbage collection relevant here? It sounds
just like a bit of Azure trivia, implementation detail that could go
stale. If it is relevant, could you add a comment explaining why?

[3]

+func (s *testDeleteAllBlobs) getListingResponse() *http.Response {

s/get/make/

[4]

+    c.Check(
+        transport.Exchanges[0].Request.URL.String(),
+        Matches, context.getContainerURL("container")+"?.*")

The ? at the end means something in a regex. How about:

+        Matches, context.getContainerURL("container")+"[?].*")

Fwiw, I prefer to use square brackets instead of \ escapes, because I
can't remember if the escape will be interpreted by the Go compiler or
the regex compiler. Square brackets makes it unambiguous.

[5]

+func (s *testDeleteAllBlobs) TestErrorsListing(c *C) {
...
+    err := context.DeleteAllBlobs(&DeleteAllBlobsRequest{Container: "c"})
+    c.Assert(err, NotNil)

Check that err is the error you expected, so that this tests doesn't
mask future mistakes. Here and in one other place.

review: Approve

[Raphaël Badin (rvb)]

> > Well, that's definitely not the case when using DeleteAllBlobs()
> > from the provider when destroying an environment (which is
> > DeleteAllBlobs's raison d'être). The goal here is to be able to
> > cleanup an existing container so that it can be re-used to re-deploy
> > an new environment.
>
> It may be DeleteAllBlobs's raison d'être for us, but GWACL is meant to
> be a general purpose Azure library in Go too, which is why I said
> "*One* common reason".

Trying to remove the other blobs if the deletion of one blob fails is not more generic, it's just a different behavior. Turns out it's closer to the behavior we need for the provider and thus it should be preferred IMH. We're not implementing random methods just for the fun of it :).

My main point is this: Azure can be quite unreliable. A container typically contains a bunch of files: the state of the whole environment and then one file (with a name that contains a random part) per deployed node (plus probably a bunch of other things). Destroying an environment without removing the file that contains the state of the env would be catastrophic but if we fail to remove one of the filed used to store the info about one node, it's really not that bad. Hence my suggestion: always try to remove all the files, because we really don't want to leave the "state file" behind.

[Raphaël Badin (rvb)]

> On Monday 01 Jul 2013 08:47:22 you wrote:
> > > - Continuing in spite of an error has a bad failure mode. if there
> > >
> > > are, say, 10k blobs and none of them can be deleted - because of a
> > > lease or an internal server error, for example - a long time would
> > > be spent doing useless work before giving the user the chance to
> > > retry or do something else.
>
> I thought this might come up as I wrote that code. My reasoning is as Gavin's
> here - most of the time you want this to be atomic, and the error reporting
> for partial failures gets tricky.

I'm still not sure about this, see my other comment.

> > True, that's why I think we should use goroutines to remove the blobs in
> > parallel rather than in sequence (same as what we've done in the MAAS
> > provider in juju-core
> > http://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/environs/maa
> > s/storage.go#L1).
>
> Premature optimisation. There is no use case to do that. If and when there
> is, we can improve it.

Very true, it's premature. I just mentioned it because of Gavin's remark.

[Gavin Panella (allenap)]

On 1 July 2013 11:41, Raphaël Badin <email address hidden> wrote:
> Trying to remove the other blobs if the deletion of one blob fails
> is not more generic, it's just a different behavior. Turns out it's
> closer to the behavior we need for the provider and thus it should
> be preferred IMH. We're not implementing random methods just for
> the fun of it :).

The method is DeleteAllBlobs, not DeleteMostBlobs ;)

>
> My main point is this: Azure can be quite unreliable. A container
> typically contains a bunch of files: the state of the whole
> environment and then one file (with a name that contains a random
> part) per deployed node (plus probably a bunch of other things).
> Destroying an environment without removing the file that contains
> the state of the env would be catastrophic but if we fail to remove
> one of the filed used to store the info about one node, it's really
> not that bad. Hence my suggestion: always try to remove all the
> files, because we really don't want to leave the "state file"
> behind.

I don't want heuristics in the provider, and especially not GWACL,
unless we absolutely must have them. If we can't delete all the files
that we want to delete, that's an error and we stop there.

We might be happy to be relaxed about success or failure for some
operations at the moment. We're in the midst of understanding Azure.
We're learning about things like the hostname length issue. Remember,
that looked like a reliability issue until you figured it out.

As we progress we will tighten things up and be less forgiving. We may
have to make exceptions, but I'd like those to be well understood and
narrow, rather than just saying that Azure can be unreliable.

If we do need to say things like "file X must go, but files Y and Z
can stay around" then DeleteAllBlobs is not the right thing to use.

A simple contract like "delete all the blobs, stop on the first error"
is not only simpler to code, it's simpler for callers to use.

A contract like "delete all the blobs, continue when there are errors,
but return the first error" or "delete all the blobs, continue when
there are errors, but stop after 5 errors, unless the file is called Y
or Z in which case ignore it" is much more complex to code. It also
adds a cognitive load to the programmer who wants to use it.

In any case, if there's an error from DeleteAllBlobs, what will most
programmers do next? I suspect the answer is: try again, or pass the
error up.

[Julian Edwards (julian-edwards)]

On Monday 01 Jul 2013 10:43:00 you wrote:
> > I thought this might come up as I wrote that code. My reasoning is as
> > Gavin's here - most of the time you want this to be atomic, and the error
> > reporting for partial failures gets tricky.
>
> I'm still not sure about this, see my other comment.

(Gah I didn't mean atomic)

As Gavin astutely pointed out, this method is called DeleteAllBlobs not
DeleteMostBlobs :)

If any of them fail to get deleted then it's an error. The way in which they
get deleted (or not) is an implementation detail.

[Julian Edwards (julian-edwards)]

On 01/07/13 20:38, Gavin Panella wrote:
> Review: Approve
>
> Looks good. Some minor points, that's all.
>
>
> [1]
>
> +debug_check:
>
> Fwiw, make is fine with hyphens, so to safe an extra press of Shift
> you could make this debug-check.</bikeshed>

I made it "debug-test" as that's better reflecting what it's really doing.

>
>
> [2]
>
> +// RemoveAllBlobs requests a deletion of all the blobs in a container.
> +// Azure only marks the blobs for deletion; they are garbage collected later.
> +func (context *StorageContext) DeleteAllBlobs(request *DeleteAllBlobsRequest) error {
>
> Fwiw, is the line about garbage collection relevant here? It sounds
> just like a bit of Azure trivia, implementation detail that could go
> stale. If it is relevant, could you add a comment explaining why?

It's pertinent to set user expectations really, the blobs won't be in a
deleted state when the call returns. I'll try to reword the comment to
remove the trivia and state the results of the call.

>
>
> [3]
>
> +func (s *testDeleteAllBlobs) getListingResponse() *http.Response {
>
> s/get/make/

Fixed.

>
>
> [4]
>
> + c.Check(
> + transport.Exchanges[0].Request.URL.String(),
> + Matches, context.getContainerURL("container")+"?.*")
>
> The ? at the end means something in a regex. How about:
>
> + Matches, context.getContainerURL("container")+"[?].*")
>
> Fwiw, I prefer to use square brackets instead of \ escapes, because I
> can't remember if the escape will be interpreted by the Go compiler or
> the regex compiler. Square brackets makes it unambiguous.

I think I copied this from one of your tests :)

BTW we desperately need to fix the name of TestTransport2, it's
proliferating and I cringe every time I use it. I've added a card on
the board.

>
>
> [5]
>
> +func (s *testDeleteAllBlobs) TestErrorsListing(c *C) {
> ...
> + err := context.DeleteAllBlobs(&DeleteAllBlobsRequest{Container: "c"})
> + c.Assert(err, NotNil)
>
> Check that err is the error you expected, so that this tests doesn't
> mask future mistakes. Here and in one other place.
>

Wilco.

Thanks for the review.

[lp:~julian-edwards/gwacl/removeall]

151. By Julian Edwards on 2013-07-02

merge trunk

152. By Julian Edwards on 2013-07-02

tweaks and improvements from Gavin's review

=== modified file 'Makefile'
--- Makefile	2013-06-21 04:01:58 +0000
+++ Makefile	2013-07-02 02:31:26 +0000
@@ -2,6 +2,11 @@
 check: examples
 	go test -gocheck.v=true ./...
 
+debug-test:
+	go test -c -gcflags "-N -l"
+	gdb gwacl.test
+	$(RM) gwacl.test
+
 all_source := $(shell find . -name '*.go' ! -name '*_test.go')
 
 example_source := $(wildcard example/*/run.go)
@@ -22,4 +27,4 @@
 %: %.go $(all_source)
 	go build -o $@ $<
 
-.PHONY: check clean format examples
+.PHONY: check clean format examples debug_check

=== modified file 'storage.go'
--- storage.go	2013-06-26 13:42:49 +0000
+++ storage.go	2013-07-02 02:31:26 +0000
@@ -83,6 +83,31 @@
     return batch, nil
 }
 
+type DeleteAllBlobsRequest struct {
+    Container string
+    // Other params possible, add later.
+}
+
+// RemoveAllBlobs requests a deletion of all the blobs in a container.
+// The blobs are not deleted immediately, so when this call returns they
+// may still be present for a while.
+func (context *StorageContext) DeleteAllBlobs(request *DeleteAllBlobsRequest) error {
+    blobs, err := context.ListAllBlobs(&ListBlobsRequest{
+        Container: request.Container})
+    if err != nil {
+        return err
+    }
+
+    for _, blob := range blobs.Blobs {
+        err := context.DeleteBlob(request.Container, blob.Name)
+        if err != nil {
+            return err
+        }
+    }
+
+    return nil
+}
+
 // ListAllContainers requests from the storage service a list of containers
 // in the storage account.
 func (context *StorageContext) ListAllContainers() (*ContainerEnumerationResults, error) {

=== modified file 'storage_test.go'
--- storage_test.go	2013-06-27 11:53:50 +0000
+++ storage_test.go	2013-07-02 02:31:26 +0000
@@ -344,6 +344,95 @@
     c.Check(containers.Containers[1].Name, Equals, lastContainer)
 }
 
+type testDeleteAllBlobs struct{}
+
+var _ = Suite(&testDeleteAllBlobs{})
+
+func (s *testDeleteAllBlobs) makeListingResponse() *http.Response {
+    return &http.Response{
+        Status:     fmt.Sprintf("%d", http.StatusOK),
+        StatusCode: http.StatusOK,
+        Body: makeResponseBody(`
+            <?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-name</Name>
+                </Blob>
+                <Blob>
+                  <Name>blob-name2</Name>
+                </Blob>
+              </Blobs>
+              <NextMarker />
+            </EnumerationResults>`),
+    }
+}
+
+func (s *testDeleteAllBlobs) TestHappyPath(c *C) {
+    listResponse := s.makeListingResponse()
+    deleteResponse := &http.Response{
+        Status:     fmt.Sprintf("%d", http.StatusAccepted),
+        StatusCode: http.StatusAccepted,
+    }
+
+    transport := &TestTransport2{}
+    transport.AddExchange(listResponse, nil)
+    transport.AddExchange(deleteResponse, nil)
+    transport.AddExchange(deleteResponse, nil)
+    context := makeStorageContext(transport)
+
+    err := context.DeleteAllBlobs(&DeleteAllBlobsRequest{Container: "container"})
+    c.Assert(err, IsNil)
+    c.Assert(transport.ExchangeCount, Equals, 3)
+
+    // Check the ListAllBlobs exchange.
+    c.Check(
+        transport.Exchanges[0].Request.URL.String(),
+        Matches, context.getContainerURL("container")+"[?].*")
+    c.Check(transport.Exchanges[0].Request.URL.Query(),
+        DeepEquals, url.Values{
+            "restype": {"container"},
+            "comp":    {"list"},
+    })
+
+    // Check the DeleteBlob exchanges.
+    c.Check(
+        transport.Exchanges[1].Request.URL.String(),
+        Equals, context.GetFileURL("container", "blob-name"))
+    c.Check(transport.Exchanges[1].Request.Method, Equals, "DELETE")
+
+    c.Check(
+        transport.Exchanges[2].Request.URL.String(),
+        Equals, context.GetFileURL("container", "blob-name2"))
+    c.Check(transport.Exchanges[2].Request.Method, Equals, "DELETE")
+}
+
+func (s *testDeleteAllBlobs) TestErrorsListing(c *C) {
+    transport := &TestTransport2{}
+    transport.AddExchange(&http.Response{
+        Status:     fmt.Sprintf("%d", http.StatusNotFound),
+        StatusCode: http.StatusNotFound}, nil)
+    context := makeStorageContext(transport)
+    err := context.DeleteAllBlobs(&DeleteAllBlobsRequest{Container: "c"})
+    c.Assert(err, ErrorMatches, `request for blobs list failed: Azure request failed \(404: Not Found\)`)
+}
+
+func (s *testDeleteAllBlobs) TestErrorsDeleting(c *C) {
+    transport := &TestTransport2{}
+    listResponse := s.makeListingResponse()
+    transport.AddExchange(listResponse, nil)
+    transport.AddExchange(&http.Response{
+        Status:     fmt.Sprintf("%d", http.StatusBadRequest),
+        StatusCode: http.StatusBadRequest}, nil)
+    context := makeStorageContext(transport)
+    err := context.DeleteAllBlobs(&DeleteAllBlobsRequest{Container: "c"})
+    c.Assert(err, ErrorMatches, `failed to delete blob blob-name: Azure request failed \(400: Bad Request\)`)
+}
+
 type testCreateInstanceDataVHD struct{}
 
 var _ = Suite(&testCreateInstanceDataVHD{})