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 <eharney@redhat.com>
Change-Id: Ib28b6a2867469e73307482827d9e336b10a58432
Closes-Bug: #1852628
(cherry picked from commit 9bc67c8978)

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,
+            image.update_features.assert_called_once_with(
-                                                          True)
+                self.driver.RBD_FEATURE_JOURNALING, True)
         else:
-            calls = [call(exclusive_lock_feat, True),
+            calls = [call(self.driver.RBD_FEATURE_EXCLUSIVE_LOCK, True),
-                     call(journaling_feat, 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),
+            calls = [call(self.driver.RBD_FEATURE_JOURNALING, False),
-                     call(self.driver.rbd.RBD_FEATURE_EXCLUSIVE_LOCK,
+                     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"

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)
+                                  self.RBD_FEATURE_EXCLUSIVE_LOCK)
-            had_journaling = image.features() & self.rbd.RBD_FEATURE_JOURNALING
+            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:
+            change_features = self.MULTIATTACH_EXCLUSIONS & image_features
-                if image_features & feature:
+            image.update_features(change_features, False)
-                    image.update_features(feature, 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:
+                change_features = self.MULTIATTACH_EXCLUSIONS & image_features
-                msg = _('Could not find saved 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
+        try:
-        specs = getattr(volume_type, 'extra_specs', {})
+            extra_specs = volume_type.extra_specs
-        return specs.get(EXTRA_SPECS_REPL_ENABLED) == "<is> True"
+            LOG.debug('extra_specs: %s', extra_specs)
+            return extra_specs.get(EXTRA_SPECS_REPL_ENABLED) == "<is> 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
+        try:
-        specs = getattr(volume_type, 'extra_specs', {})
+            extra_specs = volume_type.extra_specs
-        return specs.get(EXTRA_SPECS_MULTIATTACH) == "<is> True"
+            LOG.debug('extra_specs: %s', extra_specs)
+            return extra_specs.get(EXTRA_SPECS_MULTIATTACH) == "<is> 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,
+                image.update_features(self.RBD_FEATURE_EXCLUSIVE_LOCK, False)
-                                      False)
         return {'replication_status': fields.ReplicationStatus.DISABLED,
                 'replication_driver_data': None}
 

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.