From 99321eb91935b9a255399894b9a3ab15cf0e83e5 Mon Sep 17 00:00:00 2001 From: Jon Bernard Date: Thu, 14 Nov 2019 13:29:45 -0500 Subject: [PATCH] RBD: catch argument exceptions when configuring multiattach If an expected flag is already in the desired state, RBD will raise an InvalidArgument exception. This is okay, however not catching this will result in volume operation errors. Co-Author: Eric Harney Change-Id: Ib28b6a2867469e73307482827d9e336b10a58432 Closes-Bug: #1852628 (cherry picked from commit 9bc67c8978faba9e879bf377652d2679e143d992) --- cinder/tests/unit/volume/drivers/test_rbd.py | 79 ++++++++++++++--- cinder/volume/drivers/rbd.py | 85 +++++++++++-------- ...ltiattach-exceptions-43066312f3b527f5.yaml | 6 ++ 3 files changed, 121 insertions(+), 49 deletions(-) create mode 100644 releasenotes/notes/rbd-multiattach-exceptions-43066312f3b527f5.yaml diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index bd65f37d8b5..f60bc30ef77 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -423,18 +423,19 @@ class RBDTestCase(test.TestCase): 'image.features()'will be both called for 'exclusive-lock' and 'journaling' in this order. """ - journaling_feat = 1 - exclusive_lock_feat = 2 - self.driver.rbd.RBD_FEATURE_JOURNALING = journaling_feat - self.driver.rbd.RBD_FEATURE_EXCLUSIVE_LOCK = exclusive_lock_feat image = self.mock_proxy.return_value.__enter__.return_value - image.features.return_value = 0 + + image_features = 0 if exclusive_lock_enabled: - image.features.return_value += exclusive_lock_feat + image_features |= self.driver.RBD_FEATURE_EXCLUSIVE_LOCK if journaling_enabled: - image.features.return_value += journaling_feat + image_features |= self.driver.RBD_FEATURE_JOURNALING + + image.features.return_value = image_features + journaling_status = str(journaling_enabled).lower() exclusive_lock_status = str(exclusive_lock_enabled).lower() + expected = { 'replication_driver_data': ('{"had_exclusive_lock":%s,' '"had_journaling":%s}' % @@ -444,14 +445,15 @@ class RBDTestCase(test.TestCase): } res = self.driver._enable_replication(self.volume_a) self.assertEqual(expected, res) + if exclusive_lock_enabled and journaling_enabled: image.update_features.assert_not_called() elif exclusive_lock_enabled and not journaling_enabled: - image.update_features.assert_called_once_with(journaling_feat, - True) + image.update_features.assert_called_once_with( + self.driver.RBD_FEATURE_JOURNALING, True) else: - calls = [call(exclusive_lock_feat, True), - call(journaling_feat, True)] + calls = [call(self.driver.RBD_FEATURE_EXCLUSIVE_LOCK, True), + call(self.driver.RBD_FEATURE_JOURNALING, True)] image.update_features.assert_has_calls(calls, any_order=False) image.mirror_image_enable.assert_called_once_with() @@ -474,10 +476,10 @@ class RBDTestCase(test.TestCase): image.update_features.assert_not_called() elif had_journaling == 'false' and had_exclusive_lock == 'true': image.update_features.assert_called_once_with( - self.driver.rbd.RBD_FEATURE_JOURNALING, False) + self.driver.RBD_FEATURE_JOURNALING, False) else: - calls = [call(self.driver.rbd.RBD_FEATURE_JOURNALING, False), - call(self.driver.rbd.RBD_FEATURE_EXCLUSIVE_LOCK, + calls = [call(self.driver.RBD_FEATURE_JOURNALING, False), + call(self.driver.RBD_FEATURE_EXCLUSIVE_LOCK, False)] image.update_features.assert_has_calls(calls, any_order=False) @@ -2459,6 +2461,55 @@ class RBDTestCase(test.TestCase): ret = driver.get_backup_device(self.context, backup) self.assertEqual(ret, (self.volume_b, False)) + @common_mocks + def test_multiattach_exclusions(self): + self.assertEqual( + self.driver.RBD_FEATURE_JOURNALING | + self.driver.RBD_FEATURE_FAST_DIFF | + self.driver.RBD_FEATURE_OBJECT_MAP | + self.driver.RBD_FEATURE_EXCLUSIVE_LOCK, + self.driver.MULTIATTACH_EXCLUSIONS) + + MULTIATTACH_FULL_FEATURES = ( + driver.RBDDriver.RBD_FEATURE_LAYERING | + driver.RBDDriver.RBD_FEATURE_EXCLUSIVE_LOCK | + driver.RBDDriver.RBD_FEATURE_OBJECT_MAP | + driver.RBDDriver.RBD_FEATURE_FAST_DIFF | + driver.RBDDriver.RBD_FEATURE_JOURNALING) + + MULTIATTACH_REDUCED_FEATURES = ( + driver.RBDDriver.RBD_FEATURE_LAYERING | + driver.RBDDriver.RBD_FEATURE_EXCLUSIVE_LOCK) + + @ddt.data(MULTIATTACH_FULL_FEATURES, MULTIATTACH_REDUCED_FEATURES) + @common_mocks + def test_enable_multiattach(self, features): + image = self.mock_proxy.return_value.__enter__.return_value + image_features = features + image.features.return_value = image_features + + ret = self.driver._enable_multiattach(self.volume_a) + + image.update_features.assert_called_once_with( + self.driver.MULTIATTACH_EXCLUSIONS & image_features, False) + + self.assertEqual( + {'provider_location': + "{\"saved_features\":%s}" % image_features}, ret) + + @ddt.data(MULTIATTACH_FULL_FEATURES, MULTIATTACH_REDUCED_FEATURES) + @common_mocks + def test_disable_multiattach(self, features): + image = self.mock_proxy.return_value.__enter__.return_value + self.volume_a.provider_location = '{"saved_features": %s}' % features + + ret = self.driver._disable_multiattach(self.volume_a) + + image.update_features.assert_called_once_with( + self.driver.MULTIATTACH_EXCLUSIONS & features, True) + + self.assertEqual({'provider_location': None}, ret) + class ManagedRBDTestCase(test_driver.BaseDriverTestCase): driver_name = "cinder.volume.drivers.rbd.RBDDriver" diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index 70adce85b73..4174ca1ac8e 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -204,7 +204,7 @@ class RADOSClient(object): def features(self): features = self.cluster.conf_get('rbd_default_features') if ((features is None) or (int(features) == 0)): - features = self.driver.rbd.RBD_FEATURE_LAYERING + features = self.driver.RBD_FEATURE_LAYERING return int(features) @@ -223,6 +223,12 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, SYSCONFDIR = '/etc/ceph/' + RBD_FEATURE_LAYERING = 1 + RBD_FEATURE_EXCLUSIVE_LOCK = 4 + RBD_FEATURE_OBJECT_MAP = 8 + RBD_FEATURE_FAST_DIFF = 16 + RBD_FEATURE_JOURNALING = 64 + def __init__(self, active_backend_id=None, *args, **kwargs): super(RBDDriver, self).__init__(*args, **kwargs) self.configuration.append_config_values(RBD_OPTS) @@ -247,6 +253,20 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, self._replication_targets = [] self._target_names = [] + if self.rbd is not None: + self.RBD_FEATURE_LAYERING = self.rbd.RBD_FEATURE_LAYERING + self.RBD_FEATURE_EXCLUSIVE_LOCK = \ + self.rbd.RBD_FEATURE_EXCLUSIVE_LOCK + self.RBD_FEATURE_OBJECT_MAP = self.rbd.RBD_FEATURE_OBJECT_MAP + self.RBD_FEATURE_FAST_DIFF = self.rbd.RBD_FEATURE_FAST_DIFF + self.RBD_FEATURE_JOURNALING = self.rbd.RBD_FEATURE_JOURNALING + + self.MULTIATTACH_EXCLUSIONS = ( + self.RBD_FEATURE_JOURNALING | + self.RBD_FEATURE_FAST_DIFF | + self.RBD_FEATURE_OBJECT_MAP | + self.RBD_FEATURE_EXCLUSIVE_LOCK) + @staticmethod def get_driver_options(): return RBD_OPTS @@ -752,13 +772,13 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, vol_name = utils.convert_str(volume.name) with RBDVolumeProxy(self, vol_name) as image: had_exclusive_lock = (image.features() & - self.rbd.RBD_FEATURE_EXCLUSIVE_LOCK) - had_journaling = image.features() & self.rbd.RBD_FEATURE_JOURNALING + self.RBD_FEATURE_EXCLUSIVE_LOCK) + had_journaling = image.features() & self.RBD_FEATURE_JOURNALING if not had_exclusive_lock: - image.update_features(self.rbd.RBD_FEATURE_EXCLUSIVE_LOCK, + image.update_features(self.RBD_FEATURE_EXCLUSIVE_LOCK, True) if not had_journaling: - image.update_features(self.rbd.RBD_FEATURE_JOURNALING, True) + image.update_features(self.RBD_FEATURE_JOURNALING, True) image.mirror_image_enable() driver_data = self._dumps({ @@ -769,53 +789,49 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, 'replication_driver_data': driver_data} def _enable_multiattach(self, volume): - multipath_feature_exclusions = [ - self.rbd.RBD_FEATURE_JOURNALING, - self.rbd.RBD_FEATURE_FAST_DIFF, - self.rbd.RBD_FEATURE_OBJECT_MAP, - self.rbd.RBD_FEATURE_EXCLUSIVE_LOCK, - ] vol_name = utils.convert_str(volume.name) - image_features = None with RBDVolumeProxy(self, vol_name) as image: image_features = image.features() - for feature in multipath_feature_exclusions: - if image_features & feature: - image.update_features(feature, False) + change_features = self.MULTIATTACH_EXCLUSIONS & image_features + image.update_features(change_features, False) return {'provider_location': self._dumps({'saved_features': image_features})} def _disable_multiattach(self, volume): - multipath_feature_exclusions = [ - self.rbd.RBD_FEATURE_EXCLUSIVE_LOCK, - self.rbd.RBD_FEATURE_OBJECT_MAP, - self.rbd.RBD_FEATURE_FAST_DIFF, - self.rbd.RBD_FEATURE_JOURNALING, - ] vol_name = utils.convert_str(volume.name) with RBDVolumeProxy(self, vol_name) as image: try: provider_location = json.loads(volume.provider_location) image_features = provider_location['saved_features'] - except Exception: - msg = _('Could not find saved image features.') + change_features = self.MULTIATTACH_EXCLUSIONS & image_features + image.update_features(change_features, True) + except IndexError: + msg = "Could not find saved image features." + raise RBDDriverException(reason=msg) + except self.rbd.InvalidArgument: + msg = "Failed to restore image features." raise RBDDriverException(reason=msg) - for feature in multipath_feature_exclusions: - if image_features & feature: - image.update_features(feature, True) return {'provider_location': None} def _is_replicated_type(self, volume_type): - # We do a safe attribute get because volume_type could be None - specs = getattr(volume_type, 'extra_specs', {}) - return specs.get(EXTRA_SPECS_REPL_ENABLED) == " True" + try: + extra_specs = volume_type.extra_specs + LOG.debug('extra_specs: %s', extra_specs) + return extra_specs.get(EXTRA_SPECS_REPL_ENABLED) == " True" + except Exception: + LOG.debug('Unable to retrieve extra specs info') + return False def _is_multiattach_type(self, volume_type): - # We do a safe attribute get because volume_type could be None - specs = getattr(volume_type, 'extra_specs', {}) - return specs.get(EXTRA_SPECS_MULTIATTACH) == " True" + try: + extra_specs = volume_type.extra_specs + LOG.debug('extra_specs: %s', extra_specs) + return extra_specs.get(EXTRA_SPECS_MULTIATTACH) == " True" + except Exception: + LOG.debug('Unable to retrieve extra specs info') + return False def _setup_volume(self, volume, volume_type=None): @@ -1224,10 +1240,9 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, # they will be disabled here. If not, it will keep # what it was before. if not driver_data['had_journaling']: - image.update_features(self.rbd.RBD_FEATURE_JOURNALING, False) + image.update_features(self.RBD_FEATURE_JOURNALING, False) if not driver_data['had_exclusive_lock']: - image.update_features(self.rbd.RBD_FEATURE_EXCLUSIVE_LOCK, - False) + image.update_features(self.RBD_FEATURE_EXCLUSIVE_LOCK, False) return {'replication_status': fields.ReplicationStatus.DISABLED, 'replication_driver_data': None} diff --git a/releasenotes/notes/rbd-multiattach-exceptions-43066312f3b527f5.yaml b/releasenotes/notes/rbd-multiattach-exceptions-43066312f3b527f5.yaml new file mode 100644 index 00000000000..d5b19f3bf49 --- /dev/null +++ b/releasenotes/notes/rbd-multiattach-exceptions-43066312f3b527f5.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Catch argument exceptions when configuring multiattach for rbd volumes. + This allows multiattach images with flags already set to continue instead + of raising an exception and failing.