openstack
/
cinder
Code Issues Proposed changes

Check type match on create from source/snap 

We used to allow creating from source/snap and specifying a
different type than the originating resource when doing so.
Once we started getting more drivers and more multi-backend
configurations, we implemented a check in volume.api that
took this away (broke it).  There have been a number of
arguments about whether this should be allowed or not, and
that it could fail after the rpc call leaving the user with
nothing more than a "failed" volume and no explanation as to
why.

This patch allows the capability, but checks validity at the
API layer before issuing the create call.  There are two
requirements for the new type specification to be valid:
1. There is only one backend (cinder-volume) topic configured
2. Both types in question specify the same volume_backend_name
If neither of these requirements are met, the user will receive
an "invalid type" error explaining that the type combination is
not compatible and that they should omit the type argument altogether.

Change-Id: I08bc5e9a8800ce3b27c7db90b7bff86d7d14359a
Closes-Bug: #1289931
This commit is contained in:
John Griffith 2015-05-12 08:03:27 -06:00
parent daa751a62e
commit 706cbf40f5
3 changed files with 225 additions and 70 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

241
cinder/tests/unit/test_volume.py
View File

@ -958,10 +958,14 @@ class VolumeTestCase(BaseVolumeTestCase):
"""Test volume create from snapshot with types including mistmatch.""" """Test volume create from snapshot with types including mistmatch."""
volume_api = cinder.volume.api.API() volume_api = cinder.volume.api.API()
db.volume_type_create(context.get_admin_context(), db.volume_type_create(
{'name': 'foo', 'extra_specs': {}}) context.get_admin_context(),
db.volume_type_create(context.get_admin_context(), {'name': 'foo',
{'name': 'biz', 'extra_specs': {}}) 'extra_specs': {'volume_backend_name': 'dev_1'}})
db.volume_type_create(
context.get_admin_context(),
{'name': 'biz', 'extra_specs': {'volume_backend_name': 'dev_2'}})
foo_type = db.volume_type_get_by_name(context.get_admin_context(), foo_type = db.volume_type_get_by_name(context.get_admin_context(),
'foo') 'foo')
@ -975,31 +979,34 @@ class VolumeTestCase(BaseVolumeTestCase):
# Make sure the case of specifying a type that # Make sure the case of specifying a type that
# doesn't match the snapshots type fails # doesn't match the snapshots type fails
self.assertRaises(exception.InvalidInput, with mock.patch.object(cinder.volume.volume_types,
volume_api.create, 'get_volume_type') as mock_get_type:
self.context, mock_get_type.return_value = biz_type
size=1, self.assertRaises(exception.InvalidInput,
name='fake_name', volume_api.create,
description='fake_desc', self.context,
volume_type=foo_type, size=1,
snapshot=snapshot) name='fake_name',
description='fake_desc',
volume_type=foo_type,
snapshot=snapshot)
# Make sure that trying to specify a type # Make sure that trying to specify a type
# when the snapshots type is None fails # when the snapshots type is None fails
snapshot['volume_type_id'] = None snapshot['volume_type_id'] = None
self.assertRaises(exception.InvalidInput, self.assertRaises(exception.InvalidInput,
volume_api.create, volume_api.create,
self.context, self.context,
size=1, size=1,
name='fake_name', name='fake_name',
description='fake_desc', description='fake_desc',
volume_type=foo_type, volume_type=foo_type,
snapshot=snapshot) snapshot=snapshot)
snapshot['volume_type_id'] = foo_type['id'] snapshot['volume_type_id'] = foo_type['id']
volume_api.create(self.context, size=1, name='fake_name', volume_api.create(self.context, size=1, name='fake_name',
description='fake_desc', volume_type=foo_type, description='fake_desc', volume_type=foo_type,
snapshot=snapshot) snapshot=snapshot)
db.volume_type_destroy(context.get_admin_context(), db.volume_type_destroy(context.get_admin_context(),
foo_type['id']) foo_type['id'])
@ -1011,10 +1018,14 @@ class VolumeTestCase(BaseVolumeTestCase):
"""Test volume create from source with types including mistmatch.""" """Test volume create from source with types including mistmatch."""
volume_api = cinder.volume.api.API() volume_api = cinder.volume.api.API()
db.volume_type_create(context.get_admin_context(), db.volume_type_create(
{'name': 'foo', 'extra_specs': {}}) context.get_admin_context(),
db.volume_type_create(context.get_admin_context(), {'name': 'foo',
{'name': 'biz', 'extra_specs': {}}) 'extra_specs': {'volume_backend_name': 'dev_1'}})
db.volume_type_create(
context.get_admin_context(),
{'name': 'biz', 'extra_specs': {'volume_backend_name': 'dev_2'}})
foo_type = db.volume_type_get_by_name(context.get_admin_context(), foo_type = db.volume_type_get_by_name(context.get_admin_context(),
'foo') 'foo')
@ -1027,40 +1038,146 @@ class VolumeTestCase(BaseVolumeTestCase):
'volume_type': biz_type, 'volume_type': biz_type,
'volume_type_id': biz_type['id']} 'volume_type_id': biz_type['id']}
# Make sure the case of specifying a type that with mock.patch.object(cinder.volume.volume_types,
# doesn't match the snapshots type fails 'get_volume_type') as mock_get_type:
self.assertRaises(exception.InvalidInput, mock_get_type.return_value = biz_type
volume_api.create, self.assertRaises(exception.InvalidInput,
self.context, volume_api.create,
size=1, self.context,
name='fake_name', size=1,
description='fake_desc', name='fake_name',
volume_type=foo_type, description='fake_desc',
source_volume=source_vol) volume_type=foo_type,
source_volume=source_vol)
# Make sure that trying to specify a type # Make sure that trying to specify a type
# when the source type is None fails # when the source type is None fails
source_vol['volume_type_id'] = None source_vol['volume_type_id'] = None
source_vol['volume_type'] = None source_vol['volume_type'] = None
self.assertRaises(exception.InvalidInput, self.assertRaises(exception.InvalidInput,
volume_api.create, volume_api.create,
self.context, self.context,
size=1, size=1,
name='fake_name', name='fake_name',
description='fake_desc', description='fake_desc',
volume_type=foo_type, volume_type=foo_type,
source_volume=source_vol) source_volume=source_vol)
source_vol['volume_type_id'] = biz_type['id'] source_vol['volume_type_id'] = biz_type['id']
source_vol['volume_type'] = biz_type source_vol['volume_type'] = biz_type
volume_api.create(self.context, size=1, name='fake_name', volume_api.create(self.context, size=1, name='fake_name',
description='fake_desc', volume_type=biz_type, description='fake_desc', volume_type=biz_type,
source_volume=source_vol) source_volume=source_vol)
db.volume_type_destroy(context.get_admin_context(), db.volume_type_destroy(context.get_admin_context(),
foo_type['id']) foo_type['id'])
db.volume_type_destroy(context.get_admin_context(), db.volume_type_destroy(context.get_admin_context(),
biz_type['id']) biz_type['id'])
@mock.patch('cinder.volume.flows.api.create_volume.get_flow')
def test_create_volume_from_source_with_same_backend(self, _get_flow):
"""Test volume create from source with type mismatch same backend."""
volume_api = cinder.volume.api.API()
foo_type = {
'name': 'foo',
'qos_specs_id': None,
'deleted': False,
'created_at': datetime.datetime(2015, 5, 8, 0, 40, 5, 408232),
'updated_at': None,
'extra_specs': {'volume_backend_name': 'dev_1'},
'is_public': True,
'deleted_at': None,
'id': '29e43b50-2cd7-4d0c-8ddd-2119daab3a38',
'description': None}
biz_type = {
'name': 'biz',
'qos_specs_id': None,
'deleted': False,
'created_at': datetime.datetime(2015, 5, 8, 0, 20, 5, 408232),
'updated_at': None,
'extra_specs': {'volume_backend_name': 'dev_1'},
'is_public': True,
'deleted_at': None,
'id': '34e54c31-3bc8-5c1d-9fff-2225bcce4b59',
'description': None}
source_vol = {'id': 1234,
'status': 'available',
'volume_size': 10,
'volume_type': biz_type,
'volume_type_id': biz_type['id']}
with mock.patch.object(cinder.volume.volume_types,
'get_volume_type') as mock_get_type:
mock_get_type.return_value = biz_type
volume_api.create(self.context,
size=1,
name='fake_name',
description='fake_desc',
volume_type=foo_type,
source_volume=source_vol)
@mock.patch('cinder.volume.flows.api.create_volume.get_flow')
def test_create_from_source_and_snap_only_one_backend(self, _get_flow):
"""Test create from source and snap with type mismatch one backend."""
volume_api = cinder.volume.api.API()
foo_type = {
'name': 'foo',
'qos_specs_id': None,
'deleted': False,
'created_at': datetime.datetime(2015, 5, 8, 0, 40, 5, 408232),
'updated_at': None,
'extra_specs': {'some_key': 3},
'is_public': True,
'deleted_at': None,
'id': '29e43b50-2cd7-4d0c-8ddd-2119daab3a38',
'description': None}
biz_type = {
'name': 'biz',
'qos_specs_id': None,
'deleted': False,
'created_at': datetime.datetime(2015, 5, 8, 0, 20, 5, 408232),
'updated_at': None,
'extra_specs': {'some_other_key': 4},
'is_public': True,
'deleted_at': None,
'id': '34e54c31-3bc8-5c1d-9fff-2225bcce4b59',
'description': None}
source_vol = {'id': 1234,
'status': 'available',
'volume_size': 10,
'volume_type': biz_type,
'volume_type_id': biz_type['id']}
snapshot = {'id': 1234,
'status': 'available',
'volume_size': 10,
'volume_type_id': biz_type['id']}
with mock.patch.object(db,
'service_get_all_by_topic') as mock_get_service, \
mock.patch.object(volume_api,
'list_availability_zones') as mock_get_azs:
mock_get_service.return_value = ['foo']
mock_get_azs.return_value = {}
volume_api.create(self.context,
size=1,
name='fake_name',
description='fake_desc',
volume_type=foo_type,
source_volume=source_vol)
volume_api.create(self.context,
size=1,
name='fake_name',
description='fake_desc',
volume_type=foo_type,
snapshot=snapshot)
def test_create_snapshot_driver_not_initialized(self): def test_create_snapshot_driver_not_initialized(self):
volume_src = tests_utils.create_volume(self.context, volume_src = tests_utils.create_volume(self.context,

45
cinder/volume/api.py
View File

@ -165,6 +165,26 @@ class API(base.Base):
LOG.info(_LI("Availability Zones retrieved successfully.")) LOG.info(_LI("Availability Zones retrieved successfully."))
return tuple(azs) return tuple(azs)
def _retype_is_possible(self, context,
first_type_id, second_type_id,
first_type=None, second_type=None):
safe = False
if len(self.db.service_get_all_by_topic(context,
'cinder-volume',
disabled=True)) == 1:
safe = True
else:
type_a = first_type or volume_types.get_volume_type(
context,
first_type_id)
type_b = second_type or volume_types.get_volume_type(
context,
second_type_id)
if(volume_utils.matching_backend_name(type_a['extra_specs'],
type_b['extra_specs'])):
safe = True
return safe
def create(self, context, size, name, description, snapshot=None, def create(self, context, size, name, description, snapshot=None,
image_id=None, volume_type=None, metadata=None, image_id=None, volume_type=None, metadata=None,
availability_zone=None, source_volume=None, availability_zone=None, source_volume=None,
@ -202,10 +222,15 @@ class API(base.Base):
if source_volume and volume_type: if source_volume and volume_type:
if volume_type['id'] != source_volume['volume_type_id']: if volume_type['id'] != source_volume['volume_type_id']:
msg = _("Invalid volume_type provided: %s (requested type " if not self._retype_is_possible(
"must match source volume, " context,
"or be omitted).") % volume_type volume_type['id'],
raise exception.InvalidInput(reason=msg) source_volume['volume_type_id'],
volume_type):
msg = _("Invalid volume_type provided: %s (requested type "
"is not compatible; either match source volume, "
"or omit type argument).") % volume_type['id']
raise exception.InvalidInput(reason=msg)
# When cloning replica (for testing), volume type must be omitted # When cloning replica (for testing), volume type must be omitted
if source_replica and volume_type: if source_replica and volume_type:
@ -215,10 +240,14 @@ class API(base.Base):
if snapshot and volume_type: if snapshot and volume_type:
if volume_type['id'] != snapshot['volume_type_id']: if volume_type['id'] != snapshot['volume_type_id']:
msg = _("Invalid volume_type provided: %s (requested " if not self._retype_is_possible(context,
"type must match source snapshot, or be " volume_type['id'],
"omitted).") % volume_type snapshot['volume_type_id'],
raise exception.InvalidInput(reason=msg) volume_type):
msg = _("Invalid volume_type provided: %s (requested "
"type is not compatible; recommend omitting "
"the type argument).") % volume_type['id']
raise exception.InvalidInput(reason=msg)
# Determine the valid availability zones that the volume could be # Determine the valid availability zones that the volume could be
# created in (a task in the flow will/can use this information to # created in (a task in the flow will/can use this information to

9
cinder/volume/utils.py
View File

@ -498,3 +498,12 @@ def append_host(host, pool):
new_host = "#".join([host, pool]) new_host = "#".join([host, pool])
return new_host return new_host
def matching_backend_name(src_volume_type, volume_type):
if src_volume_type.get('volume_backend_name') and \
volume_type.get('volume_backend_name'):
return src_volume_type.get('volume_backend_name') == \
volume_type.get('volume_backend_name')
else:
return False