Part of the work to start an Azure instance.

Part of the work to start an Azure instance.

The design was discussed with the team: create Service, create Deployment, get
Deployment's full DNS name, set DNS name as label on the Service so that
Instances() and AllInstances() can retrieve all the immediately required
information for instances without having to query each Deployment
individually.  (You'll note that this creates a state where an instance has no
DNS name yet; other providers have this state too, but the Azure one did not
have it yet.  Thanks to the extracted generic implementation of WaitDNS, there
is no change to the WaitDNS implementation.)

You'll note that internalStartInstances() is not connected to StartInstances
yet.  The plan is to refactor StartInstances such that the code that's
currently duplicated between all providers' StartInstances and Bootstrap
methods can be extracted and build on a single StartInstances method in the
Environ interface.  Currently all providers have an "internal" and an
"external" StartInstances, with Bootstrap and the external StartInstances both
building on the internal StartInstances.  In this branch you see only the
internal one for now.  It's not done yet, but the diff had gotten big enough
that it was time to get something landed.

Some of this code is tied to the "one instance, one deployment, one service"
model.  We're probably going to ditch that model before too long, and so (as
Raphaël did in an earlier branch) everything that depends on it is tagged with
"(instance==service)" in the comments.  Searching for that string should give
us a quick listing of things that need changing when we go to work on that
assumption.

https://code.launchpad.net/~jtv/juju-core/az-startinstance/+merge/173637

(do not edit description out of merge proposal)

[jtv.canonical, 2013-07-09 03:59:28]

Please take a look.

[rvb, 2013-07-09 09:23:10]

Nice branch!  Thanks for starting the work on StartInstance which is really the
provider's "pièce de résistance". LGTM but my comment about
extractDeploymentHostname needs addressing I think.

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

https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:147: return fmt.Sprintf("-(creating: %s)-",
serviceName)

Wouldn't it be simpler to use a fixed string here?  Something like
"-(creating)-"?  This way "isProvisionalServiceLabel" would be simpler.

https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:163: const location = "East US"

This is probably something we will want to turn into a config item soon.  We
will probably have a bunch of these hardcoded constants that we will turn info
config items so I suggest defining them all next to one another at the beginning
of the file so that we can spot them easily.  Maybe move the definition of
'location' out of the method and put a TODO.

https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:170: if isAzureError && azErr.HTTPStatus ==
http.StatusConflict {

I'm afraid that we might have to repeat that strategy (or retying things when we
get a "conflict" response back) in other places as well.  It's too early to do
this in a generic fashion now but I wonder if this should not be in gwacl
itself.  Just a thought.

https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:200: func extractDeploymentHostname(instanceURL
string) (string, error) {
There is a slight impedance mismatch with the code I've done recently, see
instance.go:DNSName().  DNSName assumes that the label contain the hostname of
the service, not the FQDN (i.e. myservice instead of myservice.cloudapp.net). 
You need to change either your code or the code in instance.go.

https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:263: azure.DeleteHostedService(serviceName)

It's probably better to use DestroyHostedService here.  Compared to
DeleteHostedService, it also cleans up any disks used by the deployments inside
the service.

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

https://codereview.appspot.com/11027043/diff/1/environs/azure/environ_test.go...
environs/azure/environ_test.go:501: for counter := 0; counter < 100; counter++ {

It's a detail but maybe the number of tries could be turned into a private
variable and this test could use nbTries + 1.  Not really because it saves the
creation of a few objects but because it explains how the code works a bit
better, telling the reader where to look if he wonders how many times things are
retried in case of failure.

[jtv.canonical, 2013-07-09 09:57:34]

On 2013/07/09 09:23:10, rvb wrote:

>
https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:147: return fmt.Sprintf("-(creating: %s)-",
> serviceName)
> 
> Wouldn't it be simpler to use a fixed string here?  Something like
> "-(creating)-"?  This way "isProvisionalServiceLabel" would be simpler.

Not much simpler.  And the change is entirely encapsulated inside that function,
so the impact is minimal.  I started out with a fixed label originally, but
chose to go with variable labels for several reasons:

1. This way we can display something useful for human consumption.  Given that
the label is mandatory in the first place, this seems to be really important.

2. We'd be much more vulnerable to changes in the exact text of the provisional
label.  I wanted something robust, and anything that isn't a valid DNS name
would do.

3. Given that the label is required, there might even conceivably be a
uniqueness requirement within some context, and I couldn't be bothered to find
out or deal with it.

>
https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:163: const location = "East US"
> 
> This is probably something we will want to turn into a config item soon.  We
> will probably have a bunch of these hardcoded constants that we will turn info
> config items so I suggest defining them all next to one another at the
beginning
> of the file so that we can spot them easily.  Maybe move the definition of
> 'location' out of the method and put a TODO.

Done.


>
https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:170: if isAzureError && azErr.HTTPStatus ==
> http.StatusConflict {
> 
> I'm afraid that we might have to repeat that strategy (or retying things when
we
> get a "conflict" response back) in other places as well.  It's too early to do
> this in a generic fashion now but I wonder if this should not be in gwacl
> itself.  Just a thought.

The part that I'd *really* like to extract into gwacl is the ability to
recognize this particular kind of conflict error.  I don't think it'd be wise to
pull the retrying into gwacl unless we can be sure we know what we're doing. 
Other calls might have other kind of conflicts — it's even possible that this
call has some other way of incurring this type of error, and it's annoying as
hell.


>
https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:200: func extractDeploymentHostname(instanceURL
> string) (string, error) {
> There is a slight impedance mismatch with the code I've done recently, see
> instance.go:DNSName().  DNSName assumes that the label contain the hostname of
> the service, not the FQDN (i.e. myservice instead of
http://myservice.cloudapp.net). 
> You need to change either your code or the code in instance.go.

Ah.  I built my change on the old one, which used the FQDN.

This raises an interesting question: if what we get back this way is simply the
service name, then we don't need the whole label dance.  Something to keep in
mind for later perhaps.

Meanwhile, I wasn't sure that the fixed domain is actually guaranteed.  I see it
documented in an example, but the problem with an example is that it's just an
example, and may make hidden assumptions.  Do you see any solid documentation
for this fixed domain name?


>
https://codereview.appspot.com/11027043/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:263: azure.DeleteHostedService(serviceName)
> 
> It's probably better to use DestroyHostedService here.  Compared to
> DeleteHostedService, it also cleans up any disks used by the deployments
inside
> the service.

Done.  It shouldn't matter much because I defer deletion of the entire instance
if it comes to that, but we might as well be thorough in the face of Azure's
predisposition for sloppiness with garbage.


>
https://codereview.appspot.com/11027043/diff/1/environs/azure/environ_test.go...
> environs/azure/environ_test.go:501: for counter := 0; counter < 100; counter++
{
> 
> It's a detail but maybe the number of tries could be turned into a private
> variable and this test could use nbTries + 1.  Not really because it saves the
> creation of a few objects but because it explains how the code works a bit
> better, telling the reader where to look if he wonders how many times things
are
> retried in case of failure.

Had that originally, but I found I really didn't care about the number of
retries as long as it's more than one and less than a large number!  However
there is a function for the express purpose of this retrying, as distinct from
the actual work of creating the service.  What if I just put "retry" in the name
of that function?

[jtv.canonical, 2013-07-09 10:24:20]

OK: I updated things so that we always take the entire FQDN as given by Azure. 
It seemed more robust than stripping off the (fixed) domain in one place and
then adding it back in another.

[jtv.canonical, 2013-07-09 10:26:26]

Please take a look.

[fwereade, 2013-07-09 11:22:42]

LGTM modulo testing quibbles.

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

https://codereview.appspot.com/11027043/diff/9001/environs/azure/environ_test...
environs/azure/environ_test.go:369: func (EnvironSuite)
TestAttemptCreateServiceCreatesService(c *C) {
In general it's a good idea to create suites as pointers, and use pointer
receivers throughout. Failing to do this occasionally leads to tests being
skipped silently (if you have a non-pointer suite, and a Test method with a
pointer receiver, gocheck won't run it).

https://codereview.appspot.com/11027043/diff/9001/environs/azure/environ_test...
environs/azure/environ_test.go:514: c.Check(err, NotNil)
I'd prefer to check exact errors please.

[rvb, 2013-07-09 14:56:32]

> 
> Meanwhile, I wasn't sure that the fixed domain is actually guaranteed.  I see
it
> documented in an example, but the problem with an example is that it's just an
> example, and may make hidden assumptions.  Do you see any solid documentation
> for this fixed domain name?

http://www.windowsazure.com/en-us/develop/net/common-tasks/custom-dns/

> Had that originally, but I found I really didn't care about the number of
> retries as long as it's more than one and less than a large number!  However
> there is a function for the express purpose of this retrying, as distinct from
> the actual work of creating the service.  What if I just put "retry" in the
name
> of that function?

Sounds good.

[rvb, 2013-07-09 14:59:20]

On 2013/07/09 10:24:20, jtv.canonical wrote:
> OK: I updated things so that we always take the entire FQDN as given by Azure.

> It seemed more robust than stripping off the (fixed) domain in one place and
> then adding it back in another.

Yeah, it's definitely the best option.

If we rely on the (apparently) randomly-generated hostnames we're getting from
azure, base64-encoding the whole lot (i.e. the hostname with the domain name)
should fit in the label field (limited to 100 characters) without problem.

(d3158036d203407694a0f7bad4549de0.cloudapp.net is a real-world hostname)
>>> len('d3158036d203407694a0f7bad4549de0.cloudapp.net')
45
>>> from base64 import b64encode
>>> len(b64encode('d3158036d203407694a0f7bad4549de0.cloudapp.net'))
60

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

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

https://codereview.appspot.com/11027043/diff/9001/environs/azure/environ_test...
environs/azure/environ_test.go:514: c.Check(err, NotNil)
On 2013/07/09 11:22:42, fwereade wrote:
> I'd prefer to check exact errors please.

In this case I can't in good conscience test the exact error message, because
it's highly dependent on how the parser (not our code) happens to diagnose and
describe the problem.  That would be very brittle, and as it happens the error
message itself looks a bit nonsensical so I'd expect it to be fixed soon.

So instead, I prefixed it with my own error context and tested for that.