From 49a2c85eda9fd3cddc75fd904fe62c87a6b50735 Mon Sep 17 00:00:00 2001 From: Jon Bernard Date: Wed, 14 Apr 2021 11:14:13 -0400 Subject: [PATCH] RBD: use correct stripe unit in clone operation The recent release of Ceph Pacific saw a change to the clone() logic where invalid values of stripe unit would cause an error to be returned where previous versions would correct the value at runtime. This becomes a problem when creating a volume from an image, where the source RBD image may have a larger stripe unit than cinder's RBD driver is configured for. When this happens, clone() is called with a stripe unit that is too small given that of the source image and the clone fails. The RBD driver in Cinder has a configuration parameter 'rbd_store_chunk_size' that stores the preferred object size for cloned images. If clone() is called without a stripe_unit passed in, the stripe unit defaults to the object size, which is 4MB by default. The issue arises when creating a volume from a Glance image, where Glance is creating images with a default stripe unit of 8MB (distinctly larger than that of Cinder). If we do not consider the incoming stripe unit and select the larger of the two, Ceph cannot clone an RBD image with a smaller stripe unit and raises an error. This patch adds a function in our driver's clone logic to select the larger of the two stripe unit values so that the appropriate stripe unit is chosen. It should also be noted that we're determining the correct stripe unit, but using the 'order' argument to clone(). Ceph will set the stripe unit equal to the object size (order) by default and we rely on this behaviour for the following reason: passing stripe-unit alone or with an order argument causes an invalid argument exception to be raised in pre-pacific releases of Ceph, as it's argument parsing appears to have limitations. Closes-Bug: #1931004 Change-Id: Iec111ab83e9ed8182c9679c911e3d90927d5a7c3 --- cinder/tests/unit/volume/drivers/test_rbd.py | 49 ++++++++++++------- cinder/volume/drivers/rbd.py | 24 +++++++-- ...-correct-stripe-unit-9d317f4717533fb4.yaml | 6 +++ 3 files changed, 58 insertions(+), 21 deletions(-) create mode 100644 releasenotes/notes/rbd-choose-correct-stripe-unit-9d317f4717533fb4.yaml diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index 8509d8a1d0c..5ccee4eefeb 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -890,11 +890,12 @@ class RBDTestCase(test.TestCase): self.cfg.rbd_flatten_volume_from_snapshot = False - with mock.patch.object(driver, 'LOG') as \ - mock_log: - - self.driver.create_volume_from_snapshot(self.volume_a, - self.snapshot) + with mock.patch.object(driver, 'LOG') as mock_log: + with mock.patch.object(self.driver.rbd.Image(), 'stripe_unit') as \ + mock_rbd_image_stripe_unit: + mock_rbd_image_stripe_unit.return_value = 4194304 + self.driver.create_volume_from_snapshot(self.volume_a, + self.snapshot) mock_log.info.assert_called_with('Using v2 Clone API') @@ -912,11 +913,12 @@ class RBDTestCase(test.TestCase): self.cfg.rbd_flatten_volume_from_snapshot = False - with mock.patch.object(driver, 'LOG') as \ - mock_log: - - self.driver.create_volume_from_snapshot(self.volume_a, - self.snapshot) + with mock.patch.object(driver, 'LOG') as mock_log: + with mock.patch.object(self.driver.rbd.Image(), 'stripe_unit') as \ + mock_rbd_image_stripe_unit: + mock_rbd_image_stripe_unit.return_value = 4194304 + self.driver.create_volume_from_snapshot(self.volume_a, + self.snapshot) self.assertTrue(any(m for m in mock_log.warning.call_args_list if 'Not using v2 clone API' in m[0][0])) @@ -1778,13 +1780,13 @@ class RBDTestCase(test.TestCase): self.assertEqual(cfg_file, self.driver.keyring_file) self.assertIsNone(self.driver.keyring_data) - @ddt.data({'rbd_chunk_size': 1, 'order': 20}, - {'rbd_chunk_size': 8, 'order': 23}, - {'rbd_chunk_size': 32, 'order': 25}) + @ddt.data({'rbd_chunk_size': 1}, + {'rbd_chunk_size': 8}, + {'rbd_chunk_size': 32}) @ddt.unpack @common_mocks @mock.patch.object(driver.RBDDriver, '_enable_replication') - def test_clone(self, mock_enable_repl, rbd_chunk_size, order): + def test_clone(self, mock_enable_repl, rbd_chunk_size): self.cfg.rbd_store_chunk_size = rbd_chunk_size src_pool = u'images' src_image = u'image-name' @@ -1802,14 +1804,20 @@ class RBDTestCase(test.TestCase): # capture both rados client used to perform the clone client.__enter__.side_effect = mock__enter__(client) - res = self.driver._clone(self.volume_a, src_pool, src_image, src_snap) + with mock.patch.object(self.driver.rbd.Image(), 'stripe_unit') as \ + mock_rbd_image_stripe_unit: + mock_rbd_image_stripe_unit.return_value = 4194304 + res = self.driver._clone(self.volume_a, src_pool, src_image, + src_snap) self.assertEqual({}, res) args = [client_stack[0].ioctx, str(src_image), str(src_snap), client_stack[1].ioctx, str(self.volume_a.name)] + stripe_unit = max(4194304, rbd_chunk_size * 1048576) + expected_order = int(math.log(stripe_unit, 2)) kwargs = {'features': client.features, - 'order': order} + 'order': expected_order} self.mock_rbd.RBD.return_value.clone.assert_called_once_with( *args, **kwargs) self.assertEqual(2, client.__enter__.call_count) @@ -1818,8 +1826,9 @@ class RBDTestCase(test.TestCase): @common_mocks @mock.patch.object(driver.RBDDriver, '_enable_replication') def test_clone_replicated(self, mock_enable_repl): - rbd_chunk_size = 1 order = 20 + rbd_chunk_size = 1 + stripe_unit = 1048576 self.volume_a.volume_type = fake_volume.fake_volume_type_obj( self.context, id=fake.VOLUME_TYPE_ID, @@ -1848,7 +1857,11 @@ class RBDTestCase(test.TestCase): # capture both rados client used to perform the clone client.__enter__.side_effect = mock__enter__(client) - res = self.driver._clone(self.volume_a, src_pool, src_image, src_snap) + with mock.patch.object(self.driver.rbd.Image(), 'stripe_unit') as \ + mock_rbd_image_stripe_unit: + mock_rbd_image_stripe_unit.return_value = stripe_unit + res = self.driver._clone(self.volume_a, src_pool, src_image, + src_snap) self.assertEqual(expected_update, res) mock_enable_repl.assert_called_once_with(self.volume_a) diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index 13129e03bf9..4fd0ea9f988 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -1005,16 +1005,35 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, with RBDVolumeProxy(self, volume_name, pool) as vol: vol.flatten() + def _get_stripe_unit(self, ioctx, volume_name): + """Return the correct stripe unit for a cloned volume. + + A cloned volume must be created with a stripe unit at least as large + as the source volume. We compute the desired stripe width from + rbd_store_chunk_size and compare that to the incoming source volume's + stripe width, selecting the larger to avoid error. + """ + default_stripe_unit = \ + self.configuration.rbd_store_chunk_size * units.Mi + + image = self.rbd.Image(ioctx, volume_name) + try: + image_stripe_unit = image.stripe_unit() + finally: + image.close() + + return max(image_stripe_unit, default_stripe_unit) + def _clone(self, volume, src_pool, src_image, src_snap): LOG.debug('cloning %(pool)s/%(img)s@%(snap)s to %(dst)s', dict(pool=src_pool, img=src_image, snap=src_snap, dst=volume.name)) - chunk_size = self.configuration.rbd_store_chunk_size * units.Mi - order = int(math.log(chunk_size, 2)) vol_name = utils.convert_str(volume.name) with RADOSClient(self, src_pool) as src_client: + stripe_unit = self._get_stripe_unit(src_client.ioctx, src_image) + order = int(math.log(stripe_unit, 2)) with RADOSClient(self) as dest_client: self.RBDProxy().clone(src_client.ioctx, utils.convert_str(src_image), @@ -1023,7 +1042,6 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, vol_name, features=src_client.features, order=order) - try: volume_update = self._setup_volume(volume) except Exception: diff --git a/releasenotes/notes/rbd-choose-correct-stripe-unit-9d317f4717533fb4.yaml b/releasenotes/notes/rbd-choose-correct-stripe-unit-9d317f4717533fb4.yaml new file mode 100644 index 00000000000..0cd362341ef --- /dev/null +++ b/releasenotes/notes/rbd-choose-correct-stripe-unit-9d317f4717533fb4.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + `Bug #1931004 `_: Fixed + use of incorrect stripe unit in RBD image clone causing volume-from-image + to fail when using raw images backed by Ceph.