From d2bebca14274a95aef80b2df47e73cb22aef8d3b Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Fri, 11 Mar 2016 08:24:27 -0300 Subject: [PATCH] Fix Share Migration access rule mapping Share Migration must transfer access rules from source instance to destination instance, but creating new ones instead of creating new mapping between existing and destination instance. This patch addresses this by copying the mapping. Closes-bug: #1555677 Change-Id: Ie5a50e4117c14f822c22cace91bb8b0b95d0799d --- manila/share/manager.py | 2 +- manila/share/migration.py | 34 ++++++------------- manila/tests/share/test_manager.py | 4 +-- manila/tests/share/test_migration.py | 51 +++++++--------------------- 4 files changed, 25 insertions(+), 66 deletions(-) diff --git a/manila/share/manager.py b/manila/share/manager.py index 95fd3991..c76802a6 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -824,7 +824,7 @@ class ShareManager(manager.SchedulerDependentManager): raise exception.ShareMigrationFailed(reason=msg) try: - helper.apply_new_access_rules(share_instance, new_share_instance) + helper.apply_new_access_rules(new_share_instance) except Exception: msg = _("Failed to apply new access rules during migration " "of share %s.") % share_ref['id'] diff --git a/manila/share/migration.py b/manila/share/migration.py index 5ae57b49..bfb0fa1b 100644 --- a/manila/share/migration.py +++ b/manila/share/migration.py @@ -120,27 +120,6 @@ class ShareMigrationHelper(object): return new_share_instance - def _add_rules_and_wait(self, share_instance, access_rules): - - for access in access_rules: - values = { - 'share_id': self.share['id'], - 'access_type': access['access_type'], - 'access_level': access['access_level'], - 'access_to': access['access_to'] - } - - # NOTE(ganso): Instance Access Mapping is created only on - # db.share_access_create. - - self.db.share_access_create(self.context, values) - - self.api.allow_access_to_instance(self.context, share_instance, - access_rules) - utils.wait_for_access_update( - self.context, self.db, share_instance, - self.migration_wait_access_rules_timeout) - # NOTE(ganso): Cleanup methods do not throw exceptions, since the # exceptions that should be thrown are the ones that call the cleanup @@ -203,13 +182,20 @@ class ShareMigrationHelper(object): add_rules=[], delete_rules=[], share_server=share_server) - def apply_new_access_rules(self, share_instance, new_share_instance): + def apply_new_access_rules(self, new_share_instance): + + self.db.share_instance_access_copy(self.context, self.share['id'], + new_share_instance['id']) rules = self.db.share_access_get_all_for_instance( - self.context, share_instance['id']) + self.context, new_share_instance['id']) if len(rules) > 0: LOG.debug("Restoring all of share %s access rules according to " "DB.", self.share['id']) - self._add_rules_and_wait(new_share_instance, rules) + self.api.allow_access_to_instance(self.context, new_share_instance, + rules) + utils.wait_for_access_update( + self.context, self.db, new_share_instance, + self.migration_wait_access_rules_timeout) diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 09a8d53c..034fb108 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -3605,7 +3605,7 @@ class ShareManagerTestCase(test.TestCase): {'task_state': constants.TASK_STATE_MIGRATION_CANCELLED}) if status == constants.TASK_STATE_DATA_COPYING_COMPLETED: migration_api.ShareMigrationHelper.apply_new_access_rules.\ - assert_called_once_with(instance, new_instance) + assert_called_once_with(new_instance) self.assertTrue(manager.LOG.exception.called) def test__migration_complete(self): @@ -3658,7 +3658,7 @@ class ShareManagerTestCase(test.TestCase): {'task_state': constants.TASK_STATE_MIGRATION_SUCCESS}), ]) migration_api.ShareMigrationHelper.apply_new_access_rules.\ - assert_called_once_with(instance, new_instance) + assert_called_once_with(new_instance) migration_api.ShareMigrationHelper.delete_instance_and_wait.\ assert_called_once_with(instance) diff --git a/manila/tests/share/test_migration.py b/manila/tests/share/test_migration.py index 18992694..de17dfb7 100644 --- a/manila/tests/share/test_migration.py +++ b/manila/tests/share/test_migration.py @@ -307,8 +307,6 @@ class ShareMigrationHelperTestCase(test.TestCase): def test_apply_new_access_rules(self): - share_instance = db_utils.create_share_instance( - share_id=self.share['id'], status=constants.STATUS_AVAILABLE) new_share_instance = db_utils.create_share_instance( share_id=self.share['id'], status=constants.STATUS_AVAILABLE, access_rules_status='active') @@ -318,18 +316,25 @@ class ShareMigrationHelperTestCase(test.TestCase): access_level='rw') # mocks + self.mock_object(db, 'share_instance_access_copy') self.mock_object(db, 'share_access_get_all_for_instance', mock.Mock(return_value=[access])) - self.mock_object(self.helper, '_add_rules_and_wait') + self.mock_object(share_api.API, 'allow_access_to_instance') + self.mock_object(utils, 'wait_for_access_update') # run - self.helper.apply_new_access_rules(share_instance, new_share_instance) + self.helper.apply_new_access_rules(new_share_instance) # asserts + 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( - self.context, share_instance['id']) - self.helper._add_rules_and_wait.assert_called_once_with( - new_share_instance, [access]) + self.context, new_share_instance['id']) + share_api.API.allow_access_to_instance.assert_called_with( + self.context, new_share_instance, [access]) + utils.wait_for_access_update.assert_called_with( + self.context, db, new_share_instance, + self.helper.migration_wait_access_rules_timeout) @ddt.data(None, Exception('fake')) def test_cleanup_new_instance(self, exc): @@ -371,35 +376,3 @@ class ShareMigrationHelperTestCase(test.TestCase): if exc: migration.LOG.warning.called - - def test__add_rules_and_wait(self): - - access = db_utils.create_access(share_id=self.share['id']) - - values = { - 'share_id': self.share['id'], - 'access_type': access['access_type'], - 'access_level': access['access_level'], - 'access_to': access['access_to'] - } - - self.helper.migration_wait_access_rules_timeout = 60 - - # mocks - self.mock_object(db, 'share_access_create') - - self.mock_object(share_api.API, 'allow_access_to_instance') - - self.mock_object(utils, 'wait_for_access_update') - - # run - self.helper._add_rules_and_wait(self.share_instance, [access]) - - # asserts - db.share_access_create.assert_called_once_with(self.context, values) - - share_api.API.allow_access_to_instance.assert_called_once_with( - self.context, self.share_instance, [access]) - - utils.wait_for_access_update.assert_called_once_with( - self.context, db, self.share_instance, 60)