Fix allow/deny error message and race in migration

Share migration code [1] merged in newton intending to allow admins
to mount the share between phase1 and phase2, but API code for
allow_access and deny_access was set incorrectly, blocking it.

After discussing this feature's purpose further, we decided we do
not want this feature at this moment, so we are just fixing the
allow_access and deny_access error messages.

Also, addressed a small case of concurrency that was happening
once in a while in CI. Update_access was being invoked while
other rules were being applied, thus setting the access_rule_state
to "UPDATING_MULTIPLE", ignoring the migration access rule change
RPC request completely, failing migration. By refreshing the
model we are able to assign the proper access_rule_state at the
time the function is invoked, setting the access_rule_state
correctly.

[1] If4bfaf7e9d963b83c13a6fea241c2eda14f7f409

APIImpact

Closes-bug: #1623051
Closes-bug: #1623052

Change-Id: I76a7d8c3bdd597b951e700350f8f3f82bfb21e03

manila/share/api.py

@@ -1348,20 +1348,7 @@ class API(base.Base):
         policy.check_policy(ctx, 'share', 'allow_access')
         share = self.db.share_get(ctx, share['id'])
         if share['status'] != constants.STATUS_AVAILABLE:
-            if not (share['status'] in (constants.STATUS_MIGRATING,
+            msg = _("Share status must be %s") % constants.STATUS_AVAILABLE
-                                        constants.STATUS_MIGRATING_TO) and
-                    share['task_state'] in (
-                        constants.TASK_STATE_DATA_COPYING_ERROR,
-                        constants.TASK_STATE_MIGRATION_ERROR,
-                        constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE,
-                        constants.TASK_STATE_DATA_COPYING_COMPLETED)):
-                msg = _("Share status must be %(available)s, or %(migrating)s "
-                        "while first phase of migration is completed.") % {
-                            'available': constants.STATUS_AVAILABLE,
-                            'migrating': constants.STATUS_MIGRATING
-                }
-            else:
-                msg = _("Share status must be %s") % constants.STATUS_AVAILABLE
             raise exception.InvalidShare(reason=msg)
         values = {
             'share_id': share['id'],
@@ -1433,20 +1420,7 @@ class API(base.Base):
             msg = _("Share doesn't have any instances")
             raise exception.InvalidShare(reason=msg)
         if share['status'] != constants.STATUS_AVAILABLE:
-            if not (share['status'] in (constants.STATUS_MIGRATING,
+            msg = _("Share status must be %s") % constants.STATUS_AVAILABLE
-                                        constants.STATUS_MIGRATING_TO) and
-                    share['task_state'] in (
-                        constants.TASK_STATE_DATA_COPYING_ERROR,
-                        constants.TASK_STATE_MIGRATION_ERROR,
-                        constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE,
-                        constants.TASK_STATE_DATA_COPYING_COMPLETED)):
-                msg = _("Share status must be %(available)s, or %(migrating)s "
-                        "while first phase of migration is completed.") % {
-                            'available': constants.STATUS_AVAILABLE,
-                            'migrating': constants.STATUS_MIGRATING
-                }
-            else:
-                msg = _("Share status must be %s") % constants.STATUS_AVAILABLE
             raise exception.InvalidShare(reason=msg)
 
         for share_instance in share.instances:

manila/share/migration.py

@@ -195,6 +195,10 @@ class ShareMigrationHelper(object):
             LOG.debug("Restoring all of share %s access rules according to "
                       "DB.", self.share['id'])
 
+            # refresh share instance
+            new_share_instance = self.db.share_instance_get(
+                self.context, new_share_instance['id'], with_share_data=True)
+
             self.api.allow_access_to_instance(self.context, new_share_instance,
                                               rules)
             utils.wait_for_access_update(

manila/tests/share/test_migration.py

@@ -345,6 +345,8 @@ class ShareMigrationHelperTestCase(test.TestCase):
                                         access_level='rw')
 
         # mocks
+        self.mock_object(db, 'share_instance_get',
+                         mock.Mock(return_value=new_share_instance))
         self.mock_object(db, 'share_instance_access_copy')
         self.mock_object(db, 'share_access_get_all_for_instance',
                          mock.Mock(return_value=[access]))
@@ -355,6 +357,8 @@ class ShareMigrationHelperTestCase(test.TestCase):
         self.helper.apply_new_access_rules(new_share_instance)
 
         # asserts
+        db.share_instance_get.assert_called_once_with(
+            self.context, new_share_instance['id'], with_share_data=True)
         db.share_instance_access_copy(self.context, self.share['id'],
                                       new_share_instance['id'])
         db.share_access_get_all_for_instance.assert_called_once_with(

releasenotes/notes/migration-access-fix-71a0f52ea7a152a3.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - Fixed access_allow and access_deny displaying incorrect error
+    message during migration of a share.
+  - Fixed access rule concurrency in migration that was preventing
+    new rules from being added to the migrated share.
+