From f8d0e0cad271baaac0f86aa3fe156272deabb48e Mon Sep 17 00:00:00 2001 From: silvacarloss Date: Tue, 24 Jan 2023 11:49:14 -0300 Subject: [PATCH] [Cephfs] Fix erroneous share mode override on ensure_shares In order to ensure that a share existed, we used to send the same request to Ceph as we would do with the share creation. In that request, we will set the share mode, which is a value set by the deployer through the `cephfs_volume_mode` config option. If the share was already created, Ceph would not override the value we were forwarding for the permission mode. After a bugfix [1], the value of mode started being reinforced when sent, even if the subvolume was already created. If the value for `cephfs_volume_mode` changed after the share was created, the mode is now being overwritten after manila startup on ensure_shares. Fixed this issue by modifying the ensure_shares to only confirm if the export exists. [1] https://tracker.ceph.com/issues/54375 Closes-Bug: #2002394 Change-Id: Ic5b5b42b882ce1346b4b46d1b29aa31740933e0e (cherry picked from commit c61c1977c25e151fd2839febd08a7fdac6b1f7b8) --- manila/share/drivers/cephfs/driver.py | 13 +++-- .../tests/share/drivers/cephfs/test_driver.py | 49 ++++++++++--------- ...ent-on-ensure-shares-a2e4d8f6c07c8cf5.yaml | 13 +++++ 3 files changed, 49 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/bug-2002394-fix-bad-mode-enforcement-on-ensure-shares-a2e4d8f6c07c8cf5.yaml diff --git a/manila/share/drivers/cephfs/driver.py b/manila/share/drivers/cephfs/driver.py index 37e8935e31..9a4a504451 100644 --- a/manila/share/drivers/cephfs/driver.py +++ b/manila/share/drivers/cephfs/driver.py @@ -465,8 +465,9 @@ class CephFSDriver(driver.ExecuteMixin, driver.GaneshaMixin, "sub_name": share["id"], "size": size, "namespace_isolated": True, - "mode": self._cephfs_volume_mode, + "mode": self._cephfs_volume_mode } + if share['share_group_id'] is not None: argdict.update({"group_name": share["share_group_id"]}) @@ -540,8 +541,14 @@ class CephFSDriver(driver.ExecuteMixin, driver.GaneshaMixin, share_server=share_server) def ensure_share(self, context, share, share_server=None): - # Creation is idempotent - return self.create_share(context, share, share_server) + try: + export_location = self._get_export_locations(share) + except exception.ShareBackendException as e: + if 'does not exist' in str(e).lower(): + raise exception.ShareResourceNotFound(share_id=share['id']) + raise + + return export_location def extend_share(self, share, new_size, share_server=None): # resize FS subvolume/share diff --git a/manila/tests/share/drivers/cephfs/test_driver.py b/manila/tests/share/drivers/cephfs/test_driver.py index 2106aa8e0b..0d8b797b40 100644 --- a/manila/tests/share/drivers/cephfs/test_driver.py +++ b/manila/tests/share/drivers/cephfs/test_driver.py @@ -236,34 +236,37 @@ class CephFSDriverTestCase(test.TestCase): share_server=None) def test_ensure_share(self): - create_share_prefix = "fs subvolume create" - get_path_prefix = "fs subvolume getpath" - - create_share_dict = { - "vol_name": self._driver.volname, - "sub_name": self._share["id"], - "size": self._share["size"] * units.Gi, - "namespace_isolated": True, - "mode": DEFAULT_VOLUME_MODE, + expected_exports = { + 'path': '1.2.3.4,5.6.7.8:/foo/bar', + 'is_admin_only': False, + 'metadata': {}, } + self.mock_object( + self._driver, '_get_export_locations', + mock.Mock(return_value=expected_exports)) - get_path_dict = { - "vol_name": self._driver.volname, - "sub_name": self._share["id"], - } + returned_exports = self._driver.ensure_share(self._context, + self._share) - self._driver.ensure_share(self._context, - self._share) + self._driver._get_export_locations.assert_called_once_with(self._share) - driver.rados_command.assert_has_calls([ - mock.call(self._driver.rados_client, - create_share_prefix, - create_share_dict), - mock.call(self._driver.rados_client, - get_path_prefix, - get_path_dict)]) + self.assertEqual(expected_exports, returned_exports) - self.assertEqual(2, driver.rados_command.call_count) + def test_ensure_share_subvolume_not_found(self): + message = f"Error ENOENT: subvolume {self._share['id']} does not exist" + expected_exception = exception.ShareBackendException(message) + + self.mock_object( + self._driver, '_get_export_locations', + mock.Mock(side_effect=expected_exception)) + + self.assertRaises( + exception.ShareResourceNotFound, + self._driver.ensure_share, + self._context, + self._share) + + self._driver._get_export_locations.assert_called_once_with(self._share) def test_delete_share(self): clone_status_prefix = "fs clone status" diff --git a/releasenotes/notes/bug-2002394-fix-bad-mode-enforcement-on-ensure-shares-a2e4d8f6c07c8cf5.yaml b/releasenotes/notes/bug-2002394-fix-bad-mode-enforcement-on-ensure-shares-a2e4d8f6c07c8cf5.yaml new file mode 100644 index 0000000000..f0844842e7 --- /dev/null +++ b/releasenotes/notes/bug-2002394-fix-bad-mode-enforcement-on-ensure-shares-a2e4d8f6c07c8cf5.yaml @@ -0,0 +1,13 @@ +--- +fixes: + - | + Fixed an issue that made the CephFS driver to override the permissions in + a share. After `a bugfix `_, Ceph's + idempotent creation of shares had a change on its behavior. If a share + mode was modified outside of Manila, or the configuration value for + `cephfs_volume_mode` was changed in Manila when shares had already been + created, these shares would have their mode changed while Manila attempted + to ensure that such share exists using the idempotent creation, potentially + breaking clients. The CephFS driver will no longer send create calls to the + backend when ensuring a share exists. For more details, please refer to + `Bug #2002394 `_