Default volume_type set too early

If a volume_type is not specified in a volume-create request, change
I4da0c13b5b3f8174a30b8557f968d6b9e641b091 (introduced in Train) sets a
default volume_type in the REST API layer.  This prevents the
selection logic in cinder.volume.flows.api.create_volume.
ExtractVolumeRequestTask from being able to infer the appropriate
volume_type from the source volume, snapshot, or image metadata, and
has caused a regression where the created volume is of the default
type instead of the inferred type.

This patch removes setting the default volume_type in the REST API
and modifies the selection code in ExtractVolumeRequestTask slightly
to make sure a volume_type is always assigned in that function, and
adds and revises some tests.

Change-Id: I05915f2e32b1229ad320cd1c5748de3d63183b91
Closes-bug: #1879578

cinder/api/v2/volumes.py

@@ -39,7 +39,6 @@ from cinder.image import glance
 from cinder import objects
 from cinder import utils
 from cinder import volume as cinder_volume
-from cinder.volume import volume_types
 from cinder.volume import volume_utils
 
 CONF = cfg.CONF
@@ -214,11 +213,6 @@ class VolumeController(wsgi.Controller):
             # Not found exception will be handled at the wsgi level
             kwargs['volume_type'] = (
                 objects.VolumeType.get_by_name_or_id(context, req_volume_type))
-        else:
-            kwargs['volume_type'] = (
-                objects.VolumeType.get_by_name_or_id(
-                    context,
-                    volume_types.get_default_volume_type()['id']))
 
         kwargs['metadata'] = volume.get('metadata', None)
 

cinder/api/v3/volumes.py

@@ -38,7 +38,6 @@ from cinder.image import glance
 from cinder import objects
 from cinder.policies import volumes as policy
 from cinder import utils
-from cinder.volume import volume_types
 
 LOG = logging.getLogger(__name__)
 
@@ -321,11 +320,6 @@ class VolumeController(volumes_v2.VolumeController):
             # Not found exception will be handled at the wsgi level
             kwargs['volume_type'] = (
                 objects.VolumeType.get_by_name_or_id(context, req_volume_type))
-        else:
-            kwargs['volume_type'] = (
-                objects.VolumeType.get_by_name_or_id(
-                    context,
-                    volume_types.get_default_volume_type()['id']))
 
         kwargs['metadata'] = volume.get('metadata', None)
 

cinder/tests/functional/api/client.py

@@ -218,6 +218,15 @@ class TestOpenStackClient(object):
     def put_volume(self, volume_id, volume):
         return self.api_put('/volumes/%s' % volume_id, volume)['volume']
 
+    def get_snapshot(self, snapshot_id):
+        return self.api_get('/snapshots/%s' % snapshot_id)['snapshot']
+
+    def post_snapshot(self, snapshot):
+        return self.api_post('/snapshots', snapshot)['snapshot']
+
+    def delete_snapshot(self, snapshot_id):
+        return self.api_delete('/snapshots/%s' % snapshot_id)
+
     def quota_set(self, project_id, quota_update):
         return self.api_put(
             'os-quota-sets/%s' % project_id,

cinder/tests/functional/functional_helpers.py

@@ -31,6 +31,7 @@ from cinder.tests.unit import test  # For the flags
 
 CONF = cfg.CONF
 VOLUME = 'VOLUME'
+SNAPSHOT = 'SNAPSHOT'
 GROUP = 'GROUP'
 GROUP_SNAPSHOT = 'GROUP_SNAPSHOT'
 
@@ -163,6 +164,8 @@ class _FunctionalTestBase(test.TestCase):
             try:
                 if res_type == VOLUME:
                     found_res = self.api.get_volume(res_id)
+                elif res_type == SNAPSHOT:
+                    found_res = self.api.get_snapshot(res_id)
                 elif res_type == GROUP:
                     found_res = self.api.get_group(res_id)
                 elif res_type == GROUP_SNAPSHOT:
@@ -194,6 +197,13 @@ class _FunctionalTestBase(test.TestCase):
                                          VOLUME, expected_end_status,
                                          max_retries, status_field)
 
+    def _poll_snapshot_while(self, snapshot_id, continue_states,
+                             expected_end_status=None, max_retries=5,
+                             status_field='status'):
+        return self._poll_resource_while(snapshot_id, continue_states,
+                                         SNAPSHOT, expected_end_status,
+                                         max_retries, status_field)
+
     def _poll_group_while(self, group_id, continue_states,
                           expected_end_status=None, max_retries=30,
                           status_field='status'):

cinder/tests/functional/test_volumes.py

@@ -17,6 +17,7 @@ from oslo_utils import uuidutils
 
 from cinder.tests.functional import functional_helpers
 from cinder.volume import configuration
+from cinder.volume import volume_types
 
 
 class VolumesTest(functional_helpers._FunctionalTestBase):
@@ -77,6 +78,146 @@ class VolumesTest(functional_helpers._FunctionalTestBase):
         # Should be gone
         self.assertIsNone(found_volume)
 
+    def test_create_no_volume_type(self):
+        """Verify volume_type is not None (should be system default type.)"""
+
+        # un-configure operator default volume type
+        self.flags(default_volume_type=None)
+
+        # Create volume
+        created_volume = self.api.post_volume({'volume': {'size': 1}})
+        self.assertTrue(uuidutils.is_uuid_like(created_volume['id']))
+        created_volume_id = created_volume['id']
+
+        # Wait (briefly) for creation. Delay is due to the 'message queue'
+        found_volume = self._poll_volume_while(created_volume_id, ['creating'])
+        self.assertEqual('available', found_volume['status'])
+
+        # It should have the system default volume_type
+        self.assertEqual(volume_types.DEFAULT_VOLUME_TYPE,
+                         found_volume['volume_type'])
+
+        # Delete the volume
+        self.api.delete_volume(created_volume_id)
+        found_volume = self._poll_volume_while(created_volume_id, ['deleting'])
+        self.assertIsNone(found_volume)
+
+    def test_create_volume_specified_type(self):
+        """Verify volume_type is not default."""
+
+        my_vol_type_name = 'my_specified_type'
+        my_vol_type_id = self.api.create_type(my_vol_type_name)['id']
+
+        # Create volume
+        created_volume = self.api.post_volume(
+            {'volume': {'size': 1,
+                        'volume_type': my_vol_type_id}})
+        self.assertTrue(uuidutils.is_uuid_like(created_volume['id']))
+        created_volume_id = created_volume['id']
+
+        # Wait (briefly) for creation. Delay is due to the 'message queue'
+        found_volume = self._poll_volume_while(created_volume_id, ['creating'])
+        self.assertEqual('available', found_volume['status'])
+
+        # It should have the specified volume_type
+        self.assertEqual(my_vol_type_name, found_volume['volume_type'])
+
+        # Delete the volume and test type
+        self.api.delete_volume(created_volume_id)
+        found_volume = self._poll_volume_while(created_volume_id, ['deleting'])
+        self.assertIsNone(found_volume)
+        self.api.delete_type(my_vol_type_id)
+
+    def test_create_volume_from_source_vol_inherits_voltype(self):
+        src_vol_type_name = 'source_vol_type'
+        src_vol_type_id = self.api.create_type(src_vol_type_name)['id']
+
+        # Create source volume
+        src_volume = self.api.post_volume(
+            {'volume': {'size': 1,
+                        'volume_type': src_vol_type_id}})
+        self.assertTrue(uuidutils.is_uuid_like(src_volume['id']))
+        src_volume_id = src_volume['id']
+
+        # Wait (briefly) for creation. Delay is due to the 'message queue'
+        src_volume = self._poll_volume_while(src_volume_id, ['creating'])
+        self.assertEqual('available', src_volume['status'])
+
+        # Create a new volume using src_volume, do not specify a volume_type
+        new_volume = self.api.post_volume(
+            {'volume': {'size': 1,
+                        'source_volid': src_volume_id}})
+        new_volume_id = new_volume['id']
+
+        # Wait for creation ...
+        new_volume = self._poll_volume_while(new_volume_id, ['creating'])
+        self.assertEqual('available', new_volume['status'])
+
+        # It should have the same type as the source volume
+        self.assertEqual(src_vol_type_name, new_volume['volume_type'])
+
+        # Delete the volumes and test type
+        self.api.delete_volume(src_volume_id)
+        found_volume = self._poll_volume_while(src_volume_id, ['deleting'])
+        self.assertIsNone(found_volume)
+        self.api.delete_volume(new_volume_id)
+        found_volume = self._poll_volume_while(new_volume_id, ['deleting'])
+        self.assertIsNone(found_volume)
+        self.api.delete_type(src_vol_type_id)
+
+    def test_create_volume_from_snapshot_inherits_voltype(self):
+        src_vol_type_name = 'a_very_new_vol_type'
+        src_vol_type_id = self.api.create_type(src_vol_type_name)['id']
+
+        # Create source volume
+        src_volume = self.api.post_volume(
+            {'volume': {'size': 1,
+                        'volume_type': src_vol_type_id}})
+        src_volume_id = src_volume['id']
+
+        # Wait (briefly) for creation. Delay is due to the 'message queue'
+        src_volume = self._poll_volume_while(src_volume_id, ['creating'])
+        self.assertEqual('available', src_volume['status'])
+
+        # Create a snapshot of src_volume
+        snapshot = self.api.post_snapshot(
+            {'snapshot': {'volume_id': src_volume_id,
+                          'name': 'test_snapshot'}})
+        self.assertEqual(src_volume_id, snapshot['volume_id'])
+        snapshot_id = snapshot['id']
+
+        # make sure the snapshot is ready
+        snapshot = self._poll_snapshot_while(snapshot_id, ['creating'])
+        self.assertEqual('available', snapshot['status'])
+
+        # create a new volume from the snapshot, do not specify a volume_type
+        new_volume = self.api.post_volume(
+            {'volume': {'size': 1,
+                        'snapshot_id': snapshot_id}})
+        new_volume_id = new_volume['id']
+
+        # Wait for creation ...
+        new_volume = self._poll_volume_while(new_volume_id, ['creating'])
+        self.assertEqual('available', new_volume['status'])
+
+        # Finally, here's the whole point of this test:
+        self.assertEqual(src_vol_type_name, new_volume['volume_type'])
+
+        # Delete the snapshot, volumes, and test type
+        self.api.delete_snapshot(snapshot_id)
+        snapshot = self._poll_snapshot_while(snapshot_id, ['deleting'])
+        self.assertIsNone(snapshot)
+
+        self.api.delete_volume(src_volume_id)
+        src_volume = self._poll_volume_while(src_volume_id, ['deleting'])
+        self.assertIsNone(src_volume)
+
+        self.api.delete_volume(new_volume_id)
+        new_volume = self._poll_volume_while(new_volume_id, ['deleting'])
+        self.assertIsNone(new_volume)
+
+        self.api.delete_type(src_vol_type_id)
+
     def test_create_volume_with_metadata(self):
         """Creates a volume with metadata."""
 

cinder/tests/unit/api/v2/test_volumes.py

@@ -46,7 +46,6 @@ from cinder.tests.unit.image import fake as fake_image
 from cinder.tests.unit import test
 from cinder.tests.unit import utils
 from cinder.volume import api as volume_api
-from cinder.volume import volume_types
 
 CONF = cfg.CONF
 
@@ -275,10 +274,6 @@ class VolumeApiTest(test.TestCase):
             self.controller.volume_api, context,
             vol['size'], v2_fakes.DEFAULT_VOL_NAME,
             v2_fakes.DEFAULT_VOL_DESCRIPTION,
-            volume_type=
-            objects.VolumeType.get_by_name_or_id(
-                context,
-                volume_types.get_default_volume_type()['id']),
             **kwargs)
 
     @mock.patch.object(volume_api.API, 'get_snapshot', autospec=True)
@@ -340,10 +335,6 @@ class VolumeApiTest(test.TestCase):
             self.controller.volume_api, context,
             vol['size'], v2_fakes.DEFAULT_VOL_NAME,
             v2_fakes.DEFAULT_VOL_DESCRIPTION,
-            volume_type=
-            objects.VolumeType.get_by_name_or_id(
-                context,
-                volume_types.get_default_volume_type()['id']),
             **kwargs)
 
     @mock.patch.object(volume_api.API, 'get_volume', autospec=True)

cinder/tests/unit/api/v3/test_volumes.py

@@ -47,7 +47,6 @@ from cinder.tests.unit import test
 from cinder.tests.unit import utils as test_utils
 from cinder.volume import api as volume_api
 from cinder.volume import api as vol_get
-from cinder.volume import volume_types
 
 DEFAULT_AZ = "zone1:host1"
 
@@ -347,9 +346,6 @@ class VolumeApiTest(test.TestCase):
             self.controller.volume_api, context,
             vol['size'], v2_fakes.DEFAULT_VOL_NAME,
             v2_fakes.DEFAULT_VOL_DESCRIPTION,
-            volume_type=objects.VolumeType.get_by_name_or_id(
-                context,
-                volume_types.get_default_volume_type()['id']),
             **kwargs)
 
     def test_volumes_summary_in_unsupport_version(self):
@@ -680,9 +676,6 @@ class VolumeApiTest(test.TestCase):
             self.controller.volume_api, context,
             vol['size'], v2_fakes.DEFAULT_VOL_NAME,
             v2_fakes.DEFAULT_VOL_DESCRIPTION,
-            volume_type=objects.VolumeType.get_by_name_or_id(
-                context,
-                volume_types.get_default_volume_type()['id']),
             **kwargs)
 
     @ddt.data(mv.VOLUME_CREATE_FROM_BACKUP,
@@ -723,9 +716,6 @@ class VolumeApiTest(test.TestCase):
             vol['size'],
             v2_fakes.DEFAULT_VOL_NAME,
             v2_fakes.DEFAULT_VOL_DESCRIPTION,
-            volume_type=objects.VolumeType.get_by_name_or_id(
-                context,
-                volume_types.get_default_volume_type()['id']),
             **kwargs)
 
     def test_volume_creation_with_scheduler_hints(self):

cinder/tests/unit/volume/flows/api/test_create_volume.py

@@ -0,0 +1,199 @@
+# Copyright 2020 Red Hat Inc.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+""" Tests for create_volume in the TaskFlow volume.flow.api"""
+
+from unittest import mock
+
+import ddt
+
+from cinder import context
+from cinder import exception
+from cinder.tests.unit import test
+from cinder.volume.flows.api import create_volume
+from cinder.volume import volume_types
+
+
+@ddt.ddt
+class ExtractVolumeRequestTaskValidationsTestCase(test.TestCase):
+    """Test validation code.
+
+    The ExtractVolumeRequestTask takes a set of inputs that will form a
+    volume-create request and validates them, inferring values for "missing"
+    inputs.
+
+    This class tests the validation code, not the Task itself.
+    """
+
+    def setUp(self):
+        super(ExtractVolumeRequestTaskValidationsTestCase, self).setUp()
+        self.context = context.get_admin_context()
+
+    fake_vol_type = 'vt-from-volume_type'
+    fake_source_vol = {'volume_type_id': 'vt-from-source_vol'}
+    fake_snapshot = {'volume_type_id': 'vt-from-snapshot'}
+    fake_img_vol_type_id = 'vt-from-image_volume_type_id'
+    fake_config_value = 'vt-from-config-value'
+
+    big_ass_data_tuple = (
+        # case 0: null params and no configured default should
+        # result in the system default volume type
+        {'param_vol_type': None,
+         'param_source_vol': None,
+         'param_snap': None,
+         'param_img_vol_type_id': None,
+         'config_value': None,
+         'expected_vol_type': volume_types.DEFAULT_VOLUME_TYPE},
+        # case set 1: if a volume_type is passed, should always be selected
+        {'param_vol_type': fake_vol_type,
+         'param_source_vol': None,
+         'param_snap': None,
+         'param_img_vol_type_id': None,
+         'config_value': None,
+         'expected_vol_type': 'vt-from-volume_type'},
+        {'param_vol_type': fake_vol_type,
+         'param_source_vol': fake_source_vol,
+         'param_snap': fake_snapshot,
+         'param_img_vol_type_id': fake_img_vol_type_id,
+         'config_value': fake_config_value,
+         'expected_vol_type': 'vt-from-volume_type'},
+        # case set 2: if no volume_type is passed, the vt from the
+        # source_volume should be selected
+        {'param_vol_type': None,
+         'param_source_vol': fake_source_vol,
+         'param_snap': None,
+         'param_img_vol_type_id': None,
+         'config_value': None,
+         'expected_vol_type': 'vt-from-source_vol'},
+        {'param_vol_type': None,
+         'param_source_vol': fake_source_vol,
+         'param_snap': fake_snapshot,
+         'param_img_vol_type_id': fake_img_vol_type_id,
+         'config_value': fake_config_value,
+         'expected_vol_type': 'vt-from-source_vol'},
+        # case set 3: no volume_type, no source_volume, so snapshot's type
+        # should be selected
+        {'param_vol_type': None,
+         'param_source_vol': None,
+         'param_snap': fake_snapshot,
+         'param_img_vol_type_id': None,
+         'config_value': None,
+         'expected_vol_type': 'vt-from-snapshot'},
+        {'param_vol_type': None,
+         'param_source_vol': None,
+         'param_snap': fake_snapshot,
+         'param_img_vol_type_id': fake_img_vol_type_id,
+         'config_value': fake_config_value,
+         'expected_vol_type': 'vt-from-snapshot'},
+        # case set 4: no volume_type, no source_volume, no snapshot --
+        # use the volume_type from the image metadata
+        {'param_vol_type': None,
+         'param_source_vol': None,
+         'param_snap': None,
+         'param_img_vol_type_id': fake_img_vol_type_id,
+         'config_value': None,
+         'expected_vol_type': 'vt-from-image_volume_type_id'},
+        {'param_vol_type': None,
+         'param_source_vol': None,
+         'param_snap': None,
+         'param_img_vol_type_id': fake_img_vol_type_id,
+         'config_value': fake_config_value,
+         'expected_vol_type': 'vt-from-image_volume_type_id'},
+        # case 5: params all null, should use configured volume_type
+        {'param_vol_type': None,
+         'param_source_vol': None,
+         'param_snap': None,
+         'param_img_vol_type_id': None,
+         'config_value': fake_config_value,
+         'expected_vol_type': 'vt-from-config-value'})
+
+    def reflect_second(a, b):
+        return b
+
+    @ddt.data(*big_ass_data_tuple)
+    @mock.patch('cinder.objects.VolumeType.get_by_name_or_id',
+                side_effect = reflect_second)
+    @mock.patch('cinder.volume.volume_types.get_volume_type_by_name',
+                side_effect = reflect_second)
+    @ddt.unpack
+    def test__get_volume_type(self,
+                              mock_get_volume_type_by_name,
+                              mock_get_by_name_or_id,
+                              param_vol_type,
+                              param_source_vol,
+                              param_snap,
+                              param_img_vol_type_id,
+                              config_value,
+                              expected_vol_type):
+
+        self.flags(default_volume_type=config_value)
+
+        test_fn = create_volume.ExtractVolumeRequestTask._get_volume_type
+
+        self.assertEqual(expected_vol_type,
+                         test_fn(self.context,
+                                 param_vol_type,
+                                 param_source_vol,
+                                 param_snap,
+                                 param_img_vol_type_id))
+
+    # Before the Train release, an invalid volume type specifier
+    # would not raise an exception; it would log an error and you'd
+    # get a volume with volume_type == None.  We want to verify that
+    # specifying a non-existent volume_type always raises an exception
+    smaller_data_tuple = (
+        {'param_source_vol': fake_source_vol,
+         'param_snap': None,
+         'param_img_vol_type_id': None,
+         'config_value': None},
+        {'param_source_vol': None,
+         'param_snap': fake_snapshot,
+         'param_img_vol_type_id': None,
+         'config_value': None},
+        {'param_source_vol': None,
+         'param_snap': None,
+         'param_img_vol_type_id': fake_img_vol_type_id,
+         'config_value': None},
+        {'param_source_vol': None,
+         'param_snap': None,
+         'param_img_vol_type_id': None,
+         'config_value': fake_config_value})
+
+    @ddt.data(*smaller_data_tuple)
+    @mock.patch('cinder.objects.VolumeType.get_by_name_or_id',
+                side_effect = exception.VolumeTypeNotFoundByName(
+                    volume_type_name="get_by_name_or_id"))
+    @mock.patch('cinder.volume.volume_types.get_volume_type_by_name',
+                side_effect = exception.VolumeTypeNotFoundByName(
+                    volume_type_name="get_by_name"))
+    @ddt.unpack
+    def test_neg_get_volume_type(self,
+                                 mock_get_volume_type_by_name,
+                                 mock_get_by_name_or_id,
+                                 param_source_vol,
+                                 param_snap,
+                                 param_img_vol_type_id,
+                                 config_value):
+
+        self.flags(default_volume_type=config_value)
+
+        test_fn = create_volume.ExtractVolumeRequestTask._get_volume_type
+
+        self.assertRaises(exception.VolumeTypeNotFoundByName,
+                          test_fn,
+                          self.context,
+                          None,
+                          param_source_vol,
+                          param_snap,
+                          param_img_vol_type_id)

cinder/tests/unit/volume/flows/test_create_volume_flow.py

@@ -845,20 +845,13 @@ class CreateVolumeFlowTestCase(test.TestCase):
                            'backup_id': None}
         self.assertEqual(expected_result, result)
 
-    @mock.patch('cinder.db.volume_type_get_by_name')
-    @mock.patch('cinder.volume.volume_types.is_encrypted')
-    @mock.patch('cinder.volume.volume_types.get_volume_type_qos_specs')
-    @mock.patch('cinder.volume.volume_types.get_default_volume_type')
     @mock.patch('cinder.objects.volume_type.VolumeType.get_by_name_or_id')
     def test_extract_image_volume_type_from_image_invalid_type(
             self,
-            fake_get_type,
+            fake_get_type):
-            fake_get_def_vol_type,
+        # Expected behavior: if the cinder_img_volume_type image property
-            fake_get_qos,
+        # specifies an invalid type, it should raise an exception
-            fake_is_encrypted,
+        image_volume_type = 'an_invalid_type'
-            fake_db_get_vol_type):
-
-        image_volume_type = 'invalid'
         fake_image_service = fake_image.FakeImageService()
         image_id = 7
         image_meta = {}
@@ -874,13 +867,14 @@ class CreateVolumeFlowTestCase(test.TestCase):
             fake_image_service,
             {'nova'})
 
-        fake_is_encrypted.return_value = False
+        def raise_with_id(stuff, id):
-        fake_get_def_vol_type.return_value = {'name': 'fake_vol_type'}
+            raise exception.VolumeTypeNotFoundByName(volume_type_name=id)
-        fake_get_type.return_value = {'name': 'fake_vol_type'}
+
-        fake_db_get_vol_type.side_effect = (
+        fake_get_type.side_effect = raise_with_id
-            exception.VolumeTypeNotFoundByName(volume_type_name='invalid'))
+
-        fake_get_qos.return_value = {'qos_specs': None}
+        e = self.assertRaises(exception.VolumeTypeNotFoundByName,
-        result = task.execute(self.ctxt,
+                              task.execute,
+                              self.ctxt,
                               size=1,
                               snapshot=None,
                               image_id=image_id,
@@ -894,39 +888,26 @@ class CreateVolumeFlowTestCase(test.TestCase):
                               group=None,
                               group_snapshot=None,
                               backup=None)
-        expected_result = {'size': 1,
+        self.assertIn(image_volume_type, str(e))
-                           'snapshot_id': None,
-                           'source_volid': None,
-                           'availability_zones': ['nova'],
-                           'volume_type': {'name': 'fake_vol_type'},
-                           'volume_type_id': None,
-                           'encryption_key_id': None,
-                           'qos_specs': None,
-                           'consistencygroup_id': None,
-                           'cgsnapshot_id': None,
-                           'group_id': None,
-                           'multiattach': False,
-                           'refresh_az': False,
-                           'replication_status': 'disabled',
-                           'backup_id': None}
-        self.assertEqual(expected_result, result)
 
-    @mock.patch('cinder.db.volume_type_get_by_name')
     @mock.patch('cinder.volume.volume_types.is_encrypted')
     @mock.patch('cinder.volume.volume_types.get_volume_type_qos_specs')
     @mock.patch('cinder.objects.volume_type.VolumeType.get_by_name_or_id')
+    @mock.patch('cinder.volume.volume_types.get_default_volume_type')
     @ddt.data((8, None), (9, {'cinder_img_volume_type': None}))
     @ddt.unpack
     def test_extract_image_volume_type_from_image_properties_error(
             self,
             image_id,
             fake_img_properties,
-            fake_get_type,
+            fake_get_default_vol_type,
+            fake_get_by_name_or_id,
             fake_get_qos,
-            fake_is_encrypted,
+            fake_is_encrypted):
-            fake_db_get_vol_type):
+        # Expected behavior: if the image has no properties
-
+        # or the cinder_img_volume_type is present but has no
-        self.flags(default_volume_type='fake_volume_type')
+        # value, the default volume type should be used
+        self.flags(default_volume_type='fake_default_volume_type')
         fake_image_service = fake_image.FakeImageService()
         image_meta = {}
         image_meta['id'] = image_id
@@ -941,8 +922,19 @@ class CreateVolumeFlowTestCase(test.TestCase):
             {'nova'})
 
         fake_is_encrypted.return_value = False
-        fake_get_type.return_value = None
         fake_get_qos.return_value = {'qos_specs': None}
+
+        fake_volume_type = {'name': 'fake_default_volume_type',
+                            'id': fakes.VOLUME_TYPE_ID}
+        fake_get_default_vol_type.return_value = fake_volume_type
+
+        # yeah, I don't like this either, but until someone figures
+        # out why we re-get the volume_type object in the execute
+        # function, we have to do this.  At least I will check later
+        # and make sure we called it with the correct vol_type_id, so
+        # I'm not completely cheating
+        fake_get_by_name_or_id.return_value = fake_volume_type
+
         result = task.execute(self.ctxt,
                               size=1,
                               snapshot=None,
@@ -957,12 +949,17 @@ class CreateVolumeFlowTestCase(test.TestCase):
                               group=None,
                               group_snapshot=None,
                               backup=None)
+
+        fake_get_default_vol_type.assert_called_once()
+        fake_get_by_name_or_id.assert_called_once_with(
+            self.ctxt, fakes.VOLUME_TYPE_ID)
+
         expected_result = {'size': 1,
                            'snapshot_id': None,
                            'source_volid': None,
                            'availability_zones': ['nova'],
-                           'volume_type': None,
+                           'volume_type': fake_volume_type,
-                           'volume_type_id': None,
+                           'volume_type_id': fakes.VOLUME_TYPE_ID,
                            'encryption_key_id': None,
                            'qos_specs': None,
                            'consistencygroup_id': None,
@@ -974,46 +971,43 @@ class CreateVolumeFlowTestCase(test.TestCase):
                            'backup_id': None}
         self.assertEqual(expected_result, result)
 
-    @mock.patch('cinder.db.volume_type_get_by_name')
+    glance_nonactive_statuses = ('queued', 'saving', 'deleted', 'deactivated',
-    @mock.patch('cinder.volume.volume_types.is_encrypted')
+                                 'uploading', 'importing', 'not_a_vaild_state',
-    @mock.patch('cinder.volume.volume_types.get_volume_type_qos_specs')
+                                 'error')
-    def test_extract_image_volume_type_from_image_invalid_input(
-            self,
-            fake_get_qos,
-            fake_is_encrypted,
-            fake_db_get_vol_type):
 
+    @ddt.data(*glance_nonactive_statuses)
+    def test_extract_image_volume_type_from_image_invalid_input(
+            self, status):
+        # Expected behavior: an image must be in 'active' status
+        # or we should not create an image from it
         fake_image_service = fake_image.FakeImageService()
-        image_id = 10
+        image_meta = {'status': status}
-        image_meta = {}
+        image_id = fake_image_service.create(self.ctxt, image_meta)['id']
-        image_meta['id'] = image_id
-        image_meta['status'] = 'inactive'
-        fake_image_service.create(self.ctxt, image_meta)
         fake_key_manager = mock_key_manager.MockKeyManager()
 
         task = create_volume.ExtractVolumeRequestTask(
             fake_image_service,
             {'nova'})
 
-        fake_is_encrypted.return_value = False
+        e = self.assertRaises(exception.InvalidInput,
-        fake_get_qos.return_value = {'qos_specs': None}
+                              task.execute,
-
+                              self.ctxt,
-        self.assertRaises(exception.InvalidInput,
+                              size=1,
-                          task.execute,
+                              snapshot=None,
-                          self.ctxt,
+                              image_id=image_id,
-                          size=1,
+                              source_volume=None,
-                          snapshot=None,
+                              availability_zone='nova',
-                          image_id=image_id,
+                              volume_type=None,
-                          source_volume=None,
+                              metadata=None,
-                          availability_zone='nova',
+                              key_manager=fake_key_manager,
-                          volume_type=None,
+                              consistencygroup=None,
-                          metadata=None,
+                              cgsnapshot=None,
-                          key_manager=fake_key_manager,
+                              group=None,
-                          consistencygroup=None,
+                              group_snapshot=None,
-                          cgsnapshot=None,
+                              backup=None)
-                          group=None,
+        self.assertIn("Invalid input received", str(e))
-                          group_snapshot=None,
+        self.assertIn("Image {} is not active".format(image_id), str(e))
-                          backup=None)
+        fake_image_service.delete(self.ctxt, image_id)
 
 
 @ddt.ddt

cinder/volume/flows/api/create_volume.py

@@ -10,8 +10,6 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import collections
-
 from oslo_config import cfg
 from oslo_log import log as logging
 import six
@@ -351,33 +349,37 @@ class ExtractVolumeRequestTask(flow_utils.CinderTask):
 
         return encryption_key_id
 
-    def _get_volume_type(self, context, volume_type,
+    @staticmethod
+    def _get_volume_type(context, volume_type,
                          source_volume, snapshot, image_volume_type_id):
+        """Returns a volume_type object or raises.  Never returns None."""
         if volume_type:
             return volume_type
-        identifier = collections.defaultdict(str)
+
-        try:
+        identifier = None
-            if source_volume:
+        if source_volume:
-                identifier = {'source': 'volume',
+            identifier = {'source': 'volume',
-                              'id': source_volume['volume_type_id']}
+                          'id': source_volume['volume_type_id']}
-            elif snapshot:
+        elif snapshot:
-                identifier = {'source': 'snapshot',
+            identifier = {'source': 'snapshot',
-                              'id': snapshot['volume_type_id']}
+                          'id': snapshot['volume_type_id']}
-            elif image_volume_type_id:
+        elif image_volume_type_id:
-                identifier = {'source': 'image',
+            identifier = {'source': 'image',
-                              'id': image_volume_type_id}
+                          'id': image_volume_type_id}
-            elif CONF.default_volume_type:
+        if identifier:
-                identifier = {'source': 'default volume type config',
+            try:
-                              'id': CONF.default_volume_type}
-            if identifier:
                 return objects.VolumeType.get_by_name_or_id(
                     context, identifier['id'])
-        except (exception.VolumeTypeNotFound,
+            except (exception.VolumeTypeNotFound,
-                exception.VolumeTypeNotFoundByName,
+                    exception.VolumeTypeNotFoundByName,
-                exception.InvalidVolumeType):
+                    exception.InvalidVolumeType):
-            LOG.exception("Failed to find volume type from "
+                LOG.exception("Failed to find volume type from "
-                          "source %(source)s, identifier %(id)s", identifier)
+                              "source %(source)s, identifier %(id)s",
-        return None
+                              identifier)
+                raise
+
+        # otherwise, use the default volume type
+        return volume_types.get_default_volume_type()
 
     def execute(self, context, size, snapshot, image_id, source_volume,
                 availability_zone, volume_type, metadata, key_manager,

releasenotes/notes/bug-1879578-volume_type-regression-de82f4152c7b2f77.yaml

@@ -0,0 +1,33 @@
+---
+fixes:
+  - |
+    `Bug #1879578 <https://bugs.launchpad.net/cinder/+bug/1879578>`_:
+    A regression in the Train release caused Cinder to assign the default
+    volume type too aggressively when a volume type was not specified in
+    a volume-create request.  As a result, some alternative methods of
+    specifying the volume type were ignored and the default type (either
+    configured by the operator or the system default) would be assigned.
+
+    This release restores the intended behavior, which is described as
+    follows:
+
+    If a ``volume_type`` is not specified when a volume is created, Cinder
+    tries to infer the volume type from other information in the
+    volume-create request:
+
+    * if a ``source_volid`` is supplied in the request, the volume type
+      is inferred from the source volume's volume type
+    * if a ``snapshot_id`` is supplied in the request, the volume type
+      is inferred from the volume type associated with the snapshot
+    * if an ``imageRef`` is supplied in the request, and the image has
+      a ``cinder_img_volume_type`` image property, the volume type is
+      inferred from the value of that image property
+
+    Otherwise, the volume type is the default volume type configured by
+    the operator, and if no volume type is so configured, the volume type
+    is the system default volume type, namely, ``__DEFAULT__``.
+
+    When a volume type is specified explicitly in a volume-create call, Cinder
+    will use the specified type.  If the specified type cannot be assigned due
+    to a conflict with other parameters in the volume-create call, however, the
+    call will result in a 400 (Bad Request) response.