openstack
/
nova
Code Issues Proposed changes

Fix undetected races when getting BDMs by volume id 

This deprecates the BDM.get_by_volume_id() method and introduces two new ones.
The existing method will fail to detect that multiple instances can have BDMs
pointing at the same volume. This could happen via race, and will be normal
behavior when we get volume multi-attach support.

Instead of this method which takes instance as an optional parameter, a new
method is introduced which requires the property when searching for a single
BDM for a volume. Another method is added that allows us to fetch BDMs for
volumes where there should be only one instance attached. This method checks
that the single-owner consistency is correct, or fails otherwise.

Closes-Bug: #1533834
Co-Authored-By: Ildiko Vancsa <ildiko.vancsa@ericsson.com>
Co-Authored-By: Krisztian Gacsal <krisztian.gacsal@ericsson.com>
Change-Id: I06cdeef38aeb3f3e65dfc0ced8a75960b5d57f49
This commit is contained in:
Dan Smith 2016-01-13 13:12:20 -08:00 committed by Matt Riedemann
parent 5dfdf35c96
commit c1a61fcc37
8 changed files with 182 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
nova/db/api.py
View File

@ -1225,13 +1225,21 @@ def block_device_mapping_get_all_by_instance(context, instance_uuid,
use_slave)
def block_device_mapping_get_by_volume_id(context, volume_id,
def block_device_mapping_get_all_by_volume_id(context, volume_id,
columns_to_join=None):
"""Get block device mapping for a given volume."""
return IMPL.block_device_mapping_get_by_volume_id(context, volume_id,
return IMPL.block_device_mapping_get_all_by_volume_id(context, volume_id,
columns_to_join)
def block_device_mapping_get_by_instance_and_volume_id(context, volume_id,
instance_uuid,
columns_to_join=None):
"""Get block device mapping for a given volume ID and instance UUID."""
return IMPL.block_device_mapping_get_by_instance_and_volume_id(
context, volume_id, instance_uuid, columns_to_join)
def block_device_mapping_destroy(context, bdm_id):
"""Destroy the block device mapping."""
return IMPL.block_device_mapping_destroy(context, bdm_id)

13
nova/db/sqlalchemy/api.py
View File

@ -4027,11 +4027,22 @@ def block_device_mapping_get_all_by_instance(context, instance_uuid,
@require_context
def block_device_mapping_get_by_volume_id(context, volume_id,
def block_device_mapping_get_all_by_volume_id(context, volume_id,
columns_to_join=None):
return _block_device_mapping_get_query(context,
columns_to_join=columns_to_join).\
filter_by(volume_id=volume_id).\
all()
@require_context
def block_device_mapping_get_by_instance_and_volume_id(context, volume_id,
instance_uuid,
columns_to_join=None):
return _block_device_mapping_get_query(context,
columns_to_join=columns_to_join).\
filter_by(volume_id=volume_id).\
filter_by(instance_uuid=instance_uuid).\
first()

5
nova/exception.py
View File

@ -635,6 +635,11 @@ class VolumeBDMNotFound(NotFound):
msg_fmt = _("No volume Block Device Mapping with id %(volume_id)s.")
class VolumeBDMIsMultiAttach(Invalid):
msg_fmt = _("Block Device Mapping %(volume_id)s is a multi-attach volume"
" and is not valid for this operation.")
class VolumeBDMPathNotFound(VolumeBDMNotFound):
msg_fmt = _("No volume Block Device Mapping at path: %(path)s")

43
nova/objects/block_device.py
View File

@ -19,7 +19,7 @@ from nova.cells import opts as cells_opts
from nova.cells import rpcapi as cells_rpcapi
from nova import db
from nova import exception
from nova.i18n import _
from nova.i18n import _, _LW
from nova import objects
from nova.objects import base
from nova.objects import fields
@ -58,7 +58,9 @@ class BlockDeviceMapping(base.NovaPersistentObject, base.NovaObject,
# Version 1.13: Instance version 1.21
# Version 1.14: Instance version 1.22
# Version 1.15: Instance version 1.23
VERSION = '1.15'
# Version 1.16: Deprecate get_by_volume_id(), add
# get_by_volume() and get_by_volume_and_instance()
VERSION = '1.16'
fields = {
'id': fields.IntegerField(),
@ -195,15 +197,22 @@ class BlockDeviceMapping(base.NovaPersistentObject, base.NovaObject,
cells_api.bdm_update_or_create_at_top(self._context, self,
create=create)
# NOTE(danms): This method is deprecated and will be removed in
# v2.0 of the object
@base.remotable_classmethod
def get_by_volume_id(cls, context, volume_id,
instance_uuid=None, expected_attrs=None):
if expected_attrs is None:
expected_attrs = []
db_bdm = db.block_device_mapping_get_by_volume_id(
db_bdms = db.block_device_mapping_get_all_by_volume_id(
context, volume_id, _expected_cols(expected_attrs))
if not db_bdm:
if not db_bdms:
raise exception.VolumeBDMNotFound(volume_id=volume_id)
if len(db_bdms) > 1:
LOG.warning(_LW('Legacy get_by_volume_id() call found multiple '
'BDMs for volume %(volume)s'),
{'volume': volume_id})
db_bdm = db_bdms[0]
# NOTE (ndipanov): Move this to the db layer into a
# get_by_instance_and_volume_id method
if instance_uuid and instance_uuid != db_bdm['instance_uuid']:
@ -213,6 +222,32 @@ class BlockDeviceMapping(base.NovaPersistentObject, base.NovaObject,
return cls._from_db_object(context, cls(), db_bdm,
expected_attrs=expected_attrs)
@base.remotable_classmethod
def get_by_volume_and_instance(cls, context, volume_id, instance_uuid,
expected_attrs=None):
if expected_attrs is None:
expected_attrs = []
db_bdm = db.block_device_mapping_get_by_instance_and_volume_id(
context, volume_id, instance_uuid,
_expected_cols(expected_attrs))
if not db_bdm:
raise exception.VolumeBDMNotFound(volume_id=volume_id)
return cls._from_db_object(context, cls(), db_bdm,
expected_attrs=expected_attrs)
@base.remotable_classmethod
def get_by_volume(cls, context, volume_id, expected_attrs=None):
if expected_attrs is None:
expected_attrs = []
db_bdms = db.block_device_mapping_get_all_by_volume_id(
context, volume_id, _expected_cols(expected_attrs))
if not db_bdms:
raise exception.VolumeBDMNotFound(volume_id=volume_id)
if len(db_bdms) > 1:
raise exception.VolumeBDMIsMultiAttach(volume_id=volume_id)
return cls._from_db_object(context, cls(), db_bdms[0],
expected_attrs=expected_attrs)
@property
def is_root(self):
return self.boot_index == 0

4
nova/tests/unit/compute/test_compute_mgr.py
View File

@ -1718,8 +1718,8 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
fake_vol_unreserve)
self.stubs.Set(self.compute.volume_api, 'terminate_connection',
fake_vol_api_func)
self.stubs.Set(db, 'block_device_mapping_get_by_volume_id',
lambda x, y, z: fake_bdm)
self.stubs.Set(db, 'block_device_mapping_get_all_by_volume_id',
lambda x, y, z: [fake_bdm])
self.stubs.Set(self.compute.driver, 'get_volume_connector',
lambda x: {})
self.stubs.Set(self.compute.driver, 'swap_volume',

54
nova/tests/unit/db/test_db_api.py
View File

@ -6153,17 +6153,57 @@ class BlockDeviceMappingTestCase(test.TestCase):
self.assertEqual(len(bdms), 1)
self.assertEqual(bdms[0]['device_name'], '/dev/vda')
def test_block_device_mapping_get_by_volume_id(self):
def test_block_device_mapping_get_all_by_volume_id(self):
self._create_bdm({'volume_id': 'fake_id'})
bdm = db.block_device_mapping_get_by_volume_id(self.ctxt, 'fake_id')
self.assertEqual(bdm['volume_id'], 'fake_id')
self._create_bdm({'volume_id': 'fake_id'})
bdms = db.block_device_mapping_get_all_by_volume_id(self.ctxt,
'fake_id')
self.assertEqual(bdms[0]['volume_id'], 'fake_id')
self.assertEqual(bdms[1]['volume_id'], 'fake_id')
self.assertEqual(2, len(bdms))
def test_block_device_mapping_get_by_volume_id_join_instance(self):
def test_block_device_mapping_get_all_by_volume_id_join_instance(self):
self._create_bdm({'volume_id': 'fake_id'})
bdm = db.block_device_mapping_get_by_volume_id(self.ctxt, 'fake_id',
['instance'])
bdms = db.block_device_mapping_get_all_by_volume_id(self.ctxt,
'fake_id',
['instance'])
self.assertEqual(bdms[0]['volume_id'], 'fake_id')
self.assertEqual(bdms[0]['instance']['uuid'], self.instance['uuid'])
def test_block_device_mapping_get_by_instance_and_volume_id(self):
self._create_bdm({'volume_id': 'fake_id'})
bdm = db.block_device_mapping_get_by_instance_and_volume_id(self.ctxt,
'fake_id', self.instance['uuid'])
self.assertEqual(bdm['volume_id'], 'fake_id')
self.assertEqual(bdm['instance']['uuid'], self.instance['uuid'])
self.assertEqual(bdm['instance_uuid'], self.instance['uuid'])
def test_block_device_mapping_get_by_instance_and_volume_id_multiplebdms(
self):
self._create_bdm({'volume_id': 'fake_id',
'instance_uuid': self.instance['uuid']})
self._create_bdm({'volume_id': 'fake_id',
'instance_uuid': self.instance['uuid']})
db_bdm = db.block_device_mapping_get_by_instance_and_volume_id(
self.ctxt, 'fake_id', self.instance['uuid'])
self.assertIsNotNone(db_bdm)
self.assertEqual(self.instance['uuid'], db_bdm['instance_uuid'])
def test_block_device_mapping_get_by_instance_and_volume_id_multiattach(
self):
self.instance2 = db.instance_create(self.ctxt, {})
self._create_bdm({'volume_id': 'fake_id',
'instance_uuid': self.instance['uuid']})
self._create_bdm({'volume_id': 'fake_id',
'instance_uuid': self.instance2['uuid']})
bdm = db.block_device_mapping_get_by_instance_and_volume_id(self.ctxt,
'fake_id', self.instance['uuid'])
self.assertEqual(bdm['volume_id'], 'fake_id')
self.assertEqual(bdm['instance_uuid'], self.instance['uuid'])
bdm2 = db.block_device_mapping_get_by_instance_and_volume_id(
self.ctxt, 'fake_id', self.instance2['uuid'])
self.assertEqual(bdm2['volume_id'], 'fake_id')
self.assertEqual(bdm2['instance_uuid'], self.instance2['uuid'])
class AgentBuildTestCase(test.TestCase, ModelsObjectComparatorMixin):

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

@ -110,16 +110,20 @@ class _TestBlockDeviceMappingObject(object):
bdm_object.id = 123
self.assertRaises(exception.BDMNotFound, bdm_object.save)
@mock.patch.object(db, 'block_device_mapping_get_by_volume_id')
@mock.patch.object(db, 'block_device_mapping_get_all_by_volume_id')
def test_get_by_volume_id(self, get_by_vol_id):
get_by_vol_id.return_value = self.fake_bdm()
# NOTE(danms): Include two results to make sure the first was picked.
# An invalid second item shouldn't be touched -- if it is, it'll
# fail from_db_object().
get_by_vol_id.return_value = [self.fake_bdm(),
None]
vol_bdm = objects.BlockDeviceMapping.get_by_volume_id(
self.context, 'fake-volume-id')
for attr in block_device_obj.BLOCK_DEVICE_OPTIONAL_ATTRS:
self.assertFalse(vol_bdm.obj_attr_is_set(attr))
@mock.patch.object(db, 'block_device_mapping_get_by_volume_id')
@mock.patch.object(db, 'block_device_mapping_get_all_by_volume_id')
def test_get_by_volume_id_not_found(self, get_by_vol_id):
get_by_vol_id.return_value = None
@ -127,20 +131,20 @@ class _TestBlockDeviceMappingObject(object):
objects.BlockDeviceMapping.get_by_volume_id,
self.context, 'fake-volume-id')
@mock.patch.object(db, 'block_device_mapping_get_by_volume_id')
@mock.patch.object(db, 'block_device_mapping_get_all_by_volume_id')
def test_get_by_volume_instance_uuid_missmatch(self, get_by_vol_id):
fake_bdm_vol = self.fake_bdm(instance={'uuid': 'other-fake-instance'})
get_by_vol_id.return_value = fake_bdm_vol
get_by_vol_id.return_value = [fake_bdm_vol]
self.assertRaises(exception.InvalidVolume,
objects.BlockDeviceMapping.get_by_volume_id,
self.context, 'fake-volume-id',
instance_uuid='fake-instance')
@mock.patch.object(db, 'block_device_mapping_get_by_volume_id')
@mock.patch.object(db, 'block_device_mapping_get_all_by_volume_id')
def test_get_by_volume_id_with_expected(self, get_by_vol_id):
get_by_vol_id.return_value = self.fake_bdm(
fake_instance.fake_db_instance())
get_by_vol_id.return_value = [self.fake_bdm(
fake_instance.fake_db_instance())]
vol_bdm = objects.BlockDeviceMapping.get_by_volume_id(
self.context, 'fake-volume-id', expected_attrs=['instance'])
@ -149,6 +153,60 @@ class _TestBlockDeviceMappingObject(object):
get_by_vol_id.assert_called_once_with(self.context, 'fake-volume-id',
['instance'])
@mock.patch.object(db, 'block_device_mapping_get_all_by_volume_id')
def test_get_by_volume_returned_single(self, get_all):
fake_bdm_vol = self.fake_bdm()
get_all.return_value = [fake_bdm_vol]
vol_bdm = objects.BlockDeviceMapping.get_by_volume(
self.context, 'fake-volume-id')
self.assertEqual(fake_bdm_vol['id'], vol_bdm.id)
@mock.patch.object(db, 'block_device_mapping_get_all_by_volume_id')
def test_get_by_volume_returned_multiple(self, get_all):
fake_bdm_vol1 = self.fake_bdm()
fake_bdm_vol2 = self.fake_bdm()
get_all.return_value = [fake_bdm_vol1, fake_bdm_vol2]
self.assertRaises(exception.VolumeBDMIsMultiAttach,
objects.BlockDeviceMapping.get_by_volume,
self.context, 'fake-volume-id')
@mock.patch.object(db,
'block_device_mapping_get_by_instance_and_volume_id')
def test_get_by_instance_and_volume_id(self, mock_get):
fake_inst = fake_instance.fake_db_instance()
mock_get.return_value = self.fake_bdm(fake_inst)
obj_bdm = objects.BlockDeviceMapping
vol_bdm = obj_bdm.get_by_volume_and_instance(
self.context, 'fake-volume-id', 'fake-instance-id')
for attr in block_device_obj.BLOCK_DEVICE_OPTIONAL_ATTRS:
self.assertFalse(vol_bdm.obj_attr_is_set(attr))
@mock.patch.object(db,
'block_device_mapping_get_by_instance_and_volume_id')
def test_test_get_by_instance_and_volume_id_with_expected(self, mock_get):
fake_inst = fake_instance.fake_db_instance()
mock_get.return_value = self.fake_bdm(fake_inst)
obj_bdm = objects.BlockDeviceMapping
vol_bdm = obj_bdm.get_by_volume_and_instance(
self.context, 'fake-volume-id', fake_inst['uuid'],
expected_attrs=['instance'])
for attr in block_device_obj.BLOCK_DEVICE_OPTIONAL_ATTRS:
self.assertTrue(vol_bdm.obj_attr_is_set(attr))
mock_get.assert_called_once_with(self.context, 'fake-volume-id',
fake_inst['uuid'], ['instance'])
@mock.patch.object(db,
'block_device_mapping_get_by_instance_and_volume_id')
def test_get_by_instance_and_volume_id_not_found(self, mock_get):
mock_get.return_value = None
obj_bdm = objects.BlockDeviceMapping
self.assertRaises(exception.VolumeBDMNotFound,
obj_bdm.get_by_volume_and_instance,
self.context, 'fake-volume-id', 'fake-instance-id')
def _test_create_mocked(self, cell_type=None, update_or_create=False,
device_name=None):
if cell_type:

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

@ -1113,7 +1113,7 @@ object_data = {
'AggregateList': '1.2-fb6e19f3c3a3186b04eceb98b5dadbfa',
'BandwidthUsage': '1.2-c6e4c779c7f40f2407e3d70022e3cd1c',
'BandwidthUsageList': '1.2-5fe7475ada6fe62413cbfcc06ec70746',
'BlockDeviceMapping': '1.15-d44d8d694619e79c172a99b3c1d6261d',
'BlockDeviceMapping': '1.16-12319f6f47f740a67a88a23f7c7ee6ef',
'BlockDeviceMappingList': '1.17-1e568eecb91d06d4112db9fd656de235',
'CellMapping': '1.0-7f1a7e85a22bbb7559fc730ab658b9bd',
'ComputeNode': '1.14-a396975707b66281c5f404a68fccd395',