From 143decf4bc3d1e4f686a7dc7e5336791edc487a3 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Wed, 30 Mar 2016 08:53:17 -0300 Subject: [PATCH] Fix Share status when driver migrates If a driver implements optimized migration, share status is stuck 'migrating' and task state is stuck 'migration_in_progress' if 2-phase migration is not used. This change addresses it by properly setting the task state and status for this specific scenario. This change also adds 'host' to model update performed by manager, to prevent confusion related to drivers having to return such value in their model update or not. Change-Id: I4b2c1b02771d8bbb6a8d3ca2719186ad41e5d9af Closes-bug: #1563823 --- manila/share/driver.py | 6 +- manila/share/manager.py | 33 +++++-- manila/tests/share/test_manager.py | 148 +++++++++++++++++++++-------- 3 files changed, 137 insertions(+), 50 deletions(-) diff --git a/manila/share/driver.py b/manila/share/driver.py index 492128578c..b9f94112c7 100644 --- a/manila/share/driver.py +++ b/manila/share/driver.py @@ -326,7 +326,8 @@ class ShareDriver(object): 2nd phase call. Driver may throw exception when validating this parameter, exception if does not support 1-phase or 2-phase approach. :returns: Boolean value indicating if driver migration succeeded. - :returns: Dictionary containing a model update. + :returns: Dictionary containing a model update with relevant data to + be updated after migration, such as export locations. """ return None, None @@ -342,7 +343,8 @@ class ShareDriver(object): :param share_server: Share server model or None. :param dest_driver_migration_info: Migration information provided by destination host. - :returns: Dictionary containing a model update. + :returns: Dictionary containing a model update with relevant data to + be updated after migration, such as export locations. """ return None diff --git a/manila/share/manager.py b/manila/share/manager.py index 7048add3b5..2d5c75c66d 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -614,7 +614,6 @@ class ShareManager(manager.SchedulerDependentManager): if not force_host_copy: try: - dest_driver_migration_info = rpcapi.migration_get_driver_info( context, share_instance) @@ -632,12 +631,6 @@ class ShareManager(manager.SchedulerDependentManager): context, share_instance, share_server, host, dest_driver_migration_info, notify) - if moved and not notify: - self.db.share_update( - context, share_ref['id'], - {'task_state': - constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE}) - # NOTE(ganso): Here we are allowing the driver to perform # changes even if it has not performed migration. While this # scenario may not be valid, I do not think it should be @@ -654,10 +647,14 @@ class ShareManager(manager.SchedulerDependentManager): "with generic migration approach.") % share_id) if not moved: - try: - LOG.debug("Starting generic migration " - "for share %s.", share_id) + LOG.debug("Starting generic migration " + "for share %s.", share_id) + self.db.share_update( + context, share_id, + {'task_state': constants.TASK_STATE_MIGRATION_IN_PROGRESS}) + + try: self._migration_start_generic(context, share_ref, share_instance, host, notify) except Exception: @@ -670,6 +667,22 @@ class ShareManager(manager.SchedulerDependentManager): context, share_instance['id'], {'status': constants.STATUS_AVAILABLE}) raise exception.ShareMigrationFailed(reason=msg) + elif not notify: + self.db.share_update( + context, share_ref['id'], + {'task_state': + constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE}) + else: + self.db.share_instance_update( + context, share_instance['id'], + {'status': constants.STATUS_AVAILABLE, + 'host': host['host']}) + self.db.share_update( + context, share_ref['id'], + {'task_state': constants.TASK_STATE_MIGRATION_SUCCESS}) + + LOG.info(_LI("Share Migration for share %s" + " completed successfully."), share_ref['id']) def _migration_start_generic(self, context, share, share_instance, host, notify): diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 77b8b29c32..9aaccc514f 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -3677,8 +3677,94 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.driver.migration_get_driver_info.\ assert_called_once_with(self.context, share_instance, share_server) - @ddt.data((True, 'fake_model_update'), exception.ManilaException()) - def test_migration_start(self, exc): + @ddt.data({'return_value': (True, 'fake_model_update'), 'notify': True}, + {'return_value': (False, 'fake_model_update'), 'notify': True}, + {'return_value': (True, 'fake_model_update'), 'notify': False}, + {'return_value': (False, 'fake_model_update'), 'notify': False}) + @ddt.unpack + def test_migration_start(self, return_value, notify): + + server = 'fake_share_server' + instance = db_utils.create_share_instance( + share_id='fake_id', + status=constants.STATUS_AVAILABLE, + share_server_id='fake_server_id') + share = db_utils.create_share(id='fake_id', instances=[instance]) + host = {'host': 'fake_host'} + driver_migration_info = 'driver_fake_info' + + # mocks + self.mock_object(self.share_manager.db, 'share_get', + mock.Mock(return_value=share)) + self.mock_object(self.share_manager.db, 'share_instance_get', + mock.Mock(return_value=instance)) + self.mock_object(self.share_manager.db, 'share_server_get', + mock.Mock(return_value=server)) + self.mock_object(self.share_manager.db, 'share_update') + self.mock_object(self.share_manager.db, 'share_instance_update') + self.mock_object(rpcapi.ShareAPI, 'migration_get_driver_info', + mock.Mock(return_value=driver_migration_info)) + self.mock_object(self.share_manager.driver, 'migration_start', + mock.Mock(return_value=return_value)) + if not return_value[0]: + self.mock_object(self.share_manager, '_migration_start_generic') + + # run + self.share_manager.migration_start( + self.context, 'fake_id', host, False, notify) + + # asserts + self.share_manager.db.share_get.assert_called_once_with( + self.context, share['id']) + self.share_manager.db.share_instance_get.assert_called_once_with( + self.context, instance['id'], with_share_data=True) + self.share_manager.db.share_server_get.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), + instance['share_server_id']) + + share_update_calls = [ + mock.call( + self.context, share['id'], + {'task_state': constants.TASK_STATE_MIGRATION_IN_PROGRESS}), + mock.call( + self.context, share['id'], + {'task_state': ( + constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS)}) + ] + share_instance_update_calls = [ + mock.call(self.context, instance['id'], + {'status': constants.STATUS_MIGRATING}), + mock.call(self.context, instance['id'], 'fake_model_update') + ] + + if not notify and return_value[0]: + share_update_calls.append(mock.call( + self.context, share['id'], + {'task_state': + constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE})) + elif notify and return_value[0]: + share_update_calls.append(mock.call( + self.context, share['id'], + {'task_state': constants.TASK_STATE_MIGRATION_SUCCESS})) + share_instance_update_calls.append(mock.call( + self.context, instance['id'], + {'status': constants.STATUS_AVAILABLE, + 'host': host['host']})) + elif not return_value[0]: + share_update_calls.append(mock.call( + self.context, share['id'], + {'task_state': constants.TASK_STATE_MIGRATION_IN_PROGRESS})) + + self.share_manager.db.share_update.assert_has_calls(share_update_calls) + self.share_manager.db.share_instance_update.assert_has_calls( + share_instance_update_calls) + rpcapi.ShareAPI.migration_get_driver_info.assert_called_once_with( + self.context, instance) + self.share_manager.driver.migration_start.assert_called_once_with( + self.context, instance, server, host, driver_migration_info, + notify) + + def test_migration_start_exception(self): server = 'fake_share_server' instance = db_utils.create_share_instance( @@ -3701,24 +3787,16 @@ class ShareManagerTestCase(test.TestCase): self.mock_object(rpcapi.ShareAPI, 'migration_get_driver_info', mock.Mock(return_value=driver_migration_info)) - if isinstance(exc, exception.ManilaException): - self.mock_object(self.share_manager.driver, 'migration_start', - mock.Mock(side_effect=exc)) - self.mock_object(self.share_manager, '_migration_start_generic', - mock.Mock(side_effect=Exception('fake'))) - self.mock_object(manager.LOG, 'exception') - else: - self.mock_object(self.share_manager.driver, 'migration_start', - mock.Mock(return_value=exc)) + self.mock_object(self.share_manager.driver, 'migration_start', + mock.Mock(side_effect=Exception('fake'))) + self.mock_object(self.share_manager, '_migration_start_generic', + mock.Mock(side_effect=Exception('fake'))) + self.mock_object(manager.LOG, 'exception') # run - if isinstance(exc, exception.ManilaException): - self.assertRaises(exception.ShareMigrationFailed, - self.share_manager.migration_start, - self.context, 'fake_id', host, False, False) - else: - self.share_manager.migration_start( - self.context, 'fake_id', host, False, False) + self.assertRaises(exception.ShareMigrationFailed, + self.share_manager.migration_start, + self.context, 'fake_id', host, False, False) # asserts self.share_manager.db.share_get.assert_called_once_with( @@ -3736,30 +3814,24 @@ class ShareManagerTestCase(test.TestCase): mock.call( self.context, share['id'], {'task_state': ( - constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS)}) + constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS)}), + mock.call( + self.context, share['id'], + {'task_state': constants.TASK_STATE_MIGRATION_IN_PROGRESS}), + mock.call( + self.context, share['id'], + {'task_state': constants.TASK_STATE_MIGRATION_ERROR}) ] share_instance_update_calls = [ mock.call(self.context, instance['id'], - {'status': constants.STATUS_MIGRATING}) + {'status': constants.STATUS_MIGRATING}), + mock.call(self.context, instance['id'], + {'status': constants.STATUS_AVAILABLE}) ] - if isinstance(exc, exception.ManilaException): - share_update_calls.append(mock.call( - self.context, share['id'], - {'task_state': constants.TASK_STATE_MIGRATION_ERROR})) - share_instance_update_calls.append( - mock.call(self.context, instance['id'], - {'status': constants.STATUS_AVAILABLE})) - self.share_manager._migration_start_generic.\ - assert_called_once_with(self.context, share, instance, host, - False) - self.assertTrue(manager.LOG.exception.called) - else: - share_update_calls.append(mock.call( - self.context, share['id'], - {'task_state': - constants.TASK_STATE_MIGRATION_DRIVER_PHASE1_DONE})) - share_instance_update_calls.append( - mock.call(self.context, instance['id'], 'fake_model_update')) + (self.share_manager._migration_start_generic. + assert_called_once_with(self.context, share, instance, host, + False)) + self.assertTrue(manager.LOG.exception.called) self.share_manager.db.share_update.assert_has_calls(share_update_calls) self.share_manager.db.share_instance_update.assert_has_calls(