From fec7643a85b4700f61ab6d52f66a66cd650b62e2 Mon Sep 17 00:00:00 2001
From: Maurice Escher <maurice.escher@sap.com>
Date: Fri, 31 Jul 2020 14:04:04 +0200
Subject: [PATCH] Improve migration_get_progress error handling

Share manager may raise InvalidShare because the migration phase
has already been completed. In such cases total_progress 100 can
be reported instead of raising ShareMigrationError.

Change-Id: I6e1b1abf6a2fd8c1268e446b7ee8e364bdc496bc
Closes-Bug: #1889549
---
 manila/share/api.py                           |  4 +++
 manila/tests/share/test_api.py                | 35 +++++++++++++++++++
 ...on-get-progress-race-15aea537efec6daf.yaml |  7 ++++
 3 files changed, 46 insertions(+)
 create mode 100644 releasenotes/notes/bug-1889549-fix-migration-get-progress-race-15aea537efec6daf.yaml

diff --git a/manila/share/api.py b/manila/share/api.py
index e0464a89cb..cee4d7de1d 100644
--- a/manila/share/api.py
+++ b/manila/share/api.py
@@ -1565,6 +1565,10 @@ class API(base.Base):
                 try:
                     result = self.share_rpcapi.migration_get_progress(
                         context, share_instance_ref, migrating_instance_id)
+                except exception.InvalidShare:
+                    # reload to get the latest task_state
+                    share = self.db.share_get(context, share['id'])
+                    result = self._migration_get_progress_state(share)
                 except Exception:
                     msg = _("Failed to obtain migration progress of share "
                             "%s.") % share['id']
diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py
index 847ac7f483..0d31c35619 100644
--- a/manila/tests/share/test_api.py
+++ b/manila/tests/share/test_api.py
@@ -5513,6 +5513,41 @@ class ShareAPITestCase(test.TestCase):
         mock_get_destination.assert_called_once_with(
             self.context, 'fake_src_server_id', status=constants.STATUS_ACTIVE)
 
+    def test_migration_get_progress_race(self):
+
+        instance1 = db_utils.create_share_instance(
+            share_id='fake_id',
+            status=constants.STATUS_MIGRATING,
+            host='some_host')
+        instance2 = db_utils.create_share_instance(
+            share_id='fake_id',
+            status=constants.STATUS_MIGRATING_TO)
+        share = db_utils.create_share(
+            id='fake_id',
+            task_state=constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS,
+            instances=[instance1, instance2])
+        share_ref = fakes.fake_share(
+            id='fake_id',
+            task_state=constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE)
+        service = 'fake_service'
+        expected = {'total_progress': 100}
+
+        self.mock_object(utils, 'service_is_up', mock.Mock(return_value=True))
+        self.mock_object(db_api, 'service_get_by_args',
+                         mock.Mock(return_value=service))
+        self.mock_object(db_api, 'share_instance_get',
+                         mock.Mock(return_value=instance1))
+        self.mock_object(db_api, 'share_get',
+                         mock.Mock(return_value=share_ref))
+        self.mock_object(self.api.share_rpcapi, 'migration_get_progress',
+                         mock.Mock(side_effect=exception.InvalidShare('fake')))
+
+        result = self.api.migration_get_progress(self.context, share)
+        self.assertEqual(expected, result)
+
+        self.api.share_rpcapi.migration_get_progress.assert_called_once_with(
+            self.context, instance1, instance2['id'])
+
 
 class OtherTenantsShareActionsTestCase(test.TestCase):
     def setUp(self):
diff --git a/releasenotes/notes/bug-1889549-fix-migration-get-progress-race-15aea537efec6daf.yaml b/releasenotes/notes/bug-1889549-fix-migration-get-progress-race-15aea537efec6daf.yaml
new file mode 100644
index 0000000000..f5fbb65a91
--- /dev/null
+++ b/releasenotes/notes/bug-1889549-fix-migration-get-progress-race-15aea537efec6daf.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    In the share migration_get_progress API a race condition was fixed. If the
+    share manager reports ``InvalidShare`` the share's task state is evaluated
+    again to return progress 0 or 100 based on known task states instead of
+    raising ``ShareMigrationError``.