From 7f49b41eefbbbe5b5b7f95b5d936ee9118861ba5 Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Tue, 14 May 2024 13:33:48 -0400 Subject: [PATCH] RBD: Fix handling of RBD errors in get_manageable_volumes The "except ImageNotFound" was outside of the required try block, so ImageNotFound errors would result in exceptions being thrown up to the API layer. Add handling for other errors that could be thrown by RBD here, too. Reference info: https://github.com/ceph/ceph/blob/08d7ff952d/src/pybind/rbd/rbd.pyx#L8 Closes-Bug: #2065713 Change-Id: Ifaceb04263973b1445c7f13a6155dc63647d7176 --- cinder/tests/unit/volume/drivers/test_rbd.py | 32 +++++++++++++++++++ cinder/volume/drivers/rbd.py | 15 +++++---- ...-driver-exc-handling-f8de823cd9acd767.yaml | 7 ++++ 3 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/rbd-bug-2065713-driver-exc-handling-f8de823cd9acd767.yaml diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index 9952f8a722d..e58385bab43 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -81,6 +81,10 @@ class MockPermissionError(MockException): errno = errno.EPERM +class MockTimeOutException(MockException): + """Used as mock for TimeOut.""" + + class MockImageHasSnapshotsException(MockException): """Used as mock for rbd.ImageHasSnapshots.""" @@ -114,12 +118,14 @@ def common_mocks(f): inst.mock_proxy = mock_proxy inst.mock_rbd.RBD.Error = Exception inst.mock_rados.Error = Exception + inst.mock_rbd.Error = Exception inst.mock_rbd.ImageBusy = MockImageBusyException inst.mock_rbd.ImageNotFound = MockImageNotFoundException inst.mock_rbd.ImageExists = MockImageExistsException inst.mock_rbd.ImageHasSnapshots = MockImageHasSnapshotsException inst.mock_rbd.InvalidArgument = MockInvalidArgument inst.mock_rbd.PermissionError = MockPermissionError + inst.mock_rbd.TimeOut = MockTimeOutException inst.driver.rbd = inst.mock_rbd aux = inst.driver.rbd @@ -766,6 +772,32 @@ class RBDTestCase(test.TestCase): ] self.assertEqual(exp, res) + @common_mocks + @mock.patch.object(driver.RBDDriver, '_get_image_status') + def test_get_manageable_volumes_exc(self, mock_get_image_status): + cinder_vols = [{'id': '00000000-0000-0000-0000-000000000000'}] + vols = ['volume-00000000-0000-0000-0000-000000000000', 'vol1', 'vol2', + 'volume-11111111-1111-1111-1111-111111111111.deleted'] + self.mock_rbd.RBD.return_value.list.return_value = vols + image = self.mock_proxy.return_value.__enter__.return_value + # Four images are present, but the third image can't be opened + image.size.side_effect = [2 * units.Gi, + self.mock_rbd.ImageNotFound, + self.mock_rbd.TimeOut, + self.mock_rbd.PermissionError] + mock_get_image_status.side_effect = [ + {'watchers': []}, + {'watchers': []}, + {'watchers': []}] + res = self.driver.get_manageable_volumes( + cinder_vols, None, 1000, 0, ['size'], ['asc']) + exp = [{'size': 2, 'reason_not_safe': 'already managed', + 'extra_info': None, 'safe_to_manage': False, + 'reference': {'source-name': + 'volume-00000000-0000-0000-0000-000000000000'}, + 'cinder_id': '00000000-0000-0000-0000-000000000000'}] + self.assertEqual(exp, res) + @common_mocks def test_delete_backup_snaps(self): self.driver.rbd.Image.remove_snap = mock.Mock() diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index 2d4017314b5..8160837b26b 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -2205,10 +2205,10 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, with RADOSClient(self) as client: for image_name in self.RBDProxy().list(client.ioctx): image_id = volume_utils.extract_id_from_volume_name(image_name) - with RBDVolumeProxy(self, image_name, read_only=True, - client=client.cluster, - ioctx=client.ioctx) as image: - try: + try: + with RBDVolumeProxy(self, image_name, read_only=True, + client=client.cluster, + ioctx=client.ioctx) as image: image_info = { 'reference': {'source-name': image_name}, 'size': int(math.ceil( @@ -2236,8 +2236,11 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, image_info['safe_to_manage'] = True image_info['reason_not_safe'] = None manageable_volumes.append(image_info) - except self.rbd.ImageNotFound: - LOG.debug("Image %s is not found.", image_name) + except self.rbd.ImageNotFound: + LOG.debug("Image %s is not found.", image_name) + except self.rbd.Error as error: + LOG.debug("Cannot open image %(image)s. (%(error)s)", + ({'image': image_name, 'error': error})) return volume_utils.paginate_entries_list( manageable_volumes, marker, limit, offset, sort_keys, sort_dirs) diff --git a/releasenotes/notes/rbd-bug-2065713-driver-exc-handling-f8de823cd9acd767.yaml b/releasenotes/notes/rbd-bug-2065713-driver-exc-handling-f8de823cd9acd767.yaml new file mode 100644 index 00000000000..ca0a009fdc0 --- /dev/null +++ b/releasenotes/notes/rbd-bug-2065713-driver-exc-handling-f8de823cd9acd767.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + `Bug #2065713 `_: Due to + incorrect exception handling, ImageNotFound errors in the RBD driver's + get_manageable_volumes operation would propagate up to the API layer rather + than being caught and handled in the driver.