diff --git a/nova/compute/manager.py b/nova/compute/manager.py index e34f334b6bbd..5364f9ad5252 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -7242,7 +7242,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 8e129d8e85f4..fd971846bc16 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 573d9cd0ab89..abf4a3c9434a 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -8950,6 +8950,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.'