environs/ec2: instance-type logic and data

environs/ec2: instance-type logic and data

Not hooked up to anything else yet, no change in functionality.

https://code.launchpad.net/~fwereade/juju-core/bootstrap-constraints-3a/+merge/153725

Requires: https://code.launchpad.net/~fwereade/juju-core/bootstrap-constraints-2/+merge/153600

(do not edit description out of merge proposal)

[fwereade, 2013-03-18 08:43:10]

Please take a look.

[fwereade, 2013-03-18 11:08:27]

Please take a look.

[dimitern, 2013-03-18 12:21:41]

Generally LGTM, but I'd really like it if we can somehow make these less
provider-specific (getting cloud images url and parsing it to get instance
types, etc.)

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go#...
environs/ec2/export_test.go:103: delete(allRegionCosts, "test")
deleting from a nil map is ok? didn't know this.

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

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:61: var defaultCpuPower uint64 = 100
should this be a const?

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:116: var allInstanceTypes = []instanceType{
it good to think about having these externally and filling them in initially,
using some parsing.

[jameinel, 2013-03-18 19:44:09]

LGTM overall. While I agree it would be nice to parse this from somewhere, I
think it is very specific to ec2. For Openstack, we'll have to poll the Flavors
API to do the constraint matching. Is there something more programatic for EC2?
(For example, the pricing list seems like something that you should check on
demand, rather than assuming a static pricing, though I'm guessing it doesn't
change a whole lot, and they certainly would be expected to retain their
relative costs.)

[dave_cheney.net, 2013-03-18 23:37:52]

I have a strong reservation to using floats for measuring instance cost, partly
because floats should not be used to model decimal values, but mainly because I
think it's going to bite us in the arse as floats don't behave like integers for
addition and multiplication.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go#...
environs/ec2/export_test.go:103: delete(allRegionCosts, "test")
On 2013/03/18 12:21:41, dimitern wrote:
> deleting from a nil map is ok? didn't know this.

It is possible in Go 1.1, not in 1.0. 

http://play.golang.org/p/ZXIvfj1DlQ (playground runs 1.0)

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

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:29: return nothing, false
nice name

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:50: continue outer
i don't think this needs to be this complicated, why not just break, which will
break the outer loop, right ? (i always find labeled breaks and continues
confusing)

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:61: var defaultCpuPower uint64 = 100
On 2013/03/18 12:21:41, dimitern wrote:
> should this be a const?

probably, but it's hairy and difficult to make work,
http://play.golang.org/p/Cp6a-Luyn0

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:116: var allInstanceTypes = []instanceType{
On 2013/03/18 12:21:41, dimitern wrote:
> it good to think about having these externally and filling them in initially,
> using some parsing.

+1, i'd like to see this move to goamz, someday.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:244: "ap-northeast-1": { // Tokyo.
If these are costs they should be integer cents, or 10ths of a cent. Floats
should not be used to represent money.

[thumper, 2013-03-19 00:03:18]

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

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:244: "ap-northeast-1": { // Tokyo.
On 2013/03/18 23:37:52, dfc wrote:
> If these are costs they should be integer cents, or 10ths of a cent. Floats
> should not be used to represent money.

If we were charging people, I'd certainly agree.

If however the costs are only being used to order options, I think floats
representing dollar values are fine.

[thumper, 2013-03-19 01:15:06]

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go#...
environs/ec2/export_test.go:91: var TestInstanceTypeContent =
map[string]float64{
Would be good to have a comment here as to what the map is a map to.  The
numbers by themselves are not obvious.

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

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:43: // filterArches returns every element of src
that also exists in filter.
Yet another wonderful use case for a set type.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:67: cons.CpuPower = &defaultCpuPower
Taking the address of a locally defined variable feels wrong.

[dave_cheney.net, 2013-03-19 01:20:15]

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

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:67: cons.CpuPower = &defaultCpuPower
On 2013/03/19 01:15:06, thumper wrote:
> Taking the address of a locally defined variable feels wrong.

Its not a locally defined var, it's a package level var. This is safe in Go.

[thumper, 2013-03-19 01:39:38]

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

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:67: cons.CpuPower = &defaultCpuPower
On 2013/03/19 01:20:15, dfc wrote:
> On 2013/03/19 01:15:06, thumper wrote:
> > Taking the address of a locally defined variable feels wrong.
> 
> Its not a locally defined var, it's a package level var. This is safe in Go.

This is safe in C++ too, but it doesn't mean it is a good idea.  cons.CpuPower
can now change the value of the package level variable.  If this is the expected
behaviour, I'd prefer a comment.

[fwereade, 2013-03-20 09:38:31]

*** Submitted:

environs/ec2: instance-type logic and data

Not hooked up to anything else yet, no change in functionality.

R=dimitern, jameinel, dfc, thumper
CC=
https://codereview.appspot.com/7693048

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go#...
environs/ec2/export_test.go:91: var TestInstanceTypeContent =
map[string]float64{
On 2013/03/19 01:15:06, thumper wrote:
> Would be good to have a comment here as to what the map is a map to.  The
> numbers by themselves are not obvious.

Done.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go#...
environs/ec2/export_test.go:103: delete(allRegionCosts, "test")
On 2013/03/18 12:21:41, dimitern wrote:
> deleting from a nil map is ok? didn't know this.

It's not nil.

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

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:29: return nothing, false
On 2013/03/18 23:37:52, dfc wrote:
> nice name

Cheers :)

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:50: continue outer
On 2013/03/18 23:37:52, dfc wrote:
> i don't think this needs to be this complicated, why not just break, which
will
> break the outer loop, right ? (i always find labeled breaks and continues
> confusing)

D'oh. Sorry.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:67: cons.CpuPower = &defaultCpuPower
On 2013/03/19 01:39:38, thumper wrote:
> On 2013/03/19 01:20:15, dfc wrote:
> > On 2013/03/19 01:15:06, thumper wrote:
> > > Taking the address of a locally defined variable feels wrong.
> > 
> > Its not a locally defined var, it's a package level var. This is safe in Go.
> 
> This is safe in C++ too, but it doesn't mean it is a good idea.  cons.CpuPower
> can now change the value of the package level variable.  If this is the
expected
> behaviour, I'd prefer a comment.

Better to just use a fresh copy, I think. Thanks all.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:116: var allInstanceTypes = []instanceType{
On 2013/03/18 23:37:52, dfc wrote:
> On 2013/03/18 12:21:41, dimitern wrote:
> > it good to think about having these externally and filling them in
initially,
> > using some parsing.
> 
> +1, i'd like to see this move to goamz, someday.
> 

Long-term, I think there's no question that we should be maintaining this list
in a central location. However, I don't think that's the best use of my
attention in the current context. I hope that in oakland we'll be able to use
the examples of maas, openstack, and ec2 to have a useful discussion about a
common instance-data format and write a shared implementation.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:244: "ap-northeast-1": { // Tokyo.
On 2013/03/19 00:03:18, thumper wrote:
> On 2013/03/18 23:37:52, dfc wrote:
> > If these are costs they should be integer cents, or 10ths of a cent. Floats
> > should not be used to represent money.
> 
> If we were charging people, I'd certainly agree.
> 
> If however the costs are only being used to order options, I think floats
> representing dollar values are fine.

I think dave's point is good -- it makes me froth when people calculate money
with floats, and by storing a money value as a float I am only enabling and
encouraging such behaviour in the future. And we can sort by int costs just as
easily as floats :).