From 10487a14a3673d4a28a4ea85a62619e69d5c669b Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Mon, 12 Sep 2016 16:22:34 -0300 Subject: [PATCH] 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 | 30 ++----------------- manila/share/migration.py | 4 +++ manila/tests/share/test_migration.py | 4 +++ ...migration-access-fix-71a0f52ea7a152a3.yaml | 7 +++++ 4 files changed, 17 insertions(+), 28 deletions(-) create mode 100644 releasenotes/notes/migration-access-fix-71a0f52ea7a152a3.yaml diff --git a/manila/share/api.py b/manila/share/api.py index fa6369b236..30983e0abd 100644 --- a/manila/share/api.py +++ b/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, - 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 + 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, - 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 + msg = _("Share status must be %s") % constants.STATUS_AVAILABLE raise exception.InvalidShare(reason=msg) for share_instance in share.instances: diff --git a/manila/share/migration.py b/manila/share/migration.py index e3b7d04ae6..333f8fadb6 100644 --- a/manila/share/migration.py +++ b/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( diff --git a/manila/tests/share/test_migration.py b/manila/tests/share/test_migration.py index 695e300bfa..e82353b52b 100644 --- a/manila/tests/share/test_migration.py +++ b/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( diff --git a/releasenotes/notes/migration-access-fix-71a0f52ea7a152a3.yaml b/releasenotes/notes/migration-access-fix-71a0f52ea7a152a3.yaml new file mode 100644 index 0000000000..b214c449a2 --- /dev/null +++ b/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. +