Openstack provider constraints support.

Openstack provider constraints support.

Allows openstack providers to utilize standard cli constraints
when bootstrapping or deploying services. Dynamically introspects
flavors/instance-types from service provider.

Deprecates default-instance-type (logged and ignored).

Tested against hpcloud.

https://code.launchpad.net/~hazmat/juju/ostack-constraints/+merge/116027

(do not edit description out of merge proposal)

[hazmat, 2012-07-20 16:17:54]

Please take a look.

[bcsaller, 2012-07-20 17:05:18]

LGTM +1

Just a couple of nitpicks. Otherwise this looks good and brings some of the
conventions more in line with the coding std. 

I glanced at the Nova docs, it didn't look like they supported 'arch' on a quick
read, I did find that surprising.

https://codereview.appspot.com/6421053/diff/1/juju/providers/common/instance_...
File juju/providers/common/instance_type.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/common/instance_...
juju/providers/common/instance_type.py:16: instance_type =
constraints["instance-type"]
.get("instance-type") might be more defensive as I know you can handle the None,
but maybe not a KeyError.

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch.py
File juju/providers/openstack/launch.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:83: "default-instance-type is deprecated, use
cli --constraints")
With no users of the provider yet (or are there) you could remove the option
from the config and skip even the warning.

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:125: '686', f['vcpus'], f['ram'])
I see that the underlying impl won't search 'arch' but using a fake value here
that looks real might be an issue down the line. Maybe 'ignored-arch' or
'unused' is better?

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/tests/...
File juju/providers/openstack/tests/test_launch.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/tests/...
juju/providers/openstack/tests/test_launch.py:75: return provider.launch("1",
constraints=["cpu=2", "mem=3G"])
Isn't the return value ignored here, unless I'm missing something the 'return'
is undesirable on any of these tests.

[hazmat, 2012-07-20 17:15:19]

Thanks for the review.

On Fri, Jul 20, 2012 at 1:05 PM, <bcsaller@gmail.com> wrote:

> LGTM +1
>
> Just a couple of nitpicks. Otherwise this looks good and brings some of
> the conventions more in line with the coding std.
>
> I glanced at the Nova docs, it didn't look like they supported 'arch' on
> a quick read, I did find that surprising.
>
>
It is surprising, trystack now has arm support, i'll have to check and see
how it describes that, my understanding is that they do implicitly by
segmenting an entire region for it. we'll need to have some external zk
support though to make that viable in practice do to arm java gymnastics.


>
> https://codereview.appspot.**com/6421053/diff/1/juju/**
>
providers/common/instance_**type.py<https://codereview.appspot.com/6421053/diff/1/juju/providers/common/instance_type.py>
> File juju/providers/common/**instance_type.py (right):
>
> https://codereview.appspot.**com/6421053/diff/1/juju/**
>
providers/common/instance_**type.py#newcode16<https://codereview.appspot.com/6421053/diff/1/juju/providers/common/instance_type.py#newcode16>
> juju/providers/common/**instance_type.py:16: instance_type =
> constraints["instance-type"]
> .get("instance-type") might be more defensive as I know you can handle
> the None, but maybe not a KeyError.
>

for the solver to even be used means the provider is explicitly using it
which means it registered instance type constraints, which means
instance-type will always be available as a key when its constraint set
parse any values.  ie.  keyerror can't happen here, and if does i think its
better for it to blow up as something is very wrong.


>
> https://codereview.appspot.**com/6421053/diff/1/juju/**
>
providers/openstack/launch.py<https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch.py>
> File juju/providers/openstack/**launch.py (right):
>
> https://codereview.appspot.**com/6421053/diff/1/juju/**
>
providers/openstack/launch.py#**newcode83<https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch.py#newcode83>
> juju/providers/openstack/**launch.py:83: "default-instance-type is
> deprecated, use cli --constraints")
> With no users of the provider yet (or are there) you could remove the
> option from the config and skip even the warning.
>

doh.. i should remove the option from the config, even then it feels good
to warn about this. the config is kinda of worthless, since its validation
of known values only, unknown values silently pass through.



> https://codereview.appspot.**com/6421053/diff/1/juju/**
>
providers/openstack/launch.py#**newcode125<https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch.py#newcode125>
> juju/providers/openstack/**launch.py:125: '686', f['vcpus'], f['ram'])
> I see that the underlying impl won't search 'arch' but using a fake
> value here that looks real might be an issue down the line. Maybe
> 'ignored-arch' or 'unused' is better?
>

hmm. the type solver should be searching arch, oversight. i'll fix that. i
had ripped it out my azure provider.


>
> https://codereview.appspot.**com/6421053/diff/1/juju/**
>
providers/openstack/tests/**test_launch.py<https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/tests/test_launch.py>
> File juju/providers/openstack/**tests/test_launch.py (right):
>
> https://codereview.appspot.**com/6421053/diff/1/juju/**
>
providers/openstack/tests/**test_launch.py#newcode75<https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/tests/test_launch.py#newcode75>
> juju/providers/openstack/**tests/test_launch.py:75: return
> provider.launch("1", constraints=["cpu=2", "mem=3G"])
> Isn't the return value ignored here, unless I'm missing something the
> 'return' is undesirable on any of these tests.
>

the tests return deferreds, instead of using inlinecallbacks, the trial
test runner does the right thing with them.

[gz, 2012-07-24 14:36:59]

Generic whine about mixing imports rearrangement into a code change that needs
review, could easily have been a separate branch to make life easier.

https://codereview.appspot.com/6421053/diff/1/juju/providers/common/instance_...
File juju/providers/common/instance_type.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/common/instance_...
juju/providers/common/instance_type.py:25: def _solve(self, constraints):
Not that it really matters given the small number of flavors generally, but all
this faffing around with filters seems complicated given these constraints have
order, and sort/bisect would work just as well.

https://codereview.appspot.com/6421053/diff/1/juju/providers/common/instance_...
juju/providers/common/instance_type.py:35: key=lambda x:
self._instance_types[x].cpu)[0]
Instead just do `possible_types.sort(); return possible_types[0]` which would
give a deterministic result for instances with same cpu but different mem as
well?

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/client.py
File juju/providers/openstack/client.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/client...
juju/providers/openstack/client.py:321: def list_flavors_details(self):
Rename to list_flavor_detail so it's the same form as the others and the url.

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch.py
File juju/providers/openstack/launch.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:84: flavors = yield
self._provider.nova.list_flavors_details()
Rename list_flavors_details -> list_flavors_detail.

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:120: def _solve_flavor(self, flavor_name,
flavors):
Private helper isn't related to launch and doesn't use self. I'd make it a
function rather than a method if we're not going down the route of an interface
on the provider.

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/provid...
File juju/providers/openstack/provider.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/provid...
juju/providers/openstack/provider.py:67: # Fetch provider defined instance types
(just names)
I'd prefer if this created a provider instance shared store of flavor
information, that launch then accessed to resolve its constraints. Would mean
one list_flavor_detail call rather than one or two per launch, and would also
tie flavor business into a single class.

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/tests/...
File juju/providers/openstack/tests/test_launch.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/tests/...
juju/providers/openstack/tests/test_launch.py:30:
self.nova.list_flavors_details()
Rename list_flavors_details -> list_flavors_detail.

[hazmat, 2012-07-25 02:54:13]

*** Submitted:

Openstack provider constraints support.

Allows openstack providers to utilize standard cli constraints
when bootstrapping or deploying services. Dynamically introspects
flavors/instance-types from service provider.

Deprecates default-instance-type (logged and ignored).

Tested against hpcloud.

R=bcsaller, gz
CC=
https://codereview.appspot.com/6421053