diff --git a/nova/compute/manager.py b/nova/compute/manager.py index faa2f3857788..151c3c51e75a 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -1422,37 +1422,6 @@ class ComputeManager(manager.Manager): LOG.error('Error: %s', exc_info[1], instance_uuid=instance_uuid, exc_info=exc_info) - # TODO(mriedem): This method is confusing and only ever used for resize - # reschedules; remove it and merge into _reschedule_resize_or_reraise. - def _reschedule(self, context, request_spec, filter_properties, - instance, reschedule_method, method_args, task_state, - exc_info=None, host_list=None): - """Attempt to re-schedule a compute operation.""" - - instance_uuid = instance.uuid - retry = filter_properties.get('retry') - if not retry: - # no retry information, do not reschedule. - LOG.debug("Retry info not present, will not reschedule", - instance_uuid=instance_uuid) - return - - LOG.debug("Re-scheduling %(method)s: attempt %(num)d", - {'method': reschedule_method.__name__, - 'num': retry['num_attempts']}, instance_uuid=instance_uuid) - - # reset the task state: - self._instance_update(context, instance, task_state=task_state) - - if exc_info: - # stringify to avoid circular ref problem in json serialization: - retry['exc'] = traceback.format_exception_only(exc_info[0], - exc_info[1]) - - reschedule_method(context, *method_args, request_spec=request_spec, - host_list=host_list) - return True - @periodic_task.periodic_task def _check_instance_build_time(self, context): """Ensure that instances are not stuck in build.""" @@ -4458,14 +4427,33 @@ class ComputeManager(manager.Manager): instance_uuid = instance.uuid try: - reschedule_method = self.compute_task_api.resize_instance - scheduler_hint = dict(filter_properties=filter_properties) - method_args = (instance, None, scheduler_hint, instance_type) - task_state = task_states.RESIZE_PREP + retry = filter_properties.get('retry') + if retry: + LOG.debug('Rescheduling, attempt %d', retry['num_attempts'], + instance_uuid=instance_uuid) - rescheduled = self._reschedule(context, request_spec, - filter_properties, instance, reschedule_method, - method_args, task_state, exc_info, host_list=host_list) + # reset the task state + task_state = task_states.RESIZE_PREP + self._instance_update(context, instance, task_state=task_state) + + if exc_info: + # stringify to avoid circular ref problem in json + # serialization + retry['exc'] = traceback.format_exception_only( + exc_info[0], exc_info[1]) + + scheduler_hint = {'filter_properties': filter_properties} + + self.compute_task_api.resize_instance( + context, instance, None, scheduler_hint, instance_type, + request_spec=request_spec, host_list=host_list) + + rescheduled = True + else: + # no retry information, do not reschedule. + LOG.debug('Retry info not present, will not reschedule', + instance_uuid=instance_uuid) + rescheduled = False except Exception as error: rescheduled = False LOG.exception("Error trying to reschedule", @@ -4481,6 +4469,7 @@ class ComputeManager(manager.Manager): phase=fields.NotificationPhase.ERROR, exception=error, tb=','.join(traceback.format_exception(*exc_info))) + if rescheduled: self._log_original_error(exc_info, instance_uuid) compute_utils.add_instance_fault_from_exc(context, diff --git a/nova/tests/functional/notification_sample_tests/test_instance.py b/nova/tests/functional/notification_sample_tests/test_instance.py index f1a6169f6ee0..33acb7393231 100644 --- a/nova/tests/functional/notification_sample_tests/test_instance.py +++ b/nova/tests/functional/notification_sample_tests/test_instance.py @@ -1056,11 +1056,9 @@ class TestInstanceNotificationSample( 'uuid': server['id']}, actual=fake_notifier.VERSIONED_NOTIFICATIONS[1]) - @mock.patch('nova.compute.manager.ComputeManager._reschedule', - return_value=True) @mock.patch('nova.compute.manager.ComputeManager._prep_resize') def test_resize_server_error_but_reschedule_was_success( - self, mock_prep_resize, mock_reschedule): + self, mock_prep_resize): """Test it, when the prep_resize method raise an exception, but the reschedule_resize_or_reraise was successful and scheduled the resize. In this case we get a notification @@ -1089,6 +1087,14 @@ class TestInstanceNotificationSample( } fake_notifier.reset() mock_prep_resize.side_effect = _build_resources + # NOTE(gibi): the first resize_instance call (from the API) should be + # unaffected so that we can reach _prep_resize at all. But the + # subsequent resize_instance call (from _reschedule_resize_or_reraise) + # needs to be mocked as there is no alternative host to resize to. + patcher = mock.patch.object(self.compute.manager.compute_task_api, + 'resize_instance') + self.addCleanup(patcher.stop) + patcher.start() self.api.post_server_action(server['id'], post) self._wait_for_notification('instance.resize.error') self._pop_and_verify_dest_select_notification(server['id'], @@ -1120,10 +1126,9 @@ class TestInstanceNotificationSample( }, actual=fake_notifier.VERSIONED_NOTIFICATIONS[2]) - @mock.patch('nova.compute.manager.ComputeManager._reschedule') @mock.patch('nova.compute.manager.ComputeManager._prep_resize') def test_resize_server_error_and_reschedule_was_failed( - self, mock_prep_resize, mock_reschedule): + self, mock_prep_resize): """Test it, when the prep_resize method raise an exception, after trying again with the reschedule_resize_or_reraise method call, but the rescheduled also was unsuccessful. In this @@ -1156,9 +1161,17 @@ class TestInstanceNotificationSample( } fake_notifier.reset() mock_prep_resize.side_effect = _build_resources - # This isn't realistic that _reschedule would raise FlavorDiskTooSmall, - # but it's needed for the notification sample to work. - mock_reschedule.side_effect = _build_resources + # NOTE(gibi): the first resize_instance call (from the API) should be + # unaffected so that we can reach _prep_resize at all. But the + # subsequent resize_instance call (from _reschedule_resize_or_reraise) + # needs to fail. It isn't realistic that resize_instance would raise + # FlavorDiskTooSmall, but it's needed for the notification sample + # to work. + patcher = mock.patch.object(self.compute.manager.compute_task_api, + 'resize_instance', + side_effect=_build_resources) + self.addCleanup(patcher.stop) + patcher.start() self.api.post_server_action(server['id'], post) self._wait_for_state_change(self.api, server, expected_status='ERROR') self._wait_for_notification('compute.exception') diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 64d5277ab281..364b333150f9 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -22,7 +22,6 @@ from itertools import chain import operator import sys import time -import traceback import ddt @@ -12789,62 +12788,6 @@ class DisabledInstanceTypesTestCase(BaseTestCase): self.compute_api.resize, self.context, instance, '4') -class ComputeReschedulingTestCase(BaseTestCase): - """Tests re-scheduling logic for new build requests.""" - - def setUp(self): - super(ComputeReschedulingTestCase, self).setUp() - - self.expected_task_state = task_states.SCHEDULING - - def fake_update(*args, **kwargs): - self.updated_task_state = kwargs.get('task_state') - self.stub_out('nova.compute.manager.ComputeManager._instance_update', - fake_update) - - def _reschedule(self, request_spec=None, filter_properties=None, - exc_info=None): - if not filter_properties: - filter_properties = {} - fake_taskapi = FakeComputeTaskAPI() - with mock.patch.object(self.compute, 'compute_task_api', - fake_taskapi): - instance = self._create_fake_instance_obj() - - scheduler_method = self.compute.compute_task_api.resize_instance - method_args = (instance, None, - dict(filter_properties=filter_properties), - {}, None) - return self.compute._reschedule(self.context, request_spec, - filter_properties, instance, scheduler_method, - method_args, self.expected_task_state, exc_info=exc_info) - - def test_reschedule_no_filter_properties(self): - # no filter_properties will disable re-scheduling. - self.assertFalse(self._reschedule()) - - def test_reschedule_no_retry_info(self): - # no retry info will also disable re-scheduling. - filter_properties = {} - self.assertFalse(self._reschedule(filter_properties=filter_properties)) - - def test_reschedule_success(self): - retry = dict(num_attempts=1) - filter_properties = dict(retry=retry) - request_spec = {'num_instances': 1} - try: - raise test.TestingException("just need an exception") - except test.TestingException: - exc_info = sys.exc_info() - exc_str = traceback.format_exception_only(exc_info[0], - exc_info[1]) - - self.assertTrue(self._reschedule(filter_properties=filter_properties, - request_spec=request_spec, exc_info=exc_info)) - self.assertEqual(self.updated_task_state, self.expected_task_state) - self.assertEqual(exc_str, filter_properties['retry']['exc']) - - class InnerTestingException(Exception): pass @@ -12859,6 +12802,7 @@ class ComputeRescheduleResizeOrReraiseTestCase(BaseTestCase): self.instance_uuid = self.instance['uuid'] self.instance_type = objects.Flavor.get_by_name( context.get_admin_context(), 'm1.tiny') + self.request_spec = objects.RequestSpec() @mock.patch('nova.compute.manager.ComputeManager._prep_resize', side_effect=test.TestingException) @@ -12873,93 +12817,113 @@ class ComputeRescheduleResizeOrReraiseTestCase(BaseTestCase): self.compute.prep_resize(self.context, image=None, instance=inst_obj, instance_type=self.instance_type, - request_spec=mock.sentinel.reqspec, + request_spec=self.request_spec, filter_properties={}, migration=mock.Mock(), node=None, clean_shutdown=True, host_list=None) mock_res.assert_called_once_with(mock.ANY, inst_obj, mock.ANY, self.instance_type, - mock.sentinel.reqspec, {}, None) + self.request_spec, {}, None) - @mock.patch.object(compute_manager.ComputeManager, "_reschedule") + def test_reschedule_resize_or_reraise_no_filter_properties(self): + """Test behavior when ``filter_properties`` is None. + + This should disable rescheduling and the original exception should be + raised. + """ + filter_properties = None + + try: + raise test.TestingException("Original") + except Exception: + exc_info = sys.exc_info() + # because we're not retrying, we should re-raise the exception + self.assertRaises(test.TestingException, + self.compute._reschedule_resize_or_reraise, self.context, + self.instance, exc_info, self.instance_type, + self.request_spec, filter_properties, None) + + def test_reschedule_resize_or_reraise_no_retry_info(self): + """Test behavior when ``filter_properties`` doesn't contain 'retry'. + + This should disable rescheduling and the original exception should be + raised. + """ + filter_properties = {} + + try: + raise test.TestingException("Original") + except Exception: + exc_info = sys.exc_info() + # because we're not retrying, we should re-raise the exception + self.assertRaises(test.TestingException, + self.compute._reschedule_resize_or_reraise, self.context, + self.instance, exc_info, self.instance_type, + self.request_spec, filter_properties, None) + + @mock.patch.object(compute_manager.ComputeManager, '_instance_update') + @mock.patch('nova.conductor.api.ComputeTaskAPI.resize_instance', + side_effect=InnerTestingException('inner')) @mock.patch('nova.compute.utils.notify_about_instance_action') - def test_reschedule_fails_with_exception(self, mock_notify, mock_res): - """Original exception should be raised if the _reschedule method - raises another exception - """ - instance = self._create_fake_instance_obj() - scheduler_hint = dict(filter_properties={}) - method_args = (instance, None, scheduler_hint, self.instance_type) - mock_res.side_effect = InnerTestingException("Inner") - - try: - raise test.TestingException("Original") - except Exception: - exc_info = sys.exc_info() - reqspec = objects.RequestSpec() - self.assertRaises(test.TestingException, - self.compute._reschedule_resize_or_reraise, self.context, - instance, exc_info, self.instance_type, reqspec, - {}, None) - - mock_res.assert_called_once_with( - self.context, reqspec, {}, instance, - self.compute.compute_task_api.resize_instance, method_args, - task_states.RESIZE_PREP, exc_info, host_list=None) - mock_notify.assert_called_once_with( - self.context, instance, 'fake-mini', action='resize', - phase='error', exception=mock_res.side_effect, tb=mock.ANY) - - @mock.patch.object(compute_manager.ComputeManager, "_reschedule") - def test_reschedule_false(self, mock_res): - """Original exception should be raised if the resize is not - rescheduled. - """ - instance = self._create_fake_instance_obj() - scheduler_hint = dict(filter_properties={}) - method_args = (instance, None, scheduler_hint, self.instance_type) - mock_res.return_value = False - - try: - raise test.TestingException("Original") - except Exception: - exc_info = sys.exc_info() - reqspec = objects.RequestSpec() - self.assertRaises(test.TestingException, - self.compute._reschedule_resize_or_reraise, self.context, - instance, exc_info, self.instance_type, reqspec, - {}, None) - - mock_res.assert_called_once_with( - self.context, reqspec, {}, instance, - self.compute.compute_task_api.resize_instance, method_args, - task_states.RESIZE_PREP, exc_info, host_list=None) - - @mock.patch.object(compute_manager.ComputeManager, "_reschedule") @mock.patch.object(compute_manager.ComputeManager, "_log_original_error") - def test_reschedule_true(self, mock_log, mock_res): - # If rescheduled, the original resize exception should be logged. - instance = self._create_fake_instance_obj() - scheduler_hint = dict(filter_properties={}) - method_args = (instance, None, scheduler_hint, self.instance_type) + def test_reschedule_fails_with_exception(self, mock_log, mock_notify, + mock_resize, mock_update): + """Original exception should be raised if the conductor call + raises another exception. + """ + filter_properties = {'retry': {'num_attempts': 0}} + + try: + raise test.TestingException('Original') + except Exception: + exc_info = sys.exc_info() + self.assertRaises(test.TestingException, + self.compute._reschedule_resize_or_reraise, self.context, + self.instance, exc_info, self.instance_type, + self.request_spec, filter_properties, None) + + mock_update.assert_called_once_with( + self.context, mock.ANY, task_state=task_states.RESIZE_PREP) + mock_resize.assert_called_once_with( + self.context, mock.ANY, None, + {'filter_properties': filter_properties}, self.instance_type, + request_spec=self.request_spec, host_list=None) + mock_notify.assert_called_once_with( + self.context, self.instance, 'fake-mini', action='resize', + phase='error', exception=mock_resize.side_effect, tb=mock.ANY) + # If not rescheduled, the original resize exception should not be + # logged. + mock_log.assert_not_called() + + @mock.patch.object(compute_manager.ComputeManager, '_instance_update') + @mock.patch('nova.conductor.api.ComputeTaskAPI.resize_instance') + @mock.patch('nova.compute.utils.notify_about_instance_action') + @mock.patch.object(compute_manager.ComputeManager, "_log_original_error") + def test_reschedule_passes(self, mock_log, mock_notify, mock_resize, + mock_update): + filter_properties = {'retry': {'num_attempts': 0}} try: raise test.TestingException("Original") except Exception: exc_info = sys.exc_info() - mock_res.return_value = True - reqspec = objects.RequestSpec() self.compute._reschedule_resize_or_reraise( - self.context, instance, exc_info, - self.instance_type, reqspec, {}, None) + self.context, self.instance, exc_info, self.instance_type, + self.request_spec, filter_properties, None) - mock_res.assert_called_once_with(self.context, reqspec, {}, - instance, self.compute.compute_task_api.resize_instance, - method_args, task_states.RESIZE_PREP, exc_info, - host_list=None) - mock_log.assert_called_once_with(exc_info, instance.uuid) + mock_update.assert_called_once_with( + self.context, mock.ANY, task_state=task_states.RESIZE_PREP) + mock_resize.assert_called_once_with( + self.context, mock.ANY, None, + {'filter_properties': filter_properties}, self.instance_type, + request_spec=self.request_spec, host_list=None) + mock_notify.assert_called_once_with( + self.context, self.instance, 'fake-mini', action='resize', + phase='error', exception=exc_info[1], tb=mock.ANY) + # If rescheduled, the original resize exception should be logged. + mock_log.assert_called_once_with(exc_info, self.instance.uuid) class ComputeInactiveImageTestCase(BaseTestCase): diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index a9ca211b8cda..eb7152fd1ee7 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -8754,13 +8754,13 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, reportclient = self.compute.reportclient - @mock.patch.object(self.compute, '_reschedule') + @mock.patch.object(self.compute, '_reschedule_resize_or_reraise') @mock.patch.object(self.compute, '_prep_resize') def doit(mock_pr, mock_r): # Mock the resource tracker, but keep the report client self._mock_rt().reportclient = reportclient - mock_r.return_value = False mock_pr.side_effect = test.TestingException + mock_r.side_effect = test.TestingException instance = objects.Instance(uuid=uuids.instance, id=1, @@ -8781,6 +8781,9 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, self.assertEqual(migration.status, 'error') migration.save.assert_called_once_with() + mock_r.assert_called_once_with( + self.context, instance, mock.ANY, flavor, + mock.sentinel.request_spec, {}, []) mock_notify_resize.assert_has_calls([ mock.call(self.context, instance, 'fake-mini', 'start', flavor),