Merge "compute: Avoid duplicate BDMs during reserve_block_device_name" into stable/wallaby23.2.0

3 files changed, 41 insertions, 6 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 2541b83274..c59fd31f21 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -6991,6 +6991,12 @@ class ComputeManager(manager.Manager):
                 objects.BlockDeviceMappingList.get_by_instance_uuid(
                     context, instance.uuid))
 
+            # Now that we have the lock check that we haven't raced another
+            # request and ensure there is no existing attachment
+            if any(b for b in bdms if b.volume_id == volume_id):
+                msg = _("volume %s already attached") % volume_id
+                raise exception.InvalidVolume(reason=msg)
+
             # NOTE(ndipanov): We need to explicitly set all the fields on the
             #                 object so that obj_load_attr does not fail
             new_bdm = objects.BlockDeviceMapping(
diff --git a/nova/tests/functional/regressions/test_bug_1937375.py b/nova/tests/functional/regressions/test_bug_1937375.py
index 007be9b5d7..88dfea0ad7 100644
--- a/nova/tests/functional/regressions/test_bug_1937375.py
+++ b/nova/tests/functional/regressions/test_bug_1937375.py
@@ -16,6 +16,7 @@ import mock
 import time
 
 from nova import context
+from nova import exception
 from nova import objects
 from nova.tests.functional import integrated_helpers
 
@@ -68,7 +69,12 @@ class TestDuplicateVolAttachRace(integrated_helpers._IntegratedTestBase):
             # twice to mimic two callers racing each other after the checks on
             # the api.
             original_bdm = original_reserve_name(*args, **kwargs)
-            original_reserve_name(*args, **kwargs)
+
+            # Assert that a repeat call fails as an attachment already exists
+            self.assertRaises(
+                exception.InvalidVolume,
+                original_reserve_name, *args, **kwargs)
+
             return original_bdm
 
         with mock.patch.object(
@@ -86,10 +92,7 @@ class TestDuplicateVolAttachRace(integrated_helpers._IntegratedTestBase):
         bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
             ctxt, server_id)
 
-        # FIXME(lyarwood): This is bug #1937375, we now have 3 bdms for the
-        # instance, the original root disk and two duplicate volume bdms for
-        # the same volume attachment.
-        self.assertEqual(3, len(bdms))
-        self.assertEqual(volume_id, bdms[2].volume_id)
+        # Assert that the correct bdms are present
+        self.assertEqual(2, len(bdms))
         self.assertEqual(volume_id, bdms[1].volume_id)
         self.assertEqual('local', bdms[0].destination_type)
diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py
index bc9b22ae2d..6dc9f4f83d 100644
--- a/nova/tests/unit/compute/test_compute_mgr.py
+++ b/nova/tests/unit/compute/test_compute_mgr.py
@@ -616,6 +616,32 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
                               'fake_device', 'fake_volume_id', 'fake_disk_bus',
                               'fake_device_type', tag=None, multiattach=True)
 
+    @mock.patch.object(compute_utils, 'add_instance_fault_from_exc',
+                       new=mock.Mock())
+    @mock.patch.object(objects.BlockDeviceMapping, 'create', new=mock.Mock())
+    @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_instance_uuid')
+    def test_reserve_block_device_name_raises_on_duplicate(self, mock_get):
+        instance = fake_instance.fake_instance_obj(self.context)
+        vol_bdm = objects.BlockDeviceMapping(
+            self.context,
+            id=1,
+            instance_uuid=instance.uuid,
+            volume_id="myinstanceuuid",
+            source_type='volume',
+            destination_type='volume',
+            delete_on_termination=True,
+            connection_info=None,
+            tag='fake-tag',
+            device_name='/dev/fake0',
+            attachment_id=uuids.attachment_id)
+        mock_get.return_value = objects.BlockDeviceMappingList(
+            objects=[vol_bdm])
+
+        self.assertRaises(exception.InvalidVolume,
+            self.compute.reserve_block_device_name,
+                self.context, instance, None, "myinstanceuuid",
+                None, None, 'foo', False)
+
     @mock.patch.object(objects.Instance, 'save')
     @mock.patch.object(time, 'sleep')
     def test_allocate_network_succeeds_after_retries(