OpenStack Compute (nova)

Merge lp:~rackspace-titan/nova/rescue_fix_admin_pass into lp:~hudson-openstack/nova/trunk

  1. rescue_fix_admin_pass
  2. Merge into trunk
Proposed by Dan Prince
Status: Merged
Approved by: Brian Lamar
Approved revision: 1529
Merged at revision: 1582
Proposed branch: lp:~rackspace-titan/nova/rescue_fix_admin_pass
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 423 lines (+124/-46)
10 files modified
nova/api/openstack/contrib/rescue.py (+10/-3)
nova/api/openstack/create_instance_helper.py (+2/-2)
nova/api/openstack/servers.py (+14/-6)
nova/compute/api.py (+7/-2)
nova/compute/manager.py (+8/-4)
nova/flags.py (+3/-0)
nova/tests/api/openstack/contrib/test_rescue.py (+21/-4)
nova/tests/api/openstack/contrib/test_volumes.py (+5/-1)
nova/tests/api/openstack/test_server_actions.py (+2/-1)
nova/tests/api/openstack/test_servers.py (+52/-23)
To merge this branch: bzr merge lp:~rackspace-titan/nova/rescue_fix_admin_pass
Reviewer Review Type Date Requested Status
Brian Lamar (community) Approve
Devin Carlen (community) Approve
Chris Behrens (community) Approve
Brian Waldon (community) Approve
Review via email: mp+74200@code.launchpad.net

Description of the change

Update the v1.0 rescue admin action and the v1.1 rescue extension to
generate 'adminPass'. Fixes an issue where rescue commands were broken
on XenServer. lp#838518

Also relocate 'password_length' flag and replace cases where password length was hard-coded.

To post a comment you must log in.
Revision history for this message
Dan Prince (dan-prince) wrote :

Also filed an issue here where we'll take care of updating python-novaclient to display the generated 'adminPass': https://github.com/rackspace/python-novaclient/issues/113

Revision history for this message
Brian Waldon (bcwaldon) wrote :

I updated the description with a note about the password_length flag changes you made. Just didn't want anybody to see it as a side-effect later on down the road.

Code looks good, but test_rescue_with_preset_password isn't passing for me

review: Needs Fixing
Revision history for this message
Dan Prince (dan-prince) wrote :

Hey Brian,

Can you give me an idea of what the failure is? Tests are all passing for me...

[dan.prince@dovetail rescue_fix_set_admin_pass]$ ./run_tests.sh -N api.openstack.contrib.test_rescue
RescueTest
    test_rescue_generates_password OK 0.40
    test_rescue_with_preset_password OK 0.31
    test_unrescue OK 0.31

----------------------------------------------------------------------
Ran 3 tests in 18.206s

OK

Revision history for this message
Dan Prince (dan-prince) wrote :

Okay. I bumped the req.method = 'POST' up a few lines. That should fix it.

Revision history for this message
Brian Waldon (bcwaldon) wrote :

Looks good to me.

review: Approve
Revision history for this message
Chris Behrens (cbehrens) wrote :

Just curious about the inconsistent use of single quotes vs double quotes. Lines 33-34, 79-81, etc.
If it was done on purpose, I'm just generally curious about why the certain quotes were chosen. :)

ltgm!

review: Approve
Revision history for this message
Chris Behrens (cbehrens) wrote :

> ltgm!

Uh, how about: lgtm!

Revision history for this message
Dan Prince (dan-prince) wrote :

>I'm just generally curious about why the certain quotes were chosen. :)
>
> ltgm!

They should match now.

Revision history for this message
Chris Behrens (cbehrens) wrote :

Dan: Cool. I wasn't going to be picky about having them match... I was just curious if there was a specific reason for it. But thanks!

Revision history for this message
Devin Carlen (devcamcar) wrote :

lgtm!

review: Approve
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :
Download full text (152.5 KiB)

The attempt to merge lp:~rackspace-titan/nova/rescue_fix_admin_pass into lp:nova failed. Below is the output from the failed tests.

CloudTestCase
    test_ajax_console OK 1.89
    test_allocate_address OK 0.27
    test_associate_disassociate_address OK 1.43
    test_authorize_revoke_security_group_ingress_by_id OK 0.36
    test_authorize_security_group_fail_missing_source_group OK 0.28
    test_authorize_security_group_ingress OK 0.27
    test_authorize_security_group_ingress_already_exists OK 0.47
    test_authorize_security_group_ingress_ip_permissions_groups OK 0.34
    test_authorize_security_group_ingress_ip_permissions_ip_rangesOK 0.27
    test_authorize_security_group_ingress_missing_group_name_or_idOK 0.19
    test_authorize_security_group_ingress_missing_protocol_paramsOK 0.26
    test_console_output OK 1.39
    test_create_delete_security_group OK 0.44
    test_create_image OK 4.66
    test_create_snapshot OK 0.46
    test_create_volume_from_snapshot OK 0.56
    test_delete_key_pair OK 0.42
    test_delete_security_group_by_id OK 0.25
    test_delete_security_group_no_params OK 0.20
    test_delete_security_group_with_bad_group_id OK 0.22
    test_delete_security_group_with_bad_name OK 0.22
    test_delete_snapshot OK 0.43
    test_deregister_image OK 0.20
    test_deregister_image_wrong_container_type OK 0.20
    test_describe_addresses OK 0.48
    test_describe_availability_zones OK 0.24
    test_describe_image_attribute OK 0.20
    test_describe_image_attribute_block_device_mapping OK 0.19
    test_describe_image_attribute_root_device_name OK 0.19
    test_describe_image_mapping OK 0.20
    test_describe_images OK 0.20
    test_describe_instance_attribute OK 0.19
    test_describe_instances OK 0.40
    test_describe_instances_bdm OK 1.47
    test_describe_instances_deleted OK 0.28
    test_describe_key_pairs OK 0.64
    test_describe_regions OK 0.19
    test_describe_security_groups OK 0.29
    test_describe_security_groups_by_id OK 0.32
    test_describe_snapshots OK 0.23
    test_describe_volumes OK 0.28
    test_format_instance_bdm ...

Revision history for this message
Dan Prince (dan-prince) wrote :

Okay. Looks like we hardcoded another password_length in test_volumes. Just fixed it.

Revision history for this message
Brian Lamar (blamar) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/api/openstack/contrib/rescue.py'
2--- nova/api/openstack/contrib/rescue.py 2011-08-18 21:07:02 +0000
3+++ nova/api/openstack/contrib/rescue.py 2011-09-16 18:39:24 +0000
4@@ -18,11 +18,14 @@
5 from webob import exc
6
7 from nova import compute
8+from nova import flags
9 from nova import log as logging
10+from nova import utils
11 from nova.api.openstack import extensions as exts
12 from nova.api.openstack import faults
13
14
15+FLAGS = flags.FLAGS
16 LOG = logging.getLogger("nova.api.contrib.rescue")
17
18
19@@ -30,7 +33,7 @@
20 """"Ensure errors are not passed along."""
21 def wrapped(*args):
22 try:
23- fn(*args)
24+ return fn(*args)
25 except Exception, e:
26 return faults.Fault(exc.HTTPInternalServerError())
27 return wrapped
28@@ -46,9 +49,13 @@
29 def _rescue(self, input_dict, req, instance_id):
30 """Rescue an instance."""
31 context = req.environ["nova.context"]
32- self.compute_api.rescue(context, instance_id)
33+ if input_dict['rescue'] and 'adminPass' in input_dict['rescue']:
34+ password = input_dict['rescue']['adminPass']
35+ else:
36+ password = utils.generate_password(FLAGS.password_length)
37+ self.compute_api.rescue(context, instance_id, rescue_password=password)
38
39- return webob.Response(status_int=202)
40+ return {'adminPass': password}
41
42 @wrap_errors
43 def _unrescue(self, input_dict, req, instance_id):
44
45=== modified file 'nova/api/openstack/create_instance_helper.py'
46--- nova/api/openstack/create_instance_helper.py 2011-09-10 17:56:54 +0000
47+++ nova/api/openstack/create_instance_helper.py 2011-09-16 18:39:24 +0000
48@@ -317,14 +317,14 @@
49
50 def _get_server_admin_password_old_style(self, server):
51 """ Determine the admin password for a server on creation """
52- return utils.generate_password(16)
53+ return utils.generate_password(FLAGS.password_length)
54
55 def _get_server_admin_password_new_style(self, server):
56 """ Determine the admin password for a server on creation """
57 password = server.get('adminPass')
58
59 if password is None:
60- return utils.generate_password(16)
61+ return utils.generate_password(FLAGS.password_length)
62 if not isinstance(password, basestring) or password == '':
63 msg = _("Invalid adminPass")
64 raise exc.HTTPBadRequest(explanation=msg)
65
66=== modified file 'nova/api/openstack/servers.py'
67--- nova/api/openstack/servers.py 2011-09-10 02:14:39 +0000
68+++ nova/api/openstack/servers.py 2011-09-16 18:39:24 +0000
69@@ -477,16 +477,22 @@
70 return webob.Response(status_int=202)
71
72 @scheduler_api.redirect_handler
73- def rescue(self, req, id):
74+ def rescue(self, req, id, body={}):
75 """Permit users to rescue the server."""
76 context = req.environ["nova.context"]
77 try:
78- self.compute_api.rescue(context, id)
79+ if 'rescue' in body and body['rescue'] and \
80+ 'adminPass' in body['rescue']:
81+ password = body['rescue']['adminPass']
82+ else:
83+ password = utils.generate_password(FLAGS.password_length)
84+ self.compute_api.rescue(context, id, rescue_password=password)
85 except Exception:
86 readable = traceback.format_exc()
87 LOG.exception(_("compute.api::rescue %s"), readable)
88 raise exc.HTTPUnprocessableEntity()
89- return webob.Response(status_int=202)
90+
91+ return {'adminPass': password}
92
93 @scheduler_api.redirect_handler
94 def unrescue(self, req, id):
95@@ -618,7 +624,7 @@
96 LOG.debug(msg)
97 raise exc.HTTPBadRequest(explanation=msg)
98
99- password = utils.generate_password(16)
100+ password = utils.generate_password(FLAGS.password_length)
101
102 try:
103 self.compute_api.rebuild(context, instance_id, image_id, password)
104@@ -760,8 +766,10 @@
105 self._validate_metadata(metadata)
106 self._decode_personalities(personalities)
107
108- password = info["rebuild"].get("adminPass",
109- utils.generate_password(16))
110+ if 'rebuild' in info and 'adminPass' in info['rebuild']:
111+ password = info['rebuild']['adminPass']
112+ else:
113+ password = utils.generate_password(FLAGS.password_length)
114
115 try:
116 self.compute_api.rebuild(context, instance_id, image_href,
117
118=== modified file 'nova/compute/api.py'
119--- nova/compute/api.py 2011-09-13 21:29:26 +0000
120+++ nova/compute/api.py 2011-09-16 18:39:24 +0000
121@@ -1274,13 +1274,18 @@
122 self._cast_compute_message('resume_instance', context, instance_id)
123
124 @scheduler_api.reroute_compute("rescue")
125- def rescue(self, context, instance_id):
126+ def rescue(self, context, instance_id, rescue_password=None):
127 """Rescue the given instance."""
128 self.update(context,
129 instance_id,
130 vm_state=vm_states.ACTIVE,
131 task_state=task_states.RESCUING)
132- self._cast_compute_message('rescue_instance', context, instance_id)
133+
134+ rescue_params = {
135+ "rescue_password": rescue_password
136+ }
137+ self._cast_compute_message('rescue_instance', context, instance_id,
138+ params=rescue_params)
139
140 @scheduler_api.reroute_compute("unrescue")
141 def unrescue(self, context, instance_id):
142
143=== modified file 'nova/compute/manager.py'
144--- nova/compute/manager.py 2011-09-12 19:39:13 +0000
145+++ nova/compute/manager.py 2011-09-16 18:39:24 +0000
146@@ -70,8 +70,6 @@
147 'Driver to use for controlling virtualization')
148 flags.DEFINE_string('stub_network', False,
149 'Stub network related code')
150-flags.DEFINE_integer('password_length', 12,
151- 'Length of generated admin passwords')
152 flags.DEFINE_string('console_host', socket.gethostname(),
153 'Console proxy host to use to connect to instances on'
154 'this host.')
155@@ -797,12 +795,18 @@
156
157 @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
158 @checks_instance_lock
159- def rescue_instance(self, context, instance_id):
160- """Rescue an instance on this host."""
161+ def rescue_instance(self, context, instance_id, **kwargs):
162+ """
163+ Rescue an instance on this host.
164+ :param rescue_password: password to set on rescue instance
165+ """
166+
167 LOG.audit(_('instance %s: rescuing'), instance_id, context=context)
168 context = context.elevated()
169
170 instance_ref = self.db.instance_get(context, instance_id)
171+ instance_ref.admin_pass = kwargs.get('rescue_password',
172+ utils.generate_password(FLAGS.password_length))
173 network_info = self._get_instance_nw_info(context, instance_ref)
174
175 # NOTE(blamar): None of the virt drivers use the 'callback' param
176
177=== modified file 'nova/flags.py'
178--- nova/flags.py 2011-08-29 15:07:34 +0000
179+++ nova/flags.py 2011-09-16 18:39:24 +0000
180@@ -421,6 +421,9 @@
181
182 DEFINE_bool('use_ipv6', False, 'use ipv6')
183
184+DEFINE_integer('password_length', 12,
185+ 'Length of generated instance admin passwords')
186+
187 DEFINE_bool('monkey_patch', False,
188 'Whether to log monkey patching')
189
190
191=== modified file 'nova/tests/api/openstack/contrib/test_rescue.py'
192--- nova/tests/api/openstack/contrib/test_rescue.py 2011-08-19 17:00:22 +0000
193+++ nova/tests/api/openstack/contrib/test_rescue.py 2011-09-16 18:39:24 +0000
194@@ -16,11 +16,14 @@
195 import webob
196
197 from nova import compute
198+from nova import flags
199 from nova import test
200 from nova.tests.api.openstack import fakes
201
202-
203-def rescue(self, context, instance_id):
204+FLAGS = flags.FLAGS
205+
206+
207+def rescue(self, context, instance_id, rescue_password=None):
208 pass
209
210
211@@ -34,7 +37,19 @@
212 self.stubs.Set(compute.api.API, "rescue", rescue)
213 self.stubs.Set(compute.api.API, "unrescue", unrescue)
214
215- def test_rescue(self):
216+ def test_rescue_with_preset_password(self):
217+ body = {"rescue": {"adminPass": "AABBCC112233"}}
218+ req = webob.Request.blank('/v1.1/123/servers/test_inst/action')
219+ req.method = "POST"
220+ req.body = json.dumps(body)
221+ req.headers["content-type"] = "application/json"
222+
223+ resp = req.get_response(fakes.wsgi_app())
224+ self.assertEqual(resp.status_int, 200)
225+ resp_json = json.loads(resp.body)
226+ self.assertEqual("AABBCC112233", resp_json['adminPass'])
227+
228+ def test_rescue_generates_password(self):
229 body = dict(rescue=None)
230 req = webob.Request.blank('/v1.1/123/servers/test_inst/action')
231 req.method = "POST"
232@@ -43,6 +58,8 @@
233
234 resp = req.get_response(fakes.wsgi_app())
235 self.assertEqual(resp.status_int, 200)
236+ resp_json = json.loads(resp.body)
237+ self.assertEqual(FLAGS.password_length, len(resp_json['adminPass']))
238
239 def test_unrescue(self):
240 body = dict(unrescue=None)
241@@ -52,4 +69,4 @@
242 req.headers["content-type"] = "application/json"
243
244 resp = req.get_response(fakes.wsgi_app())
245- self.assertEqual(resp.status_int, 200)
246+ self.assertEqual(resp.status_int, 202)
247
248=== modified file 'nova/tests/api/openstack/contrib/test_volumes.py'
249--- nova/tests/api/openstack/contrib/test_volumes.py 2011-09-15 18:05:57 +0000
250+++ nova/tests/api/openstack/contrib/test_volumes.py 2011-09-16 18:39:24 +0000
251@@ -19,6 +19,7 @@
252
253 import nova
254 from nova import context
255+from nova import flags
256 from nova import test
257 from nova.api.openstack.contrib.volumes import BootFromVolumeController
258 from nova.compute import instance_types
259@@ -26,6 +27,9 @@
260 from nova.tests.api.openstack.test_servers import fake_gen_uuid
261
262
263+FLAGS = flags.FLAGS
264+
265+
266 def fake_compute_api_create(cls, context, instance_type, image_href, **kwargs):
267 inst_type = instance_types.get_instance_type_by_flavor_id(2)
268 return [{'id': 1,
269@@ -70,4 +74,4 @@
270 self.assertEqual(2, int(server['flavor']['id']))
271 self.assertEqual(u'test_server', server['name'])
272 self.assertEqual(3, int(server['image']['id']))
273- self.assertEqual(16, len(server['adminPass']))
274+ self.assertEqual(FLAGS.password_length, len(server['adminPass']))
275
276=== modified file 'nova/tests/api/openstack/test_server_actions.py'
277--- nova/tests/api/openstack/test_server_actions.py 2011-08-31 23:13:55 +0000
278+++ nova/tests/api/openstack/test_server_actions.py 2011-09-16 18:39:24 +0000
279@@ -622,7 +622,8 @@
280 self.assertEqual(res.status_int, 202)
281 body = json.loads(res.body)
282 self.assertEqual(body['server']['image']['id'], '2')
283- self.assertEqual(len(body['server']['adminPass']), 16)
284+ self.assertEqual(len(body['server']['adminPass']),
285+ FLAGS.password_length)
286
287 def test_server_rebuild_rejected_when_building(self):
288 body = {
289
290=== modified file 'nova/tests/api/openstack/test_servers.py'
291--- nova/tests/api/openstack/test_servers.py 2011-09-13 18:00:20 +0000
292+++ nova/tests/api/openstack/test_servers.py 2011-09-16 18:39:24 +0000
293@@ -28,6 +28,7 @@
294 from nova import context
295 from nova import db
296 from nova import exception
297+from nova import flags
298 from nova import test
299 from nova import utils
300 import nova.api.openstack
301@@ -49,6 +50,7 @@
302 from nova.tests.api.openstack import fakes
303
304
305+FLAGS = flags.FLAGS
306 FAKE_UUID = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'
307 NS = "{http://docs.openstack.org/compute/api/v1.1}"
308 ATOMNS = "{http://www.w3.org/2005/Atom}"
309@@ -1575,7 +1577,7 @@
310
311 self.assertEqual(res.status_int, 202)
312 server = json.loads(res.body)['server']
313- self.assertEqual(16, len(server['adminPass']))
314+ self.assertEqual(FLAGS.password_length, len(server['adminPass']))
315 self.assertEqual('server_test', server['name'])
316 self.assertEqual(1, server['id'])
317 self.assertEqual(2, server['flavorId'])
318@@ -1776,7 +1778,7 @@
319
320 self.assertEqual(res.status_int, 202)
321 server = json.loads(res.body)['server']
322- self.assertEqual(16, len(server['adminPass']))
323+ self.assertEqual(FLAGS.password_length, len(server['adminPass']))
324 self.assertEqual(1, server['id'])
325 self.assertEqual(0, server['progress'])
326 self.assertEqual('server_test', server['name'])
327@@ -1836,7 +1838,7 @@
328
329 self.assertEqual(res.status_int, 202)
330 server = json.loads(res.body)['server']
331- self.assertEqual(16, len(server['adminPass']))
332+ self.assertEqual(FLAGS.password_length, len(server['adminPass']))
333 self.assertEqual(1, server['id'])
334 self.assertEqual("BUILD", server["status"])
335 self.assertEqual(0, server['progress'])
336@@ -2547,24 +2549,49 @@
337 self.assertEqual(res.status, '202 Accepted')
338 self.assertEqual(self.server_delete_called, True)
339
340- def test_rescue_accepted(self):
341- self.flags(allow_admin_api=True)
342- body = {}
343-
344- self.called = False
345-
346- def rescue_mock(*args, **kwargs):
347- self.called = True
348-
349- self.stubs.Set(nova.compute.api.API, 'rescue', rescue_mock)
350- req = webob.Request.blank('/v1.0/servers/1/rescue')
351- req.method = 'POST'
352- req.content_type = 'application/json'
353-
354- res = req.get_response(fakes.wsgi_app())
355-
356- self.assertEqual(self.called, True)
357- self.assertEqual(res.status_int, 202)
358+ def test_rescue_generates_password(self):
359+ self.flags(allow_admin_api=True)
360+
361+ self.called = False
362+
363+ def rescue_mock(*args, **kwargs):
364+ self.called = True
365+
366+ self.stubs.Set(nova.compute.api.API, 'rescue', rescue_mock)
367+ req = webob.Request.blank('/v1.0/servers/1/rescue')
368+ req.method = 'POST'
369+ req.content_type = 'application/json'
370+
371+ res = req.get_response(fakes.wsgi_app())
372+
373+ self.assertEqual(self.called, True)
374+ self.assertEqual(res.status_int, 200)
375+ res_body = json.loads(res.body)
376+ self.assertTrue('adminPass' in res_body)
377+ self.assertEqual(FLAGS.password_length, len(res_body['adminPass']))
378+
379+ def test_rescue_with_preset_password(self):
380+ self.flags(allow_admin_api=True)
381+
382+ self.called = False
383+
384+ def rescue_mock(*args, **kwargs):
385+ self.called = True
386+
387+ self.stubs.Set(nova.compute.api.API, 'rescue', rescue_mock)
388+ req = webob.Request.blank('/v1.0/servers/1/rescue')
389+ req.method = 'POST'
390+ body = {"rescue": {"adminPass": "AABBCC112233"}}
391+ req.body = json.dumps(body)
392+ req.content_type = 'application/json'
393+
394+ res = req.get_response(fakes.wsgi_app())
395+
396+ self.assertEqual(self.called, True)
397+ self.assertEqual(res.status_int, 200)
398+ res_body = json.loads(res.body)
399+ self.assertTrue('adminPass' in res_body)
400+ self.assertEqual('AABBCC112233', res_body['adminPass'])
401
402 def test_rescue_raises_handled(self):
403 self.flags(allow_admin_api=True)
404@@ -3621,7 +3648,8 @@
405 self.assertEquals(response.status_int, 202)
406 response = json.loads(response.body)
407 self.assertTrue('adminPass' in response['server'])
408- self.assertEqual(16, len(response['server']['adminPass']))
409+ self.assertEqual(FLAGS.password_length,
410+ len(response['server']['adminPass']))
411
412 def test_create_instance_admin_pass_xml(self):
413 request, response, dummy = \
414@@ -3630,7 +3658,8 @@
415 dom = minidom.parseString(response.body)
416 server = dom.childNodes[0]
417 self.assertEquals(server.nodeName, 'server')
418- self.assertEqual(16, len(server.getAttribute('adminPass')))
419+ self.assertEqual(FLAGS.password_length,
420+ len(server.getAttribute('adminPass')))
421
422
423 class TestGetKernelRamdiskFromImage(test.TestCase):