From f7783776b5479761ba9d6a6b63121be1c8957541 Mon Sep 17 00:00:00 2001
From: Valeriy Ponomaryov <vponomaryov@mirantis.com>
Date: Tue, 28 Feb 2017 13:05:04 +0300
Subject: [PATCH] Update share replicas after promotion in proper order

Update replica that becomes active after promotion only as last
to avoid concurrency in share replica states when we can have 2
active share replicas in some narrow timeframe.

Change-Id: Idf01daf31172a21dbb6595044c11980867346d13
Closes-Bug: #1664201
---
 manila/share/manager.py                       | 21 ++++++++++------
 manila/tests/share/test_manager.py            | 24 +++++++++++++++----
 ...ca-promotion-feature-63b15d96106c65da.yaml |  9 +++++++
 3 files changed, 42 insertions(+), 12 deletions(-)
 create mode 100644 releasenotes/notes/bug-1664201-fix-share-replica-status-update-concurrency-in-replica-promotion-feature-63b15d96106c65da.yaml

diff --git a/manila/share/manager.py b/manila/share/manager.py
index 94d2b34a93..a03b4c16a2 100644
--- a/manila/share/manager.py
+++ b/manila/share/manager.py
@@ -1996,19 +1996,26 @@ class ShareManager(manager.SchedulerDependentManager):
                     {'status': constants.STATUS_ERROR})
 
         if not updated_replica_list:
-            self.db.share_replica_update(
-                context, share_replica['id'],
-                {'status': constants.STATUS_AVAILABLE,
-                 'replica_state': constants.REPLICA_STATE_ACTIVE,
-                 'cast_rules_to_readonly': False})
-
             self.db.share_replica_update(
                 context, old_active_replica['id'],
                 {'replica_state': constants.REPLICA_STATE_OUT_OF_SYNC,
                  'cast_rules_to_readonly':
                      ensure_old_active_replica_to_readonly})
+            self.db.share_replica_update(
+                context, share_replica['id'],
+                {'status': constants.STATUS_AVAILABLE,
+                 'replica_state': constants.REPLICA_STATE_ACTIVE,
+                 'cast_rules_to_readonly': False})
         else:
-            for updated_replica in updated_replica_list:
+            while updated_replica_list:
+                # NOTE(vponomaryov): update 'active' replica last.
+                for updated_replica in updated_replica_list:
+                    if (updated_replica['id'] == share_replica['id'] and
+                            len(updated_replica_list) > 1):
+                        continue
+                    updated_replica_list.remove(updated_replica)
+                    break
+
                 updated_export_locs = updated_replica.get(
                     'export_locations')
                 if(updated_export_locs is not None
diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py
index e430f43216..d082eb5273 100644
--- a/manila/tests/share/test_manager.py
+++ b/manila/tests/share/test_manager.py
@@ -1148,7 +1148,10 @@ class ShareManagerTestCase(test.TestCase):
         active_replica = fake_replica(
             id='current_active_replica',
             replica_state=constants.REPLICA_STATE_ACTIVE)
-        replica_list = [replica, active_replica, fake_replica(id=3)]
+        replica_list = [
+            replica, active_replica, fake_replica(id=3),
+            fake_replica(id='one_more_replica'),
+        ]
         updated_replica_list = [
             {
                 'id': replica['id'],
@@ -1162,7 +1165,12 @@ class ShareManagerTestCase(test.TestCase):
             },
             {
                 'id': 'other_replica',
-                'export_locations': ['TEST1', 'TEST2'],
+                'export_locations': ['TEST3', 'TEST4'],
+            },
+            {
+                'id': replica_list[3]['id'],
+                'export_locations': ['TEST5', 'TEST6'],
+                'replica_state': constants.REPLICA_STATE_IN_SYNC,
             },
         ]
         self.mock_object(db, 'share_replica_get',
@@ -1200,13 +1208,19 @@ class ShareManagerTestCase(test.TestCase):
         demoted_replica_update_call = mock.call(
             mock.ANY, active_replica['id'], demoted_replica_updates
         )
+        additional_replica_update_call = mock.call(
+            mock.ANY, replica_list[3]['id'], {
+                'replica_state': constants.REPLICA_STATE_IN_SYNC,
+            }
+        )
 
         self.share_manager.promote_share_replica(self.context, replica)
 
-        self.assertEqual(2, mock_export_locs_update.call_count)
-        self.assertEqual(2, mock_replica_update.call_count)
+        self.assertEqual(3, mock_export_locs_update.call_count)
         mock_replica_update.assert_has_calls([
-            reset_replication_change_call, demoted_replica_update_call,
+            demoted_replica_update_call,
+            additional_replica_update_call,
+            reset_replication_change_call,
         ])
         self.assertTrue(mock_info_log.called)
         self.assertFalse(mock_snap_instance_update.called)
diff --git a/releasenotes/notes/bug-1664201-fix-share-replica-status-update-concurrency-in-replica-promotion-feature-63b15d96106c65da.yaml b/releasenotes/notes/bug-1664201-fix-share-replica-status-update-concurrency-in-replica-promotion-feature-63b15d96106c65da.yaml
new file mode 100644
index 0000000000..131c214520
--- /dev/null
+++ b/releasenotes/notes/bug-1664201-fix-share-replica-status-update-concurrency-in-replica-promotion-feature-63b15d96106c65da.yaml
@@ -0,0 +1,9 @@
+---
+fixes:
+  - Fixed share replica status update concurrency in share replica promotion
+    feature. Before it was possible to see two active replicas,
+    having 'dr' or 'readable' type of replication,
+    performing 'share replica promotion' action.
+    Now, replica that becomes active is always updated last, so, at some period
+    of time we will have zero 'active' replicas at once instead of
+    two of them.