Openstack can use constraints

Openstack can use constraints

This branch removes the manadtory requirement to specify and image id and flavor
name when using juju with openstack. These config items can still be specified and
will be used as a fallback if required, but now the expected workflow is to use
contraints just like for ec2 to determine what type and size of instance to run.

The bulk of the business logic is moved from environs.ec2 up to environs, and this is
now shared between ec2 and openstack. Each of these providers provide a bit of specific
glue logic to tie it all together. ec2 uses hard coded instance type, openstack queries
the available flavors and uses information from there.

ec2 looks to files located at http://cloud-images.ubuntu.com to see what the available
instances are by looking in metadata files. openstack clouds don't have this luxury 
so the implementation in this branch allows for the instance metadata files to be placed
in the private or public storage. The private storage takes precedence. The files live
under a directory structure named like so:
series-image-metadata/<series>/server/released.current.txt 
This mimics the layout on cloud-images (with a differently named top level directory). 

The next branch will remove the hard coded instance id and flavor name from the live
openstack tests as these are now no longer required.

https://code.launchpad.net/~wallyworld/juju-core/openstack-image-lookup/+merge/159301

(do not edit description out of merge proposal)

[wallyworld, 2013-04-19 12:40:01]

Please take a look.

[jameinel, 2013-04-21 13:09:42]

LGTM.

Thanks for pulling all this stuff into common code.

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

https://codereview.appspot.com/8816045/diff/2001/environs/ec2/image.go#newcode29
environs/ec2/image.go:29: return environs.FindInstanceSpec(r, ic,
allInstanceTypes, allRegionCosts)
I saw we defer resp.Body.Close(), do we need to do the same for the
bufio.NewReader object? Probably not, I guess.

https://codereview.appspot.com/8816045/diff/2001/environs/image.go
File environs/image.go (right):

https://codereview.appspot.com/8816045/diff/2001/environs/image.go#newcode36
environs/image.go:36: // FindInstanceSpec returns an InstanceSpec satisfying the
supplied InstanceConstraint.
It would be really good to define what 'r *bufio.Reader' is here. Something
describing what it is (a file containing image lists of the format described by
cloud-images.ubuntu.com) possibly just a link to a spec for it, or something
along those lines.
The other params are reasonably self describing, but 'r *bufio.Reader' is pretty
opaque.

https://codereview.appspot.com/8816045/diff/2001/environs/image.go#newcode150
environs/image.go:150: return false
The default initialization stuff means it is a bit hard to figure out what this
is actually matching against. (Nowhere in the code do we say Clustered: false,
because that is an implicit default. But it means you can't grep for it.)

I realize you probably didn't write it, but it is one of those cases where *I*
would prefer being explicit.

https://codereview.appspot.com/8816045/diff/2001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/8816045/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:712: }
We seem to have both 'scfg.possibleTools.Series()' and
startInstanceParams.series.

The former is a slice, while the later is a single value. Wouldn't it be better
to use the params struct?

https://codereview.appspot.com/8816045/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:727: userData, err := e.userData(scfg, tools[0])
It feels like this should be sorted or something. (maybe Match already has a
preferential sorting?)
I thought William actually went for "look for possible tools, pick an image that
matches the tools that are available", while this seems the reverse "pick an
image, then find tools that match".

I guess earlier we found the tool Arches, and the it would be a little weird to
come back without that available.

[wallyworld, 2013-04-22 02:09:34]

Please take a look.

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

https://codereview.appspot.com/8816045/diff/2001/environs/ec2/image.go#newcode29
environs/ec2/image.go:29: return environs.FindInstanceSpec(r, ic,
allInstanceTypes, allRegionCosts)
On 2013/04/21 13:09:42, jameinel wrote:
> I saw we defer resp.Body.Close(), do we need to do the same for the
> bufio.NewReader object? Probably not, I guess.

I am almost 100% sure that bufio.Reader() does not have or require a Close()
method

https://codereview.appspot.com/8816045/diff/2001/environs/image.go
File environs/image.go (right):

https://codereview.appspot.com/8816045/diff/2001/environs/image.go#newcode36
environs/image.go:36: // FindInstanceSpec returns an InstanceSpec satisfying the
supplied InstanceConstraint.
On 2013/04/21 13:09:42, jameinel wrote:
> It would be really good to define what 'r *bufio.Reader' is here. Something
> describing what it is (a file containing image lists of the format described
by
> http://cloud-images.ubuntu.com) possibly just a link to a spec for it, or
something
> along those lines.
> The other params are reasonably self describing, but 'r *bufio.Reader' is
pretty
> opaque.

Done.

https://codereview.appspot.com/8816045/diff/2001/environs/image.go#newcode150
environs/image.go:150: return false
On 2013/04/21 13:09:42, jameinel wrote:
> The default initialization stuff means it is a bit hard to figure out what
this
> is actually matching against. (Nowhere in the code do we say Clustered: false,
> because that is an implicit default. But it means you can't grep for it.)
> 
> I realize you probably didn't write it, but it is one of those cases where *I*
> would prefer being explicit.

The EC2 instance type structs were indeed written before this branch. The
attribute used to be called hvm (and defined to be a string, also not using
explicit initialisation) but I changed it to Clustered since that's more
generic. I've updated the struct constructors to explicitly say "Clustered:
false"

https://codereview.appspot.com/8816045/diff/2001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/8816045/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:712: }
On 2013/04/21 13:09:42, jameinel wrote:
> We seem to have both 'scfg.possibleTools.Series()' and
> startInstanceParams.series.
> 
> The former is a slice, while the later is a single value. Wouldn't it be
better
> to use the params struct?

Possibly. I copied the code directly from the ec2 provider implementation, so
it's wrong there too (the Openstack provider, before this branch, used a hard
wired "precise" hack). This looks like fallout from the recent work to fix the
tools provisioning and matching etc. I changed the code to use the scfg.series
attribute and added a sanity check like so:

series := scfg.possibleTools.Series()
if len(series) != 1 {
    return nil, fmt.Errorf("expected single series, got %v", series)
}
if series[0] != scfg.series {
    return nil, fmt.Errorf("tools mismatch: expected series %v, got %v", series,
series[0])
}

The tests still pass. William really needs to say yay or nay on this.

https://codereview.appspot.com/8816045/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:727: userData, err := e.userData(scfg, tools[0])
On 2013/04/21 13:09:42, jameinel wrote:
> It feels like this should be sorted or something. (maybe Match already has a
> preferential sorting?)
> I thought William actually went for "look for possible tools, pick an image
that
> matches the tools that are available", while this seems the reverse "pick an
> image, then find tools that match".
> 
> I guess earlier we found the tool Arches, and the it would be a little weird
to
> come back without that available.
> 

The work done in the openstack provider here matches what was done in the ec2
provider. The possible tools are indeed found first, then the available arches
from those possible tools used to get a suitable image. The last step is simply
to pick out the tools matching the arch of the image chosen. So it seems
correct, but William again needs to comment I think.

[dimitern, 2013-04-23 11:44:55]

Generally LGTM, with some trivials.

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/image_test.go
File environs/ec2/image_test.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/image_test.go#n...
environs/ec2/image_test.go:42: strs[i] =
fmt.Sprintf("\t\t\t\t%s\t%s\t%s\t%s\t\t\t%s\t\t\n", args...)
i'd appreciate a comment somewhere describing what each field should hold.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go
File environs/instances/image.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:33: // minMemoryForMongoDB is the assumed minimum
amount of memory (in MB) we require in order to run MongoDB (1GB)
please, as discussed, rename this to remove MongoDB from the comment and
constant. It's an internal detail that's irrelevant. minMemoryHeuristic or
something should do, and the comment can be // minMemoryHeuristic is the assumed
minimum amount of memory (in MB) we require when no explicit constraint is given
by the user.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:38: // interface for EC2 images is exposed at
http://cloud-images.ubuntu.com/query. Other cloud providers may
it's not only for EC2 now, so please remove the reference and reword it to be
more generic.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:39: // provide similar files for their own images.
eg the Openstack provider has been configured to look for
s/eg/e.g./

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:57: //    and our own heuristic: minimum amount of
memory required to run MongoDB, or
see above about the comment wrt MongoDB.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:69: // 1. check for smallest instance type that can
run mongodb
d - already mentioned above (or move the 1. comment here)

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:77: // 2. just get the one with the largest memory
d - or move the 2. comment here.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:105: return nil, fmt.Errorf(`no %q images in %s with
arches %s, and no default specified`,
no need for ` quotes, use "

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:170: // supplied series in the supplied region.
add some comment here describing the order of fields on each line and what each
one is supposed to contain?

[wallyworld, 2013-04-24 00:56:52]

Thanks for the review, appreciated. I've addressed the issues highlighted.

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/image_test.go
File environs/ec2/image_test.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/image_test.go#n...
environs/ec2/image_test.go:42: strs[i] =
fmt.Sprintf("\t\t\t\t%s\t%s\t%s\t%s\t\t\t%s\t\t\n", args...)
On 2013/04/23 11:44:56, dimitern wrote:
> i'd appreciate a comment somewhere describing what each field should hold.

This method was copied from elsewhere and not documented.
I've included a link to https://help.ubuntu.com/community/UEC/Images

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go
File environs/instances/image.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:33: // minMemoryForMongoDB is the assumed minimum
amount of memory (in MB) we require in order to run MongoDB (1GB)
On 2013/04/23 11:44:56, dimitern wrote:
> please, as discussed, rename this to remove MongoDB from the comment and
> constant. It's an internal detail that's irrelevant. minMemoryHeuristic or
> something should do, and the comment can be // minMemoryHeuristic is the
assumed
> minimum amount of memory (in MB) we require when no explicit constraint is
given
> by the user.

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:38: // interface for EC2 images is exposed at
http://cloud-images.ubuntu.com/query. Other cloud providers may
On 2013/04/23 11:44:56, dimitern wrote:
> it's not only for EC2 now, so please remove the reference and reword it to be
> more generic.

http://cloud-images.ubuntu.com/query is only for EC2

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:39: // provide similar files for their own images.
eg the Openstack provider has been configured to look for
On 2013/04/23 11:44:56, dimitern wrote:
> s/eg/e.g./

Really? Ok

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:57: //    and our own heuristic: minimum amount of
memory required to run MongoDB, or
On 2013/04/23 11:44:56, dimitern wrote:
> see above about the comment wrt MongoDB.

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:69: // 1. check for smallest instance type that can
run mongodb
On 2013/04/23 11:44:56, dimitern wrote:
> d - already mentioned above (or move the 1. comment here)

The comment here is a reminder of what is being done since the code is complex.
The 1. comment at the top is a more detailed explanation, here it's just a quick
summary

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:77: // 2. just get the one with the largest memory
On 2013/04/23 11:44:56, dimitern wrote:
> d - or move the 2. comment here.

See previous explanation

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:105: return nil, fmt.Errorf(`no %q images in %s with
arches %s, and no default specified`,
On 2013/04/23 11:44:56, dimitern wrote:
> no need for ` quotes, use "

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:170: // supplied series in the supplied region.
On 2013/04/23 11:44:56, dimitern wrote:
> add some comment here describing the order of fields on each line and what
each
> one is supposed to contain?

This code was copied from elsewhere and was not commented there either. I'll
include a link to https://help.ubuntu.com/community/UEC/Images which explains it
all.

[fwereade, 2013-04-24 16:18:15]

Sorry, but NOT LGTM until we've spoken about this a bit. I'm keen to factor out
the bits that really are common, but I'm concerned that a lot of ec2-specific
stuff has got pulled up to the general level and mixed in with new and
openstack-specific stuff, and that it'll be very hard to make this code work
with new providers (with, for example, their own notion of "clustered").

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

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/image.go#newcode29
environs/ec2/image.go:29: return instances.FindInstanceSpec(r, ic,
allInstanceTypes, allRegionCosts)
Will the conversion of some reader into a list of images be put back at the
provider level at some point? ISTM that `ic, allImages, allInstanceTypes,
allRegionCosts` would be better than what's here (and maybe that `ic, allImages,
allInstanceTypes` would be even better (in which the instance types had their
costs filled in externally)).

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

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:12: Arches:    instances.Both,
Yeah, this should definitely be defined in this package.

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:16: Clustered: false,
Just skip the `Clustered: false,` lines, I reckon

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go
File environs/instances/image.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:23: Cluster *string
This is by no means a constraint -- AFAICT it's actually the magic key we use to
convert virtKind into a bool to then be used as a *kind* of constraint, I guess,
but... it's a bit of a stretch.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:46: func FindInstanceSpec(r *bufio.Reader, ic
*InstanceConstraint, allInstanceTypes []InstanceType, regionCosts RegionCosts)
(*InstanceSpec, error) {
I think this has been somewhat overcomplicated by making regionCosts optional.
Surely, clients should be responsible for supplying instance types with costs
already filled in.

(Apart from anything else, we're going to have to be able to supply a list of
all available instance types to the GUI -- hence, via the API -- so it's
information the environment will have to be able to access anyway)

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:59: //    albeit not the best match
A bit misleading -- please make it clear that it's about memory, not cost.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:67: typeByMemory := byMemory(fallbackTypes)
This doesn't need its own var -- it sorts fallbackTypes in place

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:98: matchingTypes[0].Id, matchingTypes[0].Name,
There are enough fields here that they could use explicit names, I think.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:128: }
The Sorter (or whatever it is) interface is usually written somewhat more
densely, and I think it's reasonable to do so here -- there aren't any surprises
in this implementation (by contrast, byArch below is condensed in the bits that
are obvious and exploded for the weird logic in Less).

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:153: Clustered bool
I don't think this is a meaningful concept at juju level. It's a magic
EC2-specific weirdness; it would be just about acceptable if it were `VirtKind
string`, and we required all providers to supply images and instance types that
match, but as it is it's not appropriate here.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:15: CpuPower uint64
Not available on openstack, but no way to represent that except an in-band 0.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:18: Clustered bool
As CpuPower.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:21: // all instance types can run amd64
images, and some can also run i386 ones.
Strongly -1 on making this information general. It's not true for openstack, and
I'm not at all sure we won't be seeing clouds with (eg) a choice of amd64 and
arm instance types...

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:44: if cons.CpuPower != nil && itype.CpuPower
> 0 && itype.CpuPower < *cons.CpuPower {
Yeah, not keen on the in-band signalling.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:72: var defaultCpuPower uint64 = 100
This is *completely* ec2-specific and has no place in a general package. What
are we meant to do about clouds that have a standard and perfectly viable
instance type with 80 cpu-power?

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:78: func getMatchingInstanceTypes(ic
*InstanceConstraint, allinstanceTypes []InstanceType, allRegionCosts
RegionCosts) ([]InstanceType, error) {
Yeah, this too will be much cleaner if we supply instance types with pre-filled
costs. We shouldn't be threading special-cases through our "general" code -- we
should be abstracting away the differences and keeping the special cases where
we need them in the providers.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:123: if defaultInstanceType != nil {
We shouldn't be using a default that directly conflicts with the constraints.

https://codereview.appspot.com/8816045/diff/9001/environs/openstack/image.go
File environs/openstack/image.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/openstack/image.go#...
environs/openstack/image.go:25: CpuCores: uint64(flavor.VCPUs),
Maybe better, if we're faking up cost as memory for openstack, to do it
explicitly at this level. Not sure though.

[wallyworld, 2013-04-26 05:02:35]

I've addressed the issue with the constraints clustered attribute and moved the
ec2 specific cpu power logic back to the ec2 provider. The cost logic has also
been moved out to each provider.

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

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/image.go#newcode29
environs/ec2/image.go:29: return instances.FindInstanceSpec(r, ic,
allInstanceTypes, allRegionCosts)
On 2013/04/24 16:18:15, fwereade wrote:
> Will the conversion of some reader into a list of images be put back at the
> provider level at some point? ISTM that `ic, allImages, allInstanceTypes,
> allRegionCosts` would be better than what's here (and maybe that `ic,
allImages,
> allInstanceTypes` would be even better (in which the instance types had their
> costs filled in externally)).

It's been done as a Reader for now because the image metadata file content is
identical for ec2 and openstack and the only difference is where the file comes
from - this bit has been left at the provider level. For now, only ec2 and
openstack are supported with this code. A subsequent branch to use the
oxygen/simplestreams stuff can refactor this further if required.

I've moved the cost calculations back out to the provider.

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

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:12: Arches:    instances.Both,
On 2013/04/24 16:18:15, fwereade wrote:
> Yeah, this should definitely be defined in this package.

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:16: Clustered: false,
On 2013/04/24 16:18:15, fwereade wrote:
> Just skip the `Clustered: false,` lines, I reckon

I added these because John asked for it. But, it's moot now since a VType
attribute is used instead.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go
File environs/instances/image.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:23: Cluster *string
On 2013/04/24 16:18:15, fwereade wrote:
> This is by no means a constraint -- AFAICT it's actually the magic key we use
to
> convert virtKind into a bool to then be used as a *kind* of constraint, I
guess,
> but... it's a bit of a stretch.

This has now been removed.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:46: func FindInstanceSpec(r *bufio.Reader, ic
*InstanceConstraint, allInstanceTypes []InstanceType, regionCosts RegionCosts)
(*InstanceSpec, error) {
On 2013/04/24 16:18:15, fwereade wrote:
> I think this has been somewhat overcomplicated by making regionCosts optional.
> Surely, clients should be responsible for supplying instance types with costs
> already filled in.
> 
> (Apart from anything else, we're going to have to be able to supply a list of
> all available instance types to the GUI -- hence, via the API -- so it's
> information the environment will have to be able to access anyway)

I've most the cost calculations back out to the provider

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:59: //    albeit not the best match
On 2013/04/24 16:18:15, fwereade wrote:
> A bit misleading -- please make it clear that it's about memory, not cost.

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:67: typeByMemory := byMemory(fallbackTypes)
On 2013/04/24 16:18:15, fwereade wrote:
> This doesn't need its own var -- it sorts fallbackTypes in place

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:98: matchingTypes[0].Id, matchingTypes[0].Name,
On 2013/04/24 16:18:15, fwereade wrote:
> There are enough fields here that they could use explicit names, I think.

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:128: }
On 2013/04/24 16:18:15, fwereade wrote:
> The Sorter (or whatever it is) interface is usually written somewhat more
> densely, and I think it's reasonable to do so here -- there aren't any
surprises
> in this implementation (by contrast, byArch below is condensed in the bits
that
> are obvious and exploded for the weird logic in Less).

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/image.go#...
environs/instances/image.go:153: Clustered bool
On 2013/04/24 16:18:15, fwereade wrote:
> I don't think this is a meaningful concept at juju level. It's a magic
> EC2-specific weirdness; it would be just about acceptable if it were `VirtKind
> string`, and we required all providers to supply images and instance types
that
> match, but as it is it's not appropriate here.

It's now a VType string attribute

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:18: Clustered bool
On 2013/04/24 16:18:15, fwereade wrote:
> As CpuPower.

Clustered is gone. We now have
VType *string
CpuPower *uint64

These are both optional in that the cloud may not support making them available.
They will be ignored if not available.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:21: // all instance types can run amd64
images, and some can also run i386 ones.
On 2013/04/24 16:18:15, fwereade wrote:
> Strongly -1 on making this information general. It's not true for openstack,
and
> I'm not at all sure we won't be seeing clouds with (eg) a choice of amd64 and
> arm instance types...

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:44: if cons.CpuPower != nil && itype.CpuPower
> 0 && itype.CpuPower < *cons.CpuPower {
On 2013/04/24 16:18:15, fwereade wrote:
> Yeah, not keen on the in-band signalling.

itype.CpuPower is now *uint64

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:72: var defaultCpuPower uint64 = 100
On 2013/04/24 16:18:15, fwereade wrote:
> This is *completely* ec2-specific and has no place in a general package. What
> are we meant to do about clouds that have a standard and perfectly viable
> instance type with 80 cpu-power?

Moved back to ec2 provider

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:72: var defaultCpuPower uint64 = 100
On 2013/04/24 16:18:15, fwereade wrote:
> This is *completely* ec2-specific and has no place in a general package. What
> are we meant to do about clouds that have a standard and perfectly viable
> instance type with 80 cpu-power?

Done.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:78: func getMatchingInstanceTypes(ic
*InstanceConstraint, allinstanceTypes []InstanceType, allRegionCosts
RegionCosts) ([]InstanceType, error) {
On 2013/04/24 16:18:15, fwereade wrote:
> Yeah, this too will be much cleaner if we supply instance types with
pre-filled
> costs. We shouldn't be threading special-cases through our "general" code --
we
> should be abstracting away the differences and keeping the special cases where
> we need them in the providers.

I've implemented the suggestion to move the cost stuff back out to the provider.

https://codereview.appspot.com/8816045/diff/9001/environs/instances/instancet...
environs/instances/instancetype.go:123: if defaultInstanceType != nil {
On 2013/04/24 16:18:15, fwereade wrote:
> We shouldn't be using a default that directly conflicts with the constraints.

This I guess is a matter for discussion, as defaultInstanceType and
defaultImageId sort of fall into the same category. Both of these attributes are
implemented to provide fallbacks only if all else fails. Since these attributes
default to "", I'd like to land this as is and we can discuss?

https://codereview.appspot.com/8816045/diff/9001/environs/openstack/image.go
File environs/openstack/image.go (right):

https://codereview.appspot.com/8816045/diff/9001/environs/openstack/image.go#...
environs/openstack/image.go:25: CpuCores: uint64(flavor.VCPUs),
On 2013/04/24 16:18:15, fwereade wrote:
> Maybe better, if we're faking up cost as memory for openstack, to do it
> explicitly at this level. Not sure though.

Done.

[wallyworld, 2013-04-26 05:04:34]

Please take a look.

[fwereade, 2013-04-28 23:01:47]

Thanks, this is much closer, I think; I still have opinions I will voice but not
force on you ;p, and there are only a couple of significant questions (about the
memory heuristic and the default instance type).

https://codereview.appspot.com/8816045/diff/2002/environs/ec2/image_test.go
File environs/ec2/image_test.go (right):

https://codereview.appspot.com/8816045/diff/2002/environs/ec2/image_test.go#n...
environs/ec2/image_test.go:31: func imagesFields(srcs ...string) string {
This is duplicated in openstack/export_test -- stick it in environs/testing?

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

https://codereview.appspot.com/8816045/diff/2002/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:16: Both  = []string{"amd64", "i386"}
Sorry, I didn't spot where we needed to export these?

https://codereview.appspot.com/8816045/diff/2002/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:162: var allRegionCosts = instances.RegionCosts{
I feel like RegionCosts is still hoisted up too high; it's very ec2.

https://codereview.appspot.com/8816045/diff/2002/environs/instances/image.go
File environs/instances/image.go (right):

https://codereview.appspot.com/8816045/diff/2002/environs/instances/image.go#...
environs/instances/image.go:46: matchingTypes, err :=
getMatchingInstanceTypes(ic, allInstanceTypes)
Shouldn't we be applying the memory heuristic first, and only falling back to
allow for less memory when that fails?

https://codereview.appspot.com/8816045/diff/2002/environs/instances/image.go#...
environs/instances/image.go:101: }
This still makes me uncomfortable, but I agree it lets the old solution coexist
adequately for a while.

https://codereview.appspot.com/8816045/diff/2002/environs/instances/image.go#...
environs/instances/image.go:112: return nil, fmt.Errorf("no %q images in %s
matching instance types %v", ic.Series, ic.Region, names)
This all still feels like it's mixing quite a lot of responsibilities, but at
least they're all related and all in one place and I agree it's progress.

https://codereview.appspot.com/8816045/diff/2002/environs/instances/instancet...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/8816045/diff/2002/environs/instances/instancet...
environs/instances/instancetype.go:94: return
[]InstanceType{*defaultInstanceType}, nil
I still don't think it's correct to return the default if it fails to match the
constraints. If it is one among many that match, it should certainly be used; if
it doesn't match, it certainly shouldn't. If we're going to let the user specify
constraints, we should actually respect them.

[wallyworld, 2013-04-29 01:07:51]

Please take a look.

https://codereview.appspot.com/8816045/diff/2002/environs/ec2/image_test.go
File environs/ec2/image_test.go (right):

https://codereview.appspot.com/8816045/diff/2002/environs/ec2/image_test.go#n...
environs/ec2/image_test.go:31: func imagesFields(srcs ...string) string {
On 2013/04/28 23:01:47, fwereade wrote:
> This is duplicated in openstack/export_test -- stick it in environs/testing?

Good idea. I was having trouble thinking of a suitable place to put it. Done.

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

https://codereview.appspot.com/8816045/diff/2002/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:16: Both  = []string{"amd64", "i386"}
On 2013/04/28 23:01:47, fwereade wrote:
> Sorry, I didn't spot where we needed to export these?

Ah, this was a carry over from previous work when they were exported but they do
not need to be now with the latest code. Fixed.

https://codereview.appspot.com/8816045/diff/2002/environs/ec2/instancetype.go...
environs/ec2/instancetype.go:162: var allRegionCosts = instances.RegionCosts{
On 2013/04/28 23:01:47, fwereade wrote:
> I feel like RegionCosts is still hoisted up too high; it's very ec2.

Yes, this was a result of previous work and needed to be moved back to the ec2
package. Done.

https://codereview.appspot.com/8816045/diff/2002/environs/instances/image.go
File environs/instances/image.go (right):

https://codereview.appspot.com/8816045/diff/2002/environs/instances/image.go#...
environs/instances/image.go:46: matchingTypes, err :=
getMatchingInstanceTypes(ic, allInstanceTypes)
On 2013/04/28 23:01:47, fwereade wrote:
> Shouldn't we be applying the memory heuristic first, and only falling back to
> allow for less memory when that fails?

Sorry if I am missing something.
I think that's what we do? As per the code comments:

// No matching instance types were found, so the fallback is to:
// 1. Sort by memory and find the smallest matching both the required
architecture
//    and our own heuristic: minimum amount of memory required to run a
realistic server, or
// 2. Sort by memory in reverse order and return the largest one, which will
hopefully work,
//    albeit not the best match

https://codereview.appspot.com/8816045/diff/2002/environs/instances/image.go#...
environs/instances/image.go:101: }
On 2013/04/28 23:01:47, fwereade wrote:
> This still makes me uncomfortable, but I agree it lets the old solution
coexist
> adequately for a while.

It's required for things like private openstack clouds where no standard
metadata exists and people just want an easy way to get this working.

https://codereview.appspot.com/8816045/diff/2002/environs/instances/image.go#...
environs/instances/image.go:112: return nil, fmt.Errorf("no %q images in %s
matching instance types %v", ic.Series, ic.Region, names)
On 2013/04/28 23:01:47, fwereade wrote:
> This all still feels like it's mixing quite a lot of responsibilities, but at
> least they're all related and all in one place and I agree it's progress.

As far as I recall, this code is pretty much aligned with the original business
logic, but with the extra rules for finding a suitable instance type added in.
We can certainly look to improve it in subsequent branches when oxygen support
is introduced.

https://codereview.appspot.com/8816045/diff/2002/environs/instances/instancet...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/8816045/diff/2002/environs/instances/instancet...
environs/instances/instancetype.go:94: return
[]InstanceType{*defaultInstanceType}, nil
On 2013/04/28 23:01:47, fwereade wrote:
> I still don't think it's correct to return the default if it fails to match
the
> constraints. If it is one among many that match, it should certainly be used;
if
> it doesn't match, it certainly shouldn't. If we're going to let the user
specify
> constraints, we should actually respect them.

I can see your point. If it's ok, can we leave it like this for now and discuss
within the team and make any changes in a subsequent branch?

[dave_cheney.net, 2013-04-29 06:14:28]

Hi Ian,

I would prefer to see this as two CLs, one splitting off the code into
environs/instance and the next with the openstack changes.

Cheers

Dave

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/ec2.go#newcode401
environs/ec2/ec2.go:401: var ebsStorage = "ebs"
can this be a const? if not, why not ?

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

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/image.go#newco...
environs/ec2/image.go:19: var defaultCpuPower uint64 = 100
can this be a const ?

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/image.go#newco...
environs/ec2/image.go:26: }
this is mutating the ic passed to this function, does the caller expect this ?

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/image.go#newco...
environs/ec2/image.go:38: }
if err != nil then r will be nil. err is never returned to the caller.

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

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/instancetype.g...
environs/ec2/instancetype.go:9: paravirtual = "paravirtual"
these consts with addresses are a real code smell. I think this is a flaw in the
constraints API.

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/instancetype.g...
environs/ec2/instancetype.go:26: CpuPower: instances.CpuPower(100),
why isn't this just a number ?

[wallyworld, 2013-04-29 07:19:17]

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/ec2.go#newcode401
environs/ec2/ec2.go:401: var ebsStorage = "ebs"
On 2013/04/29 06:14:29, dfc wrote:
> can this be a const? if not, why not ? 

The address is passed to the constraints struct for if it is a constant the
compile fails with 
"cannot take the address of ebsStorage"

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

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/image.go#newco...
environs/ec2/image.go:19: var defaultCpuPower uint64 = 100
On 2013/04/29 06:14:29, dfc wrote:
> can this be a const ?

No, since the address is passed to the constraints struct and it won't compile
if it is a const. Unless I am missing something.

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/image.go#newco...
environs/ec2/image.go:26: }
On 2013/04/29 06:14:29, dfc wrote:
> this is mutating the ic passed to this function, does the caller expect this ?

I more of less just copied this code I think (it's been a while). I guess I
could take a copy of the constraints?

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/image.go#newco...
environs/ec2/image.go:38: }
On 2013/04/29 06:14:29, dfc wrote:
> if err != nil then r will be nil. err is never returned to the caller.

I think some refactoring messed up. I'll revisit.

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

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/instancetype.g...
environs/ec2/instancetype.go:9: paravirtual = "paravirtual"
On 2013/04/29 06:14:29, dfc wrote:
> these consts with addresses are a real code smell. I think this is a flaw in
the
> constraints API.

These are optional attributes and rather than use inband values (eg "") to
communicate the missing value, allowing a pointer set to nil was the preferred
choice.

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/instancetype.g...
environs/ec2/instancetype.go:26: CpuPower: instances.CpuPower(100),
On 2013/04/29 06:14:29, dfc wrote:
> why isn't this just a number ?

It has to be a pointer since it is an optional attribute.

[fwereade, 2013-04-29 11:53:48]

a couple of thoughts for reference points in discussion

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/ec2.go#newcode401
environs/ec2/ec2.go:401: var ebsStorage = "ebs"
On 2013/04/29 07:19:18, wallyworld wrote:
> On 2013/04/29 06:14:29, dfc wrote:
> > can this be a const? if not, why not ? 
> 
> The address is passed to the constraints struct for if it is a constant the
> compile fails with 
> "cannot take the address of ebsStorage"

It can be done in two stages (var = const, val = &var)

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

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/image.go#newco...
environs/ec2/image.go:19: var defaultCpuPower uint64 = 100
On 2013/04/29 07:19:18, wallyworld wrote:
> On 2013/04/29 06:14:29, dfc wrote:
> > can this be a const ?
> 
> No, since the address is passed to the constraints struct and it won't compile
> if it is a const. Unless I am missing something.

v := defaultCpuPower
cons.CpuPower = &v

...is pretty clear, and lets the const be const

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/image.go#newco...
environs/ec2/image.go:26: }
On 2013/04/29 07:19:18, wallyworld wrote:
> On 2013/04/29 06:14:29, dfc wrote:
> > this is mutating the ic passed to this function, does the caller expect this
?
> 
> I more of less just copied this code I think (it's been a while). I guess I
> could take a copy of the constraints?

The constraints were originally copied by being passed, now they're attached to
ic. I don't believe InstanceConstraint is going to last long -- it was a bad
idea in the first place and expanding it is not helpful long-term -- but I'm
grudginly ok with it as a place to stick all the unrelated junk that's somehow
got bound up in this problem until we can factor it into something cleaner.

(I have a sketchy branch with `resource.GetSpec(resource.Map, series string,
cons constraints.Value) (resource.Spec, error)` that might be a cleaner approach
in general, but it doesn't do everything it should yet and I want to explore it
a bit further in case I've missed something obvious).

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

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/instancetype.g...
environs/ec2/instancetype.go:9: paravirtual = "paravirtual"
On 2013/04/29 07:19:18, wallyworld wrote:
> On 2013/04/29 06:14:29, dfc wrote:
> > these consts with addresses are a real code smell. I think this is a flaw in
> the
> > constraints API.
> 
> These are optional attributes and rather than use inband values (eg "") to
> communicate the missing value, allowing a pointer set to nil was the preferred
> choice.

I'd rather have them really const, though.

https://codereview.appspot.com/8816045/diff/29001/environs/ec2/instancetype.g...
environs/ec2/instancetype.go:26: CpuPower: instances.CpuPower(100),
On 2013/04/29 07:19:18, wallyworld wrote:
> On 2013/04/29 06:14:29, dfc wrote:
> > why isn't this just a number ?
> 
> It has to be a pointer since it is an optional attribute.

I generally agree with dfc's reaction to this, but I'm reasonably comfortable
that it's a step on the way towards clarity. In particular, I'm expecting that
instance types will become interfaces at some stage, but we'll worry about that
later.

https://codereview.appspot.com/8816045/diff/29001/environs/instances/image.go
File environs/instances/image.go (right):

https://codereview.appspot.com/8816045/diff/29001/environs/instances/image.go...
environs/instances/image.go:46: matchingTypes, err :=
getMatchingInstanceTypes(ic, allInstanceTypes)
If we don't get an error, we just got a matching instance type and we're done --
so we should basically never hit the following branch in a useful way, because
it we didn't find anything with the supplied constraints we certainly won't with
tighter ones.

...except, reading on, we *don't* use tighter ones -- we just discard the
supplied constraints. This is definitely wrong.

https://codereview.appspot.com/8816045/diff/29001/environs/instances/image.go...
environs/instances/image.go:49: // asked for a nominated default instance type
to be used as a fallback and that is invalid, we
"default" != "fallback"

https://codereview.appspot.com/8816045/diff/29001/environs/instances/image.go...
environs/instances/image.go:60: archCons := &InstanceConstraint{Arches:
ic.Arches}
Wait, so we just discard the original constraints? That's definitely not right.

[fwereade, 2013-04-29 12:16:28]

on discussion, LGTM on the understanding that the way constraints can be ignored
by default-instance-type will be made into a bug and addressed soon.

[dave_cheney.net, 2013-04-29 13:48:18]

If William is happy then i'm happy too, LGTM.

On Mon, Apr 29, 2013 at 10:16 PM,  <fwereade@gmail.com> wrote:
> on discussion, LGTM on the understanding that the way constraints can be
> ignored by default-instance-type will be made into a bug and addressed
> soon.
>
> https://codereview.appspot.com/8816045/

[wallyworld, 2013-04-30 05:07:07]

*** Submitted:

Openstack can use constraints

This branch removes the manadtory requirement to specify and image id and flavor
name when using juju with openstack. These config items can still be specified
and
will be used as a fallback if required, but now the expected workflow is to use
contraints just like for ec2 to determine what type and size of instance to run.

The bulk of the business logic is moved from environs.ec2 up to environs, and
this is
now shared between ec2 and openstack. Each of these providers provide a bit of
specific
glue logic to tie it all together. ec2 uses hard coded instance type, openstack
queries
the available flavors and uses information from there.

ec2 looks to files located at http://cloud-images.ubuntu.com to see what the
available
instances are by looking in metadata files. openstack clouds don't have this
luxury 
so the implementation in this branch allows for the instance metadata files to
be placed
in the private or public storage. The private storage takes precedence. The
files live
under a directory structure named like so:
series-image-metadata/<series>/server/released.current.txt 
This mimics the layout on cloud-images (with a differently named top level
directory). 

The next branch will remove the hard coded instance id and flavor name from the
live
openstack tests as these are now no longer required.

R=jameinel, dimitern, fwereade, dfc
CC=
https://codereview.appspot.com/8816045