openstack
/
manila
Code Issues Proposed changes

Fix share migration tests in gate 

Since merge of update_access(), share migration tests in Manila gate
have been facing concurrency issues on RPCAPI requests, contributing
to gate instability, while scenario test has been broken.
This patch provides an immediate fix to share migration tests so
Manila gate can be more stable.

Change-Id: I151d2a4352892096f6f3336f88e470223f792090
Closes-bug: #1546946
This commit is contained in:
Rodrigo Barbieri 2016-02-22 09:19:38 -03:00
parent 2759171336
commit 716887464c
5 changed files with 138 additions and 119 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
manila/share/driver.py
View File

@ -429,8 +429,8 @@ class ShareDriver(object):
# NOTE(ganso): Removing any previously conflicting access rules, which
# would cause the following access_allow to fail for one instance.
helper.deny_migration_access(None, src_access, False)
helper.deny_migration_access(None, dest_access, False)
helper.deny_migration_access(None, src_access, share_instance)
helper.deny_migration_access(None, dest_access, new_share_instance)
# NOTE(ganso): I would rather allow access to instances separately,
# but I require an access_id since it is a new access rule and
@ -439,7 +439,8 @@ class ShareDriver(object):
# or ignore duplicate access rule errors for some specific scenarios.
try:
src_access_ref = helper.allow_migration_access(src_access)
src_access_ref = helper.allow_migration_access(
src_access, share_instance)
except Exception as e:
LOG.error(_LE("Share migration failed attempting to allow "
"access of %(access_to)s to share "
@ -451,7 +452,8 @@ class ShareDriver(object):
raise exception.ShareMigrationFailed(reason=msg)
try:
dest_access_ref = helper.allow_migration_access(dest_access)
dest_access_ref = helper.allow_migration_access(
dest_access, new_share_instance)
except Exception as e:
LOG.error(_LE("Share migration failed attempting to allow "
"access of %(access_to)s to share "
@ -460,7 +462,8 @@ class ShareDriver(object):
'instance_id': new_share_instance['id']})
msg = six.text_type(e)
LOG.exception(msg)
helper.cleanup_migration_access(src_access_ref, src_access)
helper.cleanup_migration_access(
src_access_ref, src_access, share_instance)
raise exception.ShareMigrationFailed(reason=msg)
# NOTE(ganso): From here we have the possibility of not cleaning
@ -483,9 +486,9 @@ class ShareDriver(object):
'share_instance_id': share_instance['id'],
'new_share_instance_id': new_share_instance['id']})
helper.cleanup_migration_access(
src_access_ref, src_access)
src_access_ref, src_access, share_instance)
helper.cleanup_migration_access(
dest_access_ref, dest_access)
dest_access_ref, dest_access, new_share_instance)
raise
utils.execute('mkdir', '-p',
@ -555,8 +558,10 @@ class ShareDriver(object):
utils.execute('rmdir', ''.join((mount_path, new_share_instance['id'])),
check_exit_code=False)
helper.deny_migration_access(src_access_ref, src_access)
helper.deny_migration_access(dest_access_ref, dest_access)
helper.deny_migration_access(
src_access_ref, src_access, share_instance)
helper.deny_migration_access(
dest_access_ref, dest_access, new_share_instance)
if not migrated:
msg = ("Copying from share instance %(instance_id)s "

9
manila/share/manager.py
View File

@ -671,7 +671,8 @@ class ShareManager(manager.SchedulerDependentManager):
readonly_support = self.driver.configuration.safe_get(
'migration_readonly_support')
saved_rules = helper.change_to_read_only(readonly_support)
saved_rules = helper.change_to_read_only(readonly_support,
share_instance)
try:
@ -713,10 +714,12 @@ class ShareManager(manager.SchedulerDependentManager):
LOG.exception(six.text_type(e))
LOG.error(_LE("Share migration failed, reverting access rules for "
"share %s.") % share['id'])
helper.revert_access_rules(readonly_support, saved_rules)
helper.revert_access_rules(readonly_support, share_instance, None,
saved_rules)
raise
helper.revert_access_rules(readonly_support, saved_rules)
helper.revert_access_rules(readonly_support, share_instance,
new_share_instance, saved_rules)
self.db.share_update(
context, share['id'],

121
manila/share/migration.py
View File

@ -101,27 +101,27 @@ class ShareMigrationHelper(object):
return new_share_instance
def deny_rules_and_wait(self, context, share, saved_rules):
def deny_rules_and_wait(self, context, share_instance, saved_rules):
api = share_api.API()
api.deny_access_to_instance(context, share.instance, saved_rules)
api.deny_access_to_instance(context, share_instance, saved_rules)
self.wait_for_access_update(share.instance)
self.wait_for_access_update(share_instance)
def add_rules_and_wait(self, context, share, saved_rules,
def add_rules_and_wait(self, context, share_instance, access_rules,
access_level=None):
rules = []
for access in saved_rules:
for access in access_rules:
values = {
'share_id': share['id'],
'share_id': share_instance['share_id'],
'access_type': access['access_type'],
'access_level': access_level or access['access_level'],
'access_to': access['access_to'],
}
rules.append(self.db.share_access_create(context, values))
self.api.allow_access_to_instance(context, share.instance, rules)
self.wait_for_access_update(share.instance)
self.api.allow_access_to_instance(context, share_instance, rules)
self.wait_for_access_update(share_instance)
def wait_for_access_update(self, share_instance):
starttime = time.time()
@ -151,32 +151,32 @@ class ShareMigrationHelper(object):
else:
time.sleep(tries ** 2)
def allow_migration_access(self, access):
allowed = False
access_ref = None
try:
access_ref = self.api.allow_access(
self.context, self.share,
access['access_type'], access['access_to'])
allowed = True
except exception.ShareAccessExists:
LOG.warning(_LW("Access rule already allowed. "
"Access %(access_to)s - Share "
"%(share_id)s") % {
'access_to': access['access_to'],
'share_id': self.share['id']})
access_list = self.api.access_get_all(self.context, self.share)
for access_item in access_list:
if access_item['access_to'] == access['access_to']:
access_ref = access_item
def allow_migration_access(self, access, share_instance):
if access_ref and allowed:
self.wait_for_access_update(self.share.instance)
values = {
'share_id': self.share['id'],
'access_type': access['access_type'],
'access_level': access['access_level'],
'access_to': access['access_to']
}
share_access_list = self.db.share_access_get_all_by_type_and_access(
self.context, self.share['id'], access['access_type'],
access['access_to'])
if len(share_access_list) == 0:
access_ref = self.db.share_access_create(self.context, values)
else:
access_ref = share_access_list[0]
self.api.allow_access_to_instance(
self.context, share_instance, access_ref)
self.wait_for_access_update(share_instance)
return access_ref
def deny_migration_access(self, access_ref, access, throw_not_found=True):
denied = False
def deny_migration_access(self, access_ref, access, share_instance):
if access_ref:
try:
# Update status
@ -196,29 +196,17 @@ class ShareMigrationHelper(object):
access_ref = access_item
break
if access_ref:
try:
self.api.deny_access(self.context, self.share, access_ref)
denied = True
except (exception.InvalidShareAccess, exception.NotFound) as e:
LOG.exception(six.text_type(e))
LOG.warning(_LW("Access rule not found. "
"Access %(access_to)s - Share "
"%(share_id)s") % {
'access_to': access['access_to'],
'share_id': self.share['id']})
if throw_not_found:
raise
if denied:
self.wait_for_access_update(self.share.instance)
self.api.deny_access_to_instance(
self.context, share_instance, access_ref)
self.wait_for_access_update(share_instance)
# NOTE(ganso): Cleanup methods do not throw exception, since the
# exceptions that should be thrown are the ones that call the cleanup
def cleanup_migration_access(self, access_ref, access):
def cleanup_migration_access(self, access_ref, access, share_instance):
try:
self.deny_migration_access(access_ref, access)
self.deny_migration_access(access_ref, access, share_instance)
except Exception as mae:
LOG.exception(six.text_type(mae))
LOG.error(_LE("Could not cleanup access rule of share "
@ -250,7 +238,7 @@ class ShareMigrationHelper(object):
'instance_id': instance['id'],
'share_id': self.share['id']})
def change_to_read_only(self, readonly_support):
def change_to_read_only(self, readonly_support, share_instance):
# NOTE(ganso): If the share does not allow readonly mode we
# should remove all access rules and prevent any access
@ -258,28 +246,37 @@ class ShareMigrationHelper(object):
saved_rules = self.db.share_access_get_all_for_share(
self.context, self.share['id'])
self.deny_rules_and_wait(self.context, self.share, saved_rules)
if len(saved_rules) > 0:
self.deny_rules_and_wait(self.context, share_instance, saved_rules)
if readonly_support:
if readonly_support:
LOG.debug("Changing all of share %s access rules "
"to read-only.", self.share['id'])
LOG.debug("Changing all of share %s access rules "
"to read-only.", self.share['id'])
self.add_rules_and_wait(self.context, self.share,
saved_rules, 'ro')
self.add_rules_and_wait(self.context, share_instance,
saved_rules, 'ro')
return saved_rules
def revert_access_rules(self, readonly_support, saved_rules):
def revert_access_rules(self, readonly_support, share_instance,
new_share_instance, saved_rules):
if readonly_support:
if len(saved_rules) > 0:
if readonly_support:
readonly_rules = self.db.share_access_get_all_for_share(
self.context, self.share['id'])
readonly_rules = self.db.share_access_get_all_for_share(
self.context, self.share['id'])
LOG.debug("Removing all of share %s read-only "
"access rules.", self.share['id'])
LOG.debug("Removing all of share %s read-only "
"access rules.", self.share['id'])
self.deny_rules_and_wait(self.context, self.share, readonly_rules)
self.deny_rules_and_wait(self.context, share_instance,
readonly_rules)
self.add_rules_and_wait(self.context, self.share, saved_rules)
if new_share_instance:
self.add_rules_and_wait(self.context, new_share_instance,
saved_rules)
else:
self.add_rules_and_wait(self.context, share_instance,
saved_rules)

28
manila/tests/share/test_driver.py
View File

@ -577,10 +577,10 @@ class ShareDriverTestCase(test.TestCase):
fake_share_instance, None,
local, remote)
args = ((None, local['access'], False),
(None, remote['access'], False),
('fake_access_ref', local['access']),
('fake_access_ref', remote['access']))
args = ((None, local['access'], fake_share_instance),
(None, remote['access'], fake_share_instance),
('fake_access_ref', local['access'], fake_share_instance),
('fake_access_ref', remote['access'], fake_share_instance))
migration.ShareMigrationHelper.deny_migration_access.assert_has_calls(
[mock.call(*a) for a in args])
@ -613,8 +613,8 @@ class ShareDriverTestCase(test.TestCase):
fake_share, fake_share_instance, None,
fake_share_instance, None, local, remote)
args = ((None, local['access'], False),
(None, remote['access'], False))
args = ((None, local['access'], fake_share_instance),
(None, remote['access'], fake_share_instance))
migration.ShareMigrationHelper.deny_migration_access.assert_has_calls(
[mock.call(*a) for a in args])
@ -646,8 +646,8 @@ class ShareDriverTestCase(test.TestCase):
fake_share, fake_share_instance, None,
fake_share_instance, None, local, remote)
args = ((None, local['access'], False),
(None, remote['access'], False))
args = ((None, local['access'], fake_share_instance),
(None, remote['access'], fake_share_instance))
migration.ShareMigrationHelper.deny_migration_access.assert_has_calls(
[mock.call(*a) for a in args])
@ -682,8 +682,8 @@ class ShareDriverTestCase(test.TestCase):
fake_share, fake_share_instance, None,
fake_share_instance, None, local, remote)
args = ((None, local['access'], False),
(None, remote['access'], False))
args = ((None, local['access'], fake_share_instance),
(None, remote['access'], fake_share_instance))
migration.ShareMigrationHelper.deny_migration_access.assert_has_calls(
[mock.call(*a) for a in args])
@ -723,8 +723,8 @@ class ShareDriverTestCase(test.TestCase):
fake_share_instance, None,
fake_share_instance, None,
local, remote)
args = ((None, local['access'], False),
(None, remote['access'], False))
args = ((None, local['access'], fake_share_instance),
(None, remote['access'], fake_share_instance))
migration.ShareMigrationHelper.deny_migration_access.assert_has_calls(
[mock.call(*a) for a in args])
@ -767,8 +767,8 @@ class ShareDriverTestCase(test.TestCase):
fake_share_instance, None,
fake_share_instance, None,
local, remote)
args = ((None, local['access'], False),
(None, remote['access'], False))
args = ((None, local['access'], fake_share_instance),
(None, remote['access'], fake_share_instance))
migration.ShareMigrationHelper.deny_migration_access.assert_has_calls(
[mock.call(*a) for a in args])

76
manila/tests/share/test_migration.py
View File

@ -57,7 +57,7 @@ class ShareMigrationHelperTestCase(test.TestCase):
self.mock_object(time, 'sleep')
self.helper.deny_rules_and_wait(
self.context, self.share, saved_rules)
self.context, self.share.instance, saved_rules)
db.share_instance_get.assert_any_call(
self.context, self.share.instance['id'])
@ -73,7 +73,7 @@ class ShareMigrationHelperTestCase(test.TestCase):
self.mock_object(self.helper, 'wait_for_access_update')
self.mock_object(db, 'share_access_create')
self.helper.add_rules_and_wait(self.context, self.share,
self.helper.add_rules_and_wait(self.context, self.share.instance,
fake_access_rules)
share_api.API.allow_access_to_instance.assert_called_once_with(
@ -247,17 +247,23 @@ class ShareMigrationHelperTestCase(test.TestCase):
def test_allow_migration_access(self):
access = {'access_to': 'fake_ip',
'access_type': 'fake_type'}
'access_type': 'fake_type',
'access_level': constants.ACCESS_LEVEL_RW}
access_active = db_utils.create_access(share_id=self.share['id'])
self.mock_object(self.helper, 'wait_for_access_update',
mock.Mock(return_value=access_active))
self.mock_object(self.helper.api, 'allow_access',
self.mock_object(self.helper.db, 'share_access_create',
mock.Mock(return_value=access_active))
self.mock_object(
self.helper.db, 'share_access_get_all_by_type_and_access',
mock.Mock(return_value=[]))
self.mock_object(self.helper.api, 'allow_access_to_instance')
result = self.helper.allow_migration_access(access)
result = self.helper.allow_migration_access(
access, self.share.instance)
self.assertEqual(access_active, result)
@ -266,19 +272,20 @@ class ShareMigrationHelperTestCase(test.TestCase):
def test_allow_migration_access_exists(self):
access = {'access_to': 'fake_ip',
'access_type': 'fake_type'}
'access_type': 'fake_type',
'access_level': 'fake_level'}
access_active = db_utils.create_access(share_id=self.share['id'],
access_to='fake_ip')
self.mock_object(self.helper.api, 'allow_access_to_instance')
self.mock_object(
self.helper.api, 'allow_access',
mock.Mock(side_effect=[exception.ShareAccessExists('fake')]))
self.helper.db, 'share_access_get_all_by_type_and_access',
mock.Mock(return_value=[access_active]))
self.mock_object(self.helper.api, 'access_get_all',
mock.Mock(return_value=[access_active]))
result = self.helper.allow_migration_access(access)
result = self.helper.allow_migration_access(
access, self.share.instance)
self.assertEqual(access_active, result)
@ -293,11 +300,12 @@ class ShareMigrationHelperTestCase(test.TestCase):
self.mock_object(self.helper.api, 'access_get',
mock.Mock(return_value=access_active))
self.mock_object(self.helper.api, 'deny_access')
self.mock_object(self.helper.api, 'deny_access_to_instance')
self.mock_object(self.helper, 'wait_for_access_update')
self.helper.deny_migration_access(access_active, access)
self.helper.deny_migration_access(access_active, access,
self.share.instance)
self.helper.wait_for_access_update.assert_called_once_with(
self.share.instance
@ -314,7 +322,8 @@ class ShareMigrationHelperTestCase(test.TestCase):
self.mock_object(self.helper.api, 'access_get',
mock.Mock(side_effect=exception.NotFound('fake')))
self.helper.deny_migration_access(access_active, access)
self.helper.deny_migration_access(
access_active, access, self.share.instance)
def test_deny_migration_access_none(self):
@ -327,11 +336,11 @@ class ShareMigrationHelperTestCase(test.TestCase):
self.mock_object(self.helper.api, 'access_get_all',
mock.Mock(return_value=[access_active]))
self.mock_object(self.helper.api, 'deny_access')
self.mock_object(self.helper.api, 'deny_access_to_instance')
self.mock_object(self.helper, 'wait_for_access_update')
self.helper.deny_migration_access(None, access)
self.helper.deny_migration_access(None, access, self.share.instance)
self.helper.wait_for_access_update.assert_called_once_with(
self.share.instance)
@ -347,19 +356,19 @@ class ShareMigrationHelperTestCase(test.TestCase):
self.mock_object(self.helper.api, 'access_get',
mock.Mock(return_value=access_active))
self.mock_object(self.helper.api, 'deny_access',
self.mock_object(self.helper.api, 'deny_access_to_instance',
mock.Mock(side_effect=[exception.NotFound('fake')]))
self.assertRaises(exception.NotFound,
self.helper.deny_migration_access, access_active,
access)
access, self.share.instance)
def test_cleanup_migration_access_exception(self):
self.mock_object(self.helper, 'deny_migration_access',
mock.Mock(side_effect=Exception('fake')))
self.helper.cleanup_migration_access(None, None)
self.helper.cleanup_migration_access(None, None, self.share.instance)
def test_cleanup_temp_folder_exception(self):
@ -381,40 +390,45 @@ class ShareMigrationHelperTestCase(test.TestCase):
access_to='fake_ip')
self.mock_object(db, 'share_access_get_all_for_share',
mock.Mock(return_value=access_active))
mock.Mock(return_value=[access_active]))
self.mock_object(self.helper, 'deny_rules_and_wait')
self.mock_object(self.helper, 'add_rules_and_wait')
result = self.helper.change_to_read_only(True)
result = self.helper.change_to_read_only(True, self.share.instance)
self.assertEqual(access_active, result)
self.assertEqual([access_active], result)
db.share_access_get_all_for_share.assert_called_once_with(
self.context, self.share['id'])
self.helper.deny_rules_and_wait.assert_called_once_with(
self.context, self.share, access_active)
self.context, self.share.instance, [access_active])
self.helper.add_rules_and_wait.assert_called_once_with(
self.context, self.share, access_active, 'ro')
self.context, self.share.instance, [access_active], 'ro')
def test_revert_access_rules(self):
@ddt.data(None, 'new_instance')
def test_revert_access_rules(self, new_instance):
access_active = db_utils.create_access(share_id=self.share['id'],
access_to='fake_ip')
self.mock_object(db, 'share_access_get_all_for_share',
mock.Mock(return_value=access_active))
mock.Mock(return_value=[access_active]))
self.mock_object(self.helper, 'deny_rules_and_wait')
self.mock_object(self.helper, 'add_rules_and_wait')
self.helper.revert_access_rules(True, access_active)
if new_instance:
new_instance = self.share.instance
self.helper.revert_access_rules(True, self.share.instance,
new_instance, [access_active])
db.share_access_get_all_for_share.assert_called_once_with(
self.context, self.share['id'])
self.helper.deny_rules_and_wait.assert_called_once_with(
self.context, self.share, access_active)
self.helper.add_rules_and_wait.assert_called_once_with(
self.context, self.share, access_active)
self.context, self.share.instance, [access_active])
if new_instance:
self.helper.add_rules_and_wait.assert_called_once_with(
self.context, self.share.instance, [access_active])