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
This commit is contained in:
Ildiko Vancsa 2017-12-05 16:01:32 +01:00 committed by Matt Riedemann
parent 9f46043f2f
commit 4dd0656961
2 changed files with 77 additions and 12 deletions

View File

@ -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(

View File

@ -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()