openstack
/
nova
Code Issues Proposed changes

fup: Merge duplicate volume attachment checks 

This is a follow up to I2881d77d52bcbde9f3ac6a6ddfb4a22a9bd45c8a where
it was noted that we could merge these checks into one using a single db
query for all active volume attachments for a given volume. This
requires changes to the BlockDeviceMappingList object and was thus left
as a separate change to allow the original to be backported.

Change-Id: I859be652eb8fff591899a223193bd9efc8830cf3
This commit is contained in:
Lee Yarwood 2021-02-01 11:35:23 +00:00
parent 1252588d4e
commit 81b689e5bb
6 changed files with 119 additions and 86 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

94
nova/compute/api.py
View File

@ -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'])

19
nova/objects/block_device.py
View File

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

2
nova/tests/functional/test_servers.py
View File

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

65
nova/tests/unit/compute/test_api.py
View File

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

23
nova/tests/unit/objects/test_block_device.py
View File

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

2
nova/tests/unit/objects/test_objects.py
View File

@ -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',