From 52f6981aa35e27c48587ad2320891db8364edd02 Mon Sep 17 00:00:00 2001 From: Kravchenko Pavel Date: Tue, 9 Apr 2013 10:57:53 +0300 Subject: Evacuated instance disk not deleted Adds disk cleanup in case instance is not on shared storage. Added new methods to compute.rpc and virt.driver interface to validate that instance files located on shared storage on remote compute node. Fixes bug 1156269 Change-Id: Ia4cc601d0d824ba04f595df96461cfa85ad3b90c --- nova/compute/manager.py | 44 +++++++++++++- nova/compute/rpcapi.py | 8 +++ nova/tests/compute/test_compute.py | 117 ++++++++++++++++++++++++++++++++++++- nova/virt/driver.py | 27 +++++++++ nova/virt/libvirt/driver.py | 19 ++++++ 5 files changed, 211 insertions(+), 4 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 19c8ee1a2..687594ab9 100755 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -324,7 +324,7 @@ class ComputeVirtAPI(virtapi.VirtAPI): class ComputeManager(manager.SchedulerDependentManager): """Manages the running instances from creation to destruction.""" - RPC_API_VERSION = '2.27' + RPC_API_VERSION = '2.28' def __init__(self, compute_driver=None, *args, **kwargs): """Load configuration options and connect to the hypervisor.""" @@ -440,14 +440,14 @@ class ComputeManager(manager.SchedulerDependentManager): '%(instance_host)s) is not equal to our ' 'host (%(our_host)s).'), locals(), instance=instance) - # TODO(deva): detect if instance's disk is shared or local, - # and destroy if it is local. destroy_disks = False try: network_info = self._get_instance_nw_info(context, instance) bdi = self._get_instance_volume_block_device_info(context, instance) + destroy_disks = not (self._is_instance_storage_shared( + context, instance)) except exception.InstanceNotFound: network_info = network_model.NetworkInfo() bdi = {} @@ -460,6 +460,32 @@ class ComputeManager(manager.SchedulerDependentManager): self._legacy_nw_info(network_info), bdi, destroy_disks) + def _is_instance_storage_shared(self, context, instance): + shared_storage = True + data = None + try: + data = self.driver.check_instance_shared_storage_local(context, + instance) + if data: + shared_storage = (self.compute_rpcapi. + check_instance_shared_storage(context, + instance, + data)) + except NotImplementedError: + LOG.warning(_('Hypervisor driver does not support ' + 'instance shared storage check, ' + 'assuming it\'s not on shared storage'), + instance=instance) + shared_storage = False + except Exception as e: + LOG.exception(_('Failed to check if instance shared'), + instance=instance) + finally: + if data: + self.driver.check_instance_shared_storage_cleanup(context, + data) + return shared_storage + def _init_instance(self, context, instance): '''Initialize this instance during service init.''' closing_vm_states = (vm_states.DELETED, @@ -3035,6 +3061,18 @@ class ComputeManager(manager.SchedulerDependentManager): except IndexError: raise exception.NotFound(_("Host %(host)s not found") % locals()) + @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id()) + def check_instance_shared_storage(self, ctxt, data): + """Check if the instance files are shared + + :param context: security context + :param data: result of driver.check_instance_shared_storage_local + + Returns True if instance disks located on shared storage and + False otherwise. + """ + return self.driver.check_instance_shared_storage_remote(ctxt, data) + @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id()) def check_can_live_migrate_destination(self, ctxt, instance, block_migration=False, diff --git a/nova/compute/rpcapi.py b/nova/compute/rpcapi.py index 697c85eab..1ddadde75 100644 --- a/nova/compute/rpcapi.py +++ b/nova/compute/rpcapi.py @@ -165,6 +165,7 @@ class ComputeAPI(nova.openstack.common.rpc.proxy.RpcProxy): vnc on the correct port 2.27 - Adds 'reservations' to terminate_instance() and soft_delete_instance() + 2.28 - Adds check_instance_shared_storage() ''' # @@ -247,6 +248,13 @@ class ComputeAPI(nova.openstack.common.rpc.proxy.RpcProxy): topic=_compute_topic(self.topic, ctxt, None, instance)) + def check_instance_shared_storage(self, ctxt, instance, data): + instance_p = jsonutils.to_primitive(instance) + return self.call(ctxt, self.make_msg('check_instance_shared_storage', + data=data), + topic=_compute_topic(self.topic, ctxt, None, + instance)) + def confirm_resize(self, ctxt, instance, migration, host, reservations=None, cast=True): rpc_method = self.cast if cast else self.call diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py index 44b004ed2..f15a9e1c3 100644 --- a/nova/tests/compute/test_compute.py +++ b/nova/tests/compute/test_compute.py @@ -3833,7 +3833,7 @@ class ComputeTestCase(BaseTestCase): instance = self._create_fake_instance(params) self.compute._instance_update(self.context, instance['uuid']) - def test_destroy_evacuated_instances(self): + def test_destroy_evacuated_instance_on_shared_storage(self): fake_context = context.get_admin_context() # instances in central db @@ -3858,6 +3858,8 @@ class ComputeTestCase(BaseTestCase): '_get_instance_nw_info') self.mox.StubOutWithMock(self.compute, '_get_instance_volume_block_device_info') + self.mox.StubOutWithMock(self.compute, + '_is_instance_storage_shared') self.mox.StubOutWithMock(self.compute, '_legacy_nw_info') self.mox.StubOutWithMock(self.compute.driver, 'destroy') @@ -3868,6 +3870,8 @@ class ComputeTestCase(BaseTestCase): 'fake_network_info') self.compute._get_instance_volume_block_device_info( fake_context, evacuated_instance).AndReturn('fake_bdi') + self.compute._is_instance_storage_shared(fake_context, + evacuated_instance).AndReturn(True) self.compute._legacy_nw_info('fake_network_info').AndReturn( 'fake_legacy_network_info') self.compute.driver.destroy(evacuated_instance, @@ -3878,6 +3882,117 @@ class ComputeTestCase(BaseTestCase): self.mox.ReplayAll() self.compute._destroy_evacuated_instances(fake_context) + def test_destroy_evacuated_instance_with_disks(self): + fake_context = context.get_admin_context() + + # instances in central db + instances = [ + # those are still related to this host + jsonutils.to_primitive(self._create_fake_instance( + {'host': self.compute.host})), + jsonutils.to_primitive(self._create_fake_instance( + {'host': self.compute.host})), + jsonutils.to_primitive(self._create_fake_instance( + {'host': self.compute.host})) + ] + + # those are already been evacuated to other host + evacuated_instance = self._create_fake_instance({'host': 'otherhost'}) + + instances.append(evacuated_instance) + + self.mox.StubOutWithMock(self.compute, + '_get_instances_on_driver') + self.mox.StubOutWithMock(self.compute, + '_get_instance_nw_info') + self.mox.StubOutWithMock(self.compute, + '_get_instance_volume_block_device_info') + self.mox.StubOutWithMock(self.compute.driver, + 'check_instance_shared_storage_local') + self.mox.StubOutWithMock(self.compute.compute_rpcapi, + 'check_instance_shared_storage') + self.mox.StubOutWithMock(self.compute.driver, + 'check_instance_shared_storage_cleanup') + self.mox.StubOutWithMock(self.compute, '_legacy_nw_info') + self.mox.StubOutWithMock(self.compute.driver, 'destroy') + + self.compute._get_instances_on_driver(fake_context).AndReturn( + instances) + self.compute._get_instance_nw_info(fake_context, + evacuated_instance).AndReturn( + 'fake_network_info') + self.compute._get_instance_volume_block_device_info( + fake_context, evacuated_instance).AndReturn('fake_bdi') + self.compute.driver.check_instance_shared_storage_local(fake_context, + evacuated_instance).AndReturn({'filename': 'tmpfilename'}) + self.compute.compute_rpcapi.check_instance_shared_storage(fake_context, + evacuated_instance, + {'filename': 'tmpfilename'}).AndReturn(False) + self.compute.driver.check_instance_shared_storage_cleanup(fake_context, + {'filename': 'tmpfilename'}) + self.compute._legacy_nw_info('fake_network_info').AndReturn( + 'fake_legacy_network_info') + self.compute.driver.destroy(evacuated_instance, + 'fake_legacy_network_info', + 'fake_bdi', + True) + + self.mox.ReplayAll() + self.compute._destroy_evacuated_instances(fake_context) + + def test_destroy_evacuated_instance_not_implemented(self): + fake_context = context.get_admin_context() + + # instances in central db + instances = [ + # those are still related to this host + jsonutils.to_primitive(self._create_fake_instance( + {'host': self.compute.host})), + jsonutils.to_primitive(self._create_fake_instance( + {'host': self.compute.host})), + jsonutils.to_primitive(self._create_fake_instance( + {'host': self.compute.host})) + ] + + # those are already been evacuated to other host + evacuated_instance = self._create_fake_instance({'host': 'otherhost'}) + + instances.append(evacuated_instance) + + self.mox.StubOutWithMock(self.compute, + '_get_instances_on_driver') + self.mox.StubOutWithMock(self.compute, + '_get_instance_nw_info') + self.mox.StubOutWithMock(self.compute, + '_get_instance_volume_block_device_info') + self.mox.StubOutWithMock(self.compute.driver, + 'check_instance_shared_storage_local') + self.mox.StubOutWithMock(self.compute.compute_rpcapi, + 'check_instance_shared_storage') + self.mox.StubOutWithMock(self.compute.driver, + 'check_instance_shared_storage_cleanup') + self.mox.StubOutWithMock(self.compute, '_legacy_nw_info') + self.mox.StubOutWithMock(self.compute.driver, 'destroy') + + self.compute._get_instances_on_driver(fake_context).AndReturn( + instances) + self.compute._get_instance_nw_info(fake_context, + evacuated_instance).AndReturn( + 'fake_network_info') + self.compute._get_instance_volume_block_device_info( + fake_context, evacuated_instance).AndReturn('fake_bdi') + self.compute.driver.check_instance_shared_storage_local(fake_context, + evacuated_instance).AndRaise(NotImplementedError()) + self.compute._legacy_nw_info('fake_network_info').AndReturn( + 'fake_legacy_network_info') + self.compute.driver.destroy(evacuated_instance, + 'fake_legacy_network_info', + 'fake_bdi', + True) + + self.mox.ReplayAll() + self.compute._destroy_evacuated_instances(fake_context) + def test_init_host(self): our_host = self.compute.host fake_context = 'fake-context' diff --git a/nova/virt/driver.py b/nova/virt/driver.py index 827a2b782..b3854aba0 100755 --- a/nova/virt/driver.py +++ b/nova/virt/driver.py @@ -487,6 +487,33 @@ class ComputeDriver(object): """ raise NotImplementedError() + def check_instance_shared_storage_local(self, ctxt, instance): + """Check if instance files located on shared storage. + + This runs check on the destination host, and then calls + back to the source host to check the results. + + :param ctxt: security context + :param instance: nova.db.sqlalchemy.models.Instance + """ + raise NotImplementedError() + + def check_instance_shared_storage_remote(self, ctxt, data): + """Check if instance files located on shared storage. + + :param context: security context + :param data: result of check_instance_shared_storage_local + """ + raise NotImplementedError() + + def check_instance_shared_storage_cleanup(self, ctxt, data): + """Do cleanup on host after check_instance_shared_storage calls + + :param ctxt: security context + :param data: result of check_instance_shared_storage_local + """ + pass + def check_can_live_migrate_destination(self, ctxt, instance_ref, src_compute_info, dst_compute_info, block_migration=False, diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 498ef1d82..4c1df80b8 100755 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -2820,6 +2820,25 @@ class LibvirtDriver(driver.ComputeDriver): 'disk_available_least': _get_disk_available_least()} return dic + def check_instance_shared_storage_local(self, ctxt, instance): + dirpath = libvirt_utils.get_instance_path(instance) + + if not os.path.exists(dirpath): + return None + + fd, tmp_file = tempfile.mkstemp(dir=dirpath) + LOG.debug(_("Creating tmpfile %s to verify with other " + "compute node that the instance is on " + "the same shared storage.") % tmp_file) + os.close(fd) + return {"filename": tmp_file} + + def check_instance_shared_storage_remote(self, ctxt, data): + return os.path.exists(data['filename']) + + def check_instance_shared_storage_cleanup(self, ctxt, data): + utils.delete_if_exists(data["filename"]) + def check_can_live_migrate_destination(self, ctxt, instance_ref, src_compute_info, dst_compute_info, block_migration=False, -- cgit