Merge "Evacuated instance disk not deleted"

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,
@@ -3036,6 +3062,18 @@ class ComputeManager(manager.SchedulerDependentManager):
             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,
                                            disk_over_commit=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 7785d13e2..c8494603e 100755
--- a/nova/virt/libvirt/driver.py
+++ b/nova/virt/libvirt/driver.py
@@ -2826,6 +2826,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,