Merge lp:~cbehrens/nova/lp766404 into lp:~hudson-openstack/nova/trunk
- lp766404
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Josh Kearney | ||||
Approved revision: | 1067 | ||||
Merged at revision: | 1070 | ||||
Proposed branch: | lp:~cbehrens/nova/lp766404 | ||||
Merge into: | lp:~hudson-openstack/nova/trunk | ||||
Diff against target: |
234 lines (+76/-74) 4 files modified
nova/tests/test_xenapi.py (+23/-0) nova/tests/xenapi/stubs.py (+11/-28) nova/utils.py (+2/-0) nova/virt/xenapi_conn.py (+40/-46) |
||||
To merge this branch: | bzr merge lp:~cbehrens/nova/lp766404 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vish Ishaya (community) | Approve | ||
Josh Kearney (community) | Approve | ||
Review via email: mp+60543@code.launchpad.net |
Commit message
Description of the change
XenAPI was not implemented to allow for multiple simultaneous XenAPI requests. A single XenAPIConnection (and thus XenAPISession) is used for all queries. XenAPISession's wait_for_task method would set a self.loop = for looping calls to _poll_task until task completion. Subsequent (parallel) calls to wait_for_task for another query would overwrite this. XenAPISession.
Brian Lamar (blamar) wrote : | # |
Chris Behrens (cbehrens) wrote : | # |
Good question. There are XenAPI tests in general which exercise this code (nova/tests/
Vish Ishaya (vishvananda) wrote : | # |
You can probably get this to work with a greenthread and a mock method that uses time.sleep. If you want to get a little bit more elaborate you can make a method that waits on an eventlet Event() so you can be sure that it pauses until you tell it to finish. See CacheConcurrenc
On May 11, 2011, at 10:31 AM, Chris Behrens wrote:
> Good question. There are XenAPI tests in general which exercise this code (nova/tests/
> --
> https:/
> You are subscribed to branch lp:nova.
Chris Behrens (cbehrens) wrote : | # |
Ya, was thinking along those lines. I'll take a look at that example. Thnx!
Chris Behrens (cbehrens) wrote : | # |
Ok, got a test in that fails before fixing this bug and succeeds after fixing. Also added a small improvement to LoopingCall to not sleep unnecessarily.
Josh Kearney (jk0) wrote : | # |
Looks great and tests perform as expected! Very nice fix.
OpenStack Infra (hudson-openstack) wrote : | # |
The attempt to merge lp:~cbehrens/nova/lp766404 into lp:nova failed. Below is the output from the failed tests.
AccountsTest
test_
test_
test_
test_
AdminAPITest
test_
test_
APITest
test_
Test
test_
test_
test_bad_token OK
test_
test_
test_no_user OK
test_
TestFunctional
test_
test_
TestLimiter
test_
LimiterTest
test_
test_
test_
test_
test_
test_
test_
test_
test_
test_
test_
test_
test_
ActionExtensionTest
test_
test_
test_
ExtensionContro
test_
test_index OK
ExtensionManage
test_
ResourceExtensi
test_
test_
test_
ResponseExtensi
test_
test_
TestFaults
test_
test_
test...
Chris Behrens (cbehrens) wrote : | # |
Fixed the pep8 issues with the tests.
Preview Diff
1 | === modified file 'nova/tests/test_xenapi.py' | |||
2 | --- nova/tests/test_xenapi.py 2011-05-09 19:57:56 +0000 | |||
3 | +++ nova/tests/test_xenapi.py 2011-05-13 16:47:26 +0000 | |||
4 | @@ -16,6 +16,7 @@ | |||
5 | 16 | 16 | ||
6 | 17 | """Test suite for XenAPI.""" | 17 | """Test suite for XenAPI.""" |
7 | 18 | 18 | ||
8 | 19 | import eventlet | ||
9 | 19 | import functools | 20 | import functools |
10 | 20 | import json | 21 | import json |
11 | 21 | import os | 22 | import os |
12 | @@ -198,6 +199,28 @@ | |||
13 | 198 | self.context = context.RequestContext('fake', 'fake', False) | 199 | self.context = context.RequestContext('fake', 'fake', False) |
14 | 199 | self.conn = xenapi_conn.get_connection(False) | 200 | self.conn = xenapi_conn.get_connection(False) |
15 | 200 | 201 | ||
16 | 202 | def test_parallel_builds(self): | ||
17 | 203 | stubs.stubout_loopingcall_delay(self.stubs) | ||
18 | 204 | |||
19 | 205 | def _do_build(id, proj, user, *args): | ||
20 | 206 | values = { | ||
21 | 207 | 'id': id, | ||
22 | 208 | 'project_id': proj, | ||
23 | 209 | 'user_id': user, | ||
24 | 210 | 'image_id': 1, | ||
25 | 211 | 'kernel_id': 2, | ||
26 | 212 | 'ramdisk_id': 3, | ||
27 | 213 | 'instance_type_id': '3', # m1.large | ||
28 | 214 | 'mac_address': 'aa:bb:cc:dd:ee:ff', | ||
29 | 215 | 'os_type': 'linux'} | ||
30 | 216 | instance = db.instance_create(self.context, values) | ||
31 | 217 | self.conn.spawn(instance) | ||
32 | 218 | |||
33 | 219 | gt1 = eventlet.spawn(_do_build, 1, self.project.id, self.user.id) | ||
34 | 220 | gt2 = eventlet.spawn(_do_build, 2, self.project.id, self.user.id) | ||
35 | 221 | gt1.wait() | ||
36 | 222 | gt2.wait() | ||
37 | 223 | |||
38 | 201 | def test_list_instances_0(self): | 224 | def test_list_instances_0(self): |
39 | 202 | instances = self.conn.list_instances() | 225 | instances = self.conn.list_instances() |
40 | 203 | self.assertEquals(instances, []) | 226 | self.assertEquals(instances, []) |
41 | 204 | 227 | ||
42 | === modified file 'nova/tests/xenapi/stubs.py' | |||
43 | --- nova/tests/xenapi/stubs.py 2011-03-25 23:43:21 +0000 | |||
44 | +++ nova/tests/xenapi/stubs.py 2011-05-13 16:47:26 +0000 | |||
45 | @@ -16,6 +16,7 @@ | |||
46 | 16 | 16 | ||
47 | 17 | """Stubouts, mocks and fixtures for the test suite""" | 17 | """Stubouts, mocks and fixtures for the test suite""" |
48 | 18 | 18 | ||
49 | 19 | import eventlet | ||
50 | 19 | from nova.virt import xenapi_conn | 20 | from nova.virt import xenapi_conn |
51 | 20 | from nova.virt.xenapi import fake | 21 | from nova.virt.xenapi import fake |
52 | 21 | from nova.virt.xenapi import volume_utils | 22 | from nova.virt.xenapi import volume_utils |
53 | @@ -28,29 +29,6 @@ | |||
54 | 28 | @classmethod | 29 | @classmethod |
55 | 29 | def fake_fetch_image(cls, session, instance_id, image, user, project, | 30 | def fake_fetch_image(cls, session, instance_id, image, user, project, |
56 | 30 | type): | 31 | type): |
57 | 31 | # Stubout wait_for_task | ||
58 | 32 | def fake_wait_for_task(self, task, id): | ||
59 | 33 | class FakeEvent: | ||
60 | 34 | |||
61 | 35 | def send(self, value): | ||
62 | 36 | self.rv = value | ||
63 | 37 | |||
64 | 38 | def wait(self): | ||
65 | 39 | return self.rv | ||
66 | 40 | |||
67 | 41 | done = FakeEvent() | ||
68 | 42 | self._poll_task(id, task, done) | ||
69 | 43 | rv = done.wait() | ||
70 | 44 | return rv | ||
71 | 45 | |||
72 | 46 | def fake_loop(self): | ||
73 | 47 | pass | ||
74 | 48 | |||
75 | 49 | stubs.Set(xenapi_conn.XenAPISession, 'wait_for_task', | ||
76 | 50 | fake_wait_for_task) | ||
77 | 51 | |||
78 | 52 | stubs.Set(xenapi_conn.XenAPISession, '_stop_loop', fake_loop) | ||
79 | 53 | |||
80 | 54 | from nova.virt.xenapi.fake import create_vdi | 32 | from nova.virt.xenapi.fake import create_vdi |
81 | 55 | name_label = "instance-%s" % instance_id | 33 | name_label = "instance-%s" % instance_id |
82 | 56 | #TODO: create fake SR record | 34 | #TODO: create fake SR record |
83 | @@ -62,11 +40,6 @@ | |||
84 | 62 | return vdi_uuid | 40 | return vdi_uuid |
85 | 63 | 41 | ||
86 | 64 | stubs.Set(vm_utils.VMHelper, 'fetch_image', fake_fetch_image) | 42 | stubs.Set(vm_utils.VMHelper, 'fetch_image', fake_fetch_image) |
87 | 65 | |||
88 | 66 | def fake_parse_xmlrpc_value(val): | ||
89 | 67 | return val | ||
90 | 68 | |||
91 | 69 | stubs.Set(xenapi_conn, '_parse_xmlrpc_value', fake_parse_xmlrpc_value) | ||
92 | 70 | 43 | ||
93 | 71 | def fake_wait_for_vhd_coalesce(session, instance_id, sr_ref, vdi_ref, | 44 | def fake_wait_for_vhd_coalesce(session, instance_id, sr_ref, vdi_ref, |
94 | 72 | original_parent_uuid): | 45 | original_parent_uuid): |
95 | @@ -144,6 +117,16 @@ | |||
96 | 144 | stubs.Set(utils.LoopingCall, 'start', fake_start) | 117 | stubs.Set(utils.LoopingCall, 'start', fake_start) |
97 | 145 | 118 | ||
98 | 146 | 119 | ||
99 | 120 | def stubout_loopingcall_delay(stubs): | ||
100 | 121 | def fake_start(self, interval, now=True): | ||
101 | 122 | self._running = True | ||
102 | 123 | eventlet.sleep(1) | ||
103 | 124 | self.f(*self.args, **self.kw) | ||
104 | 125 | # This would fail before parallel xenapi calls were fixed | ||
105 | 126 | assert self._running == False | ||
106 | 127 | stubs.Set(utils.LoopingCall, 'start', fake_start) | ||
107 | 128 | |||
108 | 129 | |||
109 | 147 | class FakeSessionForVMTests(fake.SessionBase): | 130 | class FakeSessionForVMTests(fake.SessionBase): |
110 | 148 | """ Stubs out a XenAPISession for VM tests """ | 131 | """ Stubs out a XenAPISession for VM tests """ |
111 | 149 | def __init__(self, uri): | 132 | def __init__(self, uri): |
112 | 150 | 133 | ||
113 | === modified file 'nova/utils.py' | |||
114 | --- nova/utils.py 2011-05-11 22:56:57 +0000 | |||
115 | +++ nova/utils.py 2011-05-13 16:47:26 +0000 | |||
116 | @@ -462,6 +462,8 @@ | |||
117 | 462 | try: | 462 | try: |
118 | 463 | while self._running: | 463 | while self._running: |
119 | 464 | self.f(*self.args, **self.kw) | 464 | self.f(*self.args, **self.kw) |
120 | 465 | if not self._running: | ||
121 | 466 | break | ||
122 | 465 | greenthread.sleep(interval) | 467 | greenthread.sleep(interval) |
123 | 466 | except LoopingCallDone, e: | 468 | except LoopingCallDone, e: |
124 | 467 | self.stop() | 469 | self.stop() |
125 | 468 | 470 | ||
126 | === modified file 'nova/virt/xenapi_conn.py' | |||
127 | --- nova/virt/xenapi_conn.py 2011-05-09 19:57:56 +0000 | |||
128 | +++ nova/virt/xenapi_conn.py 2011-05-13 16:47:26 +0000 | |||
129 | @@ -347,7 +347,6 @@ | |||
130 | 347 | "(is the Dom0 disk full?)")) | 347 | "(is the Dom0 disk full?)")) |
131 | 348 | with timeout.Timeout(FLAGS.xenapi_login_timeout, exception): | 348 | with timeout.Timeout(FLAGS.xenapi_login_timeout, exception): |
132 | 349 | self._session.login_with_password(user, pw) | 349 | self._session.login_with_password(user, pw) |
133 | 350 | self.loop = None | ||
134 | 351 | 350 | ||
135 | 352 | def get_imported_xenapi(self): | 351 | def get_imported_xenapi(self): |
136 | 353 | """Stubout point. This can be replaced with a mock xenapi module.""" | 352 | """Stubout point. This can be replaced with a mock xenapi module.""" |
137 | @@ -384,57 +383,52 @@ | |||
138 | 384 | 383 | ||
139 | 385 | def wait_for_task(self, task, id=None): | 384 | def wait_for_task(self, task, id=None): |
140 | 386 | """Return the result of the given task. The task is polled | 385 | """Return the result of the given task. The task is polled |
142 | 387 | until it completes. Not re-entrant.""" | 386 | until it completes.""" |
143 | 388 | done = event.Event() | 387 | done = event.Event() |
155 | 389 | self.loop = utils.LoopingCall(self._poll_task, id, task, done) | 388 | loop = utils.LoopingCall(f=None) |
156 | 390 | self.loop.start(FLAGS.xenapi_task_poll_interval, now=True) | 389 | |
157 | 391 | rv = done.wait() | 390 | def _poll_task(): |
158 | 392 | self.loop.stop() | 391 | """Poll the given XenAPI task, and return the result if the |
159 | 393 | return rv | 392 | action was completed successfully or not. |
160 | 394 | 393 | """ | |
161 | 395 | def _stop_loop(self): | 394 | try: |
162 | 396 | """Stop polling for task to finish.""" | 395 | name = self._session.xenapi.task.get_name_label(task) |
163 | 397 | #NOTE(sandy-walsh) Had to break this call out to support unit tests. | 396 | status = self._session.xenapi.task.get_status(task) |
164 | 398 | if self.loop: | 397 | if id: |
165 | 399 | self.loop.stop() | 398 | action = dict( |
166 | 399 | instance_id=int(id), | ||
167 | 400 | action=name[0:255], # Ensure action is never > 255 | ||
168 | 401 | error=None) | ||
169 | 402 | if status == "pending": | ||
170 | 403 | return | ||
171 | 404 | elif status == "success": | ||
172 | 405 | result = self._session.xenapi.task.get_result(task) | ||
173 | 406 | LOG.info(_("Task [%(name)s] %(task)s status:" | ||
174 | 407 | " success %(result)s") % locals()) | ||
175 | 408 | done.send(_parse_xmlrpc_value(result)) | ||
176 | 409 | else: | ||
177 | 410 | error_info = self._session.xenapi.task.get_error_info(task) | ||
178 | 411 | action["error"] = str(error_info) | ||
179 | 412 | LOG.warn(_("Task [%(name)s] %(task)s status:" | ||
180 | 413 | " %(status)s %(error_info)s") % locals()) | ||
181 | 414 | done.send_exception(self.XenAPI.Failure(error_info)) | ||
182 | 415 | |||
183 | 416 | if id: | ||
184 | 417 | db.instance_action_create(context.get_admin_context(), | ||
185 | 418 | action) | ||
186 | 419 | except self.XenAPI.Failure, exc: | ||
187 | 420 | LOG.warn(exc) | ||
188 | 421 | done.send_exception(*sys.exc_info()) | ||
189 | 422 | loop.stop() | ||
190 | 423 | |||
191 | 424 | loop.f = _poll_task | ||
192 | 425 | loop.start(FLAGS.xenapi_task_poll_interval, now=True) | ||
193 | 426 | return done.wait() | ||
194 | 400 | 427 | ||
195 | 401 | def _create_session(self, url): | 428 | def _create_session(self, url): |
196 | 402 | """Stubout point. This can be replaced with a mock session.""" | 429 | """Stubout point. This can be replaced with a mock session.""" |
197 | 403 | return self.XenAPI.Session(url) | 430 | return self.XenAPI.Session(url) |
198 | 404 | 431 | ||
199 | 405 | def _poll_task(self, id, task, done): | ||
200 | 406 | """Poll the given XenAPI task, and fire the given action if we | ||
201 | 407 | get a result. | ||
202 | 408 | """ | ||
203 | 409 | try: | ||
204 | 410 | name = self._session.xenapi.task.get_name_label(task) | ||
205 | 411 | status = self._session.xenapi.task.get_status(task) | ||
206 | 412 | if id: | ||
207 | 413 | action = dict( | ||
208 | 414 | instance_id=int(id), | ||
209 | 415 | action=name[0:255], # Ensure action is never > 255 | ||
210 | 416 | error=None) | ||
211 | 417 | if status == "pending": | ||
212 | 418 | return | ||
213 | 419 | elif status == "success": | ||
214 | 420 | result = self._session.xenapi.task.get_result(task) | ||
215 | 421 | LOG.info(_("Task [%(name)s] %(task)s status:" | ||
216 | 422 | " success %(result)s") % locals()) | ||
217 | 423 | done.send(_parse_xmlrpc_value(result)) | ||
218 | 424 | else: | ||
219 | 425 | error_info = self._session.xenapi.task.get_error_info(task) | ||
220 | 426 | action["error"] = str(error_info) | ||
221 | 427 | LOG.warn(_("Task [%(name)s] %(task)s status:" | ||
222 | 428 | " %(status)s %(error_info)s") % locals()) | ||
223 | 429 | done.send_exception(self.XenAPI.Failure(error_info)) | ||
224 | 430 | |||
225 | 431 | if id: | ||
226 | 432 | db.instance_action_create(context.get_admin_context(), action) | ||
227 | 433 | except self.XenAPI.Failure, exc: | ||
228 | 434 | LOG.warn(exc) | ||
229 | 435 | done.send_exception(*sys.exc_info()) | ||
230 | 436 | self._stop_loop() | ||
231 | 437 | |||
232 | 438 | def _unwrap_plugin_exceptions(self, func, *args, **kwargs): | 432 | def _unwrap_plugin_exceptions(self, func, *args, **kwargs): |
233 | 439 | """Parse exception details""" | 433 | """Parse exception details""" |
234 | 440 | try: | 434 | try: |
Are there tests that can show this fix in action?