From 3ddf7ca9ea9587318b8c903e2c43b1879846d1c2 Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Tue, 11 Jan 2022 21:22:52 +0900 Subject: [PATCH] rbd: Fix snapshot delete when the source volume doesn't exist Currently snapshot delete requires access to the source volume and the operation fails if the source volume doesn't exist in the backend. This prevents some snapshots from being deleted when the source volume image is deleted from the backend for some reason (for example, after cluster format). This change makes the rbd driver to skip updating the source volume if it doesn't exist. A warning log is left so that operators can be aware of any skip event. Closes-Bug: #1957073 Change-Id: Icd9dad9ad7b3ad71b3962b078e5b94670ac41c87 --- cinder/tests/unit/volume/drivers/test_rbd.py | 20 ++++++- cinder/volume/drivers/rbd.py | 56 ++++++++++--------- .../notes/bug-1957073-0d1307a8637a62b7.yaml | 7 +++ 3 files changed, 55 insertions(+), 28 deletions(-) create mode 100644 releasenotes/notes/bug-1957073-0d1307a8637a62b7.yaml diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index e33bad69d19..09dd13a2341 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -84,6 +84,10 @@ class MockImageHasSnapshotsException(MockException): """Used as mock for rbd.ImageHasSnapshots.""" +class MockInvalidArgument(MockException): + """Used as mock for rbd.InvalidArgument.""" + + class KeyObject(object): def get_encoded(arg): return "asdf".encode('utf-8') @@ -113,7 +117,7 @@ def common_mocks(f): inst.mock_rbd.ImageNotFound = MockImageNotFoundException inst.mock_rbd.ImageExists = MockImageExistsException inst.mock_rbd.ImageHasSnapshots = MockImageHasSnapshotsException - inst.mock_rbd.InvalidArgument = MockImageNotFoundException + inst.mock_rbd.InvalidArgument = MockInvalidArgument inst.mock_rbd.PermissionError = MockPermissionError inst.driver.rbd = inst.mock_rbd @@ -1081,7 +1085,7 @@ class RBDTestCase(test.TestCase): self.driver.delete_snapshot(self.snapshot) - proxy.remove_snap.assert_called_with(self.snapshot.name) + proxy.remove_snap.assert_not_called() proxy.unprotect_snap.assert_called_with(self.snapshot.name) @common_mocks @@ -1140,6 +1144,18 @@ class RBDTestCase(test.TestCase): self.assertTrue(proxy.unprotect_snap.called) self.assertFalse(proxy.remove_snap.called) + @common_mocks + @mock.patch('cinder.objects.Volume.get_by_id') + def test_delete_snapshot_volume_not_found(self, volume_get_by_id): + volume_get_by_id.return_value = self.volume_a + proxy = self.mock_proxy.return_value + proxy.__enter__.side_effect = self.mock_rbd.ImageNotFound + + self.driver.delete_snapshot(self.snapshot) + + proxy.remove_snap.assert_not_called() + proxy.unprotect_snap.assert_not_called() + @common_mocks def test_snapshot_revert_use_temp_snapshot(self): self.assertFalse(self.driver.snapshot_revert_use_temp_snapshot()) diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index b1874447caa..655a1e4d896 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -1286,34 +1286,38 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, volume_name = utils.convert_str(snapshot.volume_name) snap_name = utils.convert_str(snapshot.name) - with RBDVolumeProxy(self, volume_name) as volume: - try: - volume.unprotect_snap(snap_name) - except self.rbd.InvalidArgument: - LOG.info( - "InvalidArgument: Unable to unprotect snapshot %s.", - snap_name) - except self.rbd.ImageNotFound: - LOG.info( - "ImageNotFound: Unable to unprotect snapshot %s.", - snap_name) - except self.rbd.ImageBusy: - children_list = self._get_children_info(volume, snap_name) + try: + with RBDVolumeProxy(self, volume_name) as volume: + try: + volume.unprotect_snap(snap_name) + except self.rbd.InvalidArgument: + LOG.info( + "InvalidArgument: Unable to unprotect snapshot %s.", + snap_name) + except self.rbd.ImageNotFound: + LOG.info("Snapshot %s does not exist in backend.", + snap_name) + return + except self.rbd.ImageBusy: + children_list = self._get_children_info(volume, snap_name) - if children_list: - for (pool, image) in children_list: - LOG.info('Image %(pool)s/%(image)s is dependent ' - 'on the snapshot %(snap)s.', - {'pool': pool, - 'image': image, - 'snap': snap_name}) + if children_list: + for (pool, image) in children_list: + LOG.info('Image %(pool)s/%(image)s is dependent ' + 'on the snapshot %(snap)s.', + {'pool': pool, + 'image': image, + 'snap': snap_name}) - raise exception.SnapshotIsBusy(snapshot_name=snap_name) - try: - volume.remove_snap(snap_name) - except self.rbd.ImageNotFound: - LOG.info("Snapshot %s does not exist in backend.", - snap_name) + raise exception.SnapshotIsBusy(snapshot_name=snap_name) + + try: + volume.remove_snap(snap_name) + except self.rbd.ImageNotFound: + LOG.info("Snapshot %s does not exist in backend.", + snap_name) + except self.rbd.ImageNotFound: + LOG.warning("Volume %s does not exist in backend.", volume_name) def snapshot_revert_use_temp_snapshot(self): """Disable the use of a temporary snapshot on revert.""" diff --git a/releasenotes/notes/bug-1957073-0d1307a8637a62b7.yaml b/releasenotes/notes/bug-1957073-0d1307a8637a62b7.yaml new file mode 100644 index 00000000000..fc5d41a73a2 --- /dev/null +++ b/releasenotes/notes/bug-1957073-0d1307a8637a62b7.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + RBD Driver `bug #1957073 + `_: Snapshot delete now + succeeds and skips updating the source volume image if the image + does not exist in the backend.