From 68228eb7f41f419eec2427a5e1a14f6612a705ff Mon Sep 17 00:00:00 2001 From: john-griffith Date: Mon, 10 Feb 2014 14:42:43 -0700 Subject: [PATCH] Check type argument on create from source and snap Currently the create from snap and create from source options will set the volume-type when specified in the create call. This is actually wrong, because clone and create from snap MUST be performed on the same backend as the parent volume. The result is that if the specified type doesn't match the specs for it's parent, and actually needs to be on a different backend the new volume will "appear" to have the correct type, but it's actually invalid. The --volume-type argument should be rejected when --source-volid or --snapshot-id are used, and the parent type-id just picked up and used. Then the user can retype if desired. Auto failing this strictly causes backward compatability issues though so we'll just check for a mismatch and raise an exception in that case. This way if the user knows what they're doing and they're explicit in the command we still allow it and things are fine. Change-Id: Ia822cfe48a948045ccea6533bcf92e68ed97ef37 Closes-Bug: 1276787 --- cinder/tests/test_volume.py | 109 ++++++++++++++++++++++++++++++++++++ cinder/volume/api.py | 20 ++++++- 2 files changed, 126 insertions(+), 3 deletions(-) diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index a5a2aba95..3902f3ee1 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -521,6 +521,115 @@ class VolumeTestCase(BaseVolumeTestCase): self.volume.delete_snapshot(self.context, snapshot_id) self.volume.delete_volume(self.context, volume_src['id']) + @mock.patch('cinder.volume.flows.api.create_volume.get_flow') + def test_create_volume_from_snapshot_with_types(self, _get_flow): + """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': {}}) + + foo_type = db.volume_type_get_by_name(context.get_admin_context(), + 'foo') + biz_type = db.volume_type_get_by_name(context.get_admin_context(), + 'biz') + + snapshot = {'id': 1234, + 'status': 'available', + 'volume_size': 10, + '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, + 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) + + 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_types(self, _get_flow): + """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': {}}) + + foo_type = db.volume_type_get_by_name(context.get_admin_context(), + 'foo') + biz_type = db.volume_type_get_by_name(context.get_admin_context(), + 'biz') + + source_vol = {'id': 1234, + 'status': 'available', + 'volume_size': 10, + '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) + + # 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) + + db.volume_type_destroy(context.get_admin_context(), + foo_type['id']) + db.volume_type_destroy(context.get_admin_context(), + biz_type['id']) + def test_create_snapshot_driver_not_initialized(self): volume_src = tests_utils.create_volume(self.context, **self.volume_params) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 533b2f48d..2c1c65e83 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -136,6 +136,20 @@ class API(base.Base): availability_zone=None, source_volume=None, scheduler_hints=None, backup_source_volume=None): + if source_volume and volume_type: + if volume_type['id'] != source_volume['volume_type_id']: + msg = _("Invalid volume_type provided (requested type " + "must match source volume, or be omitted). " + "You should omit the argument.") + raise exception.InvalidInput(reason=msg) + + if snapshot and volume_type: + if volume_type['id'] != snapshot['volume_type_id']: + msg = _("Invalid volume_type provided (requested type " + "must match source snapshot, or be omitted). " + "You should omit the argument.") + raise exception.InvalidInput(reason=msg) + def check_volume_az_zone(availability_zone): try: return self._valid_availability_zone(availability_zone) @@ -219,7 +233,7 @@ class API(base.Base): # Volume is still attached, need to detach first raise exception.VolumeAttached(volume_id=volume_id) - if volume['migration_status'] != None: + if volume['migration_status'] is not None: # Volume is migrating, wait until done msg = _("Volume cannot be deleted while migrating") raise exception.InvalidVolume(reason=msg) @@ -459,7 +473,7 @@ class API(base.Base): force=False, metadata=None): check_policy(context, 'create_snapshot', volume) - if volume['migration_status'] != None: + if volume['migration_status'] is not None: # Volume is migrating, wait until done msg = _("Snapshot cannot be created while volume is migrating") raise exception.InvalidVolume(reason=msg) @@ -796,7 +810,7 @@ class API(base.Base): raise exception.InvalidVolume(reason=msg) # Make sure volume is not part of a migration - if volume['migration_status'] != None: + if volume['migration_status'] is not None: msg = _("Volume is already part of an active migration") raise exception.InvalidVolume(reason=msg) -- 2.45.2