Add an os-disk constraint

Add an os-disk constraint

The various providers supported have different ways of selecting an instance
type or starting a specific instance with a custom OS/root disk size (see
discussion at LP:1201503). This
branch implements the basic constraint type, and follow up branches will
expose the provider-specific information for the actual constraint matching.

https://code.launchpad.net/~sidnei/juju-core/os-disk-constraint/+merge/179931

(do not edit description out of merge proposal)

[sidnei.da.silva, 2013-08-13 14:29:17]

Please take a look.

[fwereade, 2013-08-13 17:27:00]

EC2 instance type data is missing, and I'd prefer not to merge into trunk
without that. Couple more comments below.

https://codereview.appspot.com/12859043/diff/1/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/12859043/diff/1/constraints/constraints.go#new...
constraints/constraints.go:46: OsDisk *uint64 `json:"os-disk,omitempty"
yaml:"os-disk,omitempty"`
I'm not sure about the name. InstanceDisk, or LocalDisk, perhaps? There are
several EC2 instance types without instance storage.

https://codereview.appspot.com/12859043/diff/1/constraints/constraints.go#new...
constraints/constraints.go:314: }
Please pull this block out into a common function, it's non-trivial and a
precise duplicate of the code in setMem.

[sidnei.da.silva, 2013-08-13 18:00:55]

Please take a look.

https://codereview.appspot.com/12859043/diff/1/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/12859043/diff/1/constraints/constraints.go#new...
constraints/constraints.go:46: OsDisk *uint64 `json:"os-disk,omitempty"
yaml:"os-disk,omitempty"`
On 2013/08/13 17:27:00, fwereade wrote:
> I'm not sure about the name. InstanceDisk, or LocalDisk, perhaps? There are
> several EC2 instance types without instance storage.

As explained in the bug comments, this constraint about the 'OS Disk', a.k.a.
root disk, which all instance types have regardless of being instance-storage or
ebs. 

Since we default to ebs, we can actually start an instance that matches the
specified constraint regardless of the picked up instance type, as long as we
pass the proper block device mapping to the run instances api call.

https://codereview.appspot.com/12859043/diff/1/constraints/constraints.go#new...
constraints/constraints.go:314: }
On 2013/08/13 17:27:00, fwereade wrote:
> Please pull this block out into a common function, it's non-trivial and a
> precise duplicate of the code in setMem.

Done.

[sidnei.da.silva, 2013-08-13 21:50:59]

Please take a look.

[rog, 2013-08-14 15:47:10]

LGTM with some thoughts below.

https://codereview.appspot.com/12859043/diff/7001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/12859043/diff/7001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:100: "hardware":    "arch=amd64 cpu-cores=1 mem=1024M
os-disk=1024M",
I'd be tempted to use a different number, say 8G, for disk than memory to guard
against possible crosstalk.

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go#...
constraints/constraints.go:42: // amount of disk space available in the OS disk,
aka root
This should specify the units (MB, presumably)

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go#...
constraints/constraints.go:275: func parseSize(str string) (*uint64, error) {
This should probably go next to parseUint64

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go#...
constraints/constraints.go:298: if err != nil {
Just "return" should be do ok here.

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go#...
constraints/constraints.go:309: if err != nil {
ditto

[sidnei.da.silva, 2013-08-14 16:45:26]

Please take a look.

https://codereview.appspot.com/12859043/diff/7001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/12859043/diff/7001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:100: "hardware":    "arch=amd64 cpu-cores=1 mem=1024M
os-disk=1024M",
On 2013/08/14 15:47:10, rog wrote:
> I'd be tempted to use a different number, say 8G, for disk than memory to
guard
> against possible crosstalk.

Done.

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go#...
constraints/constraints.go:42: // amount of disk space available in the OS disk,
aka root
On 2013/08/14 15:47:10, rog wrote:
> This should specify the units (MB, presumably)

Done.

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go#...
constraints/constraints.go:275: func parseSize(str string) (*uint64, error) {
On 2013/08/14 15:47:10, rog wrote:
> This should probably go next to parseUint64

Done.

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go#...
constraints/constraints.go:298: if err != nil {
On 2013/08/14 15:47:10, rog wrote:
> Just "return" should be do ok here.

Done.

https://codereview.appspot.com/12859043/diff/7001/constraints/constraints.go#...
constraints/constraints.go:309: if err != nil {
On 2013/08/14 15:47:10, rog wrote:
> ditto

Done.

[dimitern, 2013-08-14 17:08:59]

LGTM with a few suggestions.

I still don't get what's the distinction between hardware characteristics and
constraints and why we need so much duplicated code there.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints_te...
File constraints/constraints_test.go (right):

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints_te...
constraints/constraints_test.go:10: . "launchpad.net/gocheck"
While you're at it can you import gocheck as gc and change the references to it
below please?

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints_te...
constraints/constraints_test.go:304: {OsDisk: uint64p(98765)},
Perhaps both:
{OsDisk: uint64p(0)},
{OsDisk: uint64p(98765)},

Like Mem or CpuCores?

https://codereview.appspot.com/12859043/diff/13001/instance/instance.go
File instance/instance.go (right):

https://codereview.appspot.com/12859043/diff/13001/instance/instance.go#newco...
instance/instance.go:196: hc.Mem, err = parseSize(str)
Seems like most of this code is duplicated with the one in constraints, which
isn't great. But nothing needs to change in this CL.

[gz, 2013-08-14 17:15:09]

I'm pretty strongly against naming this 'os-disk'. My first thought looking at
the proposal was you were implementing provider-specific contraints, because
'os-' prefix denoted an openstack-specific constraint in the python version of
juju. Something involving root disk or root partition instead would be fine.

https://codereview.appspot.com/12859043/diff/13001/environs/openstack/image.go
File environs/openstack/image.go (right):

https://codereview.appspot.com/12859043/diff/13001/environs/openstack/image.g...
environs/openstack/image.go:28: OsDisk:   uint64(flavor.Disk * 1024),
0 is a valid special case, which does not mean there's no root partition.
However, this logic should be fine I think, if a disk constraint is listed not
selecting one of those flavors should be correct.

[sidnei.da.silva, 2013-08-14 17:17:17]

Please take a look.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints_te...
File constraints/constraints_test.go (right):

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints_te...
constraints/constraints_test.go:10: . "launchpad.net/gocheck"
On 2013/08/14 17:08:59, dimitern wrote:
> While you're at it can you import gocheck as gc and change the references to
it
> below please?

Done.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints_te...
constraints/constraints_test.go:304: {OsDisk: uint64p(98765)},
On 2013/08/14 17:08:59, dimitern wrote:
> Perhaps both:
> {OsDisk: uint64p(0)},
> {OsDisk: uint64p(98765)},
> 
> Like Mem or CpuCores?

Done.

https://codereview.appspot.com/12859043/diff/13001/instance/instance.go
File instance/instance.go (right):

https://codereview.appspot.com/12859043/diff/13001/instance/instance.go#newco...
instance/instance.go:196: hc.Mem, err = parseSize(str)
On 2013/08/14 17:08:59, dimitern wrote:
> Seems like most of this code is duplicated with the one in constraints, which
> isn't great. But nothing needs to change in this CL.

Done.

[sidnei.da.silva, 2013-08-14 17:35:39]

Please take a look.

[natefinch, 2013-08-14 17:39:59]

Mostly OK, please read through my comments, nothing huge.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:178: case "os-disk":
I don't like seeing the same raw string in multiple places in the code, can we
factor this out into a variable somewhere?

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:275: func (v *Value) setMem(str string) (err error) {
There seems to be no reason for a named return value here.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:283: func (v *Value) setOsDisk(str string) (err
error) {
or here

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:287: v.OsDisk, err = parseSize(str)
It seems wrong to overwrite OsDisk if parseSize returns an error. Generally you
should only make changes if everything succeeds.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:300: return &value, nil
we're treating an empty string as if it's zero... is that correct?  Seems like
an empty string should be treated as not set.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:318: return &value, nil
as above - empty string = 0, instead of unset.

https://codereview.appspot.com/12859043/diff/13001/environs/instances/instanc...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/12859043/diff/13001/environs/instances/instanc...
environs/instances/instancetype.go:152: return inst0.OsDisk < inst1.OsDisk
To avoid having to change previous code every time we add a constraint, we
should just do something like this: 

if inst0.OsDisk != inst1.OsDisk {
       return inst0.OsDisk < inst1.OsDisk
}

return false

That way you can insert new constraint checks without having to touch the
previous code.

https://codereview.appspot.com/12859043/diff/13001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/12859043/diff/13001/state/machine.go#newcode122
state/machine.go:122: OsDisk     *uint64     `bson:"osdisk,omitempty"`
I'm curious why this was inserted in the middle of the struct and not the end.
Not a problem, just wondering if there was a specific reason.

[natefinch, 2013-08-14 17:45:03]

One more thing, somehow I lost the comment - I, too, think os-disk is not a good
name. The comment on os-disk says it means root partition... can we not call it
root-partition?  That seems much less likely to be misunderstood.

[sidnei.da.silva, 2013-08-14 17:58:13]

Please take a look.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:178: case "os-disk":
On 2013/08/14 17:39:59, nate.finch wrote:
> I don't like seeing the same raw string in multiple places in the code, can we
> factor this out into a variable somewhere?

Not sure it's worth the trouble since it's only two places. I actually feel like
it's more readable to have the raw string since you can easily grep for it.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:275: func (v *Value) setMem(str string) (err error) {
On 2013/08/14 17:39:59, nate.finch wrote:
> There seems to be no reason for a named return value here.

It simplifies the code a bit since we can then skip the 'if err != nil { return
err }', which rog seems to prefer.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:287: v.OsDisk, err = parseSize(str)
On 2013/08/14 17:39:59, nate.finch wrote:
> It seems wrong to overwrite OsDisk if parseSize returns an error. Generally
you
> should only make changes if everything succeeds.

I had the same feeling at first, but the same pattern is used everywhere
parseUint64 is used. If there's agreement, I can fix both places.

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:300: return &value, nil
On 2013/08/14 17:39:59, nate.finch wrote:
> we're treating an empty string as if it's zero... is that correct?  Seems like
> an empty string should be treated as not set.

I'm not completely sure what was the original intent here.

https://codereview.appspot.com/12859043/diff/13001/environs/instances/instanc...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/12859043/diff/13001/environs/instances/instanc...
environs/instances/instancetype.go:152: return inst0.OsDisk < inst1.OsDisk
On 2013/08/14 17:39:59, nate.finch wrote:
> To avoid having to change previous code every time we add a constraint, we
> should just do something like this: 
> 
> if inst0.OsDisk != inst1.OsDisk {
>        return inst0.OsDisk < inst1.OsDisk
> }
> 
> return false
> 
> That way you can insert new constraint checks without having to touch the
> previous code.

Done.

https://codereview.appspot.com/12859043/diff/13001/environs/openstack/image.go
File environs/openstack/image.go (right):

https://codereview.appspot.com/12859043/diff/13001/environs/openstack/image.g...
environs/openstack/image.go:28: OsDisk:   uint64(flavor.Disk * 1024),
On 2013/08/14 17:15:09, gz wrote:
> 0 is a valid special case, which does not mean there's no root partition.
> However, this logic should be fine I think, if a disk constraint is listed not
> selecting one of those flavors should be correct.

Done.

https://codereview.appspot.com/12859043/diff/13001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/12859043/diff/13001/state/machine.go#newcode122
state/machine.go:122: OsDisk     *uint64     `bson:"osdisk,omitempty"`
On 2013/08/14 17:39:59, nate.finch wrote:
> I'm curious why this was inserted in the middle of the struct and not the end.
> Not a problem, just wondering if there was a specific reason.

Just tried to follow the same pattern everywhere of adding OsDisk right after
Mem. The first one was in InstanceType, where it was added in the middle because
the last two in the struct had a comment that they were not available in all
instance types.

[natefinch, 2013-08-14 18:21:02]

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/12859043/diff/13001/constraints/constraints.go...
constraints/constraints.go:275: func (v *Value) setMem(str string) (err error) {
On 2013/08/14 17:58:14, sidnei.da.silva wrote:
> On 2013/08/14 17:39:59, nate.finch wrote:
> > There seems to be no reason for a named return value here.
> 
> It simplifies the code a bit since we can then skip the 'if err != nil {
return
> err }', which rog seems to prefer.

You're just returning err either way, so you don't need an if statement. You can
just do "return err" instead of the bare return here.  It's not a big deal, it's
not *wrong* as-is, it just seems unnecessary.

[natefinch, 2013-08-14 18:25:26]

meant to say - LGTM modulo others looking at the couple of points on overwriting
on errors from parse, and empty strings being treated as zero rather than unset

[fwereade, 2013-08-15 13:58:37]

This is great; LGTM. Just one request:

https://codereview.appspot.com/12859043/diff/44001/environs/ec2/instancetype.go
File environs/ec2/instancetype.go (right):

https://codereview.appspot.com/12859043/diff/44001/environs/ec2/instancetype....
environs/ec2/instancetype.go:35: RootDisk: 8192,
Can we have a quick comment in this file referencing lp:1212688, and possibly
explaining that we'll be dropping the field again when we're able to directly
specify size via constraint?

[gz, 2013-08-15 14:16:57]

Thanks for doing the rename. With the one last fix, LGTM.

https://codereview.appspot.com/12859043/diff/44001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/12859043/diff/44001/state/machine.go#newcode122
state/machine.go:122: RootDisk   *uint64     `bson:"osdisk,omitempty"`
Serialise as 'rootdisk'.

[sidnei.da.silva, 2013-08-15 14:40:17]

Please take a look.

https://codereview.appspot.com/12859043/diff/44001/environs/ec2/instancetype.go
File environs/ec2/instancetype.go (right):

https://codereview.appspot.com/12859043/diff/44001/environs/ec2/instancetype....
environs/ec2/instancetype.go:35: RootDisk: 8192,
On 2013/08/15 13:58:37, fwereade wrote:
> Can we have a quick comment in this file referencing lp:1212688, and possibly
> explaining that we'll be dropping the field again when we're able to directly
> specify size via constraint?

Done.