diff --git a/manila/share/access.py b/manila/share/access.py index 6cbbb1b523..ec49f05a4b 100644 --- a/manila/share/access.py +++ b/manila/share/access.py @@ -100,13 +100,43 @@ class ShareInstanceAccess(object): if rule["id"] in delete_ids] delete_rules = [] + # NOTE(ganso): up to here we are certain of the rules that we are + # supposed to pass to drivers. 'rules' variable is used for validating + # the refresh mechanism later, according to the 'supposed' rules. + driver_rules = rules + + if share_instance['status'] == constants.STATUS_MIGRATING: + # NOTE(ganso): If the share instance is the source in a migration, + # it should have all its rules cast to read-only. + + readonly_support = self.driver.configuration.safe_get( + 'migration_readonly_rules_support') + + # NOTE(ganso): If the backend supports read-only rules, then all + # rules are cast to read-only here and passed to drivers. + if readonly_support: + for rule in driver_rules + add_rules: + rule['access_level'] = constants.ACCESS_LEVEL_RO + LOG.debug("All access rules of share instance %s are being " + "cast to read-only for migration.", + share_instance['id']) + # NOTE(ganso): If the backend does not support read-only rules, we + # will remove all access to the share and have only the access + # requested by the Data Service for migration as RW. + else: + LOG.debug("All previously existing access rules of share " + "instance %s are being removed for migration, as " + "driver does not support read-only mode rules.", + share_instance['id']) + driver_rules = add_rules + try: access_keys = None try: access_keys = self.driver.update_access( context, share_instance, - rules, + driver_rules, add_rules=add_rules, delete_rules=delete_rules, share_server=share_server diff --git a/manila/share/manager.py b/manila/share/manager.py index 92afc87e2c..d9e4e4bdb2 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -741,11 +741,14 @@ class ShareManager(manager.SchedulerDependentManager): raise exception.ShareMigrationFailed(reason=msg) if not compatibility.get('writable'): - readonly_support = self.driver.configuration.safe_get( - 'migration_readonly_rules_support') - helper.change_to_read_only(src_share_instance, share_server, - readonly_support, self.driver) + self.db.share_instance_update_access_status( + context, src_share_instance['id'], + constants.STATUS_OUT_OF_SYNC) + + self.access_helper.update_access_rules( + context, src_share_instance['id'], + share_server=share_server) LOG.debug("Initiating driver migration for share %s.", share_ref['id']) @@ -936,11 +939,12 @@ class ShareManager(manager.SchedulerDependentManager): share_server = self._get_share_server(context.elevated(), src_share_instance) - readonly_support = self.driver.configuration.safe_get( - 'migration_readonly_rules_support') + self.db.share_instance_update_access_status( + context, src_share_instance['id'], + constants.STATUS_OUT_OF_SYNC) - helper.change_to_read_only(src_share_instance, share_server, - readonly_support, self.driver) + self.access_helper.update_access_rules( + context, src_share_instance['id'], share_server=share_server) try: dest_share_instance = helper.create_instance_and_wait( diff --git a/manila/share/migration.py b/manila/share/migration.py index e45cc65190..68ebddf930 100644 --- a/manila/share/migration.py +++ b/manila/share/migration.py @@ -132,43 +132,21 @@ class ShareMigrationHelper(object): " migration for share %s."), self.share['id']) def cleanup_access_rules(self, share_instance, share_server, driver): + + # NOTE(ganso): For the purpose of restoring the access rules, the share + # instance status must not be "MIGRATING", else they would be cast to + # read-only. We briefly change them to "INACTIVE" so they are restored + # and after cleanup finishes, the invoking method will set the status + # back to "AVAILABLE". + self.db.share_instance_update(self.context, share_instance['id'], + {'status': constants.STATUS_INACTIVE}) + try: self.revert_access_rules(share_instance, share_server, driver) except Exception: LOG.warning(_LW("Failed to cleanup access rules during generic" " migration for share %s."), self.share['id']) - def change_to_read_only(self, share_instance, share_server, - readonly_support, driver): - - # NOTE(ganso): If the share does not allow readonly mode we - # should remove all access rules and prevent any access - - rules = self.db.share_access_get_all_for_instance( - self.context, share_instance['id']) - - if len(rules) > 0: - - if readonly_support: - - LOG.debug("Changing all of share %s access rules " - "to read-only.", self.share['id']) - - for rule in rules: - rule['access_level'] = constants.ACCESS_LEVEL_RO - - driver.update_access(self.context, share_instance, rules, - add_rules=[], delete_rules=[], - share_server=share_server) - else: - - LOG.debug("Removing all access rules for migration of " - "share %s." % self.share['id']) - - driver.update_access(self.context, share_instance, [], - add_rules=[], delete_rules=rules, - share_server=share_server) - def revert_access_rules(self, share_instance, share_server, driver): rules = self.db.share_access_get_all_for_instance( diff --git a/manila/tests/share/test_access.py b/manila/tests/share/test_access.py index 54c56d8df7..b3c0f84ae2 100644 --- a/manila/tests/share/test_access.py +++ b/manila/tests/share/test_access.py @@ -259,3 +259,54 @@ class ShareInstanceAccessTestCase(test.TestCase): mock.call(self.context, share_instance, original_rules, add_rules=[], delete_rules=[], share_server=None) ]) + + @ddt.data(True, False) + def test_update_access_rules_migrating(self, read_only_support): + + def override_conf(conf_name): + if conf_name == 'migration_readonly_rules_support': + return read_only_support + + rules = [] + for i in range(2): + rules.append( + db_utils.create_access( + id='fakeid%s' % i, + share_id=self.share['id'], + access_to='fakeip%s' % i, + )) + driver_rules = [] if not read_only_support else rules + access_rules_status = constants.STATUS_OUT_OF_SYNC + share_instance = db_utils.create_share_instance( + id='fakeid', + status=constants.STATUS_MIGRATING, + share_id=self.share['id'], + access_rules_status=access_rules_status) + + self.mock_object(db, "share_instance_get", mock.Mock( + return_value=share_instance)) + self.mock_object(db, "share_access_get_all_for_instance", + mock.Mock(return_value=rules)) + self.mock_object(db, "share_instance_update_access_status", + mock.Mock()) + self.mock_object(self.driver, "update_access", + mock.Mock(return_value=None)) + self.mock_object(self.share_access_helper, + "_remove_access_rules", mock.Mock()) + self.mock_object(self.share_access_helper, "_check_needs_refresh", + mock.Mock(return_value=False)) + self.mock_object(self.driver.configuration, 'safe_get', + mock.Mock(side_effect=override_conf)) + + self.share_access_helper.update_access_rules( + self.context, share_instance['id']) + + self.driver.update_access.assert_called_once_with( + self.context, share_instance, driver_rules, add_rules=[], + delete_rules=[], share_server=None) + self.share_access_helper._remove_access_rules.assert_called_once_with( + self.context, [], share_instance['id']) + self.share_access_helper._check_needs_refresh.assert_called_once_with( + self.context, rules, share_instance) + db.share_instance_update_access_status.assert_called_with( + self.context, share_instance['id'], constants.STATUS_ACTIVE) diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index ab917bb901..39fe887cc1 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -3792,8 +3792,10 @@ class ShareManagerTestCase(test.TestCase): mock.Mock(return_value=server)) self.mock_object(self.share_manager.db, 'share_instance_update', mock.Mock(return_value=server)) - self.mock_object(migration_api.ShareMigrationHelper, - 'change_to_read_only') + self.mock_object(self.share_manager.db, + 'share_instance_update_access_status') + self.mock_object(self.share_manager.access_helper, + 'update_access_rules') if exc is None: self.mock_object(migration_api.ShareMigrationHelper, 'create_instance_and_wait', @@ -3824,10 +3826,12 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.db.share_server_get.assert_called_once_with( utils.IsAMatcher(context.RequestContext), instance['share_server_id']) - - migration_api.ShareMigrationHelper.change_to_read_only.\ - assert_called_once_with(instance, server, True, - self.share_manager.driver) + (self.share_manager.db.share_instance_update_access_status. + assert_called_once_with(self.context, instance['id'], + constants.STATUS_OUT_OF_SYNC)) + (self.share_manager.access_helper.update_access_rules. + assert_called_once_with( + self.context, instance['id'], share_server=server)) migration_api.ShareMigrationHelper.create_instance_and_wait.\ assert_called_once_with(share, 'fake_host', 'fake_net_id', 'fake_az_id', 'fake_type_id') @@ -3896,10 +3900,12 @@ class ShareManagerTestCase(test.TestCase): self.mock_object( migration_api.ShareMigrationHelper, 'wait_for_share_server', mock.Mock(return_value=dest_server)) - self.mock_object( - migration_api.ShareMigrationHelper, 'change_to_read_only') self.mock_object(self.share_manager.driver, 'migration_start') self.mock_object(self.share_manager, '_migration_delete_instance') + self.mock_object(self.share_manager.db, + 'share_instance_update_access_status') + self.mock_object(self.share_manager.access_helper, + 'update_access_rules') # run if exc: @@ -3926,12 +3932,15 @@ class ShareManagerTestCase(test.TestCase): {'task_state': constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS}) ]) + (self.share_manager.db.share_instance_update_access_status. + assert_called_once_with(self.context, src_instance['id'], + constants.STATUS_OUT_OF_SYNC)) + (self.share_manager.access_helper.update_access_rules. + assert_called_once_with( + self.context, src_instance['id'], share_server=src_server)) self.share_manager.driver.migration_start.assert_called_once_with( self.context, src_instance, migrating_instance, src_server, dest_server) - (migration_api.ShareMigrationHelper.change_to_read_only. - assert_called_once_with(src_instance, src_server, True, - self.share_manager.driver)) self.share_manager.db.share_instance_get.assert_called_once_with( self.context, migrating_instance['id'], with_share_data=True) diff --git a/manila/tests/share/test_migration.py b/manila/tests/share/test_migration.py index 0429f09db4..5ef08493da 100644 --- a/manila/tests/share/test_migration.py +++ b/manila/tests/share/test_migration.py @@ -248,64 +248,6 @@ class ShareMigrationHelperTestCase(test.TestCase): # asserts db.share_server_get.assert_called_with(self.context, 'fake_server_id') - def test_change_to_read_only_with_ro_support(self): - - share_instance = db_utils.create_share_instance( - share_id=self.share['id'], status=constants.STATUS_AVAILABLE) - - access = db_utils.create_access(share_id=self.share['id'], - access_to='fake_ip', - access_level='rw') - - server = db_utils.create_share_server(share_id=self.share['id']) - - # mocks - share_driver = mock.Mock() - self.mock_object(share_driver, 'update_access') - - self.mock_object(db, 'share_access_get_all_for_instance', - mock.Mock(return_value=[access])) - - # run - self.helper.change_to_read_only(share_instance, server, True, - share_driver) - - # asserts - db.share_access_get_all_for_instance.assert_called_once_with( - self.context, share_instance['id']) - share_driver.update_access.assert_called_once_with( - self.context, share_instance, [access], add_rules=[], - delete_rules=[], share_server=server) - - def test_change_to_read_only_without_ro_support(self): - - share_instance = db_utils.create_share_instance( - share_id=self.share['id'], status=constants.STATUS_AVAILABLE) - - access = db_utils.create_access(share_id=self.share['id'], - access_to='fake_ip', - access_level='rw') - - server = db_utils.create_share_server(share_id=self.share['id']) - - # mocks - share_driver = mock.Mock() - self.mock_object(share_driver, 'update_access') - - self.mock_object(db, 'share_access_get_all_for_instance', - mock.Mock(return_value=[access])) - - # run - self.helper.change_to_read_only(share_instance, server, False, - share_driver) - - # asserts - db.share_access_get_all_for_instance.assert_called_once_with( - self.context, share_instance['id']) - share_driver.update_access.assert_called_once_with( - self.context, share_instance, [], add_rules=[], - delete_rules=[access], share_server=server) - def test_revert_access_rules(self): share_instance = db_utils.create_share_instance( @@ -396,6 +338,7 @@ class ShareMigrationHelperTestCase(test.TestCase): share_driver = mock.Mock() self.mock_object(self.helper, 'revert_access_rules', mock.Mock(side_effect=exc)) + self.mock_object(self.helper.db, 'share_instance_update') self.mock_object(migration.LOG, 'warning') @@ -406,6 +349,9 @@ class ShareMigrationHelperTestCase(test.TestCase): # asserts self.helper.revert_access_rules.assert_called_once_with( self.share_instance, server, share_driver) + self.helper.db.share_instance_update.assert_called_once_with( + self.context, self.share_instance['id'], + {'status': constants.STATUS_INACTIVE}) if exc: self.assertEqual(1, migration.LOG.warning.call_count) diff --git a/releasenotes/notes/bug-1626523-migration-rw-access-fix-7da3365c7b5b90a1.yaml b/releasenotes/notes/bug-1626523-migration-rw-access-fix-7da3365c7b5b90a1.yaml new file mode 100644 index 0000000000..a76cd2729a --- /dev/null +++ b/releasenotes/notes/bug-1626523-migration-rw-access-fix-7da3365c7b5b90a1.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - Fixed share remaining with read/write access rules during a + host-assisted share migration. +