From 706cbf40f5f8c6f756277057c92bf41f1ef1cd80 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Tue, 12 May 2015 08:03:27 -0600 Subject: [PATCH] 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 --- cinder/tests/unit/test_volume.py | 241 +++++++++++++++++++++++-------- cinder/volume/api.py | 45 +++++- cinder/volume/utils.py | 9 ++ 3 files changed, 225 insertions(+), 70 deletions(-) diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index 8f2d1644ff2..df53d28be42 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -958,10 +958,14 @@ class VolumeTestCase(BaseVolumeTestCase): """Test volume create from snapshot with types including mistmatch.""" volume_api = cinder.volume.api.API() - db.volume_type_create(context.get_admin_context(), - {'name': 'foo', 'extra_specs': {}}) - db.volume_type_create(context.get_admin_context(), - {'name': 'biz', 'extra_specs': {}}) + db.volume_type_create( + context.get_admin_context(), + {'name': 'foo', + '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') @@ -975,31 +979,34 @@ class VolumeTestCase(BaseVolumeTestCase): # Make sure the case of specifying a type that # doesn't match the snapshots type fails - self.assertRaises(exception.InvalidInput, - volume_api.create, - self.context, - size=1, - name='fake_name', - description='fake_desc', - volume_type=foo_type, - snapshot=snapshot) + with mock.patch.object(cinder.volume.volume_types, + 'get_volume_type') as mock_get_type: + mock_get_type.return_value = biz_type + self.assertRaises(exception.InvalidInput, + volume_api.create, + self.context, + size=1, + name='fake_name', + description='fake_desc', + volume_type=foo_type, + snapshot=snapshot) - # Make sure that trying to specify a type - # when the snapshots type is None fails - snapshot['volume_type_id'] = None - self.assertRaises(exception.InvalidInput, - volume_api.create, - self.context, - size=1, - name='fake_name', - description='fake_desc', - volume_type=foo_type, - snapshot=snapshot) + # Make sure that trying to specify a type + # when the snapshots type is None fails + snapshot['volume_type_id'] = None + self.assertRaises(exception.InvalidInput, + volume_api.create, + self.context, + size=1, + name='fake_name', + description='fake_desc', + volume_type=foo_type, + snapshot=snapshot) - snapshot['volume_type_id'] = foo_type['id'] - volume_api.create(self.context, size=1, name='fake_name', - description='fake_desc', volume_type=foo_type, - snapshot=snapshot) + snapshot['volume_type_id'] = foo_type['id'] + volume_api.create(self.context, size=1, name='fake_name', + description='fake_desc', volume_type=foo_type, + snapshot=snapshot) db.volume_type_destroy(context.get_admin_context(), foo_type['id']) @@ -1011,10 +1018,14 @@ class VolumeTestCase(BaseVolumeTestCase): """Test volume create from source with types including mistmatch.""" volume_api = cinder.volume.api.API() - db.volume_type_create(context.get_admin_context(), - {'name': 'foo', 'extra_specs': {}}) - db.volume_type_create(context.get_admin_context(), - {'name': 'biz', 'extra_specs': {}}) + db.volume_type_create( + context.get_admin_context(), + {'name': 'foo', + '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') @@ -1027,40 +1038,146 @@ class VolumeTestCase(BaseVolumeTestCase): 'volume_type': biz_type, 'volume_type_id': biz_type['id']} - # Make sure the case of specifying a type that - # doesn't match the snapshots type fails - self.assertRaises(exception.InvalidInput, - volume_api.create, - self.context, - size=1, - name='fake_name', - description='fake_desc', - volume_type=foo_type, - source_volume=source_vol) + with mock.patch.object(cinder.volume.volume_types, + 'get_volume_type') as mock_get_type: + mock_get_type.return_value = biz_type + self.assertRaises(exception.InvalidInput, + volume_api.create, + self.context, + size=1, + name='fake_name', + description='fake_desc', + volume_type=foo_type, + source_volume=source_vol) - # Make sure that trying to specify a type - # when the source type is None fails - source_vol['volume_type_id'] = None - source_vol['volume_type'] = None - self.assertRaises(exception.InvalidInput, - volume_api.create, - self.context, - size=1, - name='fake_name', - description='fake_desc', - volume_type=foo_type, - source_volume=source_vol) + # Make sure that trying to specify a type + # when the source type is None fails + source_vol['volume_type_id'] = None + source_vol['volume_type'] = None + self.assertRaises(exception.InvalidInput, + volume_api.create, + self.context, + size=1, + name='fake_name', + description='fake_desc', + volume_type=foo_type, + source_volume=source_vol) - source_vol['volume_type_id'] = biz_type['id'] - source_vol['volume_type'] = biz_type - volume_api.create(self.context, size=1, name='fake_name', - description='fake_desc', volume_type=biz_type, - source_volume=source_vol) + source_vol['volume_type_id'] = biz_type['id'] + source_vol['volume_type'] = biz_type + volume_api.create(self.context, size=1, name='fake_name', + description='fake_desc', volume_type=biz_type, + source_volume=source_vol) - db.volume_type_destroy(context.get_admin_context(), - foo_type['id']) - db.volume_type_destroy(context.get_admin_context(), - biz_type['id']) + db.volume_type_destroy(context.get_admin_context(), + foo_type['id']) + db.volume_type_destroy(context.get_admin_context(), + 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): volume_src = tests_utils.create_volume(self.context, diff --git a/cinder/volume/api.py b/cinder/volume/api.py index d4bde31fc1a..56379bfc80b 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -165,6 +165,26 @@ class API(base.Base): LOG.info(_LI("Availability Zones retrieved successfully.")) 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, image_id=None, volume_type=None, metadata=None, availability_zone=None, source_volume=None, @@ -202,10 +222,15 @@ class API(base.Base): if source_volume and volume_type: if volume_type['id'] != source_volume['volume_type_id']: - msg = _("Invalid volume_type provided: %s (requested type " - "must match source volume, " - "or be omitted).") % volume_type - raise exception.InvalidInput(reason=msg) + if not self._retype_is_possible( + context, + volume_type['id'], + 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 if source_replica and volume_type: @@ -215,10 +240,14 @@ class API(base.Base): if snapshot and volume_type: if volume_type['id'] != snapshot['volume_type_id']: - msg = _("Invalid volume_type provided: %s (requested " - "type must match source snapshot, or be " - "omitted).") % volume_type - raise exception.InvalidInput(reason=msg) + if not self._retype_is_possible(context, + volume_type['id'], + snapshot['volume_type_id'], + 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 # created in (a task in the flow will/can use this information to diff --git a/cinder/volume/utils.py b/cinder/volume/utils.py index d2421185818..48a5236e473 100644 --- a/cinder/volume/utils.py +++ b/cinder/volume/utils.py @@ -498,3 +498,12 @@ def append_host(host, pool): new_host = "#".join([host, pool]) 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