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

diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py
index a5a2aba95dad5c231980b69a6054ee1bcd3f4213..3902f3ee16ab6ffa167447e9296f193ea4601bc5 100644 (file)
--- 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 533b2f48d22f028cd3ecec166351341041538652..2c1c65e83fec4c7f90ded7e0c8afe629ea1c0540 100644 (file)
--- 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)