Merge lp:~fwereade/juju-core/bootstrap-constraints-3a into lp:~juju/juju-core/trunk

[William Reade]

environs/ec2: instance-type logic and data

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

https://codereview.appspot.com/7693048/

[William Reade (fwereade)]

Reviewers: mp+153725_code.launchpad.net,

Message:
Please take a look.

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

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

Affected files:
   A [revision details]
   M environs/ec2/export_test.go
   A environs/ec2/instancetype.go
   A environs/ec2/instancetype_test.go

[William Reade (fwereade)]

Please take a look.

https://codereview.appspot.com/7693048/

[Dimiter Naydenov (dimitern)]

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#newcode103
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#newcode61
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#newcode116
environs/ec2/instancetype.go:116: var allInstanceTypes = []instanceType{
it good to think about having these externally and filling them in
initially, using some parsing.

https://codereview.appspot.com/7693048/

[John A Meinel (jameinel)]

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

https://codereview.appspot.com/7693048/

[Tim Penhey (thumper)]

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#newcode244
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.

https://codereview.appspot.com/7693048/

[Tim Penhey (thumper)]

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#newcode91
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#newcode43
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#newcode67
environs/ec2/instancetype.go:67: cons.CpuPower = &defaultCpuPower
Taking the address of a locally defined variable feels wrong.

https://codereview.appspot.com/7693048/

[Tim Penhey (thumper)]

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#newcode67
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.

https://codereview.appspot.com/7693048/

[William Reade (fwereade)]

*** 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#newcode91
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#newcode103
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#newcode29
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#newcode50
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#newcode67
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#newcode116
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#newcode244
environs/ec2/instancetype.go:244: "ap-northeast-1": { // Tokyo....