Support root-disk constraint on EC2

Support root-disk constraint on EC2

We now can support the root-disk constraint on EC2... except that instead of constraining the images, we just use it to request that amount of disk space from amazon, since any image can have a variable amount of space.  We store the requested amount in the hardware characteristics of the machine (and just assume it's the same as what is actually on the machine for now).

https://code.launchpad.net/~natefinch/juju-core/017-ec2-rootdisk/+merge/189386

(do not edit description out of merge proposal)

[natefinch, 2013-10-04 17:22:05]

Please take a look.

[natefinch, 2013-10-07 14:14:38]

Please take a look.

[natefinch, 2013-10-07 14:26:20]

Please take a look.

[rog, 2013-10-07 14:32:48]

Looks good, with a couple of suggestions below, and... some tests would be nice.

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go#newcode384
provider/ec2/ec2.go:384: // default size for ec2 root disks
// in megabytes. 
?

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go#newcode401
provider/ec2/ec2.go:401: cons.RootDisk = nil
Rather than doing this, I wonder if it might be slightly
nicer to pass the constraints unchanged into findInstanceSpec,
which will then ignore the disk part constraint.

And you could do:

rootDisk := defaultDisk
if cons.RootDisk != nil {
    rootDisk = *cons.RootDisk
}

that way you won't need the logic on line 469.

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go#newcode438
provider/ec2/ec2.go:438: // request the root disk size in the provision request
To fit with the comment style of the rest of this file,
it would be nice to start with cap and finish with a period:

// Request the root disk size in the provisioning request.

Same applies to the comment above (and doc comments are
usually full sentences, even if internal)

[mue, 2013-10-07 14:33:28]

LGTM, only minor comments.

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go#newcode384
provider/ec2/ec2.go:384: // default size for ec2 root disks
// defaultDisk is the default size of EC2 root disks.

Go style. And also change the name to defaultDiskSize.

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go#newcode398
provider/ec2/ec2.go:398: // don't restrict images based on root-disk, since
I prefer real sentences. "Don't ... Amazon."

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/instancetype.go
File provider/ec2/instancetype.go (right):

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/instancetype.g...
provider/ec2/instancetype.go:29: // Note that while the EC2 root disk default is
8G, constraints on disk
Godoc will do no line break here. If needed add an empty line.

// ... type.
//
// Note that ...

[fwereade, 2013-10-07 15:02:57]

Needs one significant change, but it's a structure one not a logic one.

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go#newcode74
provider/ec2/ec2.go:74: rootdisk uint64
This looks really weird to me. Where do we get *any* of these fields? If they're
not necessarily valid -- and it looks like they're not -- I don't think we
should keep them here. hardwareCharacteristics should be a func taking whatever
data it needs, not a method that might arbitrarily go wrong.

https://codereview.appspot.com/14326044/diff/7001/provider/ec2/ec2.go#newcode527
provider/ec2/ec2.go:527: // TODO(wallyworld): lookup the details to fill in the
instance type data
Gaah. Yeah, please take these crazy fields off the instance type and just
calculate the hardware directly in StartInstance.

[natefinch, 2013-10-08 15:02:05]

Please take a look.

[fwereade, 2013-10-09 08:49:58]

LGTM, one trivial.

https://codereview.appspot.com/14326044/diff/15001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14326044/diff/15001/provider/ec2/ec2.go#newcod...
provider/ec2/ec2.go:429: })
Would it make sense to move the devices assignment outside the if block? If
we're ever setting it, it seems nicer to always set it in the same way.