From fae08f97f17b3dc6dfaa8df7679660707c518c3a Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Thu, 23 Feb 2017 16:34:05 -0300 Subject: [PATCH] Fix host-assisted migration stale source share Since change I88df15446ffe34f3f12770d53c3e03d232f495c2 the source share is not being deleted from backend anymore during migration_complete of a host-assisted migration. Fix this by invoking the correct helper function. Closes-bug: #1667450 Change-Id: I0687fcf0eff0da04e67750421ff0cc97dd99276f --- manila/share/manager.py | 29 ++++++---- manila/tests/share/test_manager.py | 54 +++++++++++-------- ...gration-stale-source-9c092fee267a7a0f.yaml | 4 ++ 3 files changed, 55 insertions(+), 32 deletions(-) create mode 100644 releasenotes/notes/bug-1667450-migration-stale-source-9c092fee267a7a0f.yaml diff --git a/manila/share/manager.py b/manila/share/manager.py index 028454b1ee..990c5858d8 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -1237,15 +1237,21 @@ 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.db.share_instance_update(context, src_share_instance['id'], + {'status': constants.STATUS_INACTIVE}) + + self._migration_delete_instance(context, src_share_instance['id']) + 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) - self.db.share_instance_update( - context, instance_id, {'status': constants.STATUS_INACTIVE}) - rules = self.access_helper.get_and_update_share_instance_access_rules( context, share_instance_id=instance_id) @@ -1327,12 +1333,6 @@ class ShareManager(manager.SchedulerDependentManager): {'status': constants.STATUS_AVAILABLE}) raise exception.ShareMigrationFailed(reason=msg) - 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']) @@ -1355,6 +1355,8 @@ class ShareManager(manager.SchedulerDependentManager): 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) @@ -1416,6 +1418,15 @@ class ShareManager(manager.SchedulerDependentManager): raise exception.ShareMigrationFailed(reason=msg) + self.db.share_instance_update( + context, dest_share_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) + @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 9a2762c7cd..e430f43216 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -4446,9 +4446,6 @@ 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( @@ -4461,6 +4458,7 @@ 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, @@ -4474,9 +4472,6 @@ class ShareManagerTestCase(test.TestCase): 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']) @@ -4538,12 +4533,6 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.db.share_update.assert_called_once_with( 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, constants.TASK_STATE_DATA_COPYING_COMPLETED, @@ -4559,7 +4548,7 @@ class ShareManagerTestCase(test.TestCase): # mocks self.mock_object(self.share_manager.db, 'share_instance_get', - mock.Mock(return_value=new_instance)) + mock.Mock(side_effect=[instance, new_instance])) self.mock_object(helper, 'cleanup_new_instance') self.mock_object(migration_api, 'ShareMigrationHelper', mock.Mock(return_value=helper)) @@ -4583,8 +4572,10 @@ class ShareManagerTestCase(test.TestCase): self.context, share, instance['id'], new_instance['id']) # asserts - 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_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) + ]) cancelled = not(status == constants.TASK_STATE_DATA_COPYING_CANCELLED) if status != 'other': @@ -4658,7 +4649,9 @@ 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( @@ -4688,7 +4681,13 @@ 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_has_calls([ + mock.call(self.context, dest_instance['id'], + {'status': constants.STATUS_AVAILABLE}), + mock.call(self.context, src_instance['id'], + {'status': constants.STATUS_INACTIVE})]) self.share_manager.db.share_update.assert_called_once_with( self.context, dest_instance['share_id'], {'task_state': constants.TASK_STATE_MIGRATION_COMPLETING}) @@ -4728,8 +4727,11 @@ class ShareManagerTestCase(test.TestCase): # mocks self.mock_object(self.share_manager.db, 'share_instance_get', - mock.Mock(return_value=new_instance)) + mock.Mock(side_effect=[instance, new_instance])) + self.mock_object(self.share_manager.db, 'share_instance_update') self.mock_object(self.share_manager.db, 'share_update') + delete_mock = self.mock_object(migration_api.ShareMigrationHelper, + 'delete_instance_and_wait') self.mock_object(migration_api.ShareMigrationHelper, 'apply_new_access_rules') @@ -4738,14 +4740,24 @@ class ShareManagerTestCase(test.TestCase): self.context, share, instance['id'], new_instance['id']) # asserts - 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_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_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_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) + delete_mock.assert_called_once_with(instance) @ddt.data(constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS, constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE, @@ -4875,7 +4887,6 @@ class ShareManagerTestCase(test.TestCase): # mocks self.mock_object(self.share_manager.db, 'share_instance_get', mock.Mock(return_value=instance)) - self.mock_object(self.share_manager.db, 'share_instance_update') mock_get_access_rules_call = self.mock_object( self.share_manager.access_helper, 'get_and_update_share_instance_access_rules', @@ -4899,9 +4910,6 @@ class ShareManagerTestCase(test.TestCase): # asserts self.share_manager.db.share_instance_get.assert_called_once_with( self.context, instance['id'], with_share_data=True) - self.share_manager.db.share_instance_update.assert_called_once_with( - self.context, instance['id'], - {'status': constants.STATUS_INACTIVE}) mock_get_access_rules_call.assert_called_once_with( self.context, share_instance_id=instance['id']) mock_delete_access_rules_call.assert_called_once_with( diff --git a/releasenotes/notes/bug-1667450-migration-stale-source-9c092fee267a7a0f.yaml b/releasenotes/notes/bug-1667450-migration-stale-source-9c092fee267a7a0f.yaml new file mode 100644 index 0000000000..364583bef2 --- /dev/null +++ b/releasenotes/notes/bug-1667450-migration-stale-source-9c092fee267a7a0f.yaml @@ -0,0 +1,4 @@ +fixes: + - Fixed host-assisted share migration not deleting the + source share from the storage backend upon completion. +