From e726c07948138f514706cc69440971a2105c2bc0 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 10 May 2021 10:35:57 +0200 Subject: [PATCH] RBD: Don't flatten temporary resources There are instances where cinder needs to create a temporary volume and this can trigger a flatten of the new temporary volume, which will make the operation take a lot longer. In some cases this means slower operations, but in others it leads to rpc timeout failures. A case where we see timeout failures is when doing a backup of a snapshot and we have rbd_flatten_volume_from_snapshot=true. This patch ensures that we don't flatten temporary volumes. Closes-Bug: #1916843 Change-Id: I8f55c3beb2f8df5b2227506f82ddf6ee57c951ae --- cinder/tests/unit/volume/drivers/test_rbd.py | 97 +++++++++++++++---- cinder/volume/drivers/rbd.py | 7 +- ...ph-backup-no-flatten-36557727e9d73b2b.yaml | 7 ++ 3 files changed, 92 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/ceph-backup-no-flatten-36557727e9d73b2b.yaml diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index 5ccee4eefeb..fc992829c05 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -219,12 +219,21 @@ class RBDTestCase(test.TestCase): self.context, **{'name': u'volume-0000000a', 'id': '4c39c3c7-168f-4b32-b585-77f1b3bf0a38', + 'use_quota': True, + 'size': 10}) + + self.temp_volume = fake_volume.fake_volume_obj( + self.context, + **{'name': u'volume-0000000t', + 'id': '4c39c3c7-168f-4b32-b585-77f1b3bf0a44', + 'use_quota': False, 'size': 10}) self.volume_b = fake_volume.fake_volume_obj( self.context, **{'name': u'volume-0000000b', 'id': '0c7d1f44-5a06-403f-bb82-ae7ad0d693a6', + 'use_quota': True, 'size': 10}) self.volume_c = fake_volume.fake_volume_obj( @@ -232,15 +241,17 @@ class RBDTestCase(test.TestCase): **{'name': u'volume-0000000a', 'id': '55555555-222f-4b32-b585-9991b3bf0a99', 'size': 12, + 'use_quota': True, 'encryption_key_id': fake.ENCRYPTION_KEY_ID}) self.snapshot = fake_snapshot.fake_snapshot_obj( - self.context, name='snapshot-0000000a') + self.context, name='snapshot-0000000a', use_quota=True) self.snapshot_b = fake_snapshot.fake_snapshot_obj( self.context, **{'name': u'snapshot-0000000n', 'expected_attrs': ['volume'], + 'use_quota': True, 'volume': {'id': fake.VOLUME_ID, 'name': 'cinder-volume', 'size': 128, @@ -925,6 +936,40 @@ class RBDTestCase(test.TestCase): self.assertTrue(self.driver._clone_v2_api_checked) + @common_mocks + @mock.patch('cinder.objects.Volume.get_by_id') + @mock.patch.object(driver.RBDDriver, '_get_stripe_unit', + mock.Mock(return_value=4194304)) + @mock.patch.object(driver.RBDDriver, '_resize', mock.Mock()) + @mock.patch.object(driver.RBDDriver, '_flatten') + def test_create_temp_vol_from_snap(self, flatten_mock, volume_get_by_id): + volume_get_by_id.return_value = self.temp_volume + + snapshot = mock.Mock(volume_name='volume-name', + volume_size=self.temp_volume.size) + # This is a temp vol so this option will be ignored and won't flatten + self.cfg.rbd_flatten_volume_from_snapshot = True + + self.driver.create_volume_from_snapshot(self.temp_volume, snapshot) + flatten_mock.assert_not_called() + + @common_mocks + @mock.patch('cinder.objects.Volume.get_by_id') + @mock.patch.object(driver.RBDDriver, '_get_stripe_unit', + mock.Mock(return_value=4194304)) + @mock.patch.object(driver.RBDDriver, '_resize', mock.Mock()) + @mock.patch.object(driver.RBDDriver, '_flatten') + def test_create_vol_from_snap(self, flatten_mock, volume_get_by_id): + volume_get_by_id.return_value = self.volume_a + + snapshot = mock.Mock(volume_name='volume-name', + volume_size=self.volume_a.size) + self.cfg.rbd_flatten_volume_from_snapshot = True + + self.driver.create_volume_from_snapshot(self.volume_a, snapshot) + flatten_mock.assert_called_once_with(self.cfg.rbd_pool, + self.volume_a.name) + @common_mocks @mock.patch('cinder.objects.Volume.get_by_id') def test_delete_snapshot(self, volume_get_by_id): @@ -1245,46 +1290,64 @@ class RBDTestCase(test.TestCase): self.assertEqual(1, mock_resize.call_count) + @ddt.data(True, False) @common_mocks @mock.patch.object(driver.RBDDriver, '_enable_replication') - def test_create_cloned_volume_w_flatten(self, mock_enable_repl): + def test_create_cloned_volume_max_depth(self, use_quota, mock_enable_repl): + """Test clone when we reach max depth. + + It will flatten for normal volumes and skip flattening for temporary + volumes. + """ self.cfg.rbd_max_clone_depth = 1 + dest_vol = self.volume_b if use_quota else self.temp_volume + client = self.mock_client.return_value client.__enter__.return_value = client with mock.patch.object(self.driver, '_get_clone_info') as \ mock_get_clone_info: mock_get_clone_info.return_value = ( - ('fake_pool', self.volume_b.name, - '.'.join((self.volume_b.name, 'clone_snap')))) + ('fake_pool', dest_vol.name, + '.'.join((dest_vol.name, 'clone_snap')))) with mock.patch.object(self.driver, '_get_clone_depth') as \ mock_get_clone_depth: - # Try with no flatten required + # Force flatten mock_get_clone_depth.return_value = 1 - res = self.driver.create_cloned_volume(self.volume_b, + res = self.driver.create_cloned_volume(dest_vol, self.volume_a) self.assertEqual({}, res) (self.mock_rbd.Image.return_value.create_snap .assert_called_once_with('.'.join( - (self.volume_b.name, 'clone_snap')))) + (dest_vol.name, 'clone_snap')))) (self.mock_rbd.Image.return_value.protect_snap .assert_called_once_with('.'.join( - (self.volume_b.name, 'clone_snap')))) + (dest_vol.name, 'clone_snap')))) self.assertEqual( 1, self.mock_rbd.RBD.return_value.clone.call_count) - (self.mock_rbd.Image.return_value.unprotect_snap - .assert_called_once_with('.'.join( - (self.volume_b.name, 'clone_snap')))) - (self.mock_rbd.Image.return_value.remove_snap - .assert_called_once_with('.'.join( - (self.volume_b.name, 'clone_snap')))) - self.mock_proxy.assert_called_once_with( - self.driver, self.volume_b.name, - client=client, ioctx=client.ioctx) + proxy = self.mock_proxy.return_value.__enter__.return_value + if dest_vol.use_quota: + clone_snap_name = '.'.join((dest_vol.name, 'clone_snap')) + self.mock_rbd.Image.return_value.unprotect_snap.\ + assert_called_once_with(clone_snap_name) + self.mock_rbd.Image.return_value.remove_snap.\ + assert_called_once_with(clone_snap_name) + self.mock_proxy.assert_called_once_with( + self.driver, dest_vol.name, + client=client, ioctx=client.ioctx) + proxy.flatten.assert_called_once_with() + + else: + self.mock_rbd.Image.return_value.unprotect_snap.\ + assert_not_called() + self.mock_rbd.Image.return_value.remove_snap.\ + assert_not_called() + self.mock_proxy.assert_not_called() + proxy.flatten.assert_not_called() # Source volume is closed by direct call of close() self.assertEqual( diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index 4fd0ea9f988..16163f0485b 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -749,7 +749,8 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, # If dest volume is a clone and rbd_max_clone_depth reached, # flatten the dest after cloning. Zero rbd_max_clone_depth means # volumes are always flattened. - if depth >= self.configuration.rbd_max_clone_depth: + if (volume.use_quota and + depth >= self.configuration.rbd_max_clone_depth): LOG.info("maximum clone depth (%d) has been reached - " "flattening dest volume", self.configuration.rbd_max_clone_depth) @@ -1070,7 +1071,9 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, """Creates a volume from a snapshot.""" volume_update = self._clone(volume, self.configuration.rbd_pool, snapshot.volume_name, snapshot.name) - if self.configuration.rbd_flatten_volume_from_snapshot: + # Don't flatten temporary volumes + if (volume.use_quota and + self.configuration.rbd_flatten_volume_from_snapshot): self._flatten(self.configuration.rbd_pool, volume.name) snap_vol_size = snapshot.volume_size diff --git a/releasenotes/notes/ceph-backup-no-flatten-36557727e9d73b2b.yaml b/releasenotes/notes/ceph-backup-no-flatten-36557727e9d73b2b.yaml new file mode 100644 index 00000000000..070434b4a9d --- /dev/null +++ b/releasenotes/notes/ceph-backup-no-flatten-36557727e9d73b2b.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + RBD driver `bug #1916843 + `_: Fixed + rpc timeout when backing up RBD snapshot. We no longer flatten temporary + volumes and snapshots.