Merge lp:~hazmat/pyjuju/ostack-constraints into lp:pyjuju

Proposed by Kapil Thangavelu
Status: Merged
Merged at revision: 560
Proposed branch: lp:~hazmat/pyjuju/ostack-constraints
Merge into: lp:pyjuju
Diff against target: 942 lines (+282/-129)
12 files modified
juju/providers/common/instance_type.py (+52/-0)
juju/providers/common/tests/test_instance_type.py (+53/-0)
juju/providers/openstack/__init__.py (+2/-0)
juju/providers/openstack/client.py (+23/-20)
juju/providers/openstack/files.py (+4/-4)
juju/providers/openstack/launch.py (+26/-15)
juju/providers/openstack/ports.py (+16/-18)
juju/providers/openstack/provider.py (+18/-9)
juju/providers/openstack/tests/__init__.py (+21/-2)
juju/providers/openstack/tests/test_bootstrap.py (+11/-9)
juju/providers/openstack/tests/test_files.py (+8/-11)
juju/providers/openstack/tests/test_launch.py (+48/-41)
To merge this branch: bzr merge lp:~hazmat/pyjuju/ostack-constraints
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+116027@code.launchpad.net

Description of the change

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://codereview.appspot.com/6421053/

To post a comment you must log in.
Revision history for this message
Kapil Thangavelu (hazmat) wrote :

Reviewers: mp+116027_code.launchpad.net,

Message:
Please take a look.

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

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

Affected files:
   A [revision details]
   A juju/providers/common/instance_type.py
   A juju/providers/common/tests/test_instance_type.py
   M juju/providers/openstack/__init__.py
   M juju/providers/openstack/client.py
   M juju/providers/openstack/files.py
   M juju/providers/openstack/launch.py
   M juju/providers/openstack/ports.py
   M juju/providers/openstack/provider.py
   M juju/providers/openstack/tests/__init__.py
   M juju/providers/openstack/tests/test_bootstrap.py
   M juju/providers/openstack/tests/test_files.py
   M juju/providers/openstack/tests/test_launch.py

Revision history for this message
Benjamin Saller (bcsaller) 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.

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

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?

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

https://codereview.appspot.com/6421053/

Revision history for this message
Kapil Thangavelu (hazmat) wrote :
Download full text (3.8 KiB)

Thanks for the review.

On Fri, Jul 20, 2012 at 1:05 PM, <email address hidden> 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...

Read more...

Revision history for this message
Martin Packman (gz) wrote :

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_type.py
File juju/providers/common/instance_type.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/common/instance_type.py#newcode25
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_type.py#newcode35
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.py#newcode321
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.py#newcode84
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.py#newcode120
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/provider.py
File juju/providers/openstack/provider.py (right):

https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/provider.py#newcode67
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/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#newcode30
juju/providers/openstack/tests/test_launch.py:30:
self.nova.list_flavors_details()
Rename list_flavors_details -> list_flavors_detail.

https://codereview.appspot.com/6421053/

Revision history for this message
Kapil Thangavelu (hazmat) wrote :
Download full text (4.2 KiB)

On Tue, Jul 24, 2012 at 10:40 AM, Martin Packman <
<email address hidden>> wrote:

> Generic whine about mixing imports rearrangement into a code change that
> needs review, could easily have been a separate branch to make life
> easier.
>
>
noted, sorry, i didn't think i'd be able to come back to this and make
those changes.

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

there is no guarantee of ordering, they come in a dictionary, derived
possibly from external systems. This is in providers/commons and can't
assume implementation detail of a given provider.

>
> https://codereview.appspot.com/6421053/diff/1/juju/providers/common/instance_type.py#newcode35
> 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?
>

possible_types is a list of names, a sort on type names isn't provably
correct without knowledge of types. I've added mem to the key to give the
desired result.

>
>
> 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.py#newcode321
> 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.
>

i've done this, and i'm okay with it, but the naming to me feels like it
would be listing the details of a single flavor instead of denoting that
its listing the details of all flavors.

>
>
> 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#newcode84
> juju/providers/openstack/launch.py:84: flavors = yield
> self._provider.nova.list_flavors_details()
> Rename list_flavors_details -> list_flavors_detail.
>

done

>
>
> https://codereview.appspot.com/6421053/diff/1/juju/providers/openstack/launch.py#newcode120
> 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.
>
>
done

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

Read more...

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

*** 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

https://codereview.appspot.com/6421053/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'juju/providers/common/instance_type.py'
2--- juju/providers/common/instance_type.py 1970-01-01 00:00:00 +0000
3+++ juju/providers/common/instance_type.py 2012-07-20 16:18:25 +0000
4@@ -0,0 +1,52 @@
5+import operator
6+
7+from juju.errors import ProviderError
8+from collections import namedtuple
9+
10+# Just a generic base instance type, with required attrs.
11+InstanceType = namedtuple("InstanceType", "arch cpu mem")
12+
13+
14+class TypeSolver(object):
15+
16+ def __init__(self, instance_types):
17+ self._instance_types = instance_types
18+
19+ def run(self, constraints):
20+ instance_type = constraints["instance-type"]
21+ if instance_type is not None:
22+ if not instance_type in self._instance_types:
23+ raise ProviderError(
24+ "Invalid instance type %s" % instance_type)
25+ return instance_type
26+
27+ return self._solve(constraints)
28+
29+ def _solve(self, constraints):
30+ possible_types = list(self._instance_types)
31+ for f in self._get_filters(constraints):
32+ possible_types = filter(f, possible_types)
33+
34+ if not possible_types:
35+ raise ProviderError(
36+ "No instance type satisifes %s" % dict(constraints))
37+ return sorted(
38+ possible_types,
39+ key=lambda x: self._instance_types[x].cpu)[0]
40+
41+ def _get_filters(self, constraints):
42+ return (
43+ self._filter_func("cpu", constraints, operator.ge),
44+ self._filter_func("mem", constraints, operator.ge)
45+ )
46+
47+ def _filter_func(self, name, constraints, op):
48+
49+ desired = constraints[name]
50+ if not desired:
51+ return lambda _: True
52+
53+ def f(type_):
54+ value = getattr(self._instance_types[type_], name)
55+ return op(value, desired)
56+ return f
57
58=== added file 'juju/providers/common/tests/test_instance_type.py'
59--- juju/providers/common/tests/test_instance_type.py 1970-01-01 00:00:00 +0000
60+++ juju/providers/common/tests/test_instance_type.py 2012-07-20 16:18:25 +0000
61@@ -0,0 +1,53 @@
62+from juju.lib.testing import TestCase
63+
64+from juju.providers.common.instance_type import (
65+ TypeSolver, InstanceType)
66+from juju.machine.constraints import ConstraintSet
67+
68+
69+from twisted.internet.defer import inlineCallbacks
70+
71+
72+class InstanceTypeSolverTest(TestCase):
73+
74+ @inlineCallbacks
75+ def setUp(self):
76+ self.instance_types = {
77+ "ExtraSmall": InstanceType("amd64", 0.1, 768),
78+ "Small": InstanceType("amd64", 1, 1792),
79+ "Medium": InstanceType("amd64", 2, 3584),
80+ "Large": InstanceType("amd64", 4, 7168),
81+ "ExtraLarge": InstanceType("amd64", 8, 14336),
82+ }
83+ self.solver = TypeSolver(self.instance_types)
84+ self.cs = ConstraintSet("cloud")
85+ self.cs.register_generics(self.instance_types.keys())
86+ yield super(InstanceTypeSolverTest, self).setUp()
87+
88+ def test_type_solver(self):
89+
90+ self.assertEqual(
91+ self.solver.run(self.cs.parse(["cpu=4"])),
92+ "Large")
93+
94+ self.assertEqual(
95+ self.solver.run(self.cs.parse(["cpu=3.5"])),
96+ "Large")
97+
98+ self.assertEqual(
99+ self.solver.run(self.cs.parse(["mem=2G"])),
100+ "Medium")
101+
102+ self.assertEqual(
103+ self.solver.run(self.cs.parse(["cpu=2.5", "mem=2G"])),
104+ "Large")
105+
106+ # Any auto solves for min 1 cpu, 512mb ram.
107+ self.assertEqual(
108+ self.solver.run(self.cs.parse(["instance-type=any"])),
109+ "Small")
110+
111+ self.assertEqual(
112+ self.solver.run(self.cs.parse(
113+ ["instance-type=ExtraLarge"])),
114+ "ExtraLarge")
115
116=== modified file 'juju/providers/openstack/__init__.py'
117--- juju/providers/openstack/__init__.py 2012-03-02 18:11:23 +0000
118+++ juju/providers/openstack/__init__.py 2012-07-20 16:18:25 +0000
119@@ -1,3 +1,5 @@
120 """Support for using OpenStack as a cloud provider for juju"""
121
122+__all__ = ['MachineProvider']
123+
124 from .provider import MachineProvider
125
126=== modified file 'juju/providers/openstack/client.py'
127--- juju/providers/openstack/client.py 2012-07-18 15:19:22 +0000
128+++ juju/providers/openstack/client.py 2012-07-20 16:18:25 +0000
129@@ -24,12 +24,12 @@
130 import urllib
131
132 import twisted
133-from twisted.internet import (
134- defer,
135- interfaces,
136- protocol,
137- reactor,
138- )
139+from twisted.internet.defer import (
140+ Deferred, inlineCallbacks, returnValue, succeed)
141+from twisted.internet.protocol import Protocol
142+from twisted.internet.interfaces import IProducer
143+from twisted.internet import reactor
144+
145 from twisted.web import (
146 client,
147 http_headers,
148@@ -49,7 +49,7 @@
149 class BytestringProducer(object):
150 """Wrap basic bytestring as a needlessly fancy twisted producer."""
151
152- implements(interfaces.IProducer)
153+ implements(IProducer)
154
155 def __init__(self, bytestring):
156 self.content = bytestring
157@@ -61,13 +61,13 @@
158 def startProducing(self, consumer):
159 """Write entire contents when production starts"""
160 consumer.write(self.content)
161- return defer.succeed(None)
162+ return succeed(None)
163
164 def stopProducing(self):
165 """Nothing to do when production halts"""
166
167
168-class ResponseReader(protocol.Protocol):
169+class ResponseReader(Protocol):
170 """Protocol object suitable for use with Response.deliverBody
171
172 The 'onConnectionLost' deferred will be called back once the connection
173@@ -75,7 +75,7 @@
174 """
175
176 def __init__(self):
177- self.onConnectionLost = defer.Deferred()
178+ self.onConnectionLost = Deferred()
179
180 def connectionMade(self):
181 self.data = []
182@@ -92,7 +92,7 @@
183 self.onConnectionLost.callback("".join(self.data))
184
185
186-@defer.inlineCallbacks
187+@inlineCallbacks
188 def request(method, url, extra_headers=(), body=None):
189 headers = http_headers.Headers({
190 # GZ 2012-07-03: Previously passed Accept: application/json header
191@@ -111,11 +111,11 @@
192 body = BytestringProducer(body)
193 response = yield client.Agent(reactor).request(method, url, headers, body)
194 if response.length == 0:
195- defer.returnValue((response, ""))
196+ returnValue((response, ""))
197 reader = ResponseReader()
198 response.deliverBody(reader)
199 body = yield reader.onConnectionLost
200- defer.returnValue((response, body))
201+ returnValue((response, body))
202
203
204 class _OpenStackClient(object):
205@@ -152,7 +152,7 @@
206 log.debug('access %s @ %s', service, url)
207 return url
208
209- @defer.inlineCallbacks
210+ @inlineCallbacks
211 def authenticate_v1(self):
212 deferred = request(
213 "GET",
214@@ -244,7 +244,7 @@
215 def is_authenticated(self):
216 return self.token is not None
217
218- @defer.inlineCallbacks
219+ @inlineCallbacks
220 def authed_request(self, method, url, headers=None, body=None):
221 log.debug("openstack: %s %r", method, url)
222 request_headers = [("X-Auth-Token", self.token)]
223@@ -252,7 +252,7 @@
224 request_headers += headers
225 response, body = yield request(method, url, request_headers, body)
226 log.debug("openstack: %d %r", response.code, body)
227- defer.returnValue((response, body))
228+ returnValue((response, body))
229
230 def _empty(self, result, code):
231 response, body = result
232@@ -287,13 +287,13 @@
233 def __init__(self, client):
234 self._client = client
235
236- @defer.inlineCallbacks
237+ @inlineCallbacks
238 def request(self, method, parts, headers=None, body=None):
239 if not self._client.is_authenticated():
240 yield self._client.authenticate()
241 url = self._client._make_url("compute", parts)
242 result = yield self._client.authed_request(method, url, headers, body)
243- defer.returnValue(result)
244+ returnValue(result)
245
246 def delete(self, parts, code=202):
247 deferred = self.request("DELETE", parts)
248@@ -318,6 +318,9 @@
249 def list_flavors(self):
250 return self.get("flavors", "flavors")
251
252+ def list_flavors_details(self):
253+ return self.get(["flavors", "detail"], "flavors")
254+
255 def get_server(self, server_id):
256 return self.get(["servers", server_id], "server")
257
258@@ -434,13 +437,13 @@
259 def __init__(self, client):
260 self._client = client
261
262- @defer.inlineCallbacks
263+ @inlineCallbacks
264 def request(self, method, parts, headers=None, body=None):
265 if not self._client.is_authenticated():
266 yield self._client.authenticate()
267 url = self._client._make_url("object-store", parts)
268 result = yield self._client.authed_request(method, url, headers, body)
269- defer.returnValue(result)
270+ returnValue(result)
271
272 def public_object_url(self, container, object_name):
273 if not self._client.is_authenticated():
274
275=== modified file 'juju/providers/openstack/files.py'
276--- juju/providers/openstack/files.py 2012-07-05 16:09:11 +0000
277+++ juju/providers/openstack/files.py 2012-07-20 16:18:25 +0000
278@@ -25,7 +25,7 @@
279
280 from cStringIO import StringIO
281
282-from twisted.internet import defer
283+from twisted.internet.defer import inlineCallbacks, returnValue
284
285 from juju import errors
286
287@@ -40,7 +40,7 @@
288 def get_url(self, name):
289 return self._swift.public_object_url(self._container, name)
290
291- @defer.inlineCallbacks
292+ @inlineCallbacks
293 def get(self, name):
294 """Get a file object from Swift.
295
296@@ -57,9 +57,9 @@
297 if response.code != 200:
298 raise errors.ProviderInteractionError(
299 "Couldn't fetch object %r %r" % (response.code, body))
300- defer.returnValue(StringIO(body))
301+ returnValue(StringIO(body))
302
303- @defer.inlineCallbacks
304+ @inlineCallbacks
305 def put(self, remote_path, file_object):
306 """Upload a file to Swift.
307
308
309=== modified file 'juju/providers/openstack/launch.py'
310--- juju/providers/openstack/launch.py 2012-07-18 15:19:22 +0000
311+++ juju/providers/openstack/launch.py 2012-07-20 16:18:25 +0000
312@@ -22,14 +22,13 @@
313
314 from cStringIO import StringIO
315
316-from twisted.internet import (
317- defer,
318- reactor,
319- )
320+from twisted.internet.defer import inlineCallbacks, returnValue
321+
322
323 from juju.errors import ProviderError, ProviderInteractionError
324 from juju.lib import twistutils
325 from juju.providers.common.launch import LaunchMachine
326+from juju.providers.common.instance_type import TypeSolver, InstanceType
327
328 from .machine import machine_from_instance, get_server_status
329
330@@ -41,7 +40,7 @@
331
332 _DELAY_FOR_ADDRESSES = 5 # seconds
333
334- @defer.inlineCallbacks
335+ @inlineCallbacks
336 def start_machine(self, machine_id, zookeepers):
337 """Actually launch an instance on Nova.
338
339@@ -80,17 +79,16 @@
340 # type and resolve it to a flavor id here.
341 flavor_name = self._provider.config.get("default-instance-type")
342 if flavor_name is None:
343- raise ProviderError("Need to specify a default-instance-type")
344- flavors = yield self._provider.nova.list_flavors()
345- flavor_map = dict((f['name'], f['id']) for f in flavors)
346- if not flavor_name in flavor_map:
347- ProviderError("Unknown instance type given: %r" % (flavor_name,))
348+ log.warning(
349+ "default-instance-type is deprecated, use cli --constraints")
350+ flavors = yield self._provider.nova.list_flavors_details()
351+ flavor_id = self._solve_flavor(flavor_name, flavors)
352
353 server = yield self._provider.nova.run_server(
354 name="juju %s instance %s" %
355 (self._provider.environment_name, machine_id,),
356 image_id=image_id,
357- flavor_id=flavor_map[flavor_name],
358+ flavor_id=flavor_id,
359 security_group_names=security_groups,
360 user_data=user_data)
361
362@@ -117,10 +115,23 @@
363 server = yield self._provider.nova.get_server(server['id'])
364 yield _assign_floating_ip(self._provider, server['id'])
365
366- defer.returnValue([machine_from_instance(server)])
367-
368-
369-@defer.inlineCallbacks
370+ returnValue([machine_from_instance(server)])
371+
372+ def _solve_flavor(self, flavor_name, flavors):
373+ flavor_map, flavor_types = {}, {}
374+ for f in flavors:
375+ flavor_map[f['name']] = f['id']
376+ flavor_types[f['name']] = InstanceType(
377+ '686', f['vcpus'], f['ram'])
378+
379+ solver = TypeSolver(flavor_types)
380+ flavor_name = solver.run(self._constraints)
381+ if not flavor_name in flavor_map:
382+ ProviderError("Unknown instance type given: %r" % (flavor_name,))
383+ return flavor_map[flavor_name]
384+
385+
386+@inlineCallbacks
387 def _assign_floating_ip(provider, server_id):
388 floating_ips = yield provider.nova.list_floating_ips()
389 for floating_ip in floating_ips:
390
391=== modified file 'juju/providers/openstack/ports.py'
392--- juju/providers/openstack/ports.py 2012-07-18 02:46:56 +0000
393+++ juju/providers/openstack/ports.py 2012-07-20 16:18:25 +0000
394@@ -20,9 +20,7 @@
395 looking at the details of the one with the matching name.
396 """
397
398-from twisted.internet import (
399- defer,
400- )
401+from twisted.internet.defer import inlineCallbacks, returnValue
402
403 from juju import errors
404
405@@ -47,7 +45,7 @@
406 def _machine_group_name(self, machine_id):
407 return "juju-%s-%s" % (self.tag, machine_id)
408
409- @defer.inlineCallbacks
410+ @inlineCallbacks
411 def _get_machine_group(self, machine, machine_id):
412 """Get details of the machine specific security group
413
414@@ -59,12 +57,12 @@
415 groups = yield self.nova.get_server_security_groups(server_id)
416 for group in groups:
417 if group['name'] == group_name:
418- defer.returnValue(group)
419+ returnValue(group)
420 raise errors.ProviderInteractionError(
421 "Missing security group %r for machine %r" %
422 (group_name, server_id))
423
424- @defer.inlineCallbacks
425+ @inlineCallbacks
426 def open_port(self, machine, machine_id, port, protocol="tcp"):
427 """Allow access to a port for the given machine only"""
428 group = yield self._get_machine_group(machine, machine_id)
429@@ -73,7 +71,7 @@
430 log.debug("Opened %s/%s on machine %r",
431 port, protocol, machine.instance_id)
432
433- @defer.inlineCallbacks
434+ @inlineCallbacks
435 def close_port(self, machine, machine_id, port, protocol="tcp"):
436 """Revoke access to a port for the given machine only"""
437 group = yield self._get_machine_group(machine, machine_id)
438@@ -88,7 +86,7 @@
439 "Couldn't close unopened %s/%s on machine %r",
440 port, protocol, machine.instance_id)
441
442- @defer.inlineCallbacks
443+ @inlineCallbacks
444 def get_opened_ports(self, machine, machine_id):
445 """Get a set of opened port/protocol pairs for a machine"""
446 group = yield self._get_machine_group(machine, machine_id)
447@@ -100,9 +98,9 @@
448 to_port = rule["to_port"]
449 if from_port == to_port:
450 opened_ports.add((from_port, protocol))
451- defer.returnValue(opened_ports)
452+ returnValue(opened_ports)
453
454- @defer.inlineCallbacks
455+ @inlineCallbacks
456 def ensure_groups(self, machine_id):
457 """Get names of the security groups for a machine, creating if needed
458
459@@ -133,9 +131,9 @@
460 yield self.nova.create_security_group(machine_group,
461 "juju group for %s machine %s" % (self.tag, machine_id))
462
463- defer.returnValue([juju_group, machine_group])
464+ returnValue([juju_group, machine_group])
465
466- @defer.inlineCallbacks
467+ @inlineCallbacks
468 def get_machine_groups(self, machine, with_juju_group=False):
469 try:
470 ret = yield self.get_machine_groups_pure(machine, with_juju_group)
471@@ -147,11 +145,11 @@
472 except errors.ProviderInteractionError, e:
473 pass # just rebinding e
474 if True or getattr(e, "kind", None) == "itemNotFound":
475- defer.returnValue(None)
476+ returnValue(None)
477 raise
478- defer.returnValue(ret)
479+ returnValue(ret)
480
481- @defer.inlineCallbacks
482+ @inlineCallbacks
483 def get_machine_groups_pure(self, machine, with_juju_group=False):
484 server_id = machine.instance_id
485 groups = yield self.nova.get_server_security_groups(server_id)
486@@ -160,13 +158,13 @@
487 if g['name'].startswith(juju_group))
488 if juju_group not in groups_by_name:
489 # Not a juju machine, shouldn't touch
490- defer.returnValue(None)
491+ returnValue(None)
492 if not with_juju_group:
493 groups_by_name.pop(juju_group)
494 # else assumption: only one remaining group, is the machine group
495- defer.returnValue(groups_by_name)
496+ returnValue(groups_by_name)
497
498- @defer.inlineCallbacks
499+ @inlineCallbacks
500 def delete_juju_group(self):
501 security_groups = yield self.nova.list_security_groups()
502 juju_group = self._juju_group_name()
503
504=== modified file 'juju/providers/openstack/provider.py'
505--- juju/providers/openstack/provider.py 2012-07-18 15:19:22 +0000
506+++ juju/providers/openstack/provider.py 2012-07-20 16:18:25 +0000
507@@ -12,7 +12,7 @@
508
509 import logging
510
511-from twisted.internet import defer
512+from twisted.internet.defer import inlineCallbacks, returnValue, gatherResults
513
514 from juju import errors
515 from juju.providers.common.base import MachineProviderBase
516@@ -61,6 +61,15 @@
517 """Retrieve a Swift-backed :class:`FileStorage`."""
518 return FileStorage(self.swift, self.config["control-bucket"])
519
520+ @inlineCallbacks
521+ def get_constraint_set(self):
522+ cs = yield super(MachineProvider, self).get_constraint_set()
523+ # Fetch provider defined instance types (just names)
524+ flavors = yield self.nova.list_flavors()
525+ flavor_names = [f['name'] for f in flavors]
526+ cs.register_generics(flavor_names)
527+ returnValue(cs)
528+
529 def start_machine(self, machine_data, master=False):
530 """Start an OpenStack machine.
531
532@@ -74,7 +83,7 @@
533 """
534 return NovaLaunchMachine.launch(self, machine_data, master)
535
536- @defer.inlineCallbacks
537+ @inlineCallbacks
538 def get_machines(self, instance_ids=()):
539 """List machines running in the provider.
540
541@@ -122,9 +131,9 @@
542 if missing:
543 raise errors.MachinesNotFound(missing)
544
545- defer.returnValue(machines)
546+ returnValue(machines)
547
548- @defer.inlineCallbacks
549+ @inlineCallbacks
550 def _delete_machine(self, machine, full=False):
551 server_id = machine.instance_id
552 server = yield self.nova.get_server(server_id)
553@@ -132,7 +141,7 @@
554 "juju %s instance" % self.environment_name):
555 raise errors.MachinesNotFound(set([machine.instance_id]))
556 yield self.nova.delete_server(server_id)
557- defer.returnValue(machine)
558+ returnValue(machine)
559
560 def shutdown_machine(self, machine):
561 if not isinstance(machine, NovaProviderMachine):
562@@ -142,7 +151,7 @@
563 # and can be shutdown, instead just handle an error? 404-ish?
564 return self._delete_machine(machine)
565
566- @defer.inlineCallbacks
567+ @inlineCallbacks
568 def destroy_environment(self):
569 """Terminate all associated machines and security groups.
570
571@@ -153,10 +162,10 @@
572 :rtype: :class:`twisted.internet.defer.Deferred`
573 """
574 machines = yield self.get_machines()
575- deleted_machines = yield defer.gatherResults(
576+ deleted_machines = yield gatherResults(
577 [self._delete_machine(m, True) for m in machines])
578 yield self.save_state({})
579- defer.returnValue(deleted_machines)
580+ returnValue(deleted_machines)
581
582 def shutdown_machines(self, machines):
583 """Terminate machines associated with this provider.
584@@ -172,7 +181,7 @@
585 """
586 # XXX: need to actually handle errors as non-terminated machines
587 # and not include them in the resulting list
588- return defer.gatherResults(
589+ return gatherResults(
590 [self.shutdown_machine(m) for m in machines], consumeErrors=True)
591
592 def open_port(self, machine, machine_id, port, protocol="tcp"):
593
594=== modified file 'juju/providers/openstack/tests/__init__.py'
595--- juju/providers/openstack/tests/__init__.py 2012-07-18 21:17:01 +0000
596+++ juju/providers/openstack/tests/__init__.py 2012-07-20 16:18:25 +0000
597@@ -23,6 +23,8 @@
598 provider,
599 )
600
601+from juju.machine.constraints import ConstraintSet
602+
603
604 class FakeResponse(object):
605
606@@ -34,7 +36,22 @@
607 {"Content-Type": ["application/json"]})
608
609
610-class OpenStackTestMixin(object):
611+class ConstraintSupportMixin(object):
612+
613+ provider_type = "openstack"
614+
615+ default_flavors = [
616+ {'id': 1, 'name': "standard.xsmall", 'vcpus': 1, 'ram': 1024},
617+ {'id': 2, 'name': "standard.medium", 'vcpus': 2, 'ram': 4096},
618+ ]
619+
620+ def setup_constraints(self):
621+ self.constraint_set = ConstraintSet(self.provider_type)
622+ self.constraint_set.register_generics(
623+ [f['name'] for f in self.default_flavors])
624+
625+
626+class OpenStackTestMixin(ConstraintSupportMixin):
627 """Helpers for test classes that want to exercise the OpenStack APIs
628
629 This goes all the way down to the http layer, which is really a little too
630@@ -67,6 +84,7 @@
631 }
632 ]
633 }}))))
634+ self.setup_constraints()
635 # Clear the environment so provider won't pick up config from envvars
636 self.change_environment()
637
638@@ -154,7 +172,7 @@
639 self.mocker.result(defer.succeed((FakeResponse(code), response)))
640
641
642-class MockedProvider(object):
643+class MockedProvider(ConstraintSupportMixin):
644
645 provider_type = "openstack"
646 environment_name = "testing"
647@@ -183,6 +201,7 @@
648 ports.NovaPortManager(self.nova, self.environment_name))
649 self.provider_actions = mocker.mock()
650 self.mocker = mocker
651+ self.setup_constraints()
652
653 def get_file_storage(self):
654 return files.FileStorage(self.swift, self.config['control-bucket'])
655
656=== modified file 'juju/providers/openstack/tests/test_bootstrap.py'
657--- juju/providers/openstack/tests/test_bootstrap.py 2012-07-18 15:03:39 +0000
658+++ juju/providers/openstack/tests/test_bootstrap.py 2012-07-20 16:18:25 +0000
659@@ -12,10 +12,8 @@
660 mocker,
661 testing,
662 )
663-from juju.machine import constraints
664
665 from juju.providers.openstack.machine import NovaProviderMachine
666-
667 from juju.providers.openstack.tests import OpenStackTestMixin
668
669
670@@ -74,8 +72,8 @@
671 def _match_server(self, data):
672 userdata = data['server'].pop('user_data').decode("base64")
673 self.assertEqual("#cloud-config", userdata.split("\n", 1)[0])
674- cloud_init = yaml.load(userdata)
675 # TODO: assertions on cloud-init content
676+ # cloud_init = yaml.load(userdata)
677 self.assertEqual({'server': {
678 'flavorRef': 1,
679 'imageRef': 42,
680@@ -89,8 +87,9 @@
681 return True
682
683 def expect_launch(self):
684- self.expect_nova_get("flavors",
685- response={'flavors': [{'id': 1, 'name': "standard.xsmall"}]})
686+ self.expect_nova_get("flavors/detail",
687+ response={'flavors': self.default_flavors})
688+
689 self.expect_nova_post("servers", mocker.MATCH(self._match_server),
690 code=202, response={'server': {
691 'id': '1000',
692@@ -114,8 +113,8 @@
693
694 def bootstrap(self):
695 provider = self.get_provider()
696- constraint_set = constraints.ConstraintSet(provider.provider_type)
697- return provider.bootstrap(constraint_set.load({}))
698+ return provider.bootstrap(
699+ self.constraint_set.load({'instance-type': None}))
700
701 def test_bootstrap_clean(self):
702 """Bootstrap from a clean slate makes groups and zookeeper instance"""
703@@ -130,6 +129,7 @@
704 log = self.capture_logging("juju.common", level=logging.DEBUG)
705 deferred = self.bootstrap()
706 deferred.addCallback(self._check_machine)
707+
708 def check_log(_):
709 log_text = log.getvalue()
710 self.assertIn("Launching juju bootstrap instance", log_text)
711@@ -175,9 +175,10 @@
712 self.mocker.replay()
713
714 log = self.capture_logging("juju.common")
715- self.capture_logging("juju.openstack") # Drop to avoid stderr kipple
716+ self.capture_logging("juju.openstack") # Drop to avoid stderr kipple
717 deferred = self.bootstrap()
718 deferred.addCallback(self._check_machine)
719+
720 def check_log(_):
721 self.assertEqual("juju environment previously bootstrapped.\n",
722 log.getvalue())
723@@ -203,9 +204,10 @@
724 self.mocker.replay()
725
726 log = self.capture_logging("juju.common", level=logging.DEBUG)
727- self.capture_logging("juju.openstack") # Drop to avoid stderr kipple
728+ self.capture_logging("juju.openstack") # Drop to avoid stderr kipple
729 deferred = self.bootstrap()
730 deferred.addCallback(self._check_machine)
731+
732 def check_log(_):
733 log_text = log.getvalue()
734 self.assertIn("Launching juju bootstrap instance", log_text)
735
736=== modified file 'juju/providers/openstack/tests/test_files.py'
737--- juju/providers/openstack/tests/test_files.py 2012-06-18 15:03:50 +0000
738+++ juju/providers/openstack/tests/test_files.py 2012-07-20 16:18:25 +0000
739@@ -2,14 +2,11 @@
740
741 from cStringIO import StringIO
742
743-from twisted.internet import (
744- defer,
745- )
746+from twisted.internet.defer import inlineCallbacks, fail
747
748 from juju import errors
749-from juju.lib import (
750- testing,
751- )
752+from juju.lib import testing
753+
754 from juju.providers.openstack.tests import OpenStackTestMixin
755
756
757@@ -47,12 +44,12 @@
758 """Unexpected errors from client propogate"""
759 content = "some text"
760 self._mock_swift("PUT", "testing/object", content)
761- self.mocker.result(defer.fail(ValueError("Something unexpected")))
762+ self.mocker.result(fail(ValueError("Something unexpected")))
763 self.mocker.replay()
764 deferred = self.get_storage().put("object", StringIO(content))
765 return self.assertFailure(deferred, ValueError)
766
767- @defer.inlineCallbacks
768+ @inlineCallbacks
769 def test_get_url(self):
770 """A url can be generated for any stored file."""
771 self.mocker.replay()
772@@ -61,7 +58,7 @@
773 url = storage.get_url("object")
774 self.assertEqual(self.swift_url + "/testing/object", url)
775
776- @defer.inlineCallbacks
777+ @inlineCallbacks
778 def test_get_url_unicode(self):
779 """A url can be generated for *any* stored file."""
780 self.mocker.replay()
781@@ -70,7 +67,7 @@
782 url = storage.get_url(u"\xa7")
783 self.assertEqual(self.swift_url + "/testing/%C2%A7", url)
784
785- @defer.inlineCallbacks
786+ @inlineCallbacks
787 def test_get_file(self):
788 """Retrieving a file returns a file-like object with the content"""
789 content = "some text"
790@@ -79,7 +76,7 @@
791 result = yield self.get_storage().get("object")
792 self.assertEqual(result.read(), content)
793
794- @defer.inlineCallbacks
795+ @inlineCallbacks
796 def test_get_file_unicode(self):
797 """Retrieving a file with a unicode key uses a UTF-8 url"""
798 content = "some text"
799
800=== modified file 'juju/providers/openstack/tests/test_launch.py'
801--- juju/providers/openstack/tests/test_launch.py 2012-07-18 21:17:01 +0000
802+++ juju/providers/openstack/tests/test_launch.py 2012-07-20 16:18:25 +0000
803@@ -1,62 +1,55 @@
804 """Tests for launching a new server customised for juju using Nova"""
805
806-from twisted.internet import (
807- defer,
808- )
809+from twisted.internet.defer import succeed
810
811 from juju import errors
812-from juju.lib import (
813- mocker,
814- testing,
815- )
816-from juju.machine import (
817- constraints,
818- )
819-
820-from juju.providers.openstack import (
821- launch,
822- tests,
823- )
824-
825-
826-class MockedLaunchProvider(tests.MockedProvider):
827-
828- def launch(self, machine_id, master=False):
829- constraint_set = constraints.ConstraintSet(self.provider_type)
830+from juju.lib.mocker import ANY
831+from juju.lib.testing import TestCase
832+
833+
834+from juju.providers.openstack.tests import MockedProvider
835+from juju.providers.openstack.launch import NovaLaunchMachine
836+
837+
838+class MockedLaunchProvider(MockedProvider):
839+
840+ def launch(self, machine_id, master=False, constraints=None):
841+ if constraints is None:
842+ constraints = self.constraint_set.load({'instance-type': None})
843+ else:
844+ constraints = self.constraint_set.parse(constraints)
845 details = {
846 'machine-id': machine_id,
847- 'constraints': constraint_set.load({}),
848+ 'constraints': constraints
849 }
850- return launch.NovaLaunchMachine.launch(self, details, master)
851+ return NovaLaunchMachine.launch(self, details, master)
852
853 def expect_launch_setup(self, machine_id):
854 self.port_manager.ensure_groups(machine_id)
855- self.mocker.result(defer.succeed(["juju-x", "juju-y"]))
856- self.nova.list_flavors()
857- self.mocker.result(defer.succeed([
858- {'id': 1, 'name': "standard.xsmall"},
859- ]))
860+ self.mocker.result(succeed(["juju-x", "juju-y"]))
861+ self.nova.list_flavors_details()
862+ self.mocker.result(succeed(self.default_flavors))
863
864- def expect_run_server(self, machine_id, response):
865+ def expect_run_server(self, machine_id, response, flavor_id=1):
866 self.nova.run_server(
867 name="juju testing instance " + machine_id,
868 image_id=42,
869- flavor_id=1,
870+ flavor_id=flavor_id,
871 security_group_names=["juju-x", "juju-y"],
872- user_data=mocker.ANY,
873+ user_data=ANY,
874 )
875- self.mocker.result(defer.succeed(response))
876+ self.mocker.result(succeed(response))
877
878 def expect_available_floating_ip(self, server_id):
879 self.nova.list_floating_ips()
880- self.mocker.result(defer.succeed([
881+ self.mocker.result(succeed([
882 {'instance_id': None, 'ip': "198.162.1.0"},
883 ]))
884 self.nova.add_floating_ip(server_id, "198.162.1.0")
885- self.mocker.result(defer.succeed(None))
886-
887-
888-class NovaLaunchMachineTests(testing.TestCase):
889+ self.mocker.result(succeed(None))
890+
891+
892+class NovaLaunchMachineTests(TestCase):
893
894 def test_launch_requires_default_image_id(self):
895 config = dict(MockedLaunchProvider.default_config)
896@@ -67,6 +60,20 @@
897 deferred = provider.launch("1")
898 return self.assertFailure(deferred, errors.ProviderError)
899
900+ def test_start_machine_with_constraints(self):
901+ provider = MockedLaunchProvider(self.mocker)
902+ provider.expect_zookeeper_machines(1000)
903+ provider.expect_launch_setup("1")
904+ provider.expect_run_server(
905+ "1",
906+ response={
907+ 'id': 1001,
908+ 'addresses': {'public': []},
909+ },
910+ flavor_id=2)
911+ self.mocker.replay()
912+ return provider.launch("1", constraints=["cpu=2", "mem=3G"])
913+
914 def test_start_machine(self):
915 provider = MockedLaunchProvider(self.mocker)
916 provider.expect_zookeeper_machines(1000)
917@@ -87,14 +94,14 @@
918 'id': 1001,
919 })
920 provider.nova.get_server(1001)
921- self.mocker.result(defer.succeed({
922+ self.mocker.result(succeed({
923 'id': 1001,
924 'addresses': {'public': []},
925 }))
926 provider.expect_available_floating_ip(1001)
927- self.mocker.result(defer.succeed(None))
928+ self.mocker.result(succeed(None))
929 self.mocker.replay()
930- self.patch(launch.NovaLaunchMachine, "_DELAY_FOR_ADDRESSES", 0)
931+ self.patch(NovaLaunchMachine, "_DELAY_FOR_ADDRESSES", 0)
932 return provider.launch("1")
933
934 def test_start_machine_master(self):
935@@ -107,6 +114,6 @@
936 })
937 provider.expect_swift_put("juju_master_id", "1000")
938 provider.provider_actions.save_state({'zookeeper-instances': [1000]})
939- self.mocker.result(defer.succeed(None))
940+ self.mocker.result(succeed(None))
941 self.mocker.replay()
942 return provider.launch("0", master=True)

Subscribers

People subscribed via source and target branches

to status/vote changes: