From ca342f2048993718718a7a7435ca88b9469c9220 Mon Sep 17 00:00:00 2001
From: Sulochan Acharya <sulochan@gmail.com>
Date: Wed, 26 Aug 2015 12:02:56 +0000
Subject: [PATCH] xapi: cleanup volume sr on live migration rollback

On the destination we currently attach any volume
that a live migrating instance might have, however,
when an error occurs after the volume attach, the rollback
does not clean it up, leaving the volume on destination.

This patch simply ensures that sr_forget is called for
any bdm that an instance might have.

Also, fixes xapi test to not use None as dbm param
for live migration rollback, since bdm cant be None
here.

Change-Id: I23c5bf4aa44c65e12f0403450b4c7c37dee648ce
Closes-bug: #1488884
---
 nova/tests/unit/virt/xenapi/test_vmops.py  | 25 ++++++++++++++++++++++
 nova/tests/unit/virt/xenapi/test_xenapi.py |  2 +-
 nova/virt/xenapi/driver.py                 |  7 ++++--
 nova/virt/xenapi/vmops.py                  | 17 +++++++++++++++
 4 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/nova/tests/unit/virt/xenapi/test_vmops.py b/nova/tests/unit/virt/xenapi/test_vmops.py
index ead049ea9032..c6b75f50e557 100644
--- a/nova/tests/unit/virt/xenapi/test_vmops.py
+++ b/nova/tests/unit/virt/xenapi/test_vmops.py
@@ -1174,6 +1174,31 @@ class LiveMigrateHelperTestCase(VMOpsTestBase):
                                                  "sr_uuid")
 
 
+class RollbackLiveMigrateDestinationTestCase(VMOpsTestBase):
+    @mock.patch.object(volume_utils, 'find_sr_by_uuid', return_value='sr_ref')
+    @mock.patch.object(volume_utils, 'forget_sr')
+    def test_rollback_dest_calls_sr_forget(self, forget_sr, sr_ref):
+        block_device_info = {'block_device_mapping': [{'connection_info':
+                                {'data': {'volume_id': 'fake-uuid',
+                                          'target_iqn': 'fake-iqn',
+                                          'target_portal': 'fake-portal'}}}]}
+        self.vmops.rollback_live_migration_at_destination('instance',
+                                                          block_device_info)
+        forget_sr.assert_called_once_with(self.vmops._session, 'sr_ref')
+
+    @mock.patch.object(volume_utils, 'forget_sr')
+    @mock.patch.object(volume_utils, 'find_sr_by_uuid',
+                       side_effect=test.TestingException)
+    def test_rollback_dest_handles_exception(self, find_sr_ref, forget_sr):
+        block_device_info = {'block_device_mapping': [{'connection_info':
+                                {'data': {'volume_id': 'fake-uuid',
+                                          'target_iqn': 'fake-iqn',
+                                          'target_portal': 'fake-portal'}}}]}
+        self.vmops.rollback_live_migration_at_destination('instance',
+                                                          block_device_info)
+        self.assertFalse(forget_sr.called)
+
+
 @mock.patch.object(vmops.VMOps, '_resize_ensure_vm_is_shutdown')
 @mock.patch.object(vmops.VMOps, '_apply_orig_vm_name_label')
 @mock.patch.object(vmops.VMOps, '_update_instance_progress')
diff --git a/nova/tests/unit/virt/xenapi/test_xenapi.py b/nova/tests/unit/virt/xenapi/test_xenapi.py
index 1ac71f119c86..28b50ac5ea50 100644
--- a/nova/tests/unit/virt/xenapi/test_xenapi.py
+++ b/nova/tests/unit/virt/xenapi/test_xenapi.py
@@ -3814,7 +3814,7 @@ class XenAPILiveMigrateTestCase(stubs.XenAPITestBaseNoDB):
 
         with mock.patch.object(conn, "destroy") as mock_destroy:
             conn.rollback_live_migration_at_destination("context",
-                    "instance", [], None)
+                    "instance", [], {'block_device_mapping': []})
             self.assertFalse(mock_destroy.called)
 
 
diff --git a/nova/virt/xenapi/driver.py b/nova/virt/xenapi/driver.py
index d1fc5c378e7e..785dc46e6a76 100644
--- a/nova/virt/xenapi/driver.py
+++ b/nova/virt/xenapi/driver.py
@@ -550,8 +550,11 @@ class XenAPIDriver(driver.ComputeDriver):
         # NOTE(johngarbutt) Destroying the VM is not appropriate here
         # and in the cases where it might make sense,
         # XenServer has already done it.
-        # TODO(johngarbutt) investigate if any cleanup is required here
-        pass
+        # NOTE(sulo): The only cleanup we do explicitly is to forget
+        # any volume that was attached to the destination during
+        # live migration. XAPI should take care of all other cleanup.
+        self._vmops.rollback_live_migration_at_destination(instance,
+                                                           block_device_info)
 
     def pre_live_migration(self, context, instance, block_device_info,
                            network_info, disk_info, migrate_data=None):
diff --git a/nova/virt/xenapi/vmops.py b/nova/virt/xenapi/vmops.py
index a337f0eceee0..bf7039113bdb 100644
--- a/nova/virt/xenapi/vmops.py
+++ b/nova/virt/xenapi/vmops.py
@@ -2273,6 +2273,23 @@ class VMOps(object):
         vm_ref = self._get_vm_opaque_ref(instance)
         vm_utils.strip_base_mirror_from_vdis(self._session, vm_ref)
 
+    def rollback_live_migration_at_destination(self, instance,
+                                               block_device_info):
+        bdms = block_device_info['block_device_mapping'] or []
+
+        for bdm in bdms:
+            conn_data = bdm['connection_info']['data']
+            uuid, label, params = volume_utils.parse_sr_info(conn_data)
+            try:
+                sr_ref = volume_utils.find_sr_by_uuid(self._session,
+                                                      uuid)
+
+                if sr_ref:
+                    volume_utils.forget_sr(self._session, sr_ref)
+            except Exception:
+                LOG.exception(_LE('Failed to forget the SR for volume %s'),
+                              params['id'], instance=instance)
+
     def get_per_instance_usage(self):
         """Get usage info about each active instance."""
         usage = {}