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.

5 files changed, 41 insertions, 23 deletions

diff --git a/nova/api/openstack/servers.py b/nova/api/openstack/servers.py
index fcb630fae..5c10fc916 100644
--- a/nova/api/openstack/servers.py
+++ b/nova/api/openstack/servers.py
@@ -180,7 +180,8 @@ class Controller(common.OpenstackController):
                 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 @@ class Controller(common.OpenstackController):
         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 @@ class ControllerV10(Controller):
 
     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'])
 
diff --git a/nova/compute/api.py b/nova/compute/api.py
index a12b7dee5..86cd4514f 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -134,7 +134,8 @@ class API(base.Base):
                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 @@ class API(base.Base):
                                "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 @@ class API(base.Base):
         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 @@ class API(base.Base):
 
     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."""
diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 11565c25e..d1e01f275 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -221,6 +221,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         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 @@ class ComputeManager(manager.SchedulerDependentManager):
     @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 @@ class ComputeManager(manager.SchedulerDependentManager):
                 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
diff --git a/nova/tests/api/openstack/test_servers.py b/nova/tests/api/openstack/test_servers.py
index dc8815845..fbde5c9ce 100644
--- a/nova/tests/api/openstack/test_servers.py
+++ b/nova/tests/api/openstack/test_servers.py
@@ -774,8 +774,7 @@ class ServersTest(test.TestCase):
 
         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
diff --git a/nova/virt/xenapi/vmops.py b/nova/virt/xenapi/vmops.py
index aaf5585b1..be6ef48ea 100644
--- a/nova/virt/xenapi/vmops.py
+++ b/nova/virt/xenapi/vmops.py
@@ -202,6 +202,13 @@ class VMOps(object):
                 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 @@ class VMOps(object):
                     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 @@ class VMOps(object):
         # 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):