Fallback cost sort to mem, cpu-power, cpu-cores

Fallback cost sort to mem, cpu-power, cpu-cores

Not all environments provide a cost option, so when computing an appropriate
instance type from provided constraints, use mem, cpu-power and cpu-cores as
fallback options. When any two of them are the same, we fallback to the next
option.

https://code.launchpad.net/~sidnei/juju-core/sort-by-cost-lowest-fallback/+merge/179191

(do not edit description out of merge proposal)

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

Please take a look.

[dimitern, 2013-08-08 14:13:40]

I think this is a good change, but please can you live test it on canonistack at
least? LGTM otherwise.

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetyp...
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetyp...
environs/instances/instancetype_test.go:10:
"launchpad.net/juju-core/constraints"
Blank line after gocheck please. And while you're at it, could you do:

gc "launchpad.net/gocheck"

Up there and change it below - it's a pretty small file, so it shouldn't incur
to many changes.

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetyp...
environs/instances/instancetype_test.go:237: var byCostTest = []struct {
s/byCostTest/byCostTests/ I think

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetyp...
environs/instances/instancetype_test.go:238: info           string
It's more typical to call this field "about" in the table.

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

Please take a look.

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetyp...
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetyp...
environs/instances/instancetype_test.go:10:
"launchpad.net/juju-core/constraints"
On 2013/08/08 14:13:40, dimitern wrote:
> Blank line after gocheck please. And while you're at it, could you do:
> 
> gc "launchpad.net/gocheck"
> 
> Up there and change it below - it's a pretty small file, so it shouldn't incur
> to many changes.

Done.

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetyp...
environs/instances/instancetype_test.go:237: var byCostTest = []struct {
On 2013/08/08 14:13:40, dimitern wrote:
> s/byCostTest/byCostTests/ I think

Done.

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetyp...
environs/instances/instancetype_test.go:238: info           string
On 2013/08/08 14:13:40, dimitern wrote:
> It's more typical to call this field "about" in the table.

Done.

[rog, 2013-08-08 15:07:23]

LGTM with some suggestions below.

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
environs/instances/instancetype.go:131: if bc[i].Cost != bc[j].Cost {
One way to make this slightly more readable
(and more efficient into the bargain) is
to use pointers to the two elements in question:

inst0, inst1 := &bc[i], &bc[j]

then:

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

etc

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
environs/instances/instancetype.go:137: iCpuPower := uint64(0)
or:
var iCpuPower, jCpuPower int64

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
environs/instances/instancetype.go:139: if bc[i].CpuPower != nil {
I don't think either of these should be compared
at all if they're nil.

So something like this perhaps?

if inst0.CpuPower != nil &&
    inst1.CpuPower != nil &&
    *inst0.CpuPower != *inst1.CpuPower{
    return *inst0.CpuPower < *inst1.CpuPower
}

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
environs/instances/instancetype_test.go:287: },
I'd like a test for a case where cpu power is only set in one of the instances.

[sidnei.da.silva, 2013-08-08 15:27:08]

Please take a look.

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
environs/instances/instancetype.go:131: if bc[i].Cost != bc[j].Cost {
On 2013/08/08 15:07:24, rog wrote:
> One way to make this slightly more readable
> (and more efficient into the bargain) is
> to use pointers to the two elements in question:
> 
> inst0, inst1 := &bc[i], &bc[j]
> 
> then:
> 
> if inst0.Cost != inst0.Cost {
>     return inst0.Cost < inst1.Cost
> }
> 
> etc

Done.

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
environs/instances/instancetype.go:137: iCpuPower := uint64(0)
On 2013/08/08 15:07:24, rog wrote:
> or:
> var iCpuPower, jCpuPower int64

Done.

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
environs/instances/instancetype.go:139: if bc[i].CpuPower != nil {
On 2013/08/08 15:07:24, rog wrote:
> I don't think either of these should be compared
> at all if they're nil.
> 
> So something like this perhaps?
> 
> if inst0.CpuPower != nil &&
>     inst1.CpuPower != nil &&
>     *inst0.CpuPower != *inst1.CpuPower{
>     return *inst0.CpuPower < *inst1.CpuPower
> }

Done.

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instance...
environs/instances/instancetype_test.go:287: },
On 2013/08/08 15:07:24, rog wrote:
> I'd like a test for a case where cpu power is only set in one of the
instances.

Done.

[rog, 2013-08-08 16:10:11]

still LGTM, thanks!