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

diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py
index 8f2d1644ff28a9c0cd43c8af2c2d68fa5daa83bf..df53d28be42913b86bd7214bb70da508843f5c66 100644 (file)
--- 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)
-
-        # 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)
+        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)
 
-        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)
+
+            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'])
 
-        # 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)
+    @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()
 
-        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)
+        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}
 
-        db.volume_type_destroy(context.get_admin_context(),
-                               foo_type['id'])
-        db.volume_type_destroy(context.get_admin_context(),
-                               biz_type['id'])
+        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 d4bde31fc1a3afe1d4c27fd4b6c5cf0d422ddd40..56379bfc80babc6343a14c4de09198d6cf8aec9a 100644 (file)
--- 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 d242118581818721489c8b4e8dfa4ec151f83f95..48a5236e473702d5ed2a145eeea54357c17fe197 100644 (file)
--- 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