From 32f1145b7ddf9a9a359e2359e7db63dbdd00b899 Mon Sep 17 00:00:00 2001 From: whoami-rajat Date: Thu, 23 Feb 2023 09:59:01 +0000 Subject: [PATCH] Remove multiatttach request parameter The initial cinder design[1][2][3] allowed users to create mutliattach volumes by spcifying the ``multiattach`` parameter in the request body of volume create operation (``--allow-multiattach`` option in cinderclient). This functionality changed in Queens with the introduction of microversion 3.50[4] where we used volume types to store the multiattach capabilities. Any volume created with a multiattach volume type will be a multiattach volume[5]. While implementing the new functionality, we had to keep backward compatibility with the *old way* of creating multiattach volumes. We deprecated the ``multiattach`` (``--allow-multiattach`` on cinderclient side) parameter in the queens release[6][7]. We also removed the support of the ``--allow-multiattach`` optional parameter from cinderclient in the train release[8] but the API side never removed the compatibility code to disallow functionality of creating multiattach volumes by using the ``multiattach`` parameter (instead of a multiattach volume type). This patch removes the support of providing the ``multiattach`` parameter in the request body of a volume create operation and will fail with a BadRequest exception stating the reason of failure and how it can be fixed. [1] https://blueprints.launchpad.net/cinder/+spec/multi-attach-volume [2] https://review.opendev.org/c/openstack/cinder/+/85847/ [3] https://review.opendev.org/c/openstack/python-cinderclient/+/85856 [4] https://github.com/openstack/cinder/commit/f1bfd9790d2a7cac9a3e66417b11dc8e3edd8109 [5] https://docs.openstack.org/cinder/latest/admin/volume-multiattach.html#how-to-create-a-multiattach-volume [6] https://github.com/openstack/cinder/commit/94dbf5cce2caff484460a1330feb6cbf7f3dd56a [7] https://github.com/openstack/python-cinderclient/commit/adb141a2626192e8f45a911291895716d7c1c8a4 [8] https://github.com/openstack/python-cinderclient/commit/3c1b417959689c85a2f54505057ca995fedca075 Depends-On: https://review.opendev.org/c/openstack/tempest/+/875372 Closes-Bug: 2008259 Change-Id: I0ece6e279048abcc04b3674108290a80eca6bd62 --- api-ref/source/v2/parameters.yaml | 9 ----- api-ref/source/v2/volumes-v2-volumes.inc | 1 - api-ref/source/v3/parameters.yaml | 9 ----- api-ref/source/v3/volumes-v3-volumes.inc | 1 - cinder/api/schemas/volumes.py | 6 ++++ cinder/api/v2/volumes.py | 9 ----- cinder/api/v3/volumes.py | 16 ++++----- cinder/scheduler/filter_scheduler.py | 13 ------- cinder/tests/unit/api/v2/test_volumes.py | 36 +------------------ cinder/tests/unit/api/v3/test_volumes.py | 23 +++++++++++- cinder/tests/unit/volume/test_volume.py | 23 ------------ cinder/volume/api.py | 2 -- cinder/volume/flows/api/create_volume.py | 12 ++----- ...attach-request-param-4444e02533f919da.yaml | 20 +++++++++++ 14 files changed, 59 insertions(+), 121 deletions(-) create mode 100644 releasenotes/notes/remove-multiattach-request-param-4444e02533f919da.yaml diff --git a/api-ref/source/v2/parameters.yaml b/api-ref/source/v2/parameters.yaml index 1b7fde4db00..be357736f51 100644 --- a/api-ref/source/v2/parameters.yaml +++ b/api-ref/source/v2/parameters.yaml @@ -1081,15 +1081,6 @@ mountpoint: in: body required: true type: string -multiattach_req: - description: | - To enable this volume to attach to more than one - server, set this value to ``true``. Default is ``false``. - Note that support for multiattach volumes depends on the volume - type being used. - in: body - required: false - type: boolean multiattach_resp: description: | If true, this volume can attach to more than one diff --git a/api-ref/source/v2/volumes-v2-volumes.inc b/api-ref/source/v2/volumes-v2-volumes.inc index acd507943cf..07be3422ea4 100644 --- a/api-ref/source/v2/volumes-v2-volumes.inc +++ b/api-ref/source/v2/volumes-v2-volumes.inc @@ -183,7 +183,6 @@ Request - size: size - description: description_9 - imageRef: imageRef - - multiattach: multiattach_req - availability_zone: availability_zone - source_volid: source_volid - name: volume_name_optional diff --git a/api-ref/source/v3/parameters.yaml b/api-ref/source/v3/parameters.yaml index b3bffdf39b4..45271d7a93f 100644 --- a/api-ref/source/v3/parameters.yaml +++ b/api-ref/source/v3/parameters.yaml @@ -2126,15 +2126,6 @@ multiattach: in: body required: false type: string -multiattach_req: - description: | - To enable this volume to attach to more than one - server, set this value to ``true``. Default is ``false``. - Note that support for multiattach volumes depends on the volume - type being used. See :ref:`valid boolean values ` - in: body - required: false - type: boolean multiattach_resp: description: | If true, this volume can attach to more than one diff --git a/api-ref/source/v3/volumes-v3-volumes.inc b/api-ref/source/v3/volumes-v3-volumes.inc index 41a99d1454e..99d11684089 100644 --- a/api-ref/source/v3/volumes-v3-volumes.inc +++ b/api-ref/source/v3/volumes-v3-volumes.inc @@ -218,7 +218,6 @@ Request - availability_zone: availability_zone - source_volid: source_volid - description: description_vol - - multiattach: multiattach_req - snapshot_id: snapshot_id - backup_id: backup_id - name: volume_name_optional diff --git a/cinder/api/schemas/volumes.py b/cinder/api/schemas/volumes.py index f3f68977020..08678cd56a1 100644 --- a/cinder/api/schemas/volumes.py +++ b/cinder/api/schemas/volumes.py @@ -49,6 +49,12 @@ create = { 'consistencygroup_id': parameter_types.optional_uuid, 'size': parameter_types.volume_size_allows_null, 'availability_zone': parameter_types.availability_zone, + # The functionality to create a multiattach volume by the + # multiattach parameter is removed. + # We accept the parameter but raise a BadRequest stating the + # "new way" of creating multiattach volumes i.e. with a + # multiattach volume type so users using the "old way" + # have ease of moving into the new functionality. 'multiattach': parameter_types.optional_boolean, 'image_id': {'type': ['string', 'null'], 'minLength': 0, 'maxLength': 255}, diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index ddff0471e6c..67d2d29b500 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -19,7 +19,6 @@ from http import HTTPStatus from oslo_config import cfg from oslo_log import log as logging -from oslo_log import versionutils from oslo_utils import uuidutils import webob from webob import exc @@ -261,14 +260,6 @@ class VolumeController(wsgi.Controller): kwargs['availability_zone'] = volume.get('availability_zone', None) kwargs['scheduler_hints'] = volume.get('scheduler_hints', None) - kwargs['multiattach'] = utils.get_bool_param('multiattach', volume) - - if kwargs.get('multiattach', False): - msg = ("The option 'multiattach' " - "is deprecated and will be removed in a future " - "release. The default behavior going forward will " - "be to specify multiattach enabled volume types.") - versionutils.report_deprecated_feature(LOG, msg) try: new_volume = self.volume_api.create( diff --git a/cinder/api/v3/volumes.py b/cinder/api/v3/volumes.py index ed3fd574673..48982aa4e4a 100644 --- a/cinder/api/v3/volumes.py +++ b/cinder/api/v3/volumes.py @@ -15,7 +15,6 @@ from http import HTTPStatus from oslo_log import log as logging -from oslo_log import versionutils from oslo_utils import timeutils import webob from webob import exc @@ -387,15 +386,14 @@ class VolumeController(volumes_v2.VolumeController): kwargs['availability_zone'] = volume.get('availability_zone', None) kwargs['scheduler_hints'] = volume.get('scheduler_hints', None) - multiattach = volume.get('multiattach', False) - kwargs['multiattach'] = multiattach - + multiattach = utils.get_bool_param('multiattach', volume) if multiattach: - msg = ("The option 'multiattach' " - "is deprecated and will be removed in a future " - "release. The default behavior going forward will " - "be to specify multiattach enabled volume types.") - versionutils.report_deprecated_feature(LOG, msg) + msg = _("multiattach parameter has been removed. The default " + "behavior is to use multiattach enabled volume types. " + "Contact your administrator to create a multiattach " + "enabled volume type and use it to create multiattach " + "volumes.") + raise exc.HTTPBadRequest(explanation=msg) try: new_volume = self.volume_api.create( context, size, volume.get('display_name'), diff --git a/cinder/scheduler/filter_scheduler.py b/cinder/scheduler/filter_scheduler.py index 32be969f8d8..db53196e8c0 100644 --- a/cinder/scheduler/filter_scheduler.py +++ b/cinder/scheduler/filter_scheduler.py @@ -335,19 +335,6 @@ class FilterScheduler(driver.Scheduler): self.populate_filter_properties(request_spec, filter_properties) - # If multiattach is enabled on a volume, we need to add - # multiattach to extra specs, so that the capability - # filtering is enabled. - multiattach = request_spec['volume_properties'].get('multiattach', - False) - if multiattach and 'multiattach' not in resource_type.get( - 'extra_specs', {}): - if 'extra_specs' not in resource_type: - resource_type['extra_specs'] = {} - - resource_type['extra_specs'].update( - multiattach=' True') - # Revert volume consumed capacity if it's a rescheduled request retry = filter_properties.get('retry', {}) if retry.get('backends', []): diff --git a/cinder/tests/unit/api/v2/test_volumes.py b/cinder/tests/unit/api/v2/test_volumes.py index 39e3e0768ed..7139a15c4d2 100644 --- a/cinder/tests/unit/api/v2/test_volumes.py +++ b/cinder/tests/unit/api/v2/test_volumes.py @@ -158,8 +158,7 @@ class VolumeApiTest(test.TestCase): consistencygroup_id=None, volume_type=None, image_ref=None, - image_id=None, - multiattach=False): + image_id=None): vol = {"size": size, "name": name, "description": description, @@ -168,7 +167,6 @@ class VolumeApiTest(test.TestCase): "source_volid": source_volid, "consistencygroup_id": consistencygroup_id, "volume_type": volume_type, - "multiattach": multiattach, } if image_id is not None: @@ -240,7 +238,6 @@ class VolumeApiTest(test.TestCase): 'consistencygroup': None, 'availability_zone': availability_zone, 'scheduler_hints': None, - 'multiattach': False, } @mock.patch.object(db.sqlalchemy.api, '_volume_type_get_full', @@ -570,37 +567,6 @@ class VolumeApiTest(test.TestCase): req, body=body) - def test_volume_create_with_invalid_multiattach(self): - vol = self._vol_in_request_body(multiattach="InvalidBool") - body = {"volume": vol} - req = fakes.HTTPRequest.blank('/v3/volumes') - - self.assertRaises(exception.ValidationError, - self.controller.create, - req, - body=body) - - @mock.patch.object(volume_api.API, 'create', autospec=True) - @mock.patch.object(volume_api.API, 'get', autospec=True) - @mock.patch.object(db.sqlalchemy.api, '_volume_type_get_full', - autospec=True) - def test_volume_create_with_valid_multiattach(self, - volume_type_get, - get, create): - create.side_effect = v2_fakes.fake_volume_api_create - get.side_effect = v2_fakes.fake_volume_get - volume_type_get.side_effect = v2_fakes.fake_volume_type_get - - vol = self._vol_in_request_body(multiattach=True) - body = {"volume": vol} - - ex = self._expected_vol_from_controller(multiattach=True) - - req = fakes.HTTPRequest.blank('/v3/volumes') - res_dict = self.controller.create(req, body=body) - - self.assertEqual(ex, res_dict) - @ddt.data({'a' * 256: 'a'}, {'a': 'a' * 256}, {'': 'a'}, diff --git a/cinder/tests/unit/api/v3/test_volumes.py b/cinder/tests/unit/api/v3/test_volumes.py index f20e61d65fb..52d5f3fc9f0 100644 --- a/cinder/tests/unit/api/v3/test_volumes.py +++ b/cinder/tests/unit/api/v3/test_volumes.py @@ -619,7 +619,6 @@ class VolumeApiTest(test.TestCase): 'consistencygroup': None, 'availability_zone': availability_zone, 'scheduler_hints': None, - 'multiattach': False, 'group': test_group, } @@ -1189,3 +1188,25 @@ class VolumeApiTest(test.TestCase): volumes = res_dict['volumes'] self.assertEqual(1, len(volumes)) self.assertEqual(vols[1].id, volumes[0]['id']) + + def test_create_volume_with_multiattach_param(self): + """Tests creating a volume with multiattach=True but no multiattach + + volume type. + + This test verifies that providing the multiattach parameter will error + out the request since it is removed and the recommended way is to + create a multiattach volume using a multiattach volume type. + """ + req = fakes.HTTPRequest.blank('/v3/volumes') + + body = {'volume': { + 'name': 'test name', + 'description': 'test desc', + 'size': 1, + 'multiattach': True}} + exc = self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.create, + req, body=body) + self.assertIn("multiattach parameter has been removed", + exc.explanation) diff --git a/cinder/tests/unit/volume/test_volume.py b/cinder/tests/unit/volume/test_volume.py index 0b9ac450408..fa0f1da048c 100644 --- a/cinder/tests/unit/volume/test_volume.py +++ b/cinder/tests/unit/volume/test_volume.py @@ -776,19 +776,6 @@ class VolumeTestCase(base.BaseVolumeTestCase): self.assertEqual(foo['id'], vol['volume_type_id']) self.assertTrue(vol['multiattach']) - def test_create_volume_with_multiattach_flag(self): - """Tests creating a volume with multiattach=True but no special type. - - This tests the pre 3.50 microversion behavior of being able to create - a volume with the multiattach request parameter regardless of a - multiattach-capable volume type. - """ - volume_api = cinder.volume.api.API() - volume = volume_api.create( - self.context, 1, 'name', 'description', multiattach=True, - volume_type=self.vol_type) - self.assertTrue(volume.multiattach) - def _fail_multiattach_policy_authorize(self, policy): if policy == vol_policy.MULTIATTACH_POLICY: raise exception.PolicyNotAuthorized(action='Test') @@ -813,16 +800,6 @@ class VolumeTestCase(base.BaseVolumeTestCase): 1, 'admin-vol', 'description', volume_type=foo) - def test_create_volume_with_multiattach_flag_not_authorized(self): - """Test policy unauthorized create with multiattach flag.""" - volume_api = cinder.volume.api.API() - - with mock.patch.object(self.context, 'authorize') as mock_auth: - mock_auth.side_effect = self._fail_multiattach_policy_authorize - self.assertRaises(exception.PolicyNotAuthorized, - volume_api.create, self.context, 1, 'name', - 'description', multiattach=True) - @mock.patch.object(key_manager, 'API', fake_keymgr.fake_api) def test_create_volume_with_encrypted_volume_type_multiattach(self): ctxt = context.get_admin_context() diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 39e3efa5d51..c36d1715e54 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -230,7 +230,6 @@ class API(base.Base): source_replica=None, consistencygroup: Optional[objects.ConsistencyGroup] = None, cgsnapshot: Optional[objects.CGSnapshot] = None, - multiattach: Optional[bool] = False, source_cg=None, group: Optional[objects.Group] = None, group_snapshot=None, @@ -339,7 +338,6 @@ class API(base.Base): 'optional_args': {'is_quota_committed': False}, 'consistencygroup': consistencygroup, 'cgsnapshot': cgsnapshot, - 'raw_multiattach': multiattach, 'group': group, 'group_snapshot': group_snapshot, 'source_group': source_group, diff --git a/cinder/volume/flows/api/create_volume.py b/cinder/volume/flows/api/create_volume.py index c96841f7c0e..4dbf38a8e5c 100644 --- a/cinder/volume/flows/api/create_volume.py +++ b/cinder/volume/flows/api/create_volume.py @@ -444,8 +444,7 @@ class ExtractVolumeRequestTask(flow_utils.CinderTask): cgsnapshot, group, group_snapshot, - backup: Optional[dict], - multiattach: bool = False) -> dict[str, Any]: + backup: Optional[dict]) -> dict[str, Any]: utils.check_exclusive_options(snapshot=snapshot, imageRef=image_id, @@ -493,11 +492,7 @@ class ExtractVolumeRequestTask(flow_utils.CinderTask): volume_type = objects.VolumeType.get_by_name_or_id( context, volume_type_id) extra_specs = volume_type.get('extra_specs', {}) - # NOTE(tommylikehu): Although the parameter `multiattach` from - # create volume API is deprecated now, we still need to consider - # it when multiattach is not enabled in volume type. - multiattach = (extra_specs.get( - 'multiattach', '') == ' True' or multiattach) + multiattach = (extra_specs.get('multiattach', '') == ' True') if multiattach and encryption_key_id: msg = _('Multiattach cannot be used with encrypted volumes.') raise exception.InvalidVolume(reason=msg) @@ -914,8 +909,7 @@ def get_flow(db_api, image_service_api, availability_zones, create_what, availability_zones, rebind={'size': 'raw_size', 'availability_zone': 'raw_availability_zone', - 'volume_type': 'raw_volume_type', - 'multiattach': 'raw_multiattach'})) + 'volume_type': 'raw_volume_type'})) api_flow.add(QuotaReserveTask(), EntryCreateTask(), QuotaCommitTask()) diff --git a/releasenotes/notes/remove-multiattach-request-param-4444e02533f919da.yaml b/releasenotes/notes/remove-multiattach-request-param-4444e02533f919da.yaml new file mode 100644 index 00000000000..0b11c4a4896 --- /dev/null +++ b/releasenotes/notes/remove-multiattach-request-param-4444e02533f919da.yaml @@ -0,0 +1,20 @@ +--- +fixes: + - | + `Bug #2008259 `_: + Fixed the volume create functionality where non-admin users were + able to create multiattach volumes by providing the `multiattach` + parameter in the request body. Now we can only create multiattach + volumes using a multiattach volume type, which is also the + recommended way. +other: + - | + Removed the ability to create multiattach volumes by specifying + `multiattach` parameter in the request body of a volume create + operation. This functionality is unsafe, can lead to data loss, + and has been deprecated since the Queens release. + The recommended method for creating a multiattach volume is to + use a volume type that supports multiattach. By default, volume + types can only be created by the operator. Users who have a need + for multiattach volumes should contact their operator if a suitable + volume type is not available.