From 7f788eb1672cf1731817f83e6987f7b128599154 Mon Sep 17 00:00:00 2001 From: Aaron Lee Date: Wed, 22 Feb 2012 13:30:24 -0600 Subject: refactor a conditional for testing and understanding _cleanup_running_deleted_instances was getting difficult to understand, so I extracted a method that contained the calculation about what is to be deleted. Also, added tests for that method. update: _shutdown_instance has an arity of three, the existing code gives four arguments, one was passed through to driver.destroy, which now has a default argument that defaults to True, the passed value. update 2: Doh! pep8 Change-Id: I1a32512a4e0d80ba4dcc911b96790c29c1f36710 --- nova/compute/manager.py | 67 +++++++++++++++++++++++----------------------- nova/tests/test_compute.py | 29 +++++++++++++++++++- 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): -- cgit