openstack
/
manila
Code Issues Proposed changes

Fix migration_success before completing 

Share task_state was set to migration_success
before completing migration. There was still
updates to the share model to be performed
before completing.

Also, small refactoring in host-assisted
migration's migration_complete that was not
setting the task_state migration_completing
at the proper time, allowing a bigger window
for 2 concurrent migration_complete calls.

Change-Id: I88df15446ffe34f3f12770d53c3e03d232f495c2
Closes-bug: #1665072
This commit is contained in:
Rodrigo Barbieri 2017-02-15 16:41:34 -02:00
parent 3d5b2d08d0
commit 4181373671
3 changed files with 54 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
manila/share/manager.py
View File

@ -1237,18 +1237,9 @@ class ShareManager(manager.SchedulerDependentManager):
helper.apply_new_access_rules(dest_share_instance)
self.db.share_instance_update(
context, dest_share_instance['id'],
{'status': constants.STATUS_AVAILABLE})
self._migration_delete_instance(context, src_share_instance['id'])
self.db.share_update(
context, dest_share_instance['share_id'],
{'task_state': constants.TASK_STATE_MIGRATION_SUCCESS})
def _migration_delete_instance(self, context, instance_id):
# refresh the share instance model
share_instance = self.db.share_instance_get(
context, instance_id, with_share_data=True)
@ -1336,8 +1327,19 @@ class ShareManager(manager.SchedulerDependentManager):
{'status': constants.STATUS_AVAILABLE})
raise exception.ShareMigrationFailed(reason=msg)
self._update_migrated_share_model(
context, share_ref['id'], dest_share_instance)
self.db.share_instance_update(
context, dest_share_instance['id'],
{'status': constants.STATUS_AVAILABLE})
self._migration_delete_instance(context, src_share_instance['id'])
model_update = self._get_extra_specs_from_share_type(
context, dest_share_instance['share_type_id'])
model_update['task_state'] = constants.TASK_STATE_MIGRATION_SUCCESS
self.db.share_update(
context, dest_share_instance['share_id'], model_update)
LOG.info(_LI("Share Migration for share %s"
" completed successfully."), share_ref['id'])
@ -1350,19 +1352,9 @@ class ShareManager(manager.SchedulerDependentManager):
return share_api.get_share_attributes_from_share_type(share_type)
def _update_migrated_share_model(
self, context, share_id, dest_share_instance):
update = self._get_extra_specs_from_share_type(
context, dest_share_instance['share_type_id'])
self.db.share_update(context, share_id, update)
def _migration_complete_host_assisted(self, context, share_ref,
src_instance_id, dest_instance_id):
src_share_instance = self.db.share_instance_get(
context, src_instance_id, with_share_data=True)
dest_share_instance = self.db.share_instance_get(
context, dest_instance_id, with_share_data=True)
@ -1404,6 +1396,10 @@ class ShareManager(manager.SchedulerDependentManager):
LOG.error(msg)
raise exception.ShareMigrationFailed(reason=msg)
self.db.share_update(
context, share_ref['id'],
{'task_state': constants.TASK_STATE_MIGRATION_COMPLETING})
try:
helper.apply_new_access_rules(dest_share_instance)
except Exception:
@ -1420,24 +1416,6 @@ class ShareManager(manager.SchedulerDependentManager):
raise exception.ShareMigrationFailed(reason=msg)
self.db.share_update(
context, share_ref['id'],
{'task_state': constants.TASK_STATE_MIGRATION_COMPLETING})
self.db.share_instance_update(context, dest_instance_id,
{'status': constants.STATUS_AVAILABLE})
self.db.share_instance_update(context, src_instance_id,
{'status': constants.STATUS_INACTIVE})
helper.delete_instance_and_wait(src_share_instance)
self._check_delete_share_server(context, src_share_instance)
self.db.share_update(
context, share_ref['id'],
{'task_state': constants.TASK_STATE_MIGRATION_SUCCESS})
@utils.require_driver_initialized
def migration_cancel(self, context, src_instance_id, dest_instance_id):

75
manila/tests/share/test_manager.py
View File

@ -4423,6 +4423,8 @@ class ShareManagerTestCase(test.TestCase):
instances=[instance_1, instance_2],
task_state=task_state)
model_type_update = {'create_share_from_snapshot_support': False}
share_update = model_type_update
share_update['task_state'] = constants.TASK_STATE_MIGRATION_SUCCESS
# mocks
self.mock_object(self.share_manager.db, 'share_get',
@ -4444,6 +4446,9 @@ class ShareManagerTestCase(test.TestCase):
self.share_manager, '_migration_complete_host_assisted',
mock.Mock(side_effect=exc))
si_update = self.mock_object(
self.share_manager.db, 'share_instance_update')
if exc:
snapshot = db_utils.create_snapshot(share_id=share['id'])
snapshot_ins1 = db_utils.create_snapshot_instance(
@ -4456,19 +4461,22 @@ class ShareManagerTestCase(test.TestCase):
status=constants.STATUS_MIGRATING_TO)
self.mock_object(manager.LOG, 'exception')
self.mock_object(self.share_manager.db, 'share_update')
self.mock_object(self.share_manager.db, 'share_instance_update')
self.mock_object(self.share_manager.db,
'share_snapshot_instance_update')
self.mock_object(self.share_manager.db,
'share_snapshot_instance_get_all_with_filters',
mock.Mock(
return_value=[snapshot_ins1, snapshot_ins2]))
self.assertRaises(
exception.ShareMigrationFailed,
self.share_manager.migration_complete,
self.context, instance_1['id'], instance_2['id'])
else:
instance_delete = self.mock_object(
self.share_manager, '_migration_delete_instance')
self.share_manager.migration_complete(
self.context, instance_1['id'], instance_2['id'])
@ -4528,7 +4536,13 @@ class ShareManagerTestCase(test.TestCase):
share_types.get_share_type.assert_called_once_with(
self.context, 'fake_type_id')
self.share_manager.db.share_update.assert_called_once_with(
self.context, share['id'], model_type_update)
self.context, share['id'], share_update)
instance_delete.assert_called_once_with(
self.context, instance_1['id'])
si_update.assert_called_once_with(
self.context, instance_2['id'],
{'status': constants.STATUS_AVAILABLE})
@ddt.data(constants.TASK_STATE_DATA_COPYING_ERROR,
constants.TASK_STATE_DATA_COPYING_CANCELLED,
@ -4545,7 +4559,7 @@ class ShareManagerTestCase(test.TestCase):
# mocks
self.mock_object(self.share_manager.db, 'share_instance_get',
mock.Mock(side_effect=[instance, new_instance]))
mock.Mock(return_value=new_instance))
self.mock_object(helper, 'cleanup_new_instance')
self.mock_object(migration_api, 'ShareMigrationHelper',
mock.Mock(return_value=helper))
@ -4569,10 +4583,8 @@ class ShareManagerTestCase(test.TestCase):
self.context, share, instance['id'], new_instance['id'])
# asserts
self.share_manager.db.share_instance_get.assert_has_calls([
mock.call(self.context, instance['id'], with_share_data=True),
mock.call(self.context, new_instance['id'], with_share_data=True)
])
self.share_manager.db.share_instance_get.assert_called_once_with(
self.context, new_instance['id'], with_share_data=True)
cancelled = not(status == constants.TASK_STATE_DATA_COPYING_CANCELLED)
if status != 'other':
@ -4646,9 +4658,7 @@ class ShareManagerTestCase(test.TestCase):
self.mock_object(
self.share_manager.access_helper, '_check_needs_refresh',
mock.Mock(return_value=True))
self.mock_object(self.share_manager.db, 'share_instance_update')
self.mock_object(self.share_manager.db, 'share_update')
self.mock_object(self.share_manager, '_migration_delete_instance')
self.mock_object(migration_api.ShareMigrationHelper,
'apply_new_access_rules')
self.mock_object(
@ -4678,18 +4688,11 @@ class ShareManagerTestCase(test.TestCase):
snapshot_mappings, src_server, dest_server)
(migration_api.ShareMigrationHelper.apply_new_access_rules.
assert_called_once_with(dest_instance))
self.share_manager._migration_delete_instance.assert_called_once_with(
self.context, src_instance['id'])
self.share_manager.db.share_instance_update.assert_called_once_with(
self.context, dest_instance['id'],
{'status': constants.STATUS_AVAILABLE})
self.share_manager.db.share_update.assert_has_calls([
mock.call(
self.context, dest_instance['share_id'],
{'task_state': constants.TASK_STATE_MIGRATION_COMPLETING}),
mock.call(
self.context, dest_instance['share_id'],
{'task_state': constants.TASK_STATE_MIGRATION_SUCCESS})])
self.share_manager.db.share_update.assert_called_once_with(
self.context, dest_instance['share_id'],
{'task_state': constants.TASK_STATE_MIGRATION_COMPLETING})
(self.share_manager.db.share_snapshot_instance_get_all_with_filters.
assert_has_calls([
mock.call(self.context,
@ -4725,11 +4728,8 @@ class ShareManagerTestCase(test.TestCase):
# mocks
self.mock_object(self.share_manager.db, 'share_instance_get',
mock.Mock(side_effect=[instance, new_instance]))
self.mock_object(self.share_manager.db, 'share_instance_update')
mock.Mock(return_value=new_instance))
self.mock_object(self.share_manager.db, 'share_update')
self.mock_object(migration_api.ShareMigrationHelper,
'delete_instance_and_wait')
self.mock_object(migration_api.ShareMigrationHelper,
'apply_new_access_rules')
@ -4738,29 +4738,14 @@ class ShareManagerTestCase(test.TestCase):
self.context, share, instance['id'], new_instance['id'])
# asserts
self.share_manager.db.share_instance_get.assert_has_calls([
mock.call(self.context, instance['id'], with_share_data=True),
mock.call(self.context, new_instance['id'], with_share_data=True)
])
self.share_manager.db.share_instance_get.assert_called_once_with(
self.context, new_instance['id'], with_share_data=True)
self.share_manager.db.share_instance_update.assert_has_calls([
mock.call(self.context, new_instance['id'],
{'status': constants.STATUS_AVAILABLE}),
mock.call(self.context, instance['id'],
{'status': constants.STATUS_INACTIVE})
])
self.share_manager.db.share_update.assert_has_calls([
mock.call(
self.context, share['id'],
{'task_state': constants.TASK_STATE_MIGRATION_COMPLETING}),
mock.call(
self.context, share['id'],
{'task_state': constants.TASK_STATE_MIGRATION_SUCCESS}),
])
self.share_manager.db.share_update.assert_called_once_with(
self.context, share['id'],
{'task_state': constants.TASK_STATE_MIGRATION_COMPLETING})
migration_api.ShareMigrationHelper.apply_new_access_rules.\
assert_called_once_with(new_instance)
migration_api.ShareMigrationHelper.delete_instance_and_wait.\
assert_called_once_with(instance)
@ddt.data(constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS,
constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE,

6
releasenotes/notes/bug-1665072-migration-success-fix-3da1e80fbab666de.yaml Normal file
View File

@ -0,0 +1,6 @@
---
fixes:
- Fixed ``task_state`` field in the share model
being set to ``migration_success`` before actually
completing a share migration.