api: Reject volume attach requests when an active bdm exists

When attaching volumes to instances Nova has previously relied on checks
carried out by c-api to ensure that a single non-multiattach volume is
not attached to multiple instances at once. While this works well in
most cases it does not handle PEBKAC issues when admins reset the state
of a volume to available, allowing users to request that Nova attach the
volume to another instance.

This change aims to address this by including a simple check in the
attach flow for non-multiattach volumes ensuring that there are no
existing active block device mapping records for the volume already
present within Nova.

Closes-Bug: #1908075
Change-Id: I2881d77d52bcbde9f3ac6a6ddfb4a22a9bd45c8a

nova/compute/api.py

@@ -4622,6 +4622,28 @@ class API(base.Base):
         except exception.VolumeBDMNotFound:
             pass
 
+    def _check_volume_already_attached(
+        self, context: nova_context.RequestContext, volume_id: str
+    ):
+        """Avoid allowing a non-multiattach volumes being attached twice
+
+        Unlike the above _check_volume_already_attached_to_instance check we
+        also need to ensure that non-multiattached volumes are not attached to
+        multiple instances. This check is also carried out later by c-api
+        itself but it can however be circumvented by admins resetting the state
+        of an attached volume to available. As a result we also need to perform
+        a check within Nova before creating a new BDM for the attachment.
+        """
+        try:
+            bdm = objects.BlockDeviceMapping.get_by_volume(
+                context, volume_id)
+            msg = _("volume %(volume_id)s is already attached to instance "
+                    "%(instance_uuid)s") % {'volume_id': volume_id,
+                    'instance_uuid': bdm.instance_uuid}
+            raise exception.InvalidVolume(reason=msg)
+        except exception.VolumeBDMNotFound:
+            pass
+
     def _check_attach_and_reserve_volume(self, context, volume, instance,
                                          bdm, supports_multiattach=False,
                                          validate_az=True):
@@ -4761,8 +4783,17 @@ class API(base.Base):
         self._check_volume_already_attached_to_instance(context,
                                                         instance,
                                                         volume_id)
-
         volume = self.volume_api.get(context, volume_id)
+
+        # NOTE(lyarwood): Ensure that non multiattach volumes don't already
+        # have active block device mappings present in Nova.
+        # TODO(lyarwood): Merge this into the
+        # _check_volume_already_attached_to_instance check once
+        # BlockDeviceMappingList provides a list of active bdms per volume so
+        # we can preform a single lookup for both checks.
+        if not volume.get('multiattach', False):
+            self._check_volume_already_attached(context, volume_id)
+
         is_shelved_offloaded = instance.vm_state == vm_states.SHELVED_OFFLOADED
         if is_shelved_offloaded:
             if tag:

nova/tests/functional/regressions/test_bug_1908075.py

@@ -12,6 +12,7 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+from nova.tests.functional.api import client
 from nova.tests.functional import integrated_helpers
 
 
@@ -44,24 +45,17 @@ class TestVolAttachCinderReset(integrated_helpers._IntegratedTestBase):
 
         # Launch a second server and attempt to attach the same volume again
         server_b = self._create_server(networks='none')
-        # FIXME(lyarwood): n-api shouldn't accept this request as we already
-        # have an active bdm record for this non-multiattach volume.
-        self.api.post_server_volume(
+
+        # Assert that attempting to attach this non multiattach volume to
+        # another instance is rejected by n-api
+        ex = self.assertRaises(
+            client.OpenStackApiException,
+            self.api.post_server_volume,
             server_b['id'],
             {'volumeAttachment': {'volumeId': volume_id}}
         )
 
-        # Assert that we have bdms within Nova still for this attachment
-        self.assertEqual(
-            volume_id,
-            self.api.get_server_volumes(server_a['id'])[0].get('volumeId'))
-        self.assertEqual(
-            volume_id,
-            self.api.get_server_volumes(server_b['id'])[0].get('volumeId'))
-
-        # Assert that the new attachment is the only one in the fixture
-        self.assertIn(
-            volume_id, self.cinder.volume_ids_for_instance(server_b['id']))
+        self.assertEqual(400, ex.response.status_code)
 
     def test_volume_attach_after_cinder_reset_state_multiattach_volume(self):
 

nova/tests/unit/compute/test_api.py

@@ -444,11 +444,16 @@ class _ComputeAPIUnitTestMixIn(object):
     @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
     @mock.patch.object(objects.BlockDeviceMapping,
                        'get_by_volume_and_instance')
+    @mock.patch.object(objects.BlockDeviceMapping, 'get_by_volume')
     @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
-    def test_attach_volume_new_flow(self, mock_attach, mock_bdm,
-                                    mock_reserve, mock_record):
-        mock_bdm.side_effect = exception.VolumeBDMNotFound(
-                                          volume_id='fake-volume-id')
+    def test_attach_volume_new_flow(
+            self, mock_attach, mock_get_by_volume, mock_get_by_instance,
+            mock_reserve, mock_record
+    ):
+        mock_get_by_instance.side_effect = exception.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
+        mock_get_by_volume.side_effect = exception.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
         instance = self._create_instance_obj()
         volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
                                          None, None, None, None, None)
@@ -477,11 +482,16 @@ class _ComputeAPIUnitTestMixIn(object):
     @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
     @mock.patch.object(objects.BlockDeviceMapping,
                        'get_by_volume_and_instance')
+    @mock.patch.object(objects.BlockDeviceMapping, 'get_by_volume')
     @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
-    def test_tagged_volume_attach_new_flow(self, mock_attach, mock_bdm,
-                                           mock_reserve, mock_record):
-        mock_bdm.side_effect = exception.VolumeBDMNotFound(
-                                          volume_id='fake-volume-id')
+    def test_tagged_volume_attach_new_flow(
+            self, mock_attach, mock_get_by_volume, mock_get_by_instance,
+            mock_reserve, mock_record
+    ):
+        mock_get_by_instance.side_effect = exception.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
+        mock_get_by_volume.side_effect = exception.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
         instance = self._create_instance_obj()
         volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
                                          None, None, None, None, None)
@@ -513,11 +523,16 @@ class _ComputeAPIUnitTestMixIn(object):
     @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name')
     @mock.patch.object(objects.BlockDeviceMapping,
                        'get_by_volume_and_instance')
+    @mock.patch.object(objects.BlockDeviceMapping, 'get_by_volume')
     @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
-    def test_attach_volume_attachment_create_fails(self, mock_attach, mock_bdm,
-                                                   mock_reserve):
-        mock_bdm.side_effect = exception.VolumeBDMNotFound(
-                                          volume_id='fake-volume-id')
+    def test_attach_volume_attachment_create_fails(
+            self, mock_attach, mock_get_by_volume, mock_get_by_instance,
+            mock_reserve
+    ):
+        mock_get_by_instance.side_effect = exception.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
+        mock_get_by_volume.side_effect = exception.VolumeBDMNotFound(
+            volume_id='fake-volume-id')
         instance = self._create_instance_obj()
         volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol',
                                          None, None, None, None, None)
@@ -541,6 +556,28 @@ class _ComputeAPIUnitTestMixIn(object):
             self.assertEqual(0, mock_attach.call_count)
             fake_bdm.destroy.assert_called_once_with()
 
+    @mock.patch.object(objects.BlockDeviceMapping, 'get_by_volume')
+    @mock.patch.object(
+        objects.BlockDeviceMapping, 'get_by_volume_and_instance')
+    def test_attach_volume_bdm_exists(self, mock_by_instance, mock_by_volume):
+        mock_by_instance.side_effect = exception.VolumeBDMNotFound(
+            volume_id=uuids.volume)
+        mock_by_volume.return_value = mock.Mock(
+            spec=objects.BlockDeviceMapping, volume_id=uuids.volume,
+            instance_uuid=uuids.instance)
+        instance = self._create_instance_obj()
+        volume = {'id': uuids.volume, 'multiattach': False}
+        with mock.patch.object(
+            self.compute_api, 'volume_api', mock.MagicMock(spec=cinder.API)
+        ) as mock_v_api:
+            mock_v_api.get.return_value = volume
+            # Assert that we raise InvalidVolume when we find a bdm for the vol
+            self.assertRaises(
+                exception.InvalidVolume,
+                self.compute_api.attach_volume,
+                self.context, instance, uuids.volume
+            )
+
     def test_suspend(self):
         # Ensure instance can be suspended.
         instance = self._create_instance_obj()