Skip to content

Commit

Permalink
Fix migration_success before completing
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Rodrigo Barbieri committed Feb 16, 2017
1 parent 3d5b2d0 commit 4181373
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 85 deletions.
58 changes: 18 additions & 40 deletions manila/share/manager.py
View file
Expand Up @@ -1237,18 +1237,9 @@ def _migration_complete_driver(

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)

Expand Down Expand Up @@ -1336,8 +1327,19 @@ def migration_complete(self, context, src_instance_id, dest_instance_id):
{'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'])
Expand All @@ -1350,19 +1352,9 @@ def _get_extra_specs_from_share_type(self, context, share_type_id):

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)

Expand Down Expand Up @@ -1404,6 +1396,10 @@ def _migration_complete_host_assisted(self, context, share_ref,
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:
Expand All @@ -1420,24 +1416,6 @@ def _migration_complete_host_assisted(self, context, share_ref,

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):

Expand Down
75 changes: 30 additions & 45 deletions manila/tests/share/test_manager.py
View file
Expand Up @@ -4423,6 +4423,8 @@ def test_migration_complete(self, task_state, exc):
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',
Expand All @@ -4444,6 +4446,9 @@ def test_migration_complete(self, task_state, exc):
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(
Expand All @@ -4456,19 +4461,22 @@ def test_migration_complete(self, task_state, exc):
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'])

Expand Down Expand Up @@ -4528,7 +4536,13 @@ def test_migration_complete(self, task_state, exc):
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,
Expand All @@ -4545,7 +4559,7 @@ def test__migration_complete_host_assisted_status(self, status):

# 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))
Expand All @@ -4569,10 +4583,8 @@ def test__migration_complete_host_assisted_status(self, status):
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':
Expand Down Expand Up @@ -4646,9 +4658,7 @@ def test__migration_complete_driver(
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(
Expand Down Expand Up @@ -4678,18 +4688,11 @@ def test__migration_complete_driver(
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,
Expand Down Expand Up @@ -4725,11 +4728,8 @@ def test__migration_complete_host_assisted(self):

# 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')

Expand All @@ -4738,29 +4738,14 @@ def test__migration_complete_host_assisted(self):
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,
Expand Down
6 changes: 6 additions & 0 deletions releasenotes/notes/bug-1665072-migration-success-fix-3da1e80fbab666de.yaml
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.

0 comments on commit 4181373

Please sign in to comment.