provider/azure: enable load-balancing/availability

provider/azure: enable load-balancing/availability

This change is required to enable Availability Set
support for Azure. The existing implementation was
fundamentally incompatible with Availability Sets,
so the implementation has been significantly altered,
while maintaining backwards compatibility.

In the old implementation, instances are associated
one-to-one with Cloud Services. In the new
implementation, instances are associated with Roles
within a Cloud Service; there may be multiple Roles
within a single Cloud Service.

If a service is exposed, then its ports will be load-
balanced across each instance within the cloud service
which opens that port. We currently open ports 22 for
all instances, and the Mongo and API server ports for
state server instances. State server instances are
allocated to a single Cloud Service.

There are a number of incidental improvements, e.g.
using an updated version of DeleteHostedService which
destroys associated disks/blobs server side.

All instances are now allocated to an Availability Set
called "juju", which is scoped to the Cloud Service.
We do not currently group units into Cloud Services
automatically, however the ground work is laid to do
this. A follow-up will implement this work.

When we enable grouping, the same time we will have to
make changes to how we connect to machines via SSH,
either by proxying through an arbitrary machine in the
Cloud Service, or by not load balancing port 22 and
connecting to the machine's auto-allocated public port.

https://code.launchpad.net/~axwalk/juju-core/azure-provider-roleisinstance/+merge/210353

(do not edit description out of merge proposal)

[axw, 2014-03-12 13:28:32]

Please take a look.

[thumper, 2014-03-13 04:15:05]

Lots of changes here, comments below.

I'm hoping that CI for Azure would help catch any actual problems here as it is
a bit beyond a simple review :-)

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:232: if err, ok := err.(*gwacl.AzureError); ok {
oh... didn't know you could do that...

reassigning to err I mean...

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:443: func (env *azureEnviron) createRole(azure
*gwacl.ManagementAPI, role *gwacl.Role, label string) (resultInst
instance.Instance, resultErr error) {
it isn't entirely clear why 'createRole' returns an instance

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:589: case deploymentNameV2(hostedService.ServiceName):
perhaps a blank before this line? wrapping lines makes it harder to pick out.

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:636: // don't load balance SSH and provie a way of
getting the
s/provie/provide/

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:732: } else if len(service.Deployments) != 1 {
why skip services where we have more than one deployment?

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ_tes...
File provider/azure/environ_test.go (right):

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ_tes...
provider/azure/environ_test.go:978: c.Check(inst1, gc.FitsTypeOf,
&azureInstance{})
Hmm... didn't know about this checker, but perhaps you should make that one an
Assert not a Check, because if it fails, the next line will panic.

https://codereview.appspot.com/73910043/diff/20001/provider/azure/instance.go
File provider/azure/instance.go (right):

https://codereview.appspot.com/73910043/diff/20001/provider/azure/instance.go...
provider/azure/instance.go:95: azInstance.environ.getVirtualNetworkName(),
Which parts of the *azureInstance are being protected by the mutex?

https://codereview.appspot.com/73910043/diff/20001/provider/azure/instance_te...
File provider/azure/instance_test.go (right):

https://codereview.appspot.com/73910043/diff/20001/provider/azure/instance_te...
provider/azure/instance_test.go:18: type instanceSuite struct {
Normally at least want to include the testbase.LoggingSuite to capture the
logging.

[axw, 2014-03-13 04:48:27]

Please take a look.

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:443: func (env *azureEnviron) createRole(azure
*gwacl.ManagementAPI, role *gwacl.Role, label string) (resultInst
instance.Instance, resultErr error) {
On 2014/03/13 04:15:05, thumper wrote:
> it isn't entirely clear why 'createRole' returns an instance

Renamed to instance. Instance and role are equivalent, but I'll keep it to Juju
terminology.

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:589: case deploymentNameV2(hostedService.ServiceName):
On 2014/03/13 04:15:05, thumper wrote:
> perhaps a blank before this line? wrapping lines makes it harder to pick out.

Done.

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:636: // don't load balance SSH and provie a way of
getting the
On 2014/03/13 04:15:05, thumper wrote:
> s/provie/provide/

Done.

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:732: } else if len(service.Deployments) != 1 {
On 2014/03/13 04:15:05, thumper wrote:
> why skip services where we have more than one deployment?

Because if there's more than one, that means someone's been mucking about with
the objects. Juju only creates one deployment per Cloud Service. You can have up
to two (Prod & Dev), but we only do Production.

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ_tes...
File provider/azure/environ_test.go (right):

https://codereview.appspot.com/73910043/diff/20001/provider/azure/environ_tes...
provider/azure/environ_test.go:978: c.Check(inst1, gc.FitsTypeOf,
&azureInstance{})
On 2014/03/13 04:15:05, thumper wrote:
> Hmm... didn't know about this checker, but perhaps you should make that one an
> Assert not a Check, because if it fails, the next line will panic.

Done.

https://codereview.appspot.com/73910043/diff/20001/provider/azure/instance.go
File provider/azure/instance.go (right):

https://codereview.appspot.com/73910043/diff/20001/provider/azure/instance.go...
provider/azure/instance.go:95: azInstance.environ.getVirtualNetworkName(),
On 2014/03/13 04:15:05, thumper wrote:
> Which parts of the *azureInstance are being protected by the mutex?

Only roleInstance (I've been told the convention is to put those fields that are
guarded below the mutex). All others are immutable and thread-safe.

https://codereview.appspot.com/73910043/diff/20001/provider/azure/instance_te...
File provider/azure/instance_test.go (right):

https://codereview.appspot.com/73910043/diff/20001/provider/azure/instance_te...
provider/azure/instance_test.go:18: type instanceSuite struct {
On 2014/03/13 04:15:05, thumper wrote:
> Normally at least want to include the testbase.LoggingSuite to capture the
> logging.

Done.

[fwereade, 2014-03-19 09:54:08]

pre-comment for discussion

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:554: if machineConfig.StateServer {
oooh, this is going to break sometime. HA state server nodes will not
necessarily be started with this field set. How much will we depend upon this?
If it's just cosmetic it's maybe not such a big deal, but really I think it's up
to juju itself to supply a sensible label to be applied if possible.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:566: role := env.newRole(instanceType, vhd, userData,
machineConfig.StateServer)
heh, this is the same problem, but more significant. We'll have to start
distinguishing between StateServer and BootstrapMachine or something in
MachineConfig -- StateServer wouldn't affect cloudinit (because the MA will turn
itself into a stateserver when it connects) but can be handled in this case,
while Bootstrap will be the thing that controls the special...

wait. This is more complicated than I thought. Would you ping me when you're
back from supper please?

[rog, 2014-03-19 11:44:35]

LGTM with some thoughts and suggestions below.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:407: labelBase64 :=
base64.StdEncoding.EncodeToString([]byte(label))
Shouldn't gwacl be doing this decoding, given that it's doing the encoding
itself? (it seems a bit odd that base-64 encoding is required - the azure docs
distinctly show a non-base64-encoded example)

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:443: // createInstance creates all of the necessary
Azure entities in order
// createInstance creates all of the Azure entities
// necessary for a new instance. ... 

?

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:554: if machineConfig.StateServer {
On 2014/03/19 09:54:08, fwereade wrote:
> oooh, this is going to break sometime. HA state server nodes will not
> necessarily be started with this field set. How much will we depend upon this?
> If it's just cosmetic it's maybe not such a big deal, but really I think it's
up
> to juju itself to supply a sensible label to be applied if possible.

I believe that it's not possible to assign a machine to an availability set
after it's been started, so unless this field *is* set at instance start time,
there's nothing we can do.

Perhaps it *might* make sense to add a "group" (or something similarly named)
field in MachineConfig. Then all the logic for deciding which machines are
grouped together can be outside of the provider logic (the providers can remain
blissfully ignorant of juju services); different providers might decide for
themselves how to interpret the group (ignoring it would be acceptable).

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go
File provider/azure/instance.go (right):

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go...
provider/azure/instance.go:29: mu           sync.RWMutex
is an RWMutex really worth it here? i'd just use a mutex - it's smaller, and
space is a more likely problem than time in this case, i'd say.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go...
provider/azure/instance.go:93: ip,
could we name the fields here please?

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go...
provider/azure/instance.go:96: instance.NetworkCloudLocal})
trivial formatting: i think we usually put the } on a new line, so that all the
fields look the same.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go...
provider/azure/instance.go:98: } else {
s/} else {/}/

(save a level of indentation)

[axw, 2014-03-19 11:54:31]

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:554: if machineConfig.StateServer {
On 2014/03/19 09:54:08, fwereade wrote:
> oooh, this is going to break sometime. HA state server nodes will not
> necessarily be started with this field set. How much will we depend upon this?
> If it's just cosmetic it's maybe not such a big deal, but really I think it's
up
> to juju itself to supply a sensible label to be applied if possible.

Based on discussion on IRC, I will change this to use DistributionInstances from
the other CL. DistributionInstances will return state server instances if the
machine being provisioned is a state server, otherwise it will return common
service instances.

The whole label thing will just disappear, since the cloud service can be
extracted from the instance ID.

[axw, 2014-04-03 02:42:43]

Oops, never did mail this.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:407: labelBase64 :=
base64.StdEncoding.EncodeToString([]byte(label))
On 2014/03/19 11:44:35, rog wrote:
> Shouldn't gwacl be doing this decoding, given that it's doing the encoding
> itself? (it seems a bit odd that base-64 encoding is required - the azure docs
> distinctly show a non-base64-encoded example)

I agree, the base64 encoding/decoding is a bit clumsy and leaky. I don't think
we'll be using the labels much longer, though.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:443: // createInstance creates all of the necessary
Azure entities in order
On 2014/03/19 11:44:35, rog wrote:
> // createInstance creates all of the Azure entities
> // necessary for a new instance. ... 
> 
> ?

Done.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go
File provider/azure/instance.go (right):

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go...
provider/azure/instance.go:29: mu           sync.RWMutex
On 2014/03/19 11:44:35, rog wrote:
> is an RWMutex really worth it here? i'd just use a mutex - it's smaller, and
> space is a more likely problem than time in this case, i'd say.

Done.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go...
provider/azure/instance.go:93: ip,
On 2014/03/19 11:44:35, rog wrote:
> could we name the fields here please?

Done.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go...
provider/azure/instance.go:96: instance.NetworkCloudLocal})
On 2014/03/19 11:44:35, rog wrote:
> trivial formatting: i think we usually put the } on a new line, so that all
the
> fields look the same.

Yeah, this is existing code, I usually do that too. I've reformatted it.

https://codereview.appspot.com/73910043/diff/40001/provider/azure/instance.go...
provider/azure/instance.go:98: } else {
On 2014/03/19 11:44:35, rog wrote:
> s/} else {/}/
> 
> (save a level of indentation)

Done.

[axw, 2014-04-03 07:19:42]

Please take a look.

[fwereade, 2014-04-03 07:31:15]

Thanks, this is great. LGTM.

https://codereview.appspot.com/73910043/diff/60001/provider/azure/instance.go
File provider/azure/instance.go (right):

https://codereview.appspot.com/73910043/diff/60001/provider/azure/instance.go...
provider/azure/instance.go:56: // TODO(axw) map instance status to something
more useful?
It's considered ok for status to use the provider's vocabulary.

https://codereview.appspot.com/73910043/diff/60001/provider/azure/instance.go...
provider/azure/instance.go:188: // concern.
This bothers me a bit, but it feels more like an azure problem than our problem.

[axw, 2014-04-03 07:39:07]

Please take a look.

https://codereview.appspot.com/73910043/diff/60001/provider/azure/instance.go
File provider/azure/instance.go (right):

https://codereview.appspot.com/73910043/diff/60001/provider/azure/instance.go...
provider/azure/instance.go:56: // TODO(axw) map instance status to something
more useful?
On 2014/04/03 07:31:15, fwereade wrote:
> It's considered ok for status to use the provider's vocabulary.

Removed.