compute: Avoid duplicate BDMs during reserve_block_device_name

When attaching a volume to a running instance the nova-api validates
that the volume is not already attached to the instance. However
nova-compute is responsible for actually creating the BDM entry in the
database. If sending attach requests fast enough it can be possible
that the same "attach_volume" request can be sent to nova-compute for
the same volume/instance combination.

To work around this we add a check in nova-compute to validate that
the volume has not been attached in the mean time.

Closes-Bug: #1937375
Change-Id: I92f35514efddcb071c7094370b79d91d34c5bc72
(cherry picked from commit 2209b00)

Author: felixhuettner

nova/compute/manager.py

@@ -6979,6 +6979,12 @@ def do_reserve():
                 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(

nova/tests/functional/regressions/test_bug_1937375.py

@@ -16,6 +16,7 @@
 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 @@ def wrap_reserve_block_device_name(*args, **kwargs):
             # 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 @@ def wrap_reserve_block_device_name(*args, **kwargs):
         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)

nova/tests/unit/compute/test_compute_mgr.py

@@ -614,6 +614,32 @@ def test_reserve_block_device_name_multiattach_raises(self, _):
                               '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(