From 4dd0656961552ed8418bb22352f8ed8f6f58031e Mon Sep 17 00:00:00 2001 From: Ildiko Vancsa Date: Tue, 5 Dec 2017 16:01:32 +0100 Subject: [PATCH] Add a new check to volume attach With the new Cinder volume attach API we can attach the same volume to the same instance multiple times. This is useful for live migrate however in other cases we would like to avoid this to happen. This patch adds a new check in Nova and fail the volume attach request in case this condition is met. Change-Id: I049f00f993e45eeb090a1e1a5e5696cf2f103187 --- nova/compute/api.py | 27 +++++++++ nova/tests/unit/compute/test_compute_api.py | 62 +++++++++++++++++---- 2 files changed, 77 insertions(+), 12 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 833a997afc68..ee8b4f14432b 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -3618,6 +3618,26 @@ class API(base.Base): device_type=device_type, tag=tag) 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_attach_and_reserve_volume(self, context, volume_id, instance): volume = self.volume_api.get(context, volume_id) self.volume_api.check_availability_zone(context, volume, @@ -3804,6 +3824,13 @@ class API(base.Base): # we cast to the compute host. self.volume_api.reserve_volume(context, new_volume['id']) else: + try: + self._check_volume_already_attached_to_instance( + context, instance, new_volume['id']) + except exception.InvalidVolume: + with excutils.save_and_reraise_exception(): + self.volume_api.roll_detaching(context, old_volume['id']) + # This is a new-style attachment so for the volume that we are # going to swap to, create a new volume attachment. new_attachment_id = self.volume_api.attachment_create( diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index abc5ded18466..6f78b6196e83 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -2368,7 +2368,12 @@ class _ComputeAPIUnitTestMixIn(object): self._test_swap_volume(expected_exception=AttributeError, attachment_id=uuids.attachment_id) - def _test_swap_volume(self, expected_exception=None, attachment_id=None): + def test_swap_volume_new_vol_already_attached_new_flow(self): + self._test_swap_volume(attachment_id=uuids.attachment_id, + volume_already_attached=True) + + def _test_swap_volume(self, expected_exception=None, attachment_id=None, + volume_already_attached=False): volumes = self._get_volumes_for_test_swap_volume() instance = self._get_instance_for_test_swap_volume() @@ -2402,6 +2407,12 @@ class _ComputeAPIUnitTestMixIn(object): if volumes[uuids.new_volume]['status'] == 'reserved': volumes[uuids.new_volume]['status'] = 'available' + def fake_volume_is_attached(context, instance, volume_id): + if volume_already_attached: + raise exception.InvalidVolume(reason='Volume already attached') + else: + pass + @mock.patch.object(compute_api.API, '_record_action_start') @mock.patch.object(self.compute_api.compute_rpcapi, 'swap_volume', return_value=True) @@ -2411,6 +2422,9 @@ class _ComputeAPIUnitTestMixIn(object): side_effect=fake_vol_api_attachment_delete) @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', + side_effect=fake_volume_is_attached) @mock.patch.object(self.compute_api.volume_api, 'attachment_create', side_effect=fake_vol_api_attachment_create) @mock.patch.object(self.compute_api.volume_api, 'roll_detaching', @@ -2421,8 +2435,9 @@ class _ComputeAPIUnitTestMixIn(object): side_effect=fake_vol_api_begin_detaching) def _do_test(mock_begin_detaching, mock_get_by_volume_and_instance, mock_roll_detaching, mock_attachment_create, - mock_reserve_volume, mock_attachment_delete, - mock_unreserve_volume, mock_swap_volume, mock_record): + mock_check_volume_attached, mock_reserve_volume, + mock_attachment_delete, mock_unreserve_volume, + mock_swap_volume, mock_record): bdm = objects.BlockDeviceMapping( **fake_block_device.FakeDbBlockDeviceDict( {'no_device': False, 'volume_id': '1', 'boot_index': 0, @@ -2454,6 +2469,27 @@ class _ComputeAPIUnitTestMixIn(object): mock_unreserve_volume.assert_not_called() mock_attachment_delete.assert_called_once_with( self.context, attachment_id) + + # Assert the call to the rpcapi. + mock_swap_volume.assert_called_once_with( + self.context, instance=instance, + old_volume_id=uuids.old_volume, + new_volume_id=uuids.new_volume, + new_attachment_id=attachment_id) + mock_record.assert_called_once_with( + self.context, instance, instance_actions.SWAP_VOLUME) + elif volume_already_attached: + self.assertRaises(exception.InvalidVolume, + self.compute_api.swap_volume, self.context, + instance, volumes[uuids.old_volume], + volumes[uuids.new_volume]) + self.assertEqual('in-use', volumes[uuids.old_volume]['status']) + self.assertEqual('available', + volumes[uuids.new_volume]['status']) + mock_check_volume_attached.assert_called_once_with( + self.context, instance, uuids.new_volume) + mock_roll_detaching.assert_called_once_with(self.context, + uuids.old_volume) else: self.compute_api.swap_volume(self.context, instance, volumes[uuids.old_volume], @@ -2466,22 +2502,24 @@ class _ComputeAPIUnitTestMixIn(object): mock_reserve_volume.assert_called_once_with( self.context, uuids.new_volume) mock_attachment_create.assert_not_called() + mock_check_volume_attached.assert_not_called() else: # New style attachment, so reserve was not called and # attachment_create was called. mock_reserve_volume.assert_not_called() + mock_check_volume_attached.assert_called_once_with( + self.context, instance, uuids.new_volume) mock_attachment_create.assert_called_once_with( self.context, uuids.new_volume, instance.uuid) - # Assert the call to the rpcapi. - mock_swap_volume.assert_called_once_with( - self.context, instance=instance, - old_volume_id=uuids.old_volume, - new_volume_id=uuids.new_volume, - new_attachment_id=attachment_id) - mock_record.assert_called_once_with(self.context, - instance, - instance_actions.SWAP_VOLUME) + # Assert the call to the rpcapi. + mock_swap_volume.assert_called_once_with( + self.context, instance=instance, + old_volume_id=uuids.old_volume, + new_volume_id=uuids.new_volume, + new_attachment_id=attachment_id) + mock_record.assert_called_once_with( + self.context, instance, instance_actions.SWAP_VOLUME) _do_test()