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

@@ -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')

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)
 
 

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):

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 = {}