Merge lp://staging/~wallyworld/juju-core/image-metadata-defaults into lp://staging/~juju/juju-core/trunk

Reviewers: mp+164072_code.launchpad.net,

Message:
Please take a look.

Description:
Fix default image/instance type behaviour

Both EC2/Openstack support default-instance-type and default-image-id.
Openstack also supports override-image-id. The behaviour is:

1. default-instance-type
If more than one instance type matches the constraints, use this one.
An error is raised if no valid instance types can be found, eg arch is
specified as arm but only amd64 instance types are available.

2. default-image-id
If more than one valid image is available, use this one.
Used to allow a user to use their own custom image, so long as it

matches the available instance types and is in metadata.

3. override-image-id
If no image can be found, use this one. Ignored if an image is found.

The override-image-id is used in the same way PyJuju used
default-image-id
(for openstack installs where no image metadata is available).

https://code.launchpad.net/~wallyworld/juju-core/image-metadata-defaults/+merge/164072

Requires:
https://code.launchpad.net/~wallyworld/juju-core/signed-simplestreams/+merge/163824

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/ec2/config.go
   M environs/ec2/config_test.go
   M environs/ec2/ec2.go
   M environs/ec2/export_test.go
   M environs/ec2/image_test.go
   M environs/instances/image.go
   M environs/instances/image_test.go
   M environs/instances/instancetype.go
   M environs/instances/instancetype_test.go
   M environs/openstack/config.go
   M environs/openstack/config_test.go
   M environs/openstack/export_test.go
   M environs/openstack/local_test.go
   M environs/openstack/provider.go

just going off of the description:

I really like having the settings unified. I'm not sure if the names
quite convey the intent. "override-image-id" meaning the one you use if
nothing can be found doesn't seem as good of a match (name wise) as
default-image-id.

"default" really sounds like "what to use if you can't find something
better".

Name wise, your (2) sounds more like "preferred-image-id" [if you find
multiple matches, prefer this one] and then (3) would be
"default-image-id" [if you can't find one, use this].
Though I'm not entirely sure why the use case for (3) isn't satisfied by
(2).

I think the concern was that you don't know if (2) actually matches the
request (because you only got the image-id, and not the metadata).

I would like us to think carefully if we really need to add one more
conf item.

If we do, I would probably like to see us leave default-image-id as the
one we use if we can't find one from metadata (3), and then add
something like "preferred-image-id" to indicate the way to break ties
(2).

Does that sound reasonable to you?

https://codereview.appspot.com/9303051/

code wise, this looks mostly mechanical changes due to a new config
item. And then moving the selection code into getInstanceType rather
than FindInstanceSpec, which seems fine to me.

So the code changes are good, I'd just like to discuss our specific
naming to make sure we aren't adding complexity that won't actually be
used.

https://codereview.appspot.com/9303051/

On 2013/05/16 06:50:27, wallyworld wrote:
> Please take a look.

I'm -1 on "override-image-id" -- I don't think it overrides anything. If
anything, it's "fallback-image-id" (and we need to be really clear about
what its series/arch will be).

I think that "default" meaning "tiebreaker" is ok, though.

Part of me wants to make the default- settings into lists instead of
strings but there's no justification today and I can't see any way to do
it well given the constraints we're operating under, so don't worry
about that.

https://codereview.appspot.com/9303051/

Need to go to lunch, but here's one preliminary comment that I think is
important

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

https://codereview.appspot.com/9303051/diff/1/environs/instances/instancetype.go#newcode102
environs/instances/instancetype.go:102: // No matching instance types
were found, so the fallback is to:
If no matching instance types were found, we should return an error.
Please don't preserve this incorrect behaviour.

I think you'd find life a *lot* easier if you stripped this function
down to one that filters a list of instance types by constraints *only*.
This will make it much easier to write the prefer-a-gig code, I think
(filter all for base constraints, if mem wasn't set filter those by mem
as well and replace if it doesn't mess with defaults) and do appropriate
jiggery-pokery to pick a sensible ordering.

https://codereview.appspot.com/9303051/