diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index d2b7a6bc616..86d2b082e5a 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 e951817e450..a9704292a5e 100644 --- a/cinder/api/v3/volumes.py +++ b/cinder/api/v3/volumes.py @@ -37,7 +37,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__) @@ -286,11 +285,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 8bcfffa224a..8b067a73211 100644 --- a/cinder/tests/functional/functional_helpers.py +++ b/cinder/tests/functional/functional_helpers.py @@ -33,6 +33,7 @@ from cinder.tests.unit import fake_constants as fake CONF = cfg.CONF VOLUME = 'VOLUME' +SNAPSHOT = 'SNAPSHOT' GROUP = 'GROUP' GROUP_SNAPSHOT = 'GROUP_SNAPSHOT' @@ -165,6 +166,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: @@ -196,6 +199,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 b6c001a7672..b4c4f7ddab1 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 26e5a606fca..46ddae92498 100644 --- a/cinder/tests/unit/api/v3/test_volumes.py +++ b/cinder/tests/unit/api/v3/test_volumes.py @@ -46,7 +46,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" @@ -337,9 +336,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): @@ -670,9 +666,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, @@ -713,9 +706,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..9896d73f631 --- /dev/null +++ b/cinder/tests/unit/volume/flows/api/test_create_volume.py @@ -0,0 +1,198 @@ +# 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""" + +import ddt +import mock + +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 430e6a2b011..a03932b97c7 100644 --- a/cinder/tests/unit/volume/flows/test_create_volume_flow.py +++ b/cinder/tests/unit/volume/flows/test_create_volume_flow.py @@ -846,20 +846,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 = {} @@ -875,13 +868,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, @@ -895,39 +889,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 @@ -942,8 +923,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, @@ -958,12 +950,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, @@ -975,46 +972,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 9415415162f..97e030f617d 100644 --- a/cinder/volume/flows/api/create_volume.py +++ b/cinder/volume/flows/api/create_volume.py @@ -10,7 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import collections import six from oslo_config import cfg @@ -351,33 +350,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 `_: + 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.