openstack/cinder
Code Issues Proposed changes

RBD: use correct stripe unit in clone operation

Browse Source 
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
This commit is contained in:
Jon Bernard 2021-04-14 11:14:13 -04:00
parent b06a300eac
commit 49a2c85eda
3 changed files with 58 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
cinder/tests/unit/volume/drivers/test_rbd.py
View File

@ -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)

24
cinder/volume/drivers/rbd.py
View File

@ -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:

6
releasenotes/notes/rbd-choose-correct-stripe-unit-9d317f4717533fb4.yaml Normal file
View File

@ -0,0 +1,6 @@
---
fixes:
- |
`Bug #1931004 <https://bugs.launchpad.net/cinder/+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.