environs/ec2: constrained bootstrap instance

environs/ec2: constrained bootstrap instance

The constraints passed into juju bootstrap are now used to determine the
instance type that will be launched.

https://code.launchpad.net/~fwereade/juju-core/bootstrap-constraints-4/+merge/153759

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

(do not edit description out of merge proposal)

[fwereade, 2013-03-18 11:29:25]

Please take a look.

[dave_cheney.net, 2013-03-19 00:20:25]

Despite the number of comments, LGTM.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode15
environs/ec2/image.go:15: type instanceConstraint struct {
i fret that there are a lot of strings which should really be enums, but Go
doesn't give us a lot of assistance in this area, so I'm not going to harp on
it.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode107
environs/ec2/image.go:107: defer resp.Body.Close()
does this need to move to line 102 to avoid leaking response bodies ?

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode119
environs/ec2/image.go:119: return nil, fmt.Errorf("no %q images in %s with
arches %s", series, region, strings.Join(names, ", "))
line 116 looks a bit complicated, all to avoid

no "wakka" images in us-east-1 with arches [sparc,mips]

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode140
environs/ec2/image.go:140: hvm:  f[colVtype] == "hvm",
nice, i love this.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode144
environs/ec2/image.go:144: panic("unreachable")
go 1.1 removes the need for this, so one day we can remove this untestable
panic.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/suite_test.go
File environs/ec2/suite_test.go (left):

https://codereview.appspot.com/7754046/diff/1/environs/ec2/suite_test.go#oldc...
environs/ec2/suite_test.go:10: var regenerate = flag.Bool("regenerate-images",
false, "regenerate all data in images directory")
good, this was broken and would damage all the test data if run.

[thumper, 2013-03-19 01:25:13]

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode20
environs/ec2/image.go:20: }
How much of this is common between providers?  Can it be extracted out of ec2
into something more generic?

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode57
environs/ec2/image.go:57: hvm  bool
I don't find "hvm" a useful variable name. What does it mean? And if we need to
explain it, should it have a more descriptive name?

[dimitern, 2013-03-19 09:21:42]

LGTM with a few comments.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode57
environs/ec2/image.go:57: hvm  bool
On 2013/03/19 01:25:13, thumper wrote:
> I don't find "hvm" a useful variable name. What does it mean? And if we need
to
> explain it, should it have a more descriptive name?
+1
It's EC2 specific: "Hardware-assisted virtual machine"

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode117
environs/ec2/image.go:117: names[i] = fmt.Sprintf("%q", arch)
%q really, not %s?

[fwereade, 2013-03-20 11:36:13]

*** Submitted:

environs/ec2: constrained bootstrap instance

The constraints passed into juju bootstrap are now used to determine the
instance type that will be launched.

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

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode15
environs/ec2/image.go:15: type instanceConstraint struct {
On 2013/03/19 00:20:26, dfc wrote:
> i fret that there are a lot of strings which should really be enums, but Go
> doesn't give us a lot of assistance in this area, so I'm not going to harp on
> it.

Yeah, I'm pretty well inured to that feeling.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode20
environs/ec2/image.go:20: }
On 2013/03/19 01:25:13, thumper wrote:
> How much of this is common between providers?  Can it be extracted out of ec2
> into something more generic?

Maybe; this depends on what answer we choose for openstack's cloud data. I'm
reluctant to promote this implementation at this stage, because it makes
assumptions about the urls and the important fields (not to mention the format)
that may be ec2-specific to some degree; but I would like to strongly encourage
anyone implementing similar functionality to consider whether it's worth it to
move the complete findInstanceSpec mechanism out to its own package and require
that providers use interfaces to talk to it. I *think* that is a sensible
solution, but with just one implementation down I'm not sure I'm in a position
to divide the responsibilities in a helpful way.

Something like this *might* be sensible:

type Image interface {
    Arch() string
    Series() string
    Match(itype InstanceType)
}

type InstanceType interface {
    Match(cons state.Constraints) (InstanceType, bool)
}

type Environ interface {
    ...
    GetImages(series string, arches []string) ([]Image, error)
    Environ.GetInstanceTypes(cons state.Constraints) ([]InstanceType, error)
}

...but especially in the context of maas I'm not sure it's necessarily the right
thing to impose this structure.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode57
environs/ec2/image.go:57: hvm  bool
On 2013/03/19 09:21:42, dimitern wrote:
> On 2013/03/19 01:25:13, thumper wrote:
> > I don't find "hvm" a useful variable name. What does it mean? And if we need
> to
> > explain it, should it have a more descriptive name?
> +1
> It's EC2 specific: "Hardware-assisted virtual machine"

This is one of the reasons I'm not sure this is worth promoting to Official
Image Finding Code level. Thanks; commented.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode107
environs/ec2/image.go:107: defer resp.Body.Close()
On 2013/03/19 00:20:26, dfc wrote:
> does this need to move to line 102 to avoid leaking response bodies ?

Thanks, well spotted.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode117
environs/ec2/image.go:117: names[i] = fmt.Sprintf("%q", arch)
On 2013/03/19 09:21:42, dimitern wrote:
> %q really, not %s?

It was what I intended, but dfc makes a good point below.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode119
environs/ec2/image.go:119: return nil, fmt.Errorf("no %q images in %s with
arches %s", series, region, strings.Join(names, ", "))
On 2013/03/19 00:20:26, dfc wrote:
> line 116 looks a bit complicated, all to avoid
> 
> no "wakka" images in us-east-1 with arches [sparc,mips]

Done.

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode140
environs/ec2/image.go:140: hvm:  f[colVtype] == "hvm",
On 2013/03/19 00:20:26, dfc wrote:
> nice, i love this.

Cheers

https://codereview.appspot.com/7754046/diff/1/environs/ec2/image.go#newcode144
environs/ec2/image.go:144: panic("unreachable")
On 2013/03/19 00:20:26, dfc wrote:
> go 1.1 removes the need for this, so one day we can remove this untestable
> panic.

Yay!

[SteveNg.1988, 2013-03-20 11:47:26]

https://codereview.appspot.com/7754046/diff/6001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7754046/diff/6001/environs/dummy/environs.go#n...
environs/dummy/environs.go:551: if _, ok := e.Config().CACert(); !ok {
hehe