Merge "compute: Don't delete the original attachment during pre LM rollback" into stable/stein19.3.1

3 files changed, 24 insertions, 8 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index f078c66a8a..b65a96a720 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -7345,7 +7345,12 @@ class ComputeManager(manager.Manager):
         for bdm in bdms:
             try:
                 original_bdm = original_bdms_by_volid[bdm.volume_id]
-                if bdm.attachment_id and original_bdm.attachment_id:
+                # NOTE(lyarwood): Only delete the referenced attachment if it
+                # is different to the original in order to avoid accidentally
+                # removing the source host volume attachment after it has
+                # already been rolled back by a failure in pre_live_migration.
+                if (bdm.attachment_id and original_bdm.attachment_id and
+                    bdm.attachment_id != original_bdm.attachment_id):
                     # NOTE(lyarwood): 3.44 cinder api flow. Delete the
                     # attachment used by the bdm and reset it to that of
                     # the original bdm.
diff --git a/nova/tests/functional/regressions/test_bug_1889108.py b/nova/tests/functional/regressions/test_bug_1889108.py
index 8e129d8e85..fd971846bc 100644
--- a/nova/tests/functional/regressions/test_bug_1889108.py
+++ b/nova/tests/functional/regressions/test_bug_1889108.py
@@ -26,8 +26,8 @@ class TestVolAttachmentsDuringPreLiveMigration(
     """Regression test for bug 1889108.
 
     This regression test asserts that the original source volume attachments
-    are incorrectly removed during the rollback from pre_live_migration
-    failures on the destination.
+    are not removed during the rollback from pre_live_migration failures on the
+    destination.
     """
     api_major_version = 'v2.1'
     microversion = 'latest'
@@ -106,9 +106,7 @@ class TestVolAttachmentsDuringPreLiveMigration(
         server = self.api.get_server(server['id'])
         self.assertEqual(src_host, server['OS-EXT-SRV-ATTR:host'])
 
-        # FIXME(lyarwood): Assert that both the src and dest attachments have
-        # been removed. Only the dest attachment should be removed during the
-        # rollback of a pre_live_migration failure.
+        # Assert that the src attachment is still present
         attachments = self.cinder.volume_to_attachment.get(volume_id)
-        self.assertNotIn(src_attachment_id, attachments.keys())
-        self.assertEqual(0, len(attachments))
+        self.assertIn(src_attachment_id, attachments.keys())
+        self.assertEqual(1, len(attachments))
diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py
index 074683a805..dd4431a0e3 100644
--- a/nova/tests/unit/compute/test_compute_mgr.py
+++ b/nova/tests/unit/compute/test_compute_mgr.py
@@ -8933,6 +8933,19 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase,
         self.assertIn('Exception while attempting to rollback',
                 mock_log.exception.call_args[0][0])
 
+    @mock.patch('nova.volume.cinder.API.attachment_delete')
+    def test_rollback_volume_bdms_after_pre_failure(
+            self, mock_delete_attachment):
+        instance = fake_instance.fake_instance_obj(
+            self.context, uuid=uuids.instance)
+        original_bdms = bdms = self._generate_volume_bdm_list(instance)
+        self.compute._rollback_volume_bdms(
+            self.context, bdms, original_bdms, instance)
+        # Assert that attachment_delete isn't called when the bdms have already
+        # been rolled back by a failure in pre_live_migration to reference the
+        # source bdms.
+        mock_delete_attachment.assert_not_called()
+
     @mock.patch.object(objects.ComputeNode,
                        'get_first_node_by_host_for_old_compat')
     @mock.patch('nova.scheduler.client.report.SchedulerReportClient.'