openstack/cinder
Code Issues Proposed changes

Modify default/delete volume type logic

Browse Source 
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
This commit is contained in:
Rajat Dhasmana 2020-07-16 16:51:30 +00:00
parent 641762befc
commit e5d842eb1b
14 changed files with 228 additions and 56 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
cinder/api/contrib/types_manage.py
View File

@ -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)

11
cinder/api/v2/volumes.py
View File

@ -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)

13
cinder/api/v3/volumes.py
View File

@ -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

2
cinder/common/config.py
View File

@ -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'),

3
cinder/db/sqlalchemy/api.py
View File

@ -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()

16
cinder/exception.py
View File

@ -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):

4
cinder/tests/functional/api/client.py
View File

@ -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.

56
cinder/tests/functional/test_volumes.py
View File

@ -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."""

6
cinder/tests/unit/db/test_volume_type.py
View File

@ -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'})

27
cinder/tests/unit/test_volume_types.py
View File

@ -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")

33
cinder/tests/unit/volume/flows/api/test_create_volume.py
View File

@ -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)

11
cinder/tests/unit/volume/test_volume.py
View File

@ -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"])

54
cinder/volume/volume_types.py
View File

@ -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

38
releasenotes/notes/allow-deleting-__DEFAULT__-type-d35dfb5d89760b9b.yaml Normal file
View File

@ -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 <https://bugs.launchpad.net/cinder/+bug/1886632>`_
for more information about this change.
fixes:
- |
`Bug #1886632 <https://bugs.launchpad.net/cinder/+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.