Code review comment for lp:~sidnei/juju-core/sort-by-cost-lowest-fallback

Reviewers: mp+179191_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/12569044/

Affected files:
   A [revision details]
   M environs/instances/instancetype.go
   M environs/instances/instancetype_test.go
   M environs/openstack/image.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130808095436-ia7cq6t6ybdpo2hf
+New revision: <email address hidden>

Index: environs/instances/instancetype.go
=== modified file 'environs/instances/instancetype.go'
--- environs/instances/instancetype.go 2013-05-20 23:33:29 +0000
+++ environs/instances/instancetype.go 2013-08-08 13:36:50 +0000
@@ -126,8 +126,27 @@
  // byCost is used to sort a slice of instance types by Cost.
  type byCost []InstanceType

-func (bc byCost) Len() int { return len(bc) }
-func (bc byCost) Less(i, j int) bool { return bc[i].Cost < bc[j].Cost }
+func (bc byCost) Len() int { return len(bc) }
+func (bc byCost) Less(i, j int) bool {
+ if bc[i].Cost != bc[j].Cost {
+ return bc[i].Cost < bc[j].Cost
+ }
+ if bc[i].Mem != bc[j].Mem {
+ return bc[i].Mem < bc[j].Mem
+ }
+ iCpuPower := uint64(0)
+ jCpuPower := uint64(0)
+ if bc[i].CpuPower != nil {
+ iCpuPower = *bc[i].CpuPower
+ }
+ if bc[j].CpuPower != nil {
+ jCpuPower = *bc[j].CpuPower
+ }
+ if iCpuPower != jCpuPower {
+ return iCpuPower < jCpuPower
+ }
+ return bc[i].CpuCores < bc[j].CpuCores
+}
  func (bc byCost) Swap(i, j int) {
   bc[i], bc[j] = bc[j], bc[i]
  }

Index: environs/instances/instancetype_test.go
=== modified file 'environs/instances/instancetype_test.go'
--- environs/instances/instancetype_test.go 2013-05-17 06:34:24 +0000
+++ environs/instances/instancetype_test.go 2013-08-08 13:36:50 +0000
@@ -4,6 +4,8 @@
  package instances

  import (
+ "sort"
+
   . "launchpad.net/gocheck"
   "launchpad.net/juju-core/constraints"
   "launchpad.net/juju-core/testing"
@@ -231,3 +233,68 @@
    }
   }
  }
+
+var byCostTest = []struct {
+ info string
+ itypesToUse []InstanceType
+ expectedItypes []string
+}{
+ {
+ info: "default to lowest cost",
+ itypesToUse: []InstanceType{
+ {Id: "2", Name: "it-2", CpuCores: 2, Mem: 4096, Cost: 240},
+ {Id: "1", Name: "it-1", CpuCores: 1, Mem: 2048, Cost: 241},
+ },
+ expectedItypes: []string{
+ "it-2", "it-1",
+ },
+ }, {
+ info: "when no cost associated, pick lowest ram",
+ itypesToUse: []InstanceType{
+ {Id: "2", Name: "it-2", CpuCores: 2, Mem: 4096},
+ {Id: "1", Name: "it-1", CpuCores: 1, Mem: 2048},
+ },
+ expectedItypes: []string{
+ "it-1", "it-2",
+ },
+ }, {
+ info: "when cost is the same, pick lowest ram",
+ itypesToUse: []InstanceType{
+ {Id: "2", Name: "it-2", CpuCores: 2, Mem: 4096, Cost: 240},
+ {Id: "1", Name: "it-1", CpuCores: 1, Mem: 2048, Cost: 240},
+ },
+ expectedItypes: []string{
+ "it-1", "it-2",
+ },
+ }, {
+ info: "when cost and ram is the same, pick lowest cpu power",
+ itypesToUse: []InstanceType{
+ {Id: "2", Name: "it-2", CpuCores: 2, CpuPower: CpuPower(200)},
+ {Id: "1", Name: "it-1", CpuCores: 1, CpuPower: CpuPower(100)},
+ },
+ expectedItypes: []string{
+ "it-1", "it-2",
+ },
+ }, {
+ info: "when cpu power is the same, pick the lowest cores",
+ itypesToUse: []InstanceType{
+ {Id: "2", Name: "it-2", CpuCores: 2, CpuPower: CpuPower(200)},
+ {Id: "1", Name: "it-1", CpuCores: 1, CpuPower: CpuPower(200)},
+ },
+ expectedItypes: []string{
+ "it-1", "it-2",
+ },
+ },
+}
+
+func (s *instanceTypeSuite) TestSortByCost(c *C) {
+ for i, t := range byCostTest {
+ c.Logf("test %d: %s", i, t.info)
+ sort.Sort(byCost(t.itypesToUse))
+ names := make([]string, len(t.itypesToUse))
+ for i, itype := range t.itypesToUse {
+ names[i] = itype.Name
+ }
+ c.Check(names, DeepEquals, t.expectedItypes)
+ }
+}

Index: environs/openstack/image.go
=== modified file 'environs/openstack/image.go'
--- environs/openstack/image.go 2013-07-31 10:42:27 +0000
+++ environs/openstack/image.go 2013-08-08 13:36:50 +0000
@@ -25,7 +25,6 @@
     Arches: ic.Arches,
     Mem: uint64(flavor.RAM),
     CpuCores: uint64(flavor.VCPUs),
- Cost: uint64(flavor.RAM),
    }
    allInstanceTypes = append(allInstanceTypes, instanceType)
   }