From be22375255e471bd187f98972210e79e9b7eafcd Mon Sep 17 00:00:00 2001
From: ChangBo Guo(gcb) <eric.guo@easystack.cn>
Date: Fri, 14 Nov 2014 13:42:41 +0800
Subject: [PATCH] libvirt: clean instance's directory when block migration fails

In method pre_live_migration, we create instance's directory at destination
host during block live migration. If some exceptions happen before creating
domain succesfully like failure of connecting volume to destination host,
then we can't destroy instance in method rollback_live_migration_at_destination.
In this case we don't remove the instance's directory, this will lead an
DestinationDiskExists when live migration same instance to same destination.
This commit ensure the instance's directory is always cleaned at destination
host. This commit also extracts instance path calculation into a utility function.

Closes-Bug: #1392947
Change-Id: If86e73b07f17d28078ee48983e9e92aafb30d913
---

Index: nova-2014.2.4.dev84/nova/virt/libvirt/driver.py
===================================================================
--- nova-2014.2.4.dev84.orig/nova/virt/libvirt/driver.py
+++ nova-2014.2.4.dev84/nova/virt/libvirt/driver.py
@@ -5513,8 +5513,21 @@ class LibvirtDriver(driver.ComputeDriver
                                                destroy_disks=True,
                                                migrate_data=None):
         """Clean up destination node after a failed live migration."""
-        self.destroy(context, instance, network_info, block_device_info,
-                     destroy_disks, migrate_data)
+        try:
+            self.destroy(context, instance, network_info, block_device_info,
+                         destroy_disks, migrate_data)
+        finally:
+            # NOTE(gcb): Failed block live migration may leave instance
+            # directory at destination node, ensure it is always deleted.
+            is_shared_instance_path = True
+            if migrate_data:
+                is_shared_instance_path = migrate_data.get(
+                        'is_shared_instance_path', True)
+            if not is_shared_instance_path:
+                instance_dir = libvirt_utils.get_instance_path_at_destination(
+                                instance, migrate_data)
+                if os.path.exists(instance_dir):
+                        shutil.rmtree(instance_dir)
 
     def pre_live_migration(self, context, instance, block_device_info,
                            network_info, disk_info, migrate_data=None):
@@ -5523,14 +5536,12 @@ class LibvirtDriver(driver.ComputeDriver
         is_shared_block_storage = True
         is_shared_instance_path = True
         is_block_migration = True
-        instance_relative_path = None
         if migrate_data:
             is_shared_block_storage = migrate_data.get(
                     'is_shared_block_storage', True)
             is_shared_instance_path = migrate_data.get(
                     'is_shared_instance_path', True)
             is_block_migration = migrate_data.get('block_migration', True)
-            instance_relative_path = migrate_data.get('instance_relative_path')
 
         if not (is_shared_instance_path and is_shared_block_storage):
             # NOTE(dims): Using config drive with iso format does not work
@@ -5542,14 +5553,8 @@ class LibvirtDriver(driver.ComputeDriver
                     raise exception.NoLiveMigrationForConfigDriveInLibVirt()
 
         if not is_shared_instance_path:
-            # NOTE(mikal): this doesn't use libvirt_utils.get_instance_path
-            # because we are ensuring that the same instance directory name
-            # is used as was at the source
-            if instance_relative_path:
-                instance_dir = os.path.join(CONF.instances_path,
-                                            instance_relative_path)
-            else:
-                instance_dir = libvirt_utils.get_instance_path(instance)
+            instance_dir = libvirt_utils.get_instance_path_at_destination(
+                            instance, migrate_data)
 
             if os.path.exists(instance_dir):
                 raise exception.DestinationDiskExists(path=instance_dir)
Index: nova-2014.2.4.dev84/nova/virt/libvirt/utils.py
===================================================================
--- nova-2014.2.4.dev84.orig/nova/virt/libvirt/utils.py
+++ nova-2014.2.4.dev84/nova/virt/libvirt/utils.py
@@ -482,6 +482,33 @@ def get_instance_path(instance, forceold
     return os.path.join(CONF.instances_path, instance['uuid'])
 
 
+def get_instance_path_at_destination(instance, migrate_data=None):
+    """Get the the instance path on destination node while live migration.
+
+    This method determines the directory name for instance storage on
+    destination node, while live migration.
+
+    :param instance: the instance we want a path for
+    :param migrate_data: if not None, it is a dict which holds data
+                         required for live migration without shared
+                         storage.
+
+    :returns: a path to store information about that instance
+    """
+    instance_relative_path = None
+    if migrate_data:
+        instance_relative_path = migrate_data.get('instance_relative_path')
+    # NOTE(mikal): this doesn't use libvirt_utils.get_instance_path
+    # because we are ensuring that the same instance directory name
+    # is used as was at the source
+    if instance_relative_path:
+        instance_dir = os.path.join(CONF.instances_path,
+                                    instance_relative_path)
+    else:
+        instance_dir = get_instance_path(instance)
+    return instance_dir
+
+
 def get_arch(image_meta):
     """Determine the architecture of the guest (or host).
 
Index: nova-2014.2.4.dev84/nova/tests/virt/libvirt/fake_libvirt_utils.py
===================================================================
--- nova-2014.2.4.dev84.orig/nova/tests/virt/libvirt/fake_libvirt_utils.py
+++ nova-2014.2.4.dev84/nova/tests/virt/libvirt/fake_libvirt_utils.py
@@ -195,6 +195,11 @@ def get_instance_path(instance, forceold
                                            relative=relative)
 
 
+def get_instance_path_at_destination(instance, migrate_data=None):
+    return libvirt_utils.get_instance_path_at_destination(instance,
+                                                          migrate_data)
+
+
 def pick_disk_driver_name(hypervisor_version, is_block_dev=False):
     return "qemu"
 
Index: nova-2014.2.4.dev84/nova/tests/virt/libvirt/test_driver.py
===================================================================
--- nova-2014.2.4.dev84.orig/nova/tests/virt/libvirt/test_driver.py
+++ nova-2014.2.4.dev84/nova/tests/virt/libvirt/test_driver.py
@@ -5866,13 +5866,49 @@ class LibvirtConnTestCase(test.TestCase)
                           recover_method=fake_recover_method,
                           migrate_data=migrate_data)
 
-    def test_rollback_live_migration_at_destination(self):
+    @mock.patch('shutil.rmtree')
+    @mock.patch('os.path.exists', return_value=True)
+    @mock.patch('nova.virt.libvirt.utils.get_instance_path_at_destination')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.destroy')
+    def test_rollback_live_migration_at_dest_not_shared(self, mock_destroy,
+                                                        mock_get_instance_path,
+                                                        mock_exist,
+                                                        mock_shutil
+                                                        ):
+        # destroy method may raise InstanceTerminationFailure or
+        # InstancePowerOffFailure, here use their base class Invalid.
+        mock_destroy.side_effect = exception.Invalid(reason='just test')
+        fake_instance_path = os.path.join(cfg.CONF.instances_path,
+                                          '/fake_instance_uuid')
+        mock_get_instance_path.return_value = fake_instance_path
         conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
-        with mock.patch.object(conn, "destroy") as mock_destroy:
-            conn.rollback_live_migration_at_destination("context",
-                    "instance", [], None, True, None)
-            mock_destroy.assert_called_once_with("context",
-                    "instance", [], None, True, None)
+
+        migrate_data = {'is_shared_instance_path': False}
+        self.assertRaises(exception.Invalid,
+                          conn.rollback_live_migration_at_destination,
+                          "context", "instance", [], None, True, migrate_data)
+        mock_exist.assert_called_once_with(fake_instance_path)
+        mock_shutil.assert_called_once_with(fake_instance_path)
+
+    @mock.patch('shutil.rmtree')
+    @mock.patch('os.path.exists')
+    @mock.patch('nova.virt.libvirt.utils.get_instance_path_at_destination')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.destroy')
+    def test_rollback_live_migration_at_dest_shared(self, mock_destroy,
+                                                    mock_get_instance_path,
+                                                    mock_exist,
+                                                    mock_shutil
+                                                    ):
+        conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+
+        migrate_data = {'is_shared_instance_path': True}
+        conn.rollback_live_migration_at_destination("context", "instance", [],
+                                                    None, True, migrate_data)
+        mock_destroy.assert_called_once_with("context", "instance", [],
+                                             None, True, migrate_data)
+        self.assertFalse(mock_get_instance_path.called)
+        self.assertFalse(mock_exist.called)
+        self.assertFalse(mock_shutil.called)
 
     def _do_test_create_images_and_backing(self, disk_type):
         conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
Index: nova-2014.2.4.dev84/nova/tests/virt/libvirt/test_utils.py
===================================================================
--- nova-2014.2.4.dev84.orig/nova/tests/virt/libvirt/test_utils.py
+++ nova-2014.2.4.dev84/nova/tests/virt/libvirt/test_utils.py
@@ -311,3 +311,24 @@ ID        TAG                 VM SIZE
 
     def test_valid_hostname_bad(self):
         self.assertFalse(libvirt_utils.is_valid_hostname("foo/?com=/bin/sh"))
+
+    def test_get_instance_path_at_destination(self):
+        instance = dict(name='fake_inst', uuid='fake_uuid')
+
+        migrate_data = None
+        inst_path_at_dest = libvirt_utils.get_instance_path_at_destination(
+            instance, migrate_data)
+        expected_path = os.path.join(CONF.instances_path, instance['uuid'])
+        self.assertEqual(expected_path, inst_path_at_dest)
+
+        migrate_data = {}
+        inst_path_at_dest = libvirt_utils.get_instance_path_at_destination(
+            instance, migrate_data)
+        expected_path = os.path.join(CONF.instances_path, instance['uuid'])
+        self.assertEqual(expected_path, inst_path_at_dest)
+
+        migrate_data = dict(instance_relative_path='fake_relative_path')
+        inst_path_at_dest = libvirt_utils.get_instance_path_at_destination(
+            instance, migrate_data)
+        expected_path = os.path.join(CONF.instances_path, 'fake_relative_path')
+        self.assertEqual(expected_path, inst_path_at_dest)
