api: Avoid race-condition in volume-attach timeout handling

When nova-api calls reserve_block_device_name RPC to nova-compute and
the call times out, we try to clean up by deleting the BDM entry. However,
during the timeout window a second attachment request for the same volume
can come in, create a valid BDM, and progress to talking to Cinder.
The original timed-out request then deletes this new valid BDM, leaving
the volume in an inconsistent state.

We fix this by checking if the BDM has attachment_id set before
deleting it. The attachment_id field is only populated in
_check_attach_and_reserve_volume(), which we only call after the
reserve_block_device_name RPC succeeds. If attachment_id is set,
we know the BDM belongs to a subsequent request that has already
progressed past the RPC phase, so we should not delete it.

Change-Id: I7ed649a5cab7f254690f329fac285128d8cd1c92

Author: leust

nova/compute/api.py

@@ -5145,12 +5145,24 @@ def _attach_volume(self, context, instance, volume, device,
             # The compute node might have already created the attachment but
             # we never received the answer. In this case it is safe to delete
             # the attachment as nobody will ever pick it up again.
+            # To avoid a race-condition where a second volume-attach request
+            # comes in for the same volume while we wait, we verify that the
+            # BDM we delete has no attachment_id set. attachment_id only
+            # gets set later in _check_attach_and_reserve_volume().
             with excutils.save_and_reraise_exception():
                 try:
-                    objects.BlockDeviceMapping.get_by_volume_and_instance(
-                        context, volume['id'], instance.uuid).destroy()
-                    LOG.debug("Delete BDM after compute did not respond to "
-                              f"attachment request for volume {volume['id']}")
+                    bdm = \
+                        objects.BlockDeviceMapping.get_by_volume_and_instance(
+                            context, volume['id'], instance.uuid)
+                    if bdm.attachment_id:
+                        LOG.debug("BDM for volume %s has attachment_id set, "
+                                  "not deleting to avoid race-condition",
+                                  volume['id'])
+                    else:
+                        bdm.destroy()
+                        LOG.debug("Delete BDM after compute did not respond "
+                                  "to attachment request for volume %s",
+                                  volume['id'])
                 except exception.VolumeBDMNotFound:
                     LOG.debug("BDM not found, ignoring removal. "
                               f"Error attaching volume {volume['id']}")

nova/tests/unit/compute/test_api.py

@@ -537,6 +537,48 @@ def test_attach_volume_reserve_bdm_timeout(
             volume_id='fake-volume-id')
 
         fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping)
+        # BDM has no attachment_id yet, so it's safe to delete
+        fake_bdm.attachment_id = None
+        mock_get_by_volume_and_instance.return_value = fake_bdm
+        instance = self._create_instance_obj()
+        volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
+                                         None, None, None, None, None)
+
+        mock_reserve.side_effect = oslo_exceptions.MessagingTimeout()
+
+        mock_volume_api = mock.patch.object(self.compute_api, 'volume_api',
+                                            mock.MagicMock(spec=cinder.API))
+
+        with mock_volume_api as mock_v_api:
+            mock_v_api.get.return_value = volume
+            self.assertRaises(oslo_exceptions.MessagingTimeout,
+                              self.compute_api.attach_volume,
+                              self.context, instance, volume['id'])
+            mock_get_by_volume_and_instance.assert_called_once_with(
+                self.context, volume['id'], instance.uuid)
+            fake_bdm.destroy.assert_called_once_with()
+
+    @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
+    @mock.patch.object(
+        objects.BlockDeviceMapping, 'get_by_volume_and_instance')
+    @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')
+    def test_attach_volume_reserve_bdm_timeout_with_attachment_id(
+            self, mock_get_by_volume, mock_get_by_volume_and_instance,
+            mock_reserve):
+        """Test that BDM with attachment_id is not deleted on timeout.
+
+        This tests the race condition fix: if a second attachment request
+        comes in while we're waiting for the RPC timeout, the BDM might
+        already have attachment_id set. In this case, we should NOT
+        delete the BDM as it belongs to the second request.
+        """
+        mock_get_by_volume.side_effect = exception.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
+
+        fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping)
+        # BDM has attachment_id set, indicating a second request has
+        # already progressed past the reserve_block_device_name RPC
+        fake_bdm.attachment_id = 'fake-attachment-id'
         mock_get_by_volume_and_instance.return_value = fake_bdm
         instance = self._create_instance_obj()
         volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
@@ -554,7 +596,8 @@ def test_attach_volume_reserve_bdm_timeout(
                                 self.context, instance, volume['id'])
             mock_get_by_volume_and_instance.assert_called_once_with(
                 self.context, volume['id'], instance.uuid)
-            fake_bdm.destroy.assert_called_once_with()
+            # destroy() should NOT be called when attachment_id is set
+            fake_bdm.destroy.assert_not_called()
 
     @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
     @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume')