Set replication_status automatically on retype

When retyping a volume we should be updating the replication_status in
accordance with the new volume type, but right now we are leaving it to
the drivers to do so, which means that in some cases it is not being
updated.

This patch fixes this by doing it automatically based on the
replication_enabled value in extra_specs.

Closes-Bug: #1643884
Change-Id: I10a7931ed4a73dfd2b69f0db979bc32a71aedb11

cinder/tests/unit/test_volume.py

@@ -5449,20 +5449,31 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase):
                             snap=False, policy='on-demand',
                             migrate_exc=False, exc=None, diff_equal=False,
                             replica=False, reserve_vol_type_only=False,
-                            encryption_changed=False):
+                            encryption_changed=False,
+                            replica_new=None):
         elevated = context.get_admin_context()
         project_id = self.context.project_id
 
-        db.volume_type_create(elevated, {'name': 'old', 'extra_specs': {}})
+        if replica:
+            rep_status = 'enabled'
+            extra_specs = {'replication_enabled': '<is> True'}
+        else:
+            rep_status = 'disabled'
+            extra_specs = {}
+
+        if replica_new is None:
+            replica_new = replica
+        new_specs = {'replication_enabled': '<is> True'} if replica_new else {}
+
+        db.volume_type_create(elevated, {'name': 'old',
+                                         'extra_specs': extra_specs})
         old_vol_type = db.volume_type_get_by_name(elevated, 'old')
-        db.volume_type_create(elevated, {'name': 'new', 'extra_specs': {}})
+
+        db.volume_type_create(elevated, {'name': 'new',
+                                         'extra_specs': new_specs})
         vol_type = db.volume_type_get_by_name(elevated, 'new')
         db.quota_create(elevated, project_id, 'volumes_new', 10)
 
-        if replica:
-            rep_status = 'active'
-        else:
-            rep_status = 'disabled'
         volume = tests_utils.create_volume(self.context, size=1,
                                            host=CONF.host, status='retyping',
                                            volume_type_id=old_vol_type['id'],
@@ -5514,8 +5525,14 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase):
                 'qos_specs': {},
                 'extra_specs': {},
             }
+            if replica != replica_new:
+                returned_diff['extra_specs']['replication_enabled'] = (
+                    extra_specs.get('replication_enabled'),
+                    new_specs.get('replication_enabled'))
+            expected_replica_status = 'enabled' if replica_new else 'disabled'
+
             if encryption_changed:
-                returned_diff = {'encryption': 'fake'}
+                returned_diff['encryption'] = 'fake'
             _diff.return_value = (returned_diff, diff_equal)
             if migrate_exc:
                 _mig.side_effect = KeyError
@@ -5581,10 +5598,17 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase):
             mock_notify.assert_not_called()
         if encryption_changed:
             self.assertTrue(_mig.called)
+        self.assertEqual(expected_replica_status, volume.replication_status)
 
     def test_retype_volume_driver_success(self):
         self._retype_volume_exec(True)
 
+    @ddt.data((False, False), (False, True), (True, False), (True, True))
+    @ddt.unpack
+    def test_retype_volume_replica(self, replica, replica_new):
+        self._retype_volume_exec(True, replica=replica,
+                                 replica_new=replica_new)
+
     def test_retype_volume_migration_bad_policy(self):
         # Test volume retype that requires migration by not allowed
         self._retype_volume_exec(False, policy='never',

cinder/volume/manager.py

@@ -2351,6 +2351,7 @@ class VolumeManager(manager.CleanableManager,
                             'status': status_update['status']}
             if retype_model_update:
                 model_update.update(retype_model_update)
+            self._set_replication_status(diff, model_update)
             volume.update(model_update)
             volume.save()
 
@@ -2365,6 +2366,23 @@ class VolumeManager(manager.CleanableManager,
         LOG.info(_LI("Retype volume completed successfully."),
                  resource=volume)
 
+    @staticmethod
+    def _set_replication_status(diff, model_update):
+        """Update replication_status in model_update if it has changed."""
+        if not diff or model_update.get('replication_status'):
+            return
+
+        diff_specs = diff.get('extra_specs', {})
+        replication_diff = diff_specs.get('replication_enabled')
+
+        if replication_diff:
+            is_replicated = vol_utils.is_replicated_str(replication_diff[1])
+            if is_replicated:
+                replication_status = fields.ReplicationStatus.ENABLED
+            else:
+                replication_status = fields.ReplicationStatus.DISABLED
+            model_update['replication_status'] = replication_status
+
     def manage_existing(self, ctxt, volume, ref=None):
         vol_ref = self._run_manage_existing_flow_engine(
             ctxt, volume, ref)

cinder/volume/utils.py

@@ -887,9 +887,12 @@ def create_encryption_key(context, key_manager, volume_type_id):
     return encryption_key_id
 
 
-def is_replicated_spec(extra_specs):
-    if not extra_specs:
-        return False
-    spec = extra_specs.get('replication_enabled', '').split()
+def is_replicated_str(str):
+    spec = (str or '').split()
     return (len(spec) == 2 and
             spec[0] == '<is>' and strutils.bool_from_string(spec[1]))
+
+
+def is_replicated_spec(extra_specs):
+    return (extra_specs and
+            is_replicated_str(extra_specs.get('replication_enabled')))