diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py
index 4a8c3b4f779..f5839219c44 100644
--- a/cinder/api/v2/volumes.py
+++ b/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)
diff --git a/cinder/api/v3/volumes.py b/cinder/api/v3/volumes.py
index 27d046ca2a6..8186a2846a7 100644
--- a/cinder/api/v3/volumes.py
+++ b/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)
diff --git a/cinder/tests/functional/api/client.py b/cinder/tests/functional/api/client.py
index 82d793633b6..d53ff79a6a5 100644
--- a/cinder/tests/functional/api/client.py
+++ b/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,
diff --git a/cinder/tests/functional/functional_helpers.py b/cinder/tests/functional/functional_helpers.py
index b229cb927d7..1c8f1e58475 100644
--- a/cinder/tests/functional/functional_helpers.py
+++ b/cinder/tests/functional/functional_helpers.py
@@ -31,6 +31,7 @@ from cinder.tests.unit import fake_constants as fake
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'):
diff --git a/cinder/tests/functional/test_volumes.py b/cinder/tests/functional/test_volumes.py
index cb243b318fb..44d96ec88a0 100644
--- a/cinder/tests/functional/test_volumes.py
+++ b/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."""
diff --git a/cinder/tests/unit/api/v2/test_volumes.py b/cinder/tests/unit/api/v2/test_volumes.py
index 1d9de12572f..97c59036744 100644
--- a/cinder/tests/unit/api/v2/test_volumes.py
+++ b/cinder/tests/unit/api/v2/test_volumes.py
@@ -46,7 +46,6 @@ from cinder.tests.unit import fake_volume
from cinder.tests.unit.image import fake as fake_image
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)
diff --git a/cinder/tests/unit/api/v3/test_volumes.py b/cinder/tests/unit/api/v3/test_volumes.py
index 5ffb54d1db6..587f0f7462a 100644
--- a/cinder/tests/unit/api/v3/test_volumes.py
+++ b/cinder/tests/unit/api/v3/test_volumes.py
@@ -47,7 +47,6 @@ from cinder.tests.unit.image import fake as fake_image
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):
diff --git a/cinder/tests/unit/volume/flows/api/__init__.py b/cinder/tests/unit/volume/flows/api/__init__.py
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/cinder/tests/unit/volume/flows/api/test_create_volume.py b/cinder/tests/unit/volume/flows/api/test_create_volume.py
new file mode 100644
index 00000000000..fd43214103c
--- /dev/null
+++ b/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 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)
diff --git a/cinder/tests/unit/volume/flows/test_create_volume_flow.py b/cinder/tests/unit/volume/flows/test_create_volume_flow.py
index 5f9b5cff6df..72cf09da526 100644
--- a/cinder/tests/unit/volume/flows/test_create_volume_flow.py
+++ b/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_def_vol_type,
- fake_get_qos,
- fake_is_encrypted,
- fake_db_get_vol_type):
-
- image_volume_type = 'invalid'
+ fake_get_type):
+ # Expected behavior: if the cinder_img_volume_type image property
+ # specifies an invalid type, it should raise an exception
+ image_volume_type = 'an_invalid_type'
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
- fake_get_def_vol_type.return_value = {'name': 'fake_vol_type'}
- fake_get_type.return_value = {'name': 'fake_vol_type'}
- fake_db_get_vol_type.side_effect = (
- exception.VolumeTypeNotFoundByName(volume_type_name='invalid'))
- fake_get_qos.return_value = {'qos_specs': None}
- result = task.execute(self.ctxt,
+ def raise_with_id(stuff, id):
+ raise exception.VolumeTypeNotFoundByName(volume_type_name=id)
+
+ fake_get_type.side_effect = raise_with_id
+
+ e = self.assertRaises(exception.VolumeTypeNotFoundByName,
+ 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,
- '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)
+ self.assertIn(image_volume_type, str(e))
- @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_db_get_vol_type):
-
- self.flags(default_volume_type='fake_volume_type')
+ fake_is_encrypted):
+ # Expected behavior: if the image has no properties
+ # or the cinder_img_volume_type is present but has no
+ # 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_id': None,
+ 'volume_type': fake_volume_type,
+ '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')
- @mock.patch('cinder.volume.volume_types.is_encrypted')
- @mock.patch('cinder.volume.volume_types.get_volume_type_qos_specs')
- def test_extract_image_volume_type_from_image_invalid_input(
- self,
- fake_get_qos,
- fake_is_encrypted,
- fake_db_get_vol_type):
+ glance_nonactive_statuses = ('queued', 'saving', 'deleted', 'deactivated',
+ 'uploading', 'importing', 'not_a_vaild_state',
+ 'error')
+ @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 = {}
- image_meta['id'] = image_id
- image_meta['status'] = 'inactive'
- fake_image_service.create(self.ctxt, image_meta)
+ image_meta = {'status': status}
+ image_id = fake_image_service.create(self.ctxt, image_meta)['id']
fake_key_manager = mock_key_manager.MockKeyManager()
task = create_volume.ExtractVolumeRequestTask(
fake_image_service,
{'nova'})
- fake_is_encrypted.return_value = False
- fake_get_qos.return_value = {'qos_specs': None}
-
- self.assertRaises(exception.InvalidInput,
- task.execute,
- self.ctxt,
- size=1,
- snapshot=None,
- image_id=image_id,
- source_volume=None,
- availability_zone='nova',
- volume_type=None,
- metadata=None,
- key_manager=fake_key_manager,
- consistencygroup=None,
- cgsnapshot=None,
- group=None,
- group_snapshot=None,
- backup=None)
+ e = self.assertRaises(exception.InvalidInput,
+ task.execute,
+ self.ctxt,
+ size=1,
+ snapshot=None,
+ image_id=image_id,
+ source_volume=None,
+ availability_zone='nova',
+ volume_type=None,
+ metadata=None,
+ key_manager=fake_key_manager,
+ consistencygroup=None,
+ cgsnapshot=None,
+ group=None,
+ group_snapshot=None,
+ backup=None)
+ self.assertIn("Invalid input received", str(e))
+ self.assertIn("Image {} is not active".format(image_id), str(e))
+ fake_image_service.delete(self.ctxt, image_id)
@ddt.ddt
diff --git a/cinder/volume/flows/api/create_volume.py b/cinder/volume/flows/api/create_volume.py
index 4e57eae1dc1..54c69f7cc88 100644
--- a/cinder/volume/flows/api/create_volume.py
+++ b/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:
- if source_volume:
- identifier = {'source': 'volume',
- 'id': source_volume['volume_type_id']}
- elif snapshot:
- identifier = {'source': 'snapshot',
- 'id': snapshot['volume_type_id']}
- elif image_volume_type_id:
- identifier = {'source': 'image',
- 'id': image_volume_type_id}
- elif CONF.default_volume_type:
- identifier = {'source': 'default volume type config',
- 'id': CONF.default_volume_type}
- if identifier:
+
+ identifier = None
+ if source_volume:
+ identifier = {'source': 'volume',
+ 'id': source_volume['volume_type_id']}
+ elif snapshot:
+ identifier = {'source': 'snapshot',
+ 'id': snapshot['volume_type_id']}
+ elif image_volume_type_id:
+ identifier = {'source': 'image',
+ 'id': image_volume_type_id}
+ if identifier:
+ try:
return objects.VolumeType.get_by_name_or_id(
context, identifier['id'])
- except (exception.VolumeTypeNotFound,
- exception.VolumeTypeNotFoundByName,
- exception.InvalidVolumeType):
- LOG.exception("Failed to find volume type from "
- "source %(source)s, identifier %(id)s", identifier)
- return None
+ except (exception.VolumeTypeNotFound,
+ exception.VolumeTypeNotFoundByName,
+ exception.InvalidVolumeType):
+ LOG.exception("Failed to find volume type from "
+ "source %(source)s, identifier %(id)s",
+ 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,
diff --git a/releasenotes/notes/bug-1879578-volume_type-regression-de82f4152c7b2f77.yaml b/releasenotes/notes/bug-1879578-volume_type-regression-de82f4152c7b2f77.yaml
new file mode 100644
index 00000000000..0e6a0114fd1
--- /dev/null
+++ b/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.