diff --git a/nova/compute/api.py b/nova/compute/api.py index 77785bdbc4fc..ac109d247585 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -23,6 +23,7 @@ import collections import functools import re import string +import typing as ty from castellan import key_manager import os_traits @@ -4602,47 +4603,53 @@ class API(base.Base): volume_bdm.save() return volume_bdm - def _check_volume_already_attached_to_instance(self, context, instance, - volume_id): - """Avoid attaching the same volume to the same instance twice. - - As the new Cinder flow (microversion 3.44) is handling the checks - differently and allows to attach the same volume to the same - instance twice to enable live_migrate we are checking whether the - BDM already exists for this combination for the new flow and fail - if it does. - """ - - try: - objects.BlockDeviceMapping.get_by_volume_and_instance( - context, volume_id, instance.uuid) - - msg = _("volume %s already attached") % volume_id - raise exception.InvalidVolume(reason=msg) - except exception.VolumeBDMNotFound: - pass - def _check_volume_already_attached( - self, context: nova_context.RequestContext, volume_id: str + self, + context: nova_context.RequestContext, + instance: objects.Instance, + volume: ty.Mapping[str, ty.Any], ): - """Avoid allowing a non-multiattach volumes being attached twice + """Avoid duplicate volume attachments. - 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. + Since the 3.44 Cinder microversion, Cinder allows us to attach the same + volume to the same instance twice. This is ostensibly to enable live + migration, but it's not something we want to occur outside of this + particular code path. + + In addition, 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. + + :param context: nova auth RequestContext + :param instance: Instance object + :param volume: volume dict from cinder """ + # Fetch a list of active bdms for the volume, return if none are found. 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) + bdms = objects.BlockDeviceMappingList.get_by_volume( + context, volume['id']) except exception.VolumeBDMNotFound: - pass + return + + # Fail if the volume isn't multiattach but BDMs already exist + if not volume.get('multiattach'): + instance_uuids = ' '.join(f"{b.instance_uuid}" for b in bdms) + msg = _( + "volume %(volume_id)s is already attached to instances: " + "%(instance_uuids)s" + ) % { + 'volume_id': volume['id'], + 'instance_uuids': instance_uuids + } + raise exception.InvalidVolume(reason=msg) + + # Fail if the volume is already attached to our instance + if any(b for b in bdms if b.instance_uuid == instance.uuid): + msg = _("volume %s already attached") % volume['id'] + raise exception.InvalidVolume(reason=msg) def _check_attach_and_reserve_volume(self, context, volume, instance, bdm, supports_multiattach=False, @@ -4780,19 +4787,10 @@ class API(base.Base): # _check_attach_and_reserve_volume and Cinder will allow multiple # attachments between the same volume and instance but the old flow # API semantics don't allow that so we enforce it here. - 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) + volume = self.volume_api.get(context, volume_id) + self._check_volume_already_attached(context, instance, volume) is_shelved_offloaded = instance.vm_state == vm_states.SHELVED_OFFLOADED if is_shelved_offloaded: @@ -4971,8 +4969,8 @@ class API(base.Base): self.volume_api.reserve_volume(context, new_volume['id']) else: try: - self._check_volume_already_attached_to_instance( - context, instance, new_volume['id']) + self._check_volume_already_attached( + context, instance, new_volume) except exception.InvalidVolume: with excutils.save_and_reraise_exception(): self.volume_api.roll_detaching(context, old_volume['id']) diff --git a/nova/objects/block_device.py b/nova/objects/block_device.py index f81fc0dec1ed..0e3ceae34433 100644 --- a/nova/objects/block_device.py +++ b/nova/objects/block_device.py @@ -351,7 +351,8 @@ class BlockDeviceMappingList(base.ObjectListBase, base.NovaObject): # Version 1.15: BlockDeviceMapping <= version 1.14 # Version 1.16: BlockDeviceMapping <= version 1.15 # Version 1.17: Add get_by_instance_uuids() - VERSION = '1.17' + # Version 1.18: Add get_by_volume() + VERSION = '1.18' fields = { 'objects': fields.ListOfObjectsField('BlockDeviceMapping'), @@ -398,6 +399,22 @@ class BlockDeviceMappingList(base.ObjectListBase, base.NovaObject): return base.obj_make_list( context, cls(), objects.BlockDeviceMapping, db_bdms or []) + @staticmethod + @db.select_db_reader_mode + def _db_block_device_mapping_get_all_by_volume( + context, volume_id, use_slave=False): + return db.block_device_mapping_get_all_by_volume_id( + context, volume_id) + + @base.remotable_classmethod + def get_by_volume(cls, context, volume_id, use_slave=False): + db_bdms = cls._db_block_device_mapping_get_all_by_volume( + context, volume_id, use_slave=use_slave) + if not db_bdms: + raise exception.VolumeBDMNotFound(volume_id=volume_id) + return base.obj_make_list( + context, cls(), objects.BlockDeviceMapping, db_bdms) + def root_bdm(self): """It only makes sense to call this method when the BlockDeviceMappingList contains BlockDeviceMappings from diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index e5eecd3161e0..2de9fe0618ef 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -1033,7 +1033,7 @@ class ServerTestV220(integrated_helpers._IntegratedTestBase): # Test attach volume self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get) with test.nested(mock.patch.object(compute_api.API, - '_check_volume_already_attached_to_instance'), + '_check_volume_already_attached'), mock.patch.object(volume.cinder.API, 'check_availability_zone'), mock.patch.object(volume.cinder.API, diff --git a/nova/tests/unit/compute/test_api.py b/nova/tests/unit/compute/test_api.py index e9837dee1865..0afa77bcaf18 100644 --- a/nova/tests/unit/compute/test_api.py +++ b/nova/tests/unit/compute/test_api.py @@ -442,16 +442,11 @@ class _ComputeAPIUnitTestMixIn(object): @mock.patch.object(compute_api.API, '_record_action_start') @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(objects.BlockDeviceMappingList, 'get_by_volume') @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') def test_attach_volume_new_flow( - self, mock_attach, mock_get_by_volume, mock_get_by_instance, - mock_reserve, mock_record + self, mock_attach, mock_get_by_volume, 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() @@ -480,16 +475,11 @@ class _ComputeAPIUnitTestMixIn(object): @mock.patch.object(compute_api.API, '_record_action_start') @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(objects.BlockDeviceMappingList, 'get_by_volume') @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') def test_tagged_volume_attach_new_flow( - self, mock_attach, mock_get_by_volume, mock_get_by_instance, - mock_reserve, mock_record + self, mock_attach, mock_get_by_volume, 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() @@ -521,16 +511,11 @@ class _ComputeAPIUnitTestMixIn(object): instance, fake_bdm) @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(objects.BlockDeviceMappingList, 'get_by_volume') @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') def test_attach_volume_attachment_create_fails( - self, mock_attach, mock_get_by_volume, mock_get_by_instance, - mock_reserve + self, mock_attach, mock_get_by_volume, 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() @@ -556,15 +541,20 @@ 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) + @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_volume') + def test_attach_volume_bdm_exists(self, mock_by_volume): + mock_by_volume.return_value = [ + mock.Mock( + spec=objects.BlockDeviceMapping, + volume_id=uuids.volume, + instance_uuid=uuids.instance_1 + ), + mock.Mock( + spec=objects.BlockDeviceMapping, + volume_id=uuids.volume, + instance_uuid=uuids.instance_2 + ), + ] instance = self._create_instance_obj() volume = {'id': uuids.volume, 'multiattach': False} with mock.patch.object( @@ -572,11 +562,16 @@ class _ComputeAPIUnitTestMixIn(object): ) 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( + ex = self.assertRaises( exception.InvalidVolume, self.compute_api.attach_volume, self.context, instance, uuids.volume ) + self.assertIn( + f"volume {uuids.volume} is already attached to instances: " + f"{uuids.instance_1} {uuids.instance_2}", + str(ex) + ) def test_suspend(self): # Ensure instance can be suspended. @@ -2789,7 +2784,7 @@ class _ComputeAPIUnitTestMixIn(object): @mock.patch.object(self.compute_api.volume_api, 'reserve_volume', side_effect=fake_vol_api_reserve) @mock.patch.object(self.compute_api, - '_check_volume_already_attached_to_instance', + '_check_volume_already_attached', side_effect=fake_volume_is_attached) @mock.patch.object(self.compute_api.volume_api, 'attachment_create', side_effect=fake_vol_api_attachment_create) @@ -2853,7 +2848,7 @@ class _ComputeAPIUnitTestMixIn(object): self.assertEqual('available', volumes[uuids.new_volume]['status']) mock_check_volume_attached.assert_called_once_with( - self.context, instance, uuids.new_volume) + self.context, instance, volumes[uuids.new_volume]) mock_roll_detaching.assert_called_once_with(self.context, uuids.old_volume) else: @@ -2874,7 +2869,7 @@ class _ComputeAPIUnitTestMixIn(object): # attachment_create was called. mock_reserve_volume.assert_not_called() mock_check_volume_attached.assert_called_once_with( - self.context, instance, uuids.new_volume) + self.context, instance, volumes[uuids.new_volume]) mock_attachment_create.assert_called_once_with( self.context, uuids.new_volume, instance.uuid) @@ -7181,7 +7176,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): instance = self._create_instance_obj( params={'vm_state': vm_states.SHELVED_OFFLOADED}) with mock.patch.object( - self.compute_api, '_check_volume_already_attached_to_instance', + self.compute_api, '_check_volume_already_attached', return_value=None): self.assertRaises(exception.MultiattachToShelvedNotSupported, self.compute_api.attach_volume, diff --git a/nova/tests/unit/objects/test_block_device.py b/nova/tests/unit/objects/test_block_device.py index d95465aebae6..7120c3c336c6 100644 --- a/nova/tests/unit/objects/test_block_device.py +++ b/nova/tests/unit/objects/test_block_device.py @@ -534,6 +534,29 @@ class _TestBlockDeviceMappingListObject(object): self.context, uuids.bdm_instance) self.assertRaises(exception.UndefinedRootBDM, bdm_list.root_bdm) + @mock.patch.object(db, 'block_device_mapping_get_all_by_volume_id') + def test_get_by_volume(self, get_all_by_volume_id): + fakes = [ + self.fake_bdm(123, instance_uuid=uuids.instance_1), + self.fake_bdm(456, instance_uuid=uuids.instance_2), + ] + get_all_by_volume_id.return_value = fakes + bdm_list = objects.BlockDeviceMappingList.get_by_volume( + self.context, uuids.volume_id) + for faked, res in zip(fakes, bdm_list): + self.assertIsInstance(res, objects.BlockDeviceMapping) + self.assertEqual(faked['id'], res.id) + + @mock.patch.object(db, 'block_device_mapping_get_all_by_volume_id', + new=mock.Mock(return_value=None)) + def test_get_by_volume_no_result(self): + self.assertRaises( + exception.VolumeBDMNotFound, + objects.BlockDeviceMappingList.get_by_volume, + self.context, + uuids.volume_id, + ) + class TestBlockDeviceMappingListObject(test_objects._LocalTest, _TestBlockDeviceMappingListObject): diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index a0fe7b9e981e..4fe9f0b6d203 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -1049,7 +1049,7 @@ object_data = { 'BandwidthUsage': '1.2-c6e4c779c7f40f2407e3d70022e3cd1c', 'BandwidthUsageList': '1.2-5fe7475ada6fe62413cbfcc06ec70746', 'BlockDeviceMapping': '1.20-45a6ad666ddf14bbbedece2293af77e2', - 'BlockDeviceMappingList': '1.17-1e568eecb91d06d4112db9fd656de235', + 'BlockDeviceMappingList': '1.18-73bcbbae5ef5e8adcedbc821db869306', 'BuildRequest': '1.3-077dee42bed93f8a5b62be77657b7152', 'BuildRequestList': '1.0-cd95608eccb89fbc702c8b52f38ec738', 'CellMapping': '1.1-5d652928000a5bc369d79d5bde7e497d',