Merge "refactor a conditional for testing and understanding"

2 files changed, 61 insertions, 35 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 5fb5ff9c6..fa02447c7 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -2274,42 +2274,41 @@ class ComputeManager(manager.SchedulerDependentManager):
         if action == "noop":
             return
 
-        present_name_labels = set(self.driver.list_instances())
-
         # NOTE(sirp): admin contexts don't ordinarily return deleted records
         with utils.temporary_mutation(context, read_deleted="yes"):
-            instances = self.db.instance_get_all_by_host(context, self.host)
-            for instance in instances:
-                present = instance.name in present_name_labels
-                erroneously_running = instance.deleted and present
-                old_enough = (not instance.deleted_at or utils.is_older_than(
-                        instance.deleted_at,
-                        FLAGS.running_deleted_instance_timeout))
-
-                if erroneously_running and old_enough:
-                    instance_id = instance['id']
-                    instance_uuid = instance['uuid']
-                    name_label = instance['name']
-
-                    if action == "log":
-                        LOG.warning(_("Detected instance  with name label "
-                                      "'%(name_label)s' which is marked as "
-                                      "DELETED but still present on host."),
-                                    locals(), instance=instance)
-
-                    elif action == 'reap':
-                        LOG.info(_("Destroying instance with name label "
-                                   "'%(name_label)s' which is marked as "
-                                   "DELETED but still present on host."),
-                                 locals(), instance=instance)
-                        self._shutdown_instance(
-                                context, instance, 'Terminating', True)
-                        self._cleanup_volumes(context, instance_id)
-                    else:
-                        raise Exception(_("Unrecognized value '%(action)s'"
-                                          " for FLAGS.running_deleted_"
-                                          "instance_action"), locals(),
-                                        instance=instance)
+            for instance in self._errored_instances(context):
+                if action == "log":
+                    LOG.warning(_("Detected instance  with name label "
+                                  "'%(name_label)s' which is marked as "
+                                  "DELETED but still present on host."),
+                                locals(), instance=instance)
+
+                elif action == 'reap':
+                    LOG.info(_("Destroying instance with name label "
+                               "'%(name_label)s' which is marked as "
+                               "DELETED but still present on host."),
+                             locals(), instance=instance)
+                    self._shutdown_instance(context, instance, 'Terminating')
+                    self._cleanup_volumes(context, instance['id'])
+                else:
+                    raise Exception(_("Unrecognized value '%(action)s'"
+                                      " for FLAGS.running_deleted_"
+                                      "instance_action"), locals(),
+                                    instance=instance)
+
+    def _running_deleted_instances(self, context):
+        def deleted_instance(instance):
+            present = instance.name in present_name_labels
+            erroneously_running = instance.deleted and present
+            old_enough = (not instance.deleted_at or utils.is_older_than(
+                instance.deleted_at,
+                FLAGS.running_deleted_instance_timeout))
+            if erroneously_running and old_enough:
+                return True
+            return False
+        present_name_labels = set(self.driver.list_instances())
+        instances = self.db.instance_get_all_by_host(context, self.host)
+        return [i for i in instances if deleted_instance(i)]
 
     @contextlib.contextmanager
     def error_out_instance_on_exception(self, context, instance_uuid):
diff --git a/nova/tests/test_compute.py b/nova/tests/test_compute.py
index 873b8662f..17f8beeed 100644
--- a/nova/tests/test_compute.py
+++ b/nova/tests/test_compute.py
@@ -23,7 +23,6 @@ from copy import copy
 import datetime
 import sys
 import time
-from webob import exc
 
 import mox
 import webob.exc
@@ -1577,6 +1576,34 @@ class ComputeTestCase(BaseTestCase):
         self.compute.add_instance_fault_from_exc(ctxt, instance_uuid,
                                                  NotImplementedError('test'))
 
+    def test_running_deleted_instances(self):
+        self.mox.StubOutWithMock(self.compute.driver, 'list_instances')
+        self.compute.driver.list_instances().AndReturn(['herp', 'derp'])
+        self.compute.host = 'host'
+
+        instance1 = mox.MockAnything()
+        instance1.name = 'herp'
+        instance1.deleted = True
+        instance1.deleted_at = "sometimeago"
+
+        instance2 = mox.MockAnything()
+        instance2.name = 'derp'
+        instance2.deleted = False
+        instance2.deleted_at = None
+
+        self.mox.StubOutWithMock(utils, 'is_older_than')
+        utils.is_older_than('sometimeago',
+                    FLAGS.running_deleted_instance_timeout).AndReturn(True)
+
+        self.mox.StubOutWithMock(self.compute.db, "instance_get_all_by_host")
+        self.compute.db.instance_get_all_by_host('context',
+                                                 'host').AndReturn(
+                                                                [instance1,
+                                                                 instance2])
+        self.mox.ReplayAll()
+        val = self.compute._running_deleted_instances('context')
+        self.assertEqual(val, [instance1])
+
 
 class ComputeAPITestCase(BaseTestCase):