juju-core

Merge lp:~sidnei/juju-core/sort-by-cost-lowest-fallback into lp:~go-bot/juju-core/trunk

  1. sort-by-cost-lowest-fallback
  2. Merge into trunk
Proposed by Sidnei da Silva
Status: Merged
Approved by: Sidnei da Silva
Approved revision: no longer in the source branch.
Merged at revision: 1621
Proposed branch: lp:~sidnei/juju-core/sort-by-cost-lowest-fallback
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 293 lines (+135/-34)
3 files modified
environs/instances/instancetype.go (+18/-2)
environs/instances/instancetype_test.go (+117/-31)
environs/openstack/image.go (+0/-1)
To merge this branch: bzr merge lp:~sidnei/juju-core/sort-by-cost-lowest-fallback
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+179191@code.launchpad.net

Commit message

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://codereview.appspot.com/12569044/

R=dimitern, rogpeppe

Description of the change

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://codereview.appspot.com/12569044/

To post a comment you must log in.
Revision history for this message
Sidnei da Silva (sidnei) wrote :
Download full text (4.7 KiB)

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:...

Read more...

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

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/instancetype_test.go
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetype_test.go#newcode10
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/instancetype_test.go#newcode237
environs/instances/instancetype_test.go:237: var byCostTest = []struct {
s/byCostTest/byCostTests/ I think

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

https://codereview.appspot.com/12569044/

Revision history for this message
Sidnei da Silva (sidnei) wrote :

Please take a look.

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

https://codereview.appspot.com/12569044/diff/1/environs/instances/instancetype_test.go#newcode10
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/instancetype_test.go#newcode237
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/instancetype_test.go#newcode238
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.

https://codereview.appspot.com/12569044/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with some suggestions below.

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

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instancetype.go#newcode131
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/instancetype.go#newcode137
environs/instances/instancetype.go:137: iCpuPower := uint64(0)
or:
var iCpuPower, jCpuPower int64

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instancetype.go#newcode139
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/instancetype_test.go
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instancetype_test.go#newcode287
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.

https://codereview.appspot.com/12569044/

Revision history for this message
Sidnei da Silva (sidnei) wrote :

Please take a look.

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

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instancetype.go#newcode131
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/instancetype.go#newcode137
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/instancetype.go#newcode139
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/instancetype_test.go
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/12569044/diff/6001/environs/instances/instancetype_test.go#newcode287
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.

https://codereview.appspot.com/12569044/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

still LGTM, thanks!

https://codereview.appspot.com/12569044/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/instances/instancetype.go'
2--- environs/instances/instancetype.go 2013-05-20 23:33:29 +0000
3+++ environs/instances/instancetype.go 2013-08-08 15:26:18 +0000
4@@ -126,8 +126,24 @@
5 // byCost is used to sort a slice of instance types by Cost.
6 type byCost []InstanceType
7
8-func (bc byCost) Len() int { return len(bc) }
9-func (bc byCost) Less(i, j int) bool { return bc[i].Cost < bc[j].Cost }
10+func (bc byCost) Len() int { return len(bc) }
11+
12+func (bc byCost) Less(i, j int) bool {
13+ inst0, inst1 := &bc[i], &bc[j]
14+ if inst0.Cost != inst1.Cost {
15+ return inst0.Cost < inst1.Cost
16+ }
17+ if inst0.Mem != inst1.Mem {
18+ return inst0.Mem < inst1.Mem
19+ }
20+ if inst0.CpuPower != nil &&
21+ inst1.CpuPower != nil &&
22+ *inst0.CpuPower != *inst1.CpuPower {
23+ return *inst0.CpuPower < *inst1.CpuPower
24+ }
25+ return inst0.CpuCores < inst1.CpuCores
26+}
27+
28 func (bc byCost) Swap(i, j int) {
29 bc[i], bc[j] = bc[j], bc[i]
30 }
31
32=== modified file 'environs/instances/instancetype_test.go'
33--- environs/instances/instancetype_test.go 2013-05-17 06:34:24 +0000
34+++ environs/instances/instancetype_test.go 2013-08-08 15:26:18 +0000
35@@ -4,7 +4,10 @@
36 package instances
37
38 import (
39- . "launchpad.net/gocheck"
40+ "sort"
41+
42+ gc "launchpad.net/gocheck"
43+
44 "launchpad.net/juju-core/constraints"
45 "launchpad.net/juju-core/testing"
46 )
47@@ -13,13 +16,13 @@
48 testing.LoggingSuite
49 }
50
51-var _ = Suite(&instanceTypeSuite{})
52+var _ = gc.Suite(&instanceTypeSuite{})
53
54-func (s *instanceTypeSuite) SetUpSuite(c *C) {
55+func (s *instanceTypeSuite) SetUpSuite(c *gc.C) {
56 s.LoggingSuite.SetUpSuite(c)
57 }
58
59-func (s *instanceTypeSuite) TearDownSuite(c *C) {
60+func (s *instanceTypeSuite) TearDownSuite(c *gc.C) {
61 s.LoggingSuite.TearDownTest(c)
62 }
63
64@@ -98,38 +101,38 @@
65 }
66
67 var getInstanceTypesTest = []struct {
68- info string
69+ about string
70 cons string
71 itypesToUse []InstanceType
72 expectedItypes []string
73 arches []string
74 }{
75 {
76- info: "cpu-cores",
77- cons: "cpu-cores=2",
78+ about: "cpu-cores",
79+ cons: "cpu-cores=2",
80 expectedItypes: []string{
81 "c1.medium", "m1.large", "m1.xlarge", "c1.xlarge", "cc1.4xlarge",
82 "cc2.8xlarge",
83 },
84 }, {
85- info: "cpu-power",
86+ about: "cpu-power",
87 cons: "cpu-power=2000",
88 expectedItypes: []string{"c1.xlarge", "cc1.4xlarge", "cc2.8xlarge"},
89 }, {
90- info: "mem",
91- cons: "mem=4G",
92+ about: "mem",
93+ cons: "mem=4G",
94 expectedItypes: []string{
95 "m1.large", "m1.xlarge", "c1.xlarge", "cc1.4xlarge", "cc2.8xlarge",
96 },
97 }, {
98- info: "arches filtered by constraint",
99+ about: "arches filtered by constraint",
100 cons: "cpu-power=100 arch=arm",
101 expectedItypes: []string{"m1.small", "m1.medium", "c1.medium"},
102 arches: []string{"arm"},
103 },
104 {
105- info: "fallback instance type, enough memory for mongodb",
106- cons: "mem=8G",
107+ about: "fallback instance type, enough memory for mongodb",
108+ cons: "mem=8G",
109 itypesToUse: []InstanceType{
110 {Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 4096},
111 {Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 2048},
112@@ -138,8 +141,8 @@
113 expectedItypes: []string{"it-2"},
114 },
115 {
116- info: "fallback instance type, not enough memory for mongodb",
117- cons: "mem=4G",
118+ about: "fallback instance type, not enough memory for mongodb",
119+ cons: "mem=4G",
120 itypesToUse: []InstanceType{
121 {Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256},
122 {Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512},
123@@ -155,34 +158,34 @@
124 }
125 }
126
127-func (s *instanceTypeSuite) TestGetMatchingInstanceTypes(c *C) {
128+func (s *instanceTypeSuite) TestGetMatchingInstanceTypes(c *gc.C) {
129 for i, t := range getInstanceTypesTest {
130- c.Logf("test %d: %s", i, t.info)
131+ c.Logf("test %d: %s", i, t.about)
132 itypesToUse := t.itypesToUse
133 if itypesToUse == nil {
134 itypesToUse = instanceTypes
135 }
136 itypes, err := getMatchingInstanceTypes(constraint("test", t.cons), itypesToUse)
137- c.Assert(err, IsNil)
138+ c.Assert(err, gc.IsNil)
139 names := make([]string, len(itypes))
140 for i, itype := range itypes {
141 if len(t.arches) > 0 {
142- c.Check(itype.Arches, DeepEquals, filterArches(itype.Arches, t.arches))
143+ c.Check(itype.Arches, gc.DeepEquals, filterArches(itype.Arches, t.arches))
144 } else {
145- c.Check(len(itype.Arches) > 0, Equals, true)
146+ c.Check(len(itype.Arches) > 0, gc.Equals, true)
147 }
148 names[i] = itype.Name
149 }
150- c.Check(names, DeepEquals, t.expectedItypes)
151+ c.Check(names, gc.DeepEquals, t.expectedItypes)
152 }
153 }
154
155-func (s *instanceTypeSuite) TestGetMatchingInstanceTypesErrors(c *C) {
156+func (s *instanceTypeSuite) TestGetMatchingInstanceTypesErrors(c *gc.C) {
157 _, err := getMatchingInstanceTypes(constraint("test", "cpu-power=9001"), nil)
158- c.Check(err, ErrorMatches, `no instance types in test matching constraints "cpu-power=9001"`)
159+ c.Check(err, gc.ErrorMatches, `no instance types in test matching constraints "cpu-power=9001"`)
160
161 _, err = getMatchingInstanceTypes(constraint("test", "arch=i386 mem=8G"), instanceTypes)
162- c.Check(err, ErrorMatches, `no instance types in test matching constraints "arch=i386 mem=8192M"`)
163+ c.Check(err, gc.ErrorMatches, `no instance types in test matching constraints "arch=i386 mem=8192M"`)
164 }
165
166 var instanceTypeMatchTests = []struct {
167@@ -208,7 +211,7 @@
168 {"arch=arm", "c1.xlarge", nil},
169 }
170
171-func (s *instanceTypeSuite) TestMatch(c *C) {
172+func (s *instanceTypeSuite) TestMatch(c *gc.C) {
173 for i, t := range instanceTypeMatchTests {
174 c.Logf("test %d", i)
175 cons := constraints.MustParse(t.cons)
176@@ -218,16 +221,99 @@
177 break
178 }
179 }
180- c.Assert(itype.Name, Not(Equals), "")
181+ c.Assert(itype.Name, gc.Not(gc.Equals), "")
182 itype, match := itype.match(cons)
183 if len(t.arches) > 0 {
184- c.Check(match, Equals, true)
185+ c.Check(match, gc.Equals, true)
186 expect := itype
187 expect.Arches = t.arches
188- c.Check(itype, DeepEquals, expect)
189+ c.Check(itype, gc.DeepEquals, expect)
190 } else {
191- c.Check(match, Equals, false)
192- c.Check(itype, DeepEquals, InstanceType{})
193- }
194+ c.Check(match, gc.Equals, false)
195+ c.Check(itype, gc.DeepEquals, InstanceType{})
196+ }
197+ }
198+}
199+
200+var byCostTests = []struct {
201+ about string
202+ itypesToUse []InstanceType
203+ expectedItypes []string
204+}{
205+ {
206+ about: "default to lowest cost",
207+ itypesToUse: []InstanceType{
208+ {Id: "2", Name: "it-2", CpuCores: 2, Mem: 4096, Cost: 240},
209+ {Id: "1", Name: "it-1", CpuCores: 1, Mem: 2048, Cost: 241},
210+ },
211+ expectedItypes: []string{
212+ "it-2", "it-1",
213+ },
214+ }, {
215+ about: "when no cost associated, pick lowest ram",
216+ itypesToUse: []InstanceType{
217+ {Id: "2", Name: "it-2", CpuCores: 2, Mem: 4096},
218+ {Id: "1", Name: "it-1", CpuCores: 1, Mem: 2048},
219+ },
220+ expectedItypes: []string{
221+ "it-1", "it-2",
222+ },
223+ }, {
224+ about: "when cost is the same, pick lowest ram",
225+ itypesToUse: []InstanceType{
226+ {Id: "2", Name: "it-2", CpuCores: 2, Mem: 4096, Cost: 240},
227+ {Id: "1", Name: "it-1", CpuCores: 1, Mem: 2048, Cost: 240},
228+ },
229+ expectedItypes: []string{
230+ "it-1", "it-2",
231+ },
232+ }, {
233+ about: "when cost and ram is the same, pick lowest cpu power",
234+ itypesToUse: []InstanceType{
235+ {Id: "2", Name: "it-2", CpuCores: 2, CpuPower: CpuPower(200)},
236+ {Id: "1", Name: "it-1", CpuCores: 1, CpuPower: CpuPower(100)},
237+ },
238+ expectedItypes: []string{
239+ "it-1", "it-2",
240+ },
241+ }, {
242+ about: "when cpu power is the same, pick the lowest cores",
243+ itypesToUse: []InstanceType{
244+ {Id: "2", Name: "it-2", CpuCores: 2, CpuPower: CpuPower(200)},
245+ {Id: "1", Name: "it-1", CpuCores: 1, CpuPower: CpuPower(200)},
246+ },
247+ expectedItypes: []string{
248+ "it-1", "it-2",
249+ },
250+ }, {
251+ about: "when cpu power is missing in side a, pick the lowest cores",
252+ itypesToUse: []InstanceType{
253+ {Id: "2", Name: "it-2", CpuCores: 2, CpuPower: CpuPower(200)},
254+ {Id: "1", Name: "it-1", CpuCores: 1},
255+ },
256+ expectedItypes: []string{
257+ "it-1", "it-2",
258+ },
259+ }, {
260+ about: "when cpu power is missing in side b, pick the lowest cores",
261+ itypesToUse: []InstanceType{
262+ {Id: "2", Name: "it-2", CpuCores: 2},
263+ {Id: "1", Name: "it-1", CpuCores: 1, CpuPower: CpuPower(200)},
264+ },
265+ expectedItypes: []string{
266+ "it-1", "it-2",
267+ },
268+ },
269+}
270+
271+func (s *instanceTypeSuite) TestSortByCost(c *gc.C) {
272+ for i, t := range byCostTests {
273+ c.Logf("test %d: %s", i, t.about)
274+ sort.Sort(byCost(t.itypesToUse))
275+ names := make([]string, len(t.itypesToUse))
276+ for i, itype := range t.itypesToUse {
277+ names[i] = itype.Name
278+ }
279+ c.Check(names, gc.DeepEquals, t.expectedItypes)
280 }
281 }
282
283=== modified file 'environs/openstack/image.go'
284--- environs/openstack/image.go 2013-07-31 10:42:27 +0000
285+++ environs/openstack/image.go 2013-08-08 15:26:18 +0000
286@@ -25,7 +25,6 @@
287 Arches: ic.Arches,
288 Mem: uint64(flavor.RAM),
289 CpuCores: uint64(flavor.VCPUs),
290- Cost: uint64(flavor.RAM),
291 }
292 allInstanceTypes = append(allInstanceTypes, instanceType)
293 }

Subscribers

People subscribed via source and target branches

to status/vote changes: