Azure provider: clean up storage last in Destroy()

Azure provider: clean up storage last in Destroy()

This branch changes the order of what environ.Destroy() does to make sure the storage is cleaned up last.  This is to ensure that, if something goes wrong in Destroy() (if, for instance, the Virtual Network cannot be cleaned up), the state-info file in the storage won't be cleaned up.  This way, if someone tries to bootstrap the environment again, the error message will be meaningful.

Drive-by fix: Fix the test 'TestDestroyCleansUpStorage': populate the storage before destroying the environment so that the check that verifies that the storage is empty would fail if Destroy() wouldn't have been called.

https://code.launchpad.net/~rvb/juju-core/az-destroy-clean-up-last/+merge/180326

(do not edit description out of merge proposal)

[rvb, 2013-08-15 12:02:14]

Please take a look.

[jtv.canonical, 2013-08-16 08:05:42]

It's not clear to me what advantage this brings — what is the more meaningful
error that the user gets?  And how is the user going to get the remains cleaned
up, particularly those that cost money?

My feeling is that Destroy() should be as robust as it can, and not give up
easily.

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

https://codereview.appspot.com/12850045/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:789: // up and thus a tentative to re-boostrap the
environment will lead to

A tentative?  Did you mean an attempt?

[rvb, 2013-08-16 08:10:36]

On 2013/08/16 08:05:42, jtv.canonical wrote:
> It's not clear to me what advantage this brings — what is the more meaningful
> error that the user gets?  And how is the user going to get the remains
cleaned
> up, particularly those that cost money?
> 
> My feeling is that Destroy() should be as robust as it can, and not give up
> easily.

If Destroy() fails (I'm thinking about a transient error like a conflict) and
thus the env is not completely destroyed, if one tries to bootstrap the env
again, we want the error to be:
" error: environment is already bootstrapped"

This happens only if the 'provider-state' file is still in the storage.

Hence this change (which, btw, is in sync with what the openstack/ec2 providers
do).

> https://codereview.appspot.com/12850045/diff/1/environs/azure/environ.go
> File environs/azure/environ.go (right):
> 
>
https://codereview.appspot.com/12850045/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:789: // up and thus a tentative to re-boostrap the
> environment will lead to
> 
> A tentative?  Did you mean an attempt?

Probably :)

[rvb, 2013-08-16 08:18:18]

Please take a look.

[jtv.canonical, 2013-08-16 08:18:52]

That is definitely a much clearer message than you're otherwise likely to get. 
LGTM, although I still prefer a Destroy that doesn't give up on the first sign
of trouble!

[rvb, 2013-08-16 08:25:08]

On 2013/08/16 08:18:52, jtv.canonical wrote:
> That is definitely a much clearer message than you're otherwise likely to get.

> LGTM, although I still prefer a Destroy that doesn't give up on the first sign
> of trouble!

Thanks for the review!

Destroy(), as all the other gwacl methods used in the provider, benefits from
the work I recently landed and uses a common "retry policy".  It means that all
the requests that result in 409 conflict, 500 server error and a couple of other
status codes will be retried a couple of times before we give up.  So Destroy()
really doesn't give up on the first sign of trouble.

The work here is meant to deal more gracefully (as in "present a better error")
that we currently do when it comes to Azure errors from which we can't recover
or that can't be retried or if someone manually kills the "juju
destroy-environment" command.

[rog, 2013-08-16 08:27:40]

LGTM