From e5d842eb1be8026451edeb16e849b53b3bf6ef7a Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Thu, 16 Jul 2020 16:51:30 +0000 Subject: [PATCH] Modify default/delete volume type logic This patch modifies the delete volume type logic such that a volume type cannot be deleted if: 1) It is the default volume type 2) The default type configured is wrong/doesn't exist This also implies that there will exist atleast 1 volume type in the deployment and that will be the default volume type. This also includes following 2 changes on the default_volume_type conf option: 1) It is a mandatory field 2) default value of this config option is '__DEFAULT__' All these changes ensure that we don't allow creating untyped volumes. Also it is now possible to delete the '__DEFAULT__' type as it acts as a normal type. Change-Id: Ifa3d22305060b5913332cad89ea696bf7fd84ce1 Closes-Bug: #1886632 --- cinder/api/contrib/types_manage.py | 10 +++- cinder/api/v2/volumes.py | 11 ++-- cinder/api/v3/volumes.py | 13 ++--- cinder/common/config.py | 2 + cinder/db/sqlalchemy/api.py | 3 + cinder/exception.py | 16 +++++- cinder/tests/functional/api/client.py | 4 ++ cinder/tests/functional/test_volumes.py | 56 ++++++++++++++++--- cinder/tests/unit/db/test_volume_type.py | 6 ++ cinder/tests/unit/test_volume_types.py | 27 +++++++++ .../volume/flows/api/test_create_volume.py | 33 +++++++---- cinder/tests/unit/volume/test_volume.py | 11 ++++ cinder/volume/volume_types.py | 54 ++++++++++++------ ...ing-__DEFAULT__-type-d35dfb5d89760b9b.yaml | 38 +++++++++++++ 14 files changed, 228 insertions(+), 56 deletions(-) create mode 100644 releasenotes/notes/allow-deleting-__DEFAULT__-type-d35dfb5d89760b9b.yaml diff --git a/cinder/api/contrib/types_manage.py b/cinder/api/contrib/types_manage.py index 34104095852..a43b97f58fc 100644 --- a/cinder/api/contrib/types_manage.py +++ b/cinder/api/contrib/types_manage.py @@ -158,11 +158,15 @@ class VolumeTypesManageController(wsgi.Controller): context, 'volume_type.delete', err, id=id) # Not found exception will be handled at the wsgi level raise - except exception.VolumeTypeDefault as err: + except (exception.VolumeTypeDeletionError, + exception.VolumeTypeDefaultDeletionError) as err: self._notify_volume_type_error( context, 'volume_type.delete', err, volume_type=vol_type) - msg = _('Target volume type is default and cannot be deleted.') - raise webob.exc.HTTPBadRequest(explanation=msg) + raise webob.exc.HTTPBadRequest(explanation=err.msg) + except exception.VolumeTypeDefaultMisconfiguredError as err: + self._notify_volume_type_error( + context, 'volume_type.delete', err, volume_type=vol_type) + raise webob.exc.HTTPInternalServerError(explanation=err.msg) return webob.Response(status_int=http_client.ACCEPTED) diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index f5839219c44..931cbbc58a2 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -264,11 +264,12 @@ class VolumeController(wsgi.Controller): "be to specify multiattach enabled volume types.") versionutils.report_deprecated_feature(LOG, msg) - new_volume = self.volume_api.create(context, - size, - volume.get('display_name'), - volume.get('display_description'), - **kwargs) + try: + new_volume = self.volume_api.create( + context, size, volume.get('display_name'), + volume.get('display_description'), **kwargs) + except exception.VolumeTypeDefaultMisconfiguredError as err: + raise webob.exc.HTTPInternalServerError(explanation=err.msg) retval = self._view_builder.detail(req, new_volume) diff --git a/cinder/api/v3/volumes.py b/cinder/api/v3/volumes.py index 8186a2846a7..5d97df470d4 100644 --- a/cinder/api/v3/volumes.py +++ b/cinder/api/v3/volumes.py @@ -389,15 +389,14 @@ class VolumeController(volumes_v2.VolumeController): "release. The default behavior going forward will " "be to specify multiattach enabled volume types.") versionutils.report_deprecated_feature(LOG, msg) - - new_volume = self.volume_api.create(context, - size, - volume.get('display_name'), - volume.get('display_description'), - **kwargs) + try: + new_volume = self.volume_api.create( + context, size, volume.get('display_name'), + volume.get('display_description'), **kwargs) + except exception.VolumeTypeDefaultMisconfiguredError as err: + raise exc.HTTPInternalServerError(explanation=err.msg) retval = self._view_builder.detail(req, new_volume) - return retval diff --git a/cinder/common/config.py b/cinder/common/config.py index 767f23df7d6..19418a50ab3 100644 --- a/cinder/common/config.py +++ b/cinder/common/config.py @@ -107,6 +107,8 @@ global_opts = [ 'default_availability_zone, then ' 'storage_availability_zone, instead of failing.'), cfg.StrOpt('default_volume_type', + default='__DEFAULT__', + required=True, help='Default volume type to use'), cfg.StrOpt('default_group_type', help='Default group type to use'), diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 3f12f65c1bc..2d6c6978fad 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -4157,6 +4157,9 @@ def volume_type_destroy(context, id): utcnow = timeutils.utcnow() session = get_session() with session.begin(): + vol_types = volume_type_get_all(context) + if len(vol_types) <= 1: + raise exception.VolumeTypeDeletionError(volume_type_id=id) _volume_type_get(context, id, session) results = model_query(context, models.Volume, session=session). \ filter_by(volume_type_id=id).all() diff --git a/cinder/exception.py b/cinder/exception.py index 4dbac60905c..828d391c790 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -399,9 +399,19 @@ class VolumeTypeInUse(CinderException): "volumes present with the type.") -class VolumeTypeDefault(CinderException): - message = _("The volume type %(volume_type_name)s " - "is the default volume type and cannot be deleted.") +class VolumeTypeDeletionError(Invalid): + message = _("The volume type %(volume_type_id)s is the only currently " + "defined volume type and cannot be deleted.") + + +class VolumeTypeDefaultDeletionError(Invalid): + message = _("The volume type %(volume_type_id)s is the default volume " + "type and cannot be deleted.") + + +class VolumeTypeDefaultMisconfiguredError(CinderException): + message = _("The request cannot be fulfilled as the default volume type " + "%(volume_type_name)s cannot be found.") class GroupTypeNotFound(NotFound): diff --git a/cinder/tests/functional/api/client.py b/cinder/tests/functional/api/client.py index d53ff79a6a5..bedc30bed92 100644 --- a/cinder/tests/functional/api/client.py +++ b/cinder/tests/functional/api/client.py @@ -56,6 +56,10 @@ class OpenStackApiException400(OpenStackApiException): message = _("400 Bad Request") +class OpenStackApiException500(OpenStackApiException): + message = _("500 Internal Server Error") + + class TestOpenStackClient(object): """Simple OpenStack API Client. diff --git a/cinder/tests/functional/test_volumes.py b/cinder/tests/functional/test_volumes.py index 44d96ec88a0..93593074572 100644 --- a/cinder/tests/functional/test_volumes.py +++ b/cinder/tests/functional/test_volumes.py @@ -15,9 +15,9 @@ from oslo_utils import uuidutils +from cinder.tests.functional.api import client from cinder.tests.functional import functional_helpers from cinder.volume import configuration -from cinder.volume import volume_types class VolumesTest(functional_helpers._FunctionalTestBase): @@ -79,10 +79,7 @@ class VolumesTest(functional_helpers._FunctionalTestBase): 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) + """Verify volume_type is not None""" # Create volume created_volume = self.api.post_volume({'volume': {'size': 1}}) @@ -93,15 +90,58 @@ class VolumesTest(functional_helpers._FunctionalTestBase): 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']) + # It should have a volume_type + self.assertIsNotNone(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_default_type(self): + """Verify that the configured default_volume_type is used""" + + my_vol_type_name = 'default_type' + self.api.create_type(my_vol_type_name) + self.flags(default_volume_type=my_vol_type_name) + + # 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 default volume_type + self.assertEqual(my_vol_type_name, 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_bad_default_type(self): + """Verify non-existent default volume type errors out.""" + + # configure a non-existent default type + self.flags(default_volume_type='non-existent-type') + + # Create volume and verify it errors out with 500 status + self.assertRaises(client.OpenStackApiException500, + self.api.post_volume, {'volume': {'size': 1}}) + + def test_create_volume_default_type_set_none(self): + """Verify None default volume type errors out.""" + + # configure None default type + self.flags(default_volume_type=None) + + # Create volume and verify it errors out with 500 status + self.assertRaises(client.OpenStackApiException500, + self.api.post_volume, {'volume': {'size': 1}}) + def test_create_volume_specified_type(self): """Verify volume_type is not default.""" diff --git a/cinder/tests/unit/db/test_volume_type.py b/cinder/tests/unit/db/test_volume_type.py index 7e249fa1e65..ba3d8354828 100644 --- a/cinder/tests/unit/db/test_volume_type.py +++ b/cinder/tests/unit/db/test_volume_type.py @@ -40,6 +40,12 @@ class VolumeTypeTestCase(test.TestCase): volume_types.get_by_name_or_id, self.ctxt, volume_type['id']) + def test_volume_db_delete_last_type(self): + default = volume_types.get_default_volume_type() + self.assertRaises(exception.VolumeTypeDeletionError, + db.volume_type_destroy, self.ctxt, + default['id']) + def test_volume_type_delete_with_volume_in_use(self): volume_type = db.volume_type_create(self.ctxt, {'name': 'fake volume type'}) diff --git a/cinder/tests/unit/test_volume_types.py b/cinder/tests/unit/test_volume_types.py index ef6964ea6c9..413476cb434 100644 --- a/cinder/tests/unit/test_volume_types.py +++ b/cinder/tests/unit/test_volume_types.py @@ -226,6 +226,30 @@ class VolumeTypeTestCase(test.TestCase): self.assertEqual(conf_fixture.def_vol_type, default_vol_type.get('name')) + def test_get_default_volume_type_not_found(self): + """Ensure setting non-existent default type raises error.""" + self.flags(default_volume_type='fake_type') + self.assertRaises(exception.VolumeTypeDefaultMisconfiguredError, + volume_types.get_default_volume_type) + + def test_delete_default_volume_type(self): + """Ensures default volume type cannot be deleted.""" + default = volume_types.create(self.ctxt, 'default_type') + + self.flags(default_volume_type='default_type') + self.assertRaises(exception.VolumeTypeDefaultDeletionError, + volume_types.destroy, self.ctxt, default['id']) + + def test_delete_when_default_volume_type_not_found(self): + """Ensures volume types cannot be deleted until valid default is set. + + """ + default = volume_types.create(self.ctxt, 'default_type') + + self.flags(default_volume_type='fake_default') + self.assertRaises(exception.VolumeTypeDefaultMisconfiguredError, + volume_types.destroy, self.ctxt, default['id']) + def test_default_volume_type_missing_in_db(self): """Test default volume type is missing in database. @@ -248,6 +272,9 @@ class VolumeTypeTestCase(test.TestCase): def test_non_existent_vol_type_shouldnt_delete(self): """Ensures that volume type creation fails with invalid args.""" + # create a dummy type as DB requires at least 1 type to perform the + # delete operation + volume_types.create(self.ctxt, self.vol_type1_name) self.assertRaises(exception.VolumeTypeNotFound, volume_types.destroy, self.ctxt, "sfsfsdfdfs") diff --git a/cinder/tests/unit/volume/flows/api/test_create_volume.py b/cinder/tests/unit/volume/flows/api/test_create_volume.py index 61bdc299194..7ebc747619a 100644 --- a/cinder/tests/unit/volume/flows/api/test_create_volume.py +++ b/cinder/tests/unit/volume/flows/api/test_create_volume.py @@ -53,14 +53,14 @@ class ExtractVolumeRequestTaskValidationsTestCase(test.TestCase): 'param_source_vol': None, 'param_snap': None, 'param_img_vol_type_id': None, - 'config_value': None, + 'config_value': volume_types.DEFAULT_VOLUME_TYPE, '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, + 'config_value': volume_types.DEFAULT_VOLUME_TYPE, 'expected_vol_type': 'vt-from-volume_type'}, {'param_vol_type': fake_vol_type, 'param_source_vol': fake_source_vol, @@ -74,7 +74,7 @@ class ExtractVolumeRequestTaskValidationsTestCase(test.TestCase): 'param_source_vol': fake_source_vol, 'param_snap': None, 'param_img_vol_type_id': None, - 'config_value': None, + 'config_value': volume_types.DEFAULT_VOLUME_TYPE, 'expected_vol_type': 'vt-from-source_vol'}, {'param_vol_type': None, 'param_source_vol': fake_source_vol, @@ -88,7 +88,7 @@ class ExtractVolumeRequestTaskValidationsTestCase(test.TestCase): 'param_source_vol': None, 'param_snap': fake_snapshot, 'param_img_vol_type_id': None, - 'config_value': None, + 'config_value': volume_types.DEFAULT_VOLUME_TYPE, 'expected_vol_type': 'vt-from-snapshot'}, {'param_vol_type': None, 'param_source_vol': None, @@ -102,7 +102,7 @@ class ExtractVolumeRequestTaskValidationsTestCase(test.TestCase): 'param_source_vol': None, 'param_snap': None, 'param_img_vol_type_id': fake_img_vol_type_id, - 'config_value': None, + 'config_value': volume_types.DEFAULT_VOLUME_TYPE, 'expected_vol_type': 'vt-from-image_volume_type_id'}, {'param_vol_type': None, 'param_source_vol': None, @@ -190,10 +190,19 @@ class ExtractVolumeRequestTaskValidationsTestCase(test.TestCase): 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) + if config_value: + self.assertRaises(exception.VolumeTypeDefaultMisconfiguredError, + test_fn, + self.context, + None, + param_source_vol, + param_snap, + param_img_vol_type_id) + else: + 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/test_volume.py b/cinder/tests/unit/volume/test_volume.py index a60e5b18eaf..227cf04c4d4 100644 --- a/cinder/tests/unit/volume/test_volume.py +++ b/cinder/tests/unit/volume/test_volume.py @@ -605,6 +605,17 @@ class VolumeTestCase(base.BaseVolumeTestCase): volume_type=self.vol_type) self.assertEqual('default-az', volume['availability_zone']) + def test_create_volume_with_default_type_misconfigured(self): + """Test volume creation with non-existent default volume type.""" + volume_api = cinder.volume.api.API() + + self.flags(default_volume_type='fake_type') + # Create volume with default volume type while default + # volume type doesn't exist + self.assertRaises(exception.VolumeTypeDefaultMisconfiguredError, + volume_api.create, self.context, 1, + 'name', 'description') + @mock.patch('cinder.quota.QUOTAS.rollback', new=mock.MagicMock()) @mock.patch('cinder.quota.QUOTAS.commit', new=mock.MagicMock()) @mock.patch('cinder.quota.QUOTAS.reserve', return_value=["RESERVATION"]) diff --git a/cinder/volume/volume_types.py b/cinder/volume/volume_types.py index 319a99593ab..43309c6b798 100644 --- a/cinder/volume/volume_types.py +++ b/cinder/volume/volume_types.py @@ -102,14 +102,31 @@ def update(context, id, name, description, is_public=None): def destroy(context, id): - """Marks volume types as deleted.""" + """Marks volume types as deleted. + + There must exist at least one volume type (i.e. the default type) in + the deployment. + This method achieves that by ensuring: + 1) the default_volume_type is set and is a valid one + 2) the type requested to delete isn't the default type + + :raises VolumeTypeDefaultDeletionError: when the type requested to + delete is the default type + """ if id is None: msg = _("id cannot be None") raise exception.InvalidVolumeType(reason=msg) - vol_type = get_volume_type(context, id) - if vol_type['name'] == DEFAULT_VOLUME_TYPE: - raise exception.VolumeTypeDefault(vol_type['name']) elevated = context if context.is_admin else context.elevated() + + # Default type *must* be set in order to delete any volume type. + # If the default isn't set, the following call will raise + # VolumeTypeDefaultMisconfiguredError exception which will error out the + # delete operation. + default_type = get_default_volume_type() + # don't allow delete if the type requested is the default type + if id == default_type.get('id'): + raise exception.VolumeTypeDefaultDeletionError(volume_type_id=id) + return db.volume_type_destroy(elevated, id) @@ -173,22 +190,23 @@ def get_volume_type_by_name(context, name): def get_default_volume_type(): - """Get the default volume type.""" + """Get the default volume type. + + :raises VolumeTypeDefaultMisconfiguredError: when the configured default + is not found + """ name = CONF.default_volume_type - vol_type = {} ctxt = context.get_admin_context() - if name: - try: - vol_type = get_volume_type_by_name(ctxt, name) - except exception.VolumeTypeNotFoundByName: - # Couldn't find volume type with the name in default_volume_type - # flag, record this issue and raise exception - # TODO(zhiteng) consider add notification to warn admin - LOG.exception('Default volume type is not found. ' - 'Please check default_volume_type config:') - raise exception.VolumeTypeNotFoundByName(volume_type_name=name) - else: - vol_type = get_volume_type_by_name(ctxt, DEFAULT_VOLUME_TYPE) + vol_type = {} + try: + vol_type = get_volume_type_by_name(ctxt, name) + except (exception.VolumeTypeNotFoundByName, exception.InvalidVolumeType): + # Couldn't find volume type with the name in default_volume_type + # flag, record this issue and raise exception + LOG.exception('Default volume type is not found. ' + 'Please check default_volume_type config:') + raise exception.VolumeTypeDefaultMisconfiguredError( + volume_type_name=name) return vol_type diff --git a/releasenotes/notes/allow-deleting-__DEFAULT__-type-d35dfb5d89760b9b.yaml b/releasenotes/notes/allow-deleting-__DEFAULT__-type-d35dfb5d89760b9b.yaml new file mode 100644 index 00000000000..00c9bead21c --- /dev/null +++ b/releasenotes/notes/allow-deleting-__DEFAULT__-type-d35dfb5d89760b9b.yaml @@ -0,0 +1,38 @@ +--- +upgrade: + - | + The ``default_volume_type`` configuration option is now required + to have a value. The default value is ``__DEFAULT__``, so you + should see no change in behavior whether or not you have set a + value for ``default_volume_type``. See + `Bug #1886632 `_ + for more information about this change. + +fixes: + - | + `Bug #1886632 `_: + The system defined ``__DEFAULT__`` volume type is now treated as + a regular volume-type and may be updated or deleted. Since the + configured ``default_volume_type`` cannot be deleted, however, + the ``__DEFAULT__`` volume type may not be deleted if it is the + value of that configuration option. + +other: + - | + Beginning with the Train release, untyped volumes (that is, volumes with + no volume-type) have been disallowed. To facilitate this, a + ``__DEFAULT__`` volume-type was included as part of the Train database + migration. In this release, handling of the default volume-type has been + improved: + + * The ``default_volume_type`` configuration option is required to have a + value. The default value is ``__DEFAULT__``. + * A request to delete the currently configured ``default_volume_type`` + will fail. (You can delete that volume-type, but you cannot do it + while it is the value of the configuration option.) + * There must always be at least one volume-type defined in a Cinder + installation. This is enforced by the type-delete call. + * If the ``default_volume_type`` is misconfigured (that is, if the value + refers to a non-existent volume-type), requests that rely on the + default volume-type (for example, a volume-create request that does + not specify a volume-type) will result in a HTTP 500 response. \ No newline at end of file