diff --git a/manila/share/manager.py b/manila/share/manager.py index 5c733b6ab4..028454b1ee 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -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) @@ -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']) @@ -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) @@ -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: @@ -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): diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index fd52d86055..9a2762c7cd 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -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', @@ -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( @@ -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']) @@ -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, @@ -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)) @@ -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': @@ -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( @@ -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, @@ -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') @@ -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, diff --git a/releasenotes/notes/bug-1665072-migration-success-fix-3da1e80fbab666de.yaml b/releasenotes/notes/bug-1665072-migration-success-fix-3da1e80fbab666de.yaml new file mode 100644 index 0000000000..129eefb2cf --- /dev/null +++ b/releasenotes/notes/bug-1665072-migration-success-fix-3da1e80fbab666de.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - Fixed ``task_state`` field in the share model + being set to ``migration_success`` before actually + completing a share migration. +