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.