Azure provider: utilities to start a VM.

Azure provider: utilities to start a VM.

This branch adds the utilities required to actually start an Azure Virtual Machine in the provider's internalStartInstance() method.  The utilities introduced here are all unit-tested but the change to internalStartInstance() is not.  internalStartInstance() isn't called from anywhere yet because the methods Bootstrap() and StartInstance() are being refactored by thumper.  So internalStartInstance() is really just a draft right now.  Still lots of TODOs in these utilities but I've tested that this is sane by manually adding the methods Bootstrap and StartInstance and I was able to get a machine started by running "juju  bootstrap".

https://code.launchpad.net/~rvb/juju-core/create-vm/+merge/174216

(do not edit description out of merge proposal)

[rvb, 2013-07-11 14:26:54]

Please take a look.

[dimitern, 2013-07-11 20:51:20]

LGTM with minor suggestions.

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

https://codereview.appspot.com/11166043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:323: return nil, fmt.Errorf("could not get instance:
%v", err)
maybe "could not get instance %q: %v", instanceName, err ?

https://codereview.appspot.com/11166043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:360: inputendpoint := gwacl.InputEndpoint{LocalPort:
22, Name: "sshport", Port: 22, Protocol: "TCP"}
please split it to multiple lines

https://codereview.appspot.com/11166043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:364: return gwacl.NewRole(roleSize, roleName,
[]gwacl.ConfigurationSet{*linuxConfigurationSet, *networkConfigurationSet},
[]gwacl.OSVirtualHardDisk{*vhd})
ditto

[jtv.canonical, 2013-07-12 03:36:19]

I'm doing this review in the Launchpad style, because I find the Rietveld
point-and-nag style an invitation to annoy the reviewee with unclear,
out-of-context criticisms.  It breeds unnecessary hostility, and I find the
weirdly interspersed references are harder, not easier, to work with than the
manual referring we come up with in Launchpad.  (I also worked off the Launchpad
diff to give me a smoother overview of the changes.)

The code looks fine (although like Dimiter I'd prefer it if you could break up
the long compound lines a bit).  LGTM.  My own notes are all about the
nonfunctional parts.

 * In environ.go you describe DeploymentSlot as a configuration item.  I
disagree.

The only advantage we get from the choice between staging and production is an
opportunity to streamline and optimize the code because we get to pick our own
random DNS names — but Production is already the right choice for that, and the
optimization only stays simple if we *don't* add a Staging option.

Or, in the future, Juju might evolve to support two-stage deployment, from
staging to production.  But unless and until that happens, it's not TODO
material.

So it looks to me as if the idea to make DeploymentSlot configurable is based
purely on general principle and does not connect well with the specific case. 
Unless you have a compelling reason why this must be changed, please remove that
TODO.


 * Just a thought, but does it make sense to name newOSVirtualDisk so close to
the gwacl?

This isn't just a factory for a gwacl object any more — it does significant
creative work, and happens to return a gwacl object.  And Microsoft probably
won't let us create physical disks for our Azure VMs, so why not newOSDisk?

 
 * Also in newOSVirtualDisk: you might want to make sourceImageName a parameter.

It doesn't make much difference now, but it expresses a division of
responsibilities that I think will make sense.  The reason I suggest dividing
the responsibilities like that from the start is testability.  Flat call trees
are often good for testing, but doubly so in a language where static dispatch
stops us from testing to the level of quality we're used to.  Deep call trees
are sheer hell when testing Go.

Imagine what happens once we have a function for determining the image name we
need.  If we pass the image name to newOSVirtualDisk as a parameter, we won't
have to touch the test for newOSVirtualDisk, or indeed newOSVirtualDisk itself. 
That is one piece of code that we could, with luck, be completely done with
already.  Isn't that a much better feeling?  :-)


 * newRole has several critical interface conditions that should be part of its
documentation.

This function ensures that the ssh port is open, that the system has an "ubuntu"
user, and that you don't leave any password-guessing backdoor open.  All of
these are clearly documented, but they are not mere implementation issues.  They
are part of what the caller can expect.  So state them up front.

 * There's an ambiguous comment in newRole.

I can read this to mean two different things:

    // Keep the ordering of the configuration sets like this: linux config then
network config for the sake of testing.

The sentence structure seems to say that the network config is there only to
enable testing.  But I suspect that's not what you mean.  Remember: technical
English.  One unit of meaning at a time.  Minimize the grammatical baggage that
the reader needs to keep track of.  And don't bother repeating the code.

How about:

    // The ordering of these configuration sets is significant for the tests.

Also, please break up such long lines in comments!


 * A question: are roles really things we should create for every instance?

It's still not very clear to me what Microsoft had in mind when they defined
roles, or why they matter.  But my impression is that they correspond more or
less to a hardware profile, along the lines of “m1.small” (although to confuse
us, the Azure documentation also seems to use the term Role to mean
RoleInstance).  Would it make sense at some point to keep the actual Roles
static and reusable?  Or is this a total non-issue?


 * In newDeployment, again, I'd make the virtual network name a parameter.

The same arguments from newOSVirtualDisk apply here.  In addition, the function
would become a 3-liner and none of the comments are needed.

I know, I know, it's just moving the problem somewhere else!  But numbered
comments are something to use sparingly — as those of us know who have worked
with BASIC line numbers in systems that lacked auto-renumbering.  :-)  Don't
tell anyone, but I think I've only used numbered steps in recent years as a
subliminal ploy to discourage further changes.


 * In environ_test.go, buildAzureServiceResponses is confusingly named.  Maybe
add a "Get"?


 * In TestGetInstance you go through an awkward
HostedServiceDescriptor/HostedService initialization dance.

Isn't that what makeAzureService is for?

 * A grammatical mistake in TestNewOSVirtualDisk.

I haven't been pointing these out lately, despite knowing that you're always
interested in learning about these things.  Stress kept making it a bad time. 
Here's what you wrote:

    TODO: check vhd's sourceImageName when we will use simplestreams to get the
image name.

When you have one of these "conditions in the future" (there's probably a
grammatical term for them) English always puts it in the present tense: "when we
use," not "when we will use."

Of course that quickly becomes ambiguous, because it might mean that sometimes
we use simplestreams and sometimes we don't.  So disambiguate in the semantics,
e.g: "once we start using simplestreams."

 * TestNewDeployment checks at the very end that exactly 1 request has been
made.

I think the other checks are more or less meaningless if that isn't the case. 
If no requests have been made, your test will crash at the first test.  If more
than request has been made, it's possible that you've been inspecting the wrong
request.

So I'd put this length check at the top of the checks, and make it an assertion.

[rvb, 2013-07-12 06:53:11]

On 2013/07/11 20:51:20, dimitern wrote:
> LGTM with minor suggestions.
> 
> https://codereview.appspot.com/11166043/diff/1/environs/azure/environ.go
> File environs/azure/environ.go (right):
> 
>
https://codereview.appspot.com/11166043/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:323: return nil, fmt.Errorf("could not get instance:
> %v", err)
> maybe "could not get instance %q: %v", instanceName, err ?
> 
>
https://codereview.appspot.com/11166043/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:360: inputendpoint := gwacl.InputEndpoint{LocalPort:
> 22, Name: "sshport", Port: 22, Protocol: "TCP"}
> please split it to multiple lines
> 
>
https://codereview.appspot.com/11166043/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:364: return gwacl.NewRole(roleSize, roleName,
> []gwacl.ConfigurationSet{*linuxConfigurationSet, *networkConfigurationSet},
> []gwacl.OSVirtualHardDisk{*vhd})
> ditto

All done, thanks for the review!

[rvb, 2013-07-12 07:29:07]

Thanks for the thorough review!

>  * In environ.go you describe DeploymentSlot as a configuration item.  I
> disagree.
> 
> The only advantage we get from the choice between staging and production is an
> opportunity to streamline and optimize the code because we get to pick our own
> random DNS names — but Production is already the right choice for that, and
the
> optimization only stays simple if we *don't* add a Staging option.

Hum, you might be right… I'm not entirely sure… but we won't be fixing that soon
that's sure.

>  * Just a thought, but does it make sense to name newOSVirtualDisk so close to
> the gwacl?
> 
> This isn't just a factory for a gwacl object any more — it does significant
> creative work, and happens to return a gwacl object.  And Microsoft probably
> won't let us create physical disks for our Azure VMs, so why not newOSDisk?

Okay.

>  * Also in newOSVirtualDisk: you might want to make sourceImageName a
parameter.

Done.

>  * newRole has several critical interface conditions that should be part of
its
> documentation.

Okay, done.

> How about:
> 
>     // The ordering of these configuration sets is significant for the tests.
> 
> Also, please break up such long lines in comments!

Done.

>  * A question: are roles really things we should create for every instance?
> 
> It's still not very clear to me what Microsoft had in mind when they defined
> roles, or why they matter.  But my impression is that they correspond more or
> less to a hardware profile, along the lines of “m1.small” (although to confuse
> us, the Azure documentation also seems to use the term Role to mean
> RoleInstance).  Would it make sense at some point to keep the actual Roles
> static and reusable?  Or is this a total non-issue?

I think this all stems from the confusion between Roles and RoleInstances
indeed.  But the Role here represents the VM itself so yes, we clearly have to
create one of them for each and every instance we create.

>  * In newDeployment, again, I'd make the virtual network name a parameter.

Good point, done.

>  * In environ_test.go, buildAzureServiceResponses is confusingly named.  Maybe
> add a "Get"?

Does not sound confusing to me but okay…

>  * In TestGetInstance you go through an awkward
> HostedServiceDescriptor/HostedService initialization dance.
> 
> Isn't that what makeAzureService is for?

Well, yes but a change in gwacl forced us to do the initialization differently
still.

>  * TestNewDeployment checks at the very end that exactly 1 request has been
> made.
> 
> I think the other checks are more or less meaningless if that isn't the case. 
> If no requests have been made, your test will crash at the first test.  If
more
> than request has been made, it's possible that you've been inspecting the
wrong
> request.
> 
> So I'd put this length check at the top of the checks, and make it an
assertion.

We're not checking requests here, just that the created deployment has 1 role. 
Nothing more, nothing less.

Thanks again for the review!