Merge "RBD: save and restore multiattach features"

cinder/tests/unit/volume/drivers/test_rbd.py

@@ -347,14 +347,14 @@ class RBDTestCase(test.TestCase):
                               self.context)
 
     @mock.patch.object(driver.RBDDriver, '_enable_replication',
-                       return_value=mock.sentinel.volume_update)
+                       return_value={'replication': 'enabled'})
     def test_setup_volume_with_replication(self, mock_enable):
         self.volume_a.volume_type = fake_volume.fake_volume_type_obj(
             self.context,
             id=fake.VOLUME_TYPE_ID,
             extra_specs={'replication_enabled': '<is> True'})
         res = self.driver._setup_volume(self.volume_a)
-        self.assertEqual(mock.sentinel.volume_update, res)
+        self.assertEqual('enabled', res['replication'])
         mock_enable.assert_called_once_with(self.volume_a)
 
     @ddt.data(False, True)
@@ -365,7 +365,7 @@ class RBDTestCase(test.TestCase):
         if enabled:
             expect = {'replication_status': fields.ReplicationStatus.DISABLED}
         else:
-            expect = None
+            expect = {}
         self.assertEqual(expect, res)
         mock_enable.assert_not_called()
 
@@ -458,7 +458,7 @@ class RBDTestCase(test.TestCase):
 
         res = self.driver.create_volume(self.volume_a)
 
-        self.assertIsNone(res)
+        self.assertEqual({}, res)
         chunk_size = self.cfg.rbd_store_chunk_size * units.Mi
         order = int(math.log(chunk_size, 2))
         args = [client.ioctx, str(self.volume_a.name),
@@ -1038,7 +1038,7 @@ class RBDTestCase(test.TestCase):
                 res = self.driver.create_cloned_volume(self.volume_b,
                                                        self.volume_a)
 
-                self.assertIsNone(res)
+                self.assertEqual({}, res)
                 (self.mock_rbd.Image.return_value.create_snap
                     .assert_called_once_with('.'.join(
                         (self.volume_b.name, 'clone_snap'))))
@@ -1104,7 +1104,7 @@ class RBDTestCase(test.TestCase):
                 res = self.driver.create_cloned_volume(self.volume_b,
                                                        self.volume_a)
 
-                self.assertIsNone(res)
+                self.assertEqual({}, res)
                 (self.mock_rbd.Image.return_value.create_snap
                     .assert_called_once_with('.'.join(
                         (self.volume_b.name, 'clone_snap'))))
@@ -1153,7 +1153,7 @@ class RBDTestCase(test.TestCase):
                 res = self.driver.create_cloned_volume(self.volume_b,
                                                        self.volume_a)
 
-                self.assertIsNone(res)
+                self.assertEqual({}, res)
                 (self.mock_rbd.Image.return_value.create_snap
                  .assert_called_once_with('.'.join(
                      (self.volume_b.name, 'clone_snap'))))
@@ -1706,7 +1706,7 @@ class RBDTestCase(test.TestCase):
         if enabled:
             expect = {'replication_status': fields.ReplicationStatus.DISABLED}
         else:
-            expect = None
+            expect = {}
         context = {}
         diff = {'encryption': {},
                 'extra_specs': {}}
@@ -1750,9 +1750,9 @@ class RBDTestCase(test.TestCase):
     @ddt.unpack
     @common_mocks
     @mock.patch.object(driver.RBDDriver, '_disable_replication',
-                       return_value=mock.sentinel.disable_replication)
+                       return_value={'replication': 'disabled'})
     @mock.patch.object(driver.RBDDriver, '_enable_replication',
-                       return_value=mock.sentinel.enable_replication)
+                       return_value={'replication': 'enabled'})
     def test_retype_replicated(self, mock_disable, mock_enable, old_replicated,
                                new_replicated):
         """Test retyping a non replicated volume.
@@ -1771,15 +1771,15 @@ class RBDTestCase(test.TestCase):
         if new_replicated:
             new_type = replicated_type
             if old_replicated:
-                update = None
+                update = {}
             else:
-                update = mock.sentinel.enable_replication
+                update = {'replication': 'enabled'}
         else:
             new_type = fake_volume.fake_volume_type_obj(
                 self.context,
                 id=fake.VOLUME_TYPE2_ID),
             if old_replicated:
-                update = mock.sentinel.disable_replication
+                update = {'replication': 'disabled'}
             else:
                 update = {'replication_status':
                           fields.ReplicationStatus.DISABLED}

cinder/volume/drivers/rbd.py

@@ -772,11 +772,37 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD,
             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:
+                if image_features & feature:
                     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:
+                msg = _('Could not find saved 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', {})
@@ -787,27 +813,52 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD,
         specs = getattr(volume_type, 'extra_specs', {})
         return specs.get(EXTRA_SPECS_MULTIATTACH) == "<is> True"
 
-    def _setup_volume(self, volume):
+    def _setup_volume(self, volume, volume_type=None):
-        want_replication = self._is_replicated_type(volume.volume_type)
+
-        want_multiattach = self._is_multiattach_type(volume.volume_type)
+        if volume_type:
+            had_replication = self._is_replicated_type(volume.volume_type)
+            had_multiattach = self._is_multiattach_type(volume.volume_type)
+        else:
+            had_replication = False
+            had_multiattach = False
+            volume_type = volume.volume_type
+
+        want_replication = self._is_replicated_type(volume_type)
+        want_multiattach = self._is_multiattach_type(volume_type)
 
         if want_replication and want_multiattach:
             msg = _('Replication and Multiattach are mutually exclusive.')
             raise RBDDriverException(reason=msg)
 
+        volume_update = dict()
+
         if want_replication:
-            return self._enable_replication(volume)
+            if had_multiattach:
-
+                volume_update.update(self._disable_multiattach(volume))
-        update = None
+            if not had_replication:
-
+                try:
-        if self._is_replication_enabled:
+                    volume_update.update(self._enable_replication(volume))
-            update = {'replication_status':
+                except Exception:
-                      fields.ReplicationStatus.DISABLED}
+                    err_msg = (_('Failed to enable image replication'))
+                    raise exception.ReplicationError(reason=err_msg,
+                                                     volume_id=volume.id)
+        elif had_replication:
+            try:
+                volume_update.update(self._disable_replication(volume))
+            except Exception:
+                err_msg = (_('Failed to disable image replication'))
+                raise exception.ReplicationError(reason=err_msg,
+                                                 volume_id=volume.id)
+        elif self._is_replication_enabled:
+            volume_update.update({'replication_status':
+                                  fields.ReplicationStatus.DISABLED})
 
         if want_multiattach:
-            self._enable_multiattach(volume)
+            volume_update.update(self._enable_multiattach(volume))
+        elif had_multiattach:
+            volume_update.update(self._disable_multiattach(volume))
 
-        return update
+        return volume_update
 
     def _check_encryption_provider(self, volume, context):
         """Check that this is a LUKS encryption provider.
@@ -1197,40 +1248,7 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD,
 
     def retype(self, context, volume, new_type, diff, host):
         """Retype from one volume type to another on the same backend."""
-
+        return True, self._setup_volume(volume, new_type)
-        # NOTE: There is no mechanism to store prior image features when
-        # creating a multiattach volume.  So retyping to non-multiattach
-        # would result in an RBD image that lacks several popular
-        # features (object-map, fast-diff, etc).  Without saving prior
-        # state as we do for replication, it is impossible to know which
-        # feautures to restore.
-        if self._is_multiattach_type(volume.volume_type):
-            msg = _('Retyping from multiattach is not supported.')
-            raise RBDDriverException(reason=msg)
-
-        old_vol_replicated = self._is_replicated_type(volume.volume_type)
-        new_vol_replicated = self._is_replicated_type(new_type)
-
-        if old_vol_replicated and not new_vol_replicated:
-            try:
-                return True, self._disable_replication(volume)
-            except Exception:
-                err_msg = (_('Failed to disable image replication'))
-                raise exception.ReplicationError(reason=err_msg,
-                                                 volume_id=volume.id)
-        elif not old_vol_replicated and new_vol_replicated:
-            try:
-                return True, self._enable_replication(volume)
-            except Exception:
-                err_msg = (_('Failed to enable image replication'))
-                raise exception.ReplicationError(reason=err_msg,
-                                                 volume_id=volume.id)
-
-        if not new_vol_replicated and self._is_replication_enabled:
-            update = {'replication_status': fields.ReplicationStatus.DISABLED}
-        else:
-            update = None
-        return True, update
 
     def _dumps(self, obj):
         return json.dumps(obj, separators=(',', ':'), sort_keys=True)