Removes retry of set_admin_password

* An RPC timeout may occur if an agent is missing and set_admin_password is
invoked. This causes 500 errors in the OpenStack API.
* Implemented a 501 error in API if the password set fails.
* Modified xenapi agent to use NotImplementedError instead of Exception in
  set_admin_password.
* Updated test code around set_admin_password to accept different exceptions.
* Fixes bug 1061045

Change-Id: If7fab56c20f12e0490f4774e00004ed1d94242b9

4 files changed, 59 insertions, 61 deletions

diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py
index f0fdb5a15..a6ad7a93a 100644
--- a/nova/api/openstack/compute/servers.py
+++ b/nova/api/openstack/compute/servers.py
@@ -1163,7 +1163,11 @@ class Controller(wsgi.Controller):
             msg = _("Invalid adminPass")
             raise exc.HTTPBadRequest(explanation=msg)
         server = self._get_server(context, req, id)
-        self.compute_api.set_admin_password(context, server, password)
+        try:
+            self.compute_api.set_admin_password(context, server, password)
+        except NotImplementedError:
+            msg = _("Unable to set password on instance")
+            raise exc.HTTPNotImplemented(explanation=msg)
         return webob.Response(status_int=202)
 
     def _validate_metadata(self, metadata):
diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 3bf8e61ef..64931dfee 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -1591,68 +1591,57 @@ class ComputeManager(manager.SchedulerDependentManager):
         """
 
         context = context.elevated()
-
         if new_pass is None:
             # Generate a random password
             new_pass = utils.generate_password()
 
-        max_tries = 10
-
-        for i in xrange(max_tries):
-            current_power_state = self._get_power_state(context, instance)
-            expected_state = power_state.RUNNING
+        current_power_state = self._get_power_state(context, instance)
+        expected_state = power_state.RUNNING
 
-            if current_power_state != expected_state:
-                self._instance_update(context, instance['uuid'],
+        if current_power_state != expected_state:
+            self._instance_update(context, instance['uuid'],
+                                  task_state=None,
+                                  expected_task_state=task_states.
+                                  UPDATING_PASSWORD)
+            _msg = _('Failed to set admin password. Instance %s is not'
+                     ' running') % instance["uuid"]
+            raise exception.InstancePasswordSetFailed(
+                instance=instance['uuid'], reason=_msg)
+        else:
+            try:
+                self.driver.set_admin_password(instance, new_pass)
+                LOG.audit(_("Root password set"), instance=instance)
+                self._instance_update(context,
+                                      instance['uuid'],
                                       task_state=None,
                                       expected_task_state=task_states.
-                                          UPDATING_PASSWORD)
-                _msg = _('Failed to set admin password. Instance %s is not'
-                         ' running') % instance["uuid"]
+                                      UPDATING_PASSWORD)
+            except NotImplementedError:
+                _msg = _('set_admin_password is not implemented '
+                         'by this driver or guest instance.')
+                LOG.warn(_msg, instance=instance)
+                self._instance_update(context,
+                                      instance['uuid'],
+                                      task_state=None,
+                                      expected_task_state=task_states.
+                                      UPDATING_PASSWORD)
+                raise NotImplementedError(_msg)
+            except exception.UnexpectedTaskStateError:
+                # interrupted by another (most likely delete) task
+                # do not retry
+                raise
+            except Exception, e:
+                # Catch all here because this could be anything.
+                LOG.exception(_('set_admin_password failed: %s') % e,
+                              instance=instance)
+                self._set_instance_error_state(context,
+                                               instance['uuid'])
+                # We create a new exception here so that we won't
+                # potentially reveal password information to the
+                # API caller.  The real exception is logged above
+                _msg = _('error setting admin password')
                 raise exception.InstancePasswordSetFailed(
-                        instance=instance['uuid'], reason=_msg)
-            else:
-                try:
-                    self.driver.set_admin_password(instance, new_pass)
-                    LOG.audit(_("Root password set"), instance=instance)
-                    self._instance_update(context,
-                                          instance['uuid'],
-                                          task_state=None,
-                                          expected_task_state=task_states.
-                                              UPDATING_PASSWORD)
-                    break
-                except NotImplementedError:
-                    # NOTE(dprince): if the driver doesn't implement
-                    # set_admin_password we break to avoid a loop
-                    _msg = _('set_admin_password is not implemented '
-                             'by this driver.')
-                    LOG.warn(_msg, instance=instance)
-                    self._instance_update(context,
-                                          instance['uuid'],
-                                          task_state=None,
-                                          expected_task_state=task_states.
-                                              UPDATING_PASSWORD)
-                    raise exception.InstancePasswordSetFailed(
-                            instance=instance['uuid'], reason=_msg)
-                except exception.UnexpectedTaskStateError:
-                    # interrupted by another (most likely delete) task
-                    # do not retry
-                    raise
-                except Exception, e:
-                    # Catch all here because this could be anything.
-                    LOG.exception(_('set_admin_password failed: %s') % e,
-                                  instance=instance)
-                    if i == max_tries - 1:
-                        self._set_instance_error_state(context,
-                                                       instance['uuid'])
-                        # We create a new exception here so that we won't
-                        # potentially reveal password information to the
-                        # API caller.  The real exception is logged above
-                        _msg = _('error setting admin password')
-                        raise exception.InstancePasswordSetFailed(
-                                instance=instance['uuid'], reason=_msg)
-                    time.sleep(1)
-                    continue
+                    instance=instance['uuid'], reason=_msg)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
     @reverts_task_state
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 0d9f67231..6ca548c17 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -1151,7 +1151,8 @@ class ComputeTestCase(BaseTestCase):
         self.compute.terminate_instance(self.context, instance=instance)
 
     def _do_test_set_admin_password_driver_error(self, exc, expected_vm_state,
-                                                 expected_task_state):
+                                                 expected_task_state,
+                                                 expected_exception):
         """Ensure expected exception is raised if set_admin_password fails."""
 
         def fake_sleep(_time):
@@ -1176,7 +1177,7 @@ class ComputeTestCase(BaseTestCase):
 
         #error raised from the driver should not reveal internal information
         #so a new error is raised
-        self.assertRaises(exception.InstancePasswordSetFailed,
+        self.assertRaises(expected_exception,
                           self.compute.set_admin_password,
                           self.context,
                           instance=jsonutils.to_primitive(inst_ref))
@@ -1194,9 +1195,11 @@ class ComputeTestCase(BaseTestCase):
         authorized.
         """
         exc = exception.NotAuthorized(_('Internal error'))
+        expected_exception = exception.InstancePasswordSetFailed
         self._do_test_set_admin_password_driver_error(exc,
                                                 vm_states.ERROR,
-                                                None)
+                                                None,
+                                                expected_exception)
 
     def test_set_admin_password_driver_not_implemented(self):
         """
@@ -1204,9 +1207,11 @@ class ComputeTestCase(BaseTestCase):
         implemented by driver.
         """
         exc = NotImplementedError()
+        expected_exception = NotImplementedError
         self._do_test_set_admin_password_driver_error(exc,
                                                       vm_states.ACTIVE,
-                                                      None)
+                                                      None,
+                                                      expected_exception)
 
     def test_inject_file(self):
         # Ensure we can write a file to an instance.
diff --git a/nova/virt/xenapi/agent.py b/nova/virt/xenapi/agent.py
index 61cfa9631..4345db198 100644
--- a/nova/virt/xenapi/agent.py
+++ b/nova/virt/xenapi/agent.py
@@ -185,7 +185,7 @@ class XenAPIBasedAgent(object):
         if resp['returncode'] != 'D0':
             msg = _('Failed to exchange keys: %(resp)r') % locals()
             LOG.error(msg, instance=self.instance)
-            raise Exception(msg)
+            raise NotImplementedError(msg)
 
         # Some old versions of the Windows agent have a trailing \\r\\n
         # (ie CRLF escaped) for some reason. Strip that off.
@@ -205,7 +205,7 @@ class XenAPIBasedAgent(object):
         if resp['returncode'] != '0':
             msg = _('Failed to update password: %(resp)r') % locals()
             LOG.error(msg, instance=self.instance)
-            raise Exception(msg)
+            raise NotImplementedError(msg)
 
         return resp['message']