From 1252588d4e48da1d3a753639a8a4d937acf3e037 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Thu, 24 Dec 2020 11:50:43 +0000 Subject: [PATCH] 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 | 33 +++++++++- .../regressions/test_bug_1908075.py | 22 +++---- nova/tests/unit/compute/test_api.py | 61 +++++++++++++++---- 3 files changed, 89 insertions(+), 27 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 077a848f4350..77785bdbc4fc 100644 --- a/nova/compute/api.py +++ b/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: diff --git a/nova/tests/functional/regressions/test_bug_1908075.py b/nova/tests/functional/regressions/test_bug_1908075.py index 87b3159b4bc7..534163fb2bbf 100644 --- a/nova/tests/functional/regressions/test_bug_1908075.py +++ b/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): diff --git a/nova/tests/unit/compute/test_api.py b/nova/tests/unit/compute/test_api.py index 5cf00af96b2d..e9837dee1865 100644 --- a/nova/tests/unit/compute/test_api.py +++ b/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()