Remove 'ComputeManager._reschedule'

Resolve a TODO and remove this function, folding its contents back into
its caller. This will make a future patch much simpler.

Part of blueprint remove-cells-v1

Change-Id: I02b8f07cb0a325cbb28603610f752041ef6cd02c
Signed-off-by: Stephen Finucane <[email protected]>
Co-Authored-By: Balazs Gibizer <[email protected]>

Author: 2 people

nova/compute/manager.py

@@ -1422,37 +1422,6 @@ def _log_original_error(self, exc_info, instance_uuid):
         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 @@ def _reschedule_resize_or_reraise(self, context, instance, exc_info,
         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
-
-            rescheduled = self._reschedule(context, request_spec,
-                    filter_properties, instance, reschedule_method,
-                    method_args, task_state, exc_info, host_list=host_list)
+            retry = filter_properties.get('retry')
+            if retry:
+                LOG.debug('Rescheduling, attempt %d', retry['num_attempts'],
+                          instance_uuid=instance_uuid)
+
+                # 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 @@ def _reschedule_resize_or_reraise(self, context, instance, exc_info,
                 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,

nova/tests/functional/notification_sample_tests/test_instance.py

@@ -1056,11 +1056,9 @@ def _test_resize_and_revert_server(self, server):
                 '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 @@ def _build_resources(*args, **kwargs):
         }
         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 @@ def _build_resources(*args, **kwargs):
             },
             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 @@ def _build_resources(*args, **kwargs):
         }
         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')

nova/tests/unit/compute/test_compute.py

@@ -22,7 +22,6 @@
 import operator
 import sys
 import time
-import traceback
 
 import ddt
 
@@ -12789,62 +12788,6 @@ def fake_get_flavor_by_flavor_id(flavor_id, ctxt=None,
             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 @@ def setUp(self):
         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 @@ def test_reschedule_resize_or_reraise_called(self, mock_res, mock_prep):
         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")
-    @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
+    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.
         """
-        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")
+        filter_properties = None
 
         try:
             raise test.TestingException("Original")
         except Exception:
             exc_info = sys.exc_info()
-            reqspec = objects.RequestSpec()
+            # because we're not retrying, we should re-raise the exception
             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)
+                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'.
 
-    @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.
+        This should disable rescheduling and the original exception should be
+        raised.
         """
-        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
+        filter_properties = {}
 
         try:
             raise test.TestingException("Original")
         except Exception:
             exc_info = sys.exc_info()
-            reqspec = objects.RequestSpec()
+            # because we're not retrying, we should re-raise the exception
             self.assertRaises(test.TestingException,
-                    self.compute._reschedule_resize_or_reraise, self.context,
-                    instance, exc_info, self.instance_type, reqspec,
-                    {}, None)
+                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')
+    @mock.patch.object(compute_manager.ComputeManager, "_log_original_error")
+    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}}
 
-            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)
+        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, "_reschedule")
+    @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_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_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)
-
-            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)
+                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=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):