Azure provider: delete Virtual Network

Azure provider: delete Virtual Network

This branch adds the deletion of the environment's Virtual Network and Affinity Group when the environment is destroyed.  I could have divided the test TestDestroyDeletesVirtualNetworkAndAffinityGroup into two separate tests but since the initialization of the test is pretty long and, conceptually, the Virtual Network and the Affinity Group are linked together, I decided to go with one test.

https://code.launchpad.net/~rvb/juju-core/delete-vnet-ag/+merge/175787

(do not edit description out of merge proposal)

[rvb, 2013-07-19 09:58:16]

Please take a look.

[jtv.canonical, 2013-07-19 10:30:04]

If you feel that deleting the network and deleting the affinity group form a
logical unit, it would have made sense to make them a single function.  The test
would then have broken down naturally into two parts: unit-testing the new
function (but without the overhead of first simulating a Destroy call) and
testing that Destroy calls the cleanup (but without testing its details).  It
would have made it a bit easier to test for error behaviour as well.

[jtv.canonical, 2013-07-19 10:30:48]

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

https://codereview.appspot.com/11404044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:697: err = env.StopInstances(insts)

Do we know that StopInstances is idempotent?  In other words, if all instances
have already been stopped during an abortive previous attempt, do we know that
that fact won't cause StopInstances to fail?  Otherwise we might get into a
state where we can't clean up the virtual network and affinity group.

https://codereview.appspot.com/11404044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:703: err = env.deleteVirtualNetwork()

For the record, of course deleteVirtualNetwork needs to be idempotent as well —
otherwise a failure during this stage of cleanup could get the environment into
a state where the affinity group can't be cleaned up.

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

https://codereview.appspot.com/11404044/diff/1/environs/azure/environ_test.go...
environs/azure/environ_test.go:591: func
getVnetAndAffinityGroupCleanupResponses(c *C) []gwacl.DispatcherResponse {

That's quite a mouthful!  Could you summarize it in a comment?  Maybe the "get"
should be "make" — because I think you're actually "creating" them, not
"getting" them.

[rvb, 2013-07-19 10:41:50]

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

https://codereview.appspot.com/11404044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:697: err = env.StopInstances(insts)
On 2013/07/19 10:30:48, jtv.canonical wrote:
> 
> Do we know that StopInstances is idempotent?  In other words, if all instances
> have already been stopped during an abortive previous attempt, do we know that
> that fact won't cause StopInstances to fail?  Otherwise we might get into a
> state where we can't clean up the virtual network and affinity group.

Yes, StopInstances is idemportent as it essentially calls
gwacl.DestroyHostedService() which is idempotent.

https://codereview.appspot.com/11404044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:703: err = env.deleteVirtualNetwork()
On 2013/07/19 10:30:48, jtv.canonical wrote:
> 
> For the record, of course deleteVirtualNetwork needs to be idempotent as well
—
> otherwise a failure during this stage of cleanup could get the environment
into
> a state where the affinity group can't be cleaned up.

Yes, deleteVirtualNetwork is idempotent.  I reported the problem yesterday and
it has been fixed.

[jtv.canonical, 2013-07-19 10:59:25]

Thanks for making sure.  Consider adding comments so future maintainers don't
have to wonder.  LGTM.

[rvb, 2013-07-19 11:56:16]

> That's quite a mouthful!  Could you summarize it in a comment?  Maybe the
"get"
> should be "make" — because I think you're actually "creating" them, not
> "getting" them.

I've added a comment, thanks for the review!

[rvb, 2013-07-19 11:59:14]

On 2013/07/19 10:30:04, jtv.canonical wrote:
> If you feel that deleting the network and deleting the affinity group form a
> logical unit, it would have made sense to make them a single function.  The
test
> would then have broken down naturally into two parts: unit-testing the new
> function (but without the overhead of first simulating a Destroy call) and
> testing that Destroy calls the cleanup (but without testing its details).  It
> would have made it a bit easier to test for error behaviour as well.

Well, this constitutes a logical unit in a sense but I decided against creating
a separate method for it because it would, in fact, complicate the testing (or
rather, make it much more verbose for very little benefit).  Besides, there
wouldn't be any logic in that method except a call to the two methods.  This is
just made less obvious because of the "error dance" Go imposes on us.