Azure provider: create virtual network.

Azure provider: create virtual network.

Each azure environment must be associated with an affinity group and a virtual network used for direct machine-to-machine communication. This branch adds all the utilities to create and delete the environment's affinity group and virtual network.  The Bootstrap() method is still being refactored and is thus left untested for now.  Branches to remove the environment's affinity group and virtual network (when the environment is destroyed) and use the private address for machine-to-machine communication will follow.

https://code.launchpad.net/~rvb/juju-core/create-vnet/+merge/175315

(do not edit description out of merge proposal)

[rvb, 2013-07-17 15:17:09]

Please take a look.

[jtv.canonical, 2013-07-18 07:51:29]

LGTM but I have some notes and suggestions.  As you expect.

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

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:157: func (env *azureEnviron) getAffinityGroupName()
string {

For an identifying name like this, I generally prefer to test these two simple
but essential properties:

1. The name is consistent.  In this case, a single environment always returns
the same affinity-group name.

2. The name identifies.  In this case, two envs with different prefixes always
return different affinity-group names.

Your test that the environment's name is included probably implies the second
property already, though I'd still see them as different matters: including the
name is for clarity, but having different names for different environments is
for identification.

(The same, mutatis mutandi, goes for getVirtualNetworkName.)

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:158: return env.getEnvPrefix() + "-ag"

In getAffinityGroupName, do we know that dashes are OK in affinity-group names? 
The Azure documentation I looked at didn't say anything about what was allowed. 
Probably fine, but...

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:167: affinityGroupName := env.getAffinityGroupName()

In createAffinityGroup, unless there's Azure terminology you really have to
stick to, the variable affinityGroupName could be just "name."  Easier to read,
I think, and the context is explicit enough.

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:179: affinityGroupName := env.getAffinityGroupName()

The same goes in destroyAffinityGroup: just "name" would do for me.

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:180: return
azure.DeleteAffinityGroup(&gwacl.DeleteAffinityGroupRequest{

Do we know what happens if we destroy an affinity group twice?  I suppose it
might happen if Bootstrap creates the group successfully, defers its cleanup in
the event of an error, moves on, fails somewhere else, and then tries to destroy
the whole environment.

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:187: return env.getEnvPrefix() + "-vnet"

In getVirtualNetworkName, the same just-making-sure question: are dashes OK in
virtual-network names?

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:190: func (env *azureEnviron) createVirtualNetwork()
error {

In createVirtualNetwork, just on general principle (and nothing important), I'd
prefer it if you did your data gathering before grabbing a releaseManagementAPI.
 It shortens the span of code where the matching "defer" lurks in the
background, as well as the lifetime of the variable, and so it very slightly
reduces the cognitive load on engineers working on this method.

[rvb, 2013-07-18 08:24:51]

Thanks for the review!

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

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:158: return env.getEnvPrefix() + "-ag"
On 2013/07/18 07:51:29, jtv.canonical wrote:
> 
> In getAffinityGroupName, do we know that dashes are OK in affinity-group
names? 
> The Azure documentation I looked at didn't say anything about what was
allowed. 
> Probably fine, but...

Yes, it's fine, I tested it.

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:167: affinityGroupName := env.getAffinityGroupName()
On 2013/07/18 07:51:29, jtv.canonical wrote:
> 
> In createAffinityGroup, unless there's Azure terminology you really have to
> stick to, the variable affinityGroupName could be just "name."  Easier to
read,
> I think, and the context is explicit enough.

Hum, I'd rather keep very explicit variables names.

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:180: return
azure.DeleteAffinityGroup(&gwacl.DeleteAffinityGroupRequest{
On 2013/07/18 07:51:29, jtv.canonical wrote:
> 
> Do we know what happens if we destroy an affinity group twice?  I suppose it
> might happen if Bootstrap creates the group successfully, defers its cleanup
in
> the event of an error, moves on, fails somewhere else, and then tries to
destroy
> the whole environment.

It breaks.  That's a bug in gwacl. I'll report it.

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:187: return env.getEnvPrefix() + "-vnet"
On 2013/07/18 07:51:29, jtv.canonical wrote:
> 
> In getVirtualNetworkName, the same just-making-sure question: are dashes OK in
> virtual-network names?

Yes, it's okay.

https://codereview.appspot.com/11433044/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:190: func (env *azureEnviron) createVirtualNetwork()
error {
On 2013/07/18 07:51:29, jtv.canonical wrote:
> 
> In createVirtualNetwork, just on general principle (and nothing important),
I'd
> prefer it if you did your data gathering before grabbing a
releaseManagementAPI.
>  It shortens the span of code where the matching "defer" lurks in the
> background, as well as the lifetime of the variable, and so it very slightly
> reduces the cognitive load on engineers working on this method.

Okay.