Merge lp:~cbehrens/nova/passwd-with-spawn into lp:~hudson-openstack/nova/trunk

[Chris Behrens]

During the API create call, the API would kick off a build and then loop in a greenthread waiting for the scheduler to pick a host for the instance. After API would see a host was picked, it would cast to the compute node's set_admin_password method.

The API server really should not have to do this. The password to set should be pushed along with the build request, instead. The compute node can then set the password after it detects the instance has booted. This removes a greenthread from the API server, a loop that constantly checks the DB for the host, and finally a cast to the compute node.

[Ed Leafe (ed-leafe)]

Line 86 of the diff:
   instance_ref.admin_password = ...
should be:
   instance_ref.admin_pass = ...

review: Needs Fixing

[Josh Kearney (jk0)]

This works great, but for some reason the password is not getting set in instances admin_pass column. I don't think this was working before your fix, but it might be something to look into.

[Chris Behrens (cbehrens)]

Ok. The DB is now updated with the admin_pass on create. Apparently this was never implemented before. I pushed the DB update down into where the password is actually changed, and the DB is only updated on success.

Also fixed a problem where password reset API call could cause compute to loop forever trying to reset a password is the password reset returned an error.

BTW: These changes speed up the API return to 'nova boot' a lot!

[Johannes Erdfelt (johannes.erdfelt)]

lgtm

review: Approve

[Ed Leafe (ed-leafe)]

The changes look good.

review: Approve

[Josh Kearney (jk0)]

Tested new changes. Works great.

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~cbehrens/nova/passwd-with-spawn into lp:nova failed. Below is the output from the failed tests.

AccountsTest
    test_account_create OK
    test_account_delete OK
    test_account_update OK
    test_get_account OK
AdminAPITest
    test_admin_disabled OK
    test_admin_enabled OK
APITest
    test_exceptions_are_converted_to_faults OK
Test
    test_authorize_token OK
    test_authorize_user OK
    test_bad_token OK
    test_bad_user_bad_key OK
    test_bad_user_good_key OK
    test_no_user OK
    test_token_expiry OK
TestFunctional
    test_token_doesnotexist OK
    test_token_expiry OK
TestLimiter
    test_authorize_token OK
LimiterTest
    test_limiter_custom_max_limit OK
    test_limiter_limit_and_offset OK
    test_limiter_limit_medium OK
    test_limiter_limit_over_max OK
    test_limiter_limit_zero OK
    test_limiter_negative_limit OK
    test_limiter_negative_offset OK
    test_limiter_nothing OK
    test_limiter_offset_bad OK
    test_limiter_offset_blank OK
    test_limiter_offset_medium OK
    test_limiter_offset_over_max OK
    test_limiter_offset_zero OK
ActionExtensionTest
    test_extended_action OK
    test_invalid_action OK
    test_invalid_action_body OK
ExtensionControllerTest
    test_get_by_alias OK
    test_index OK
ExtensionManagerTest
    test_get_resources OK
RequestExtensionTest
    test_get_resources_with_mgr OK
    test_get_resources_with_stub_mgr OK
ResourceExtensionTest
    test_get_resources OK
    test_get_resources_with_controller OK
    test_no_extension_present OK
TestFaults
    test_400_fault_json OK
    test_400_fault_xml OK
...

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-05-18 15:31:41 +0000
+++ nova/api/openstack/servers.py	2011-05-25 20:09:36 +0000
@@ -180,7 +180,8 @@
                 key_name=key_name,
                 key_data=key_data,
                 metadata=env['server'].get('metadata', {}),
-                injected_files=injected_files)
+                injected_files=injected_files,
+                admin_password=password)
         except quota.QuotaError as error:
             self._handle_quota_error(error)
 
@@ -190,8 +191,6 @@
         builder = self._get_view_builder(req)
         server = builder.build(inst, is_detail=True)
         server['server']['adminPass'] = password
-        self.compute_api.set_admin_password(context, server['server']['id'],
-                                            password)
         return server
 
     def _deserialize_create(self, request):
@@ -608,7 +607,6 @@
 
     def _parse_update(self, context, server_id, inst_dict, update_dict):
         if 'adminPass' in inst_dict['server']:
-            update_dict['admin_pass'] = inst_dict['server']['adminPass']
             self.compute_api.set_admin_password(context, server_id,
                     inst_dict['server']['adminPass'])
 

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-05-24 22:51:29 +0000
+++ nova/compute/api.py	2011-05-25 20:09:36 +0000
@@ -134,7 +134,8 @@
                display_name='', display_description='',
                key_name=None, key_data=None, security_group='default',
                availability_zone=None, user_data=None, metadata={},
-               injected_files=None):
+               injected_files=None,
+               admin_password=None):
         """Create the number and type of instances requested.
 
         Verifies that quota and other arguments are valid.
@@ -264,7 +265,8 @@
                                "instance_id": instance_id,
                                "instance_type": instance_type,
                                "availability_zone": availability_zone,
-                               "injected_files": injected_files}})
+                               "injected_files": injected_files,
+                               "admin_password": admin_password}})
 
         for group_id in security_groups:
             self.trigger_security_group_members_refresh(elevated, group_id)
@@ -503,15 +505,6 @@
         raise exception.Error(_("Unable to find host for Instance %s")
                                 % instance_id)
 
-    def _set_admin_password(self, context, instance_id, password):
-        """Set the root/admin password for the given instance."""
-        host = self._find_host(context, instance_id)
-
-        rpc.cast(context,
-                 self.db.queue_get_for(context, FLAGS.compute_topic, host),
-                 {"method": "set_admin_password",
-                  "args": {"instance_id": instance_id, "new_pass": password}})
-
     def snapshot(self, context, instance_id, name):
         """Snapshot the given instance.
 
@@ -665,8 +658,12 @@
 
     def set_admin_password(self, context, instance_id, password=None):
         """Set the root/admin password for the given instance."""
-        eventlet.spawn_n(self._set_admin_password, context, instance_id,
-                                                  password)
+        host = self._find_host(context, instance_id)
+
+        rpc.cast(context,
+                 self.db.queue_get_for(context, FLAGS.compute_topic, host),
+                 {"method": "set_admin_password",
+                  "args": {"instance_id": instance_id, "new_pass": password}})
 
     def inject_file(self, context, instance_id):
         """Write a file to the given instance."""

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-05-17 19:50:43 +0000
+++ nova/compute/manager.py	2011-05-25 20:09:36 +0000
@@ -221,6 +221,7 @@
         context = context.elevated()
         instance_ref = self.db.instance_get(context, instance_id)
         instance_ref.injected_files = kwargs.get('injected_files', [])
+        instance_ref.admin_pass = kwargs.get('admin_password', None)
         if instance_ref['name'] in self.driver.list_instances():
             raise exception.Error(_("Instance has already been created"))
         LOG.audit(_("instance %s: starting..."), instance_id,
@@ -405,22 +406,28 @@
     @exception.wrap_exception
     @checks_instance_lock
     def set_admin_password(self, context, instance_id, new_pass=None):
-        """Set the root/admin password for an instance on this host."""
+        """Set the root/admin password for an instance on this host.
+
+        This is generally only called by API password resets after an
+        image has been built.
+        """
+
         context = context.elevated()
 
         if new_pass is None:
             # Generate a random password
             new_pass = utils.generate_password(FLAGS.password_length)
 
-        while True:
+        max_tries = 10
+
+        for i in xrange(max_tries):
             instance_ref = self.db.instance_get(context, instance_id)
             instance_id = instance_ref["id"]
             instance_state = instance_ref["state"]
             expected_state = power_state.RUNNING
 
             if instance_state != expected_state:
-                time.sleep(5)
-                continue
+                raise exception.Error(_('Instance is not running'))
             else:
                 try:
                     self.driver.set_admin_password(instance_ref, new_pass)
@@ -436,6 +443,12 @@
                 except Exception, e:
                     # Catch all here because this could be anything.
                     LOG.exception(e)
+                    if i == max_tries - 1:
+                        # At some point this exception may make it back
+                        # to the API caller, and we don't want to reveal
+                        # too much.  The real exception is logged above
+                        raise exception.Error(_('Internal error'))
+                    time.sleep(1)
                     continue
 
     @exception.wrap_exception

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-05-18 15:31:41 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-05-25 20:09:36 +0000
@@ -774,8 +774,7 @@
 
         def server_update(context, id, params):
             filtered_dict = dict(
-                display_name='server_test',
-                admin_pass='bacon',
+                display_name='server_test'
             )
             self.assertEqual(params, filtered_dict)
             return filtered_dict

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-05-25 14:57:52 +0000
+++ nova/virt/xenapi/vmops.py	2011-05-25 20:09:36 +0000
@@ -202,6 +202,13 @@
                 for path, contents in instance.injected_files:
                     LOG.debug(_("Injecting file path: '%s'") % path)
                     self.inject_file(instance, path, contents)
+
+        def _set_admin_password():
+            admin_password = instance.admin_pass
+            if admin_password:
+                LOG.debug(_("Setting admin password"))
+                self.set_admin_password(instance, admin_password)
+
         # NOTE(armando): Do we really need to do this in virt?
         # NOTE(tr3buchet): not sure but wherever we do it, we need to call
         #                  reset_network afterwards
@@ -214,6 +221,7 @@
                     LOG.debug(_('Instance %s: booted'), instance_name)
                     timer.stop()
                     _inject_files()
+                    _set_admin_password()
                     return True
             except Exception, exc:
                 LOG.warn(exc)
@@ -458,6 +466,9 @@
         # Successful return code from password is '0'
         if resp_dict['returncode'] != '0':
             raise RuntimeError(resp_dict['message'])
+        db.instance_update(context.get_admin_context(),
+                                  instance['id'],
+                                  dict(admin_pass=new_pass))
         return resp_dict['message']
 
     def inject_file(self, instance, path, contents):