Stop providing force_hosts to the scheduler for move ops

Since now we provide the original RequestSpec for move operations (unshelve,
live-migrate and evacuate), it can also provide the original force_hosts/nodes
to the scheduler.
In that case, it means that if an admin was asking to boot an instance forcing
to an host, a later move operation could then give again the forced value and
then wouldn't permit to get a different destination which is an issue.

TBH, that is not a problem for live-migrate and evacuate that do provide an
optional host value (which bypasses then the scheduler) but since unshelve
is not having this optional value, it would mean that we could only unshelve
an forced instance to the same host.

Change-Id: I03c22ff757d0ee1da9d69fa48cc4bdd036e6b13f
Closes-Bug: #1561357
(cherry picked from commit 446d15568e)

nova/conductor/manager.py

@@ -482,6 +482,10 @@ class ComputeTaskManager(base.Base):
                         request_spec = scheduler_utils.build_request_spec(
                             context, image, [instance])
                     else:
+                        # NOTE(sbauza): Force_hosts/nodes needs to be reset
+                        # if we want to make sure that the next destination
+                        # is not forced to be the original host
+                        request_spec.reset_forced_destinations()
                         # TODO(sbauza): Provide directly the RequestSpec object
                         # when _schedule_instances(),
                         # populate_filter_properties and populate_retry()
@@ -543,6 +547,10 @@ class ComputeTaskManager(base.Base):
                     # the source host for avoiding the scheduler to pick it
                     request_spec.ignore_hosts = request_spec.ignore_hosts or []
                     request_spec.ignore_hosts.append(instance.host)
+                    # NOTE(sbauza): Force_hosts/nodes needs to be reset
+                    # if we want to make sure that the next destination
+                    # is not forced to be the original host
+                    request_spec.reset_forced_destinations()
                     # TODO(sbauza): Provide directly the RequestSpec object
                     # when _schedule_instances() and _set_vm_state_and_notify()
                     # accept it

nova/conductor/tasks/live_migrate.py

@@ -184,6 +184,10 @@ class LiveMigrationTask(base.TaskBase):
             )
         else:
             request_spec = self.request_spec
+            # NOTE(sbauza): Force_hosts/nodes needs to be reset
+            # if we want to make sure that the next destination
+            # is not forced to be the original host
+            request_spec.reset_forced_destinations()
 
         host = None
         while host is None:

nova/objects/request_spec.py

@@ -450,6 +450,18 @@ class RequestSpec(base.NovaObject):
         self._from_db_object(self._context, self, db_spec)
         self.obj_reset_changes()
 
+    def reset_forced_destinations(self):
+        """Clears the forced destination fields from the RequestSpec object.
+
+        This method is for making sure we don't ask the scheduler to give us
+        again the same destination(s) without persisting the modifications.
+        """
+        self.force_hosts = None
+        self.force_nodes = None
+        # NOTE(sbauza): Make sure we don't persist this, we need to keep the
+        # original request for the forced hosts
+        self.obj_reset_changes(['force_hosts', 'force_nodes'])
+
 
 @base.NovaObjectRegistry.register
 class SchedulerRetries(base.NovaObject):

nova/tests/unit/conductor/tasks/test_live_migrate.py

@@ -254,6 +254,8 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
     def test_find_destination_works(self):
         self.mox.StubOutWithMock(utils, 'get_image_from_system_metadata')
         self.mox.StubOutWithMock(scheduler_utils, 'setup_instance_group')
+        self.mox.StubOutWithMock(objects.RequestSpec,
+                                 'reset_forced_destinations')
         self.mox.StubOutWithMock(self.task.scheduler_client,
                                  'select_destinations')
         self.mox.StubOutWithMock(self.task,
@@ -265,6 +267,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
         fake_props = {'instance_properties': {'uuid': self.instance_uuid}}
         scheduler_utils.setup_instance_group(
             self.context, fake_props, {'ignore_hosts': [self.instance_host]})
+        self.fake_spec.reset_forced_destinations()
         self.task.scheduler_client.select_destinations(
             self.context, self.fake_spec).AndReturn(
                         [{'host': 'host1'}])

nova/tests/unit/conductor/test_conductor.py

@@ -678,13 +678,16 @@ class _BaseTaskTestCase(object):
         @mock.patch.object(objects.RequestSpec, 'to_legacy_request_spec_dict')
         @mock.patch.object(objects.RequestSpec,
                            'to_legacy_filter_properties_dict')
-        def do_test(to_filtprops, to_reqspec, sched_instances,
+        @mock.patch.object(objects.RequestSpec, 'reset_forced_destinations')
+        def do_test(reset_forced_destinations,
+                    to_filtprops, to_reqspec, sched_instances,
                     populate_retry, populate_filter_properties,
                     unshelve_instance):
             to_filtprops.return_value = filter_properties
             to_reqspec.return_value = request_spec
             sched_instances.return_value = [host]
             self.conductor.unshelve_instance(self.context, instance, fake_spec)
+            reset_forced_destinations.assert_called_once_with()
             sched_instances.assert_called_once_with(self.context, request_spec,
                                                     filter_properties)
             # NOTE(sbauza): Since the instance is dehydrated when passing thru
@@ -1038,15 +1041,17 @@ class _BaseTaskTestCase(object):
                               return_value=[{'host': expected_host,
                                              'nodename': expected_node,
                                              'limits': expected_limits}]),
+            mock.patch.object(fake_spec, 'reset_forced_destinations'),
             mock.patch.object(fake_spec, 'to_legacy_request_spec_dict',
                        return_value=request_spec),
             mock.patch.object(fake_spec, 'to_legacy_filter_properties_dict',
                        return_value=filter_properties),
-        ) as (rebuild_mock, sig_mock, fp_mock, select_dest_mock, to_reqspec,
-              to_filtprops):
+        ) as (rebuild_mock, sig_mock, fp_mock, select_dest_mock, reset_fd,
+              to_reqspec, to_filtprops):
             self.conductor_manager.rebuild_instance(context=self.context,
                                             instance=inst_obj,
                                             **rebuild_args)
+            reset_fd.assert_called_once_with()
             to_reqspec.assert_called_once_with()
             to_filtprops.assert_called_once_with()
             fp_mock.assert_called_once_with(self.context, request_spec,

nova/tests/unit/objects/test_request_spec.py

@@ -531,6 +531,18 @@ class _TestRequestSpecObject(object):
                 _test_save_args):
             req_obj.save()
 
+    def test_reset_forced_destinations(self):
+        req_obj = fake_request_spec.fake_spec_obj()
+        # Making sure the fake object has forced hosts and nodes
+        self.assertIsNotNone(req_obj.force_hosts)
+        self.assertIsNotNone(req_obj.force_nodes)
+
+        with mock.patch.object(req_obj, 'obj_reset_changes') as mock_reset:
+            req_obj.reset_forced_destinations()
+        self.assertIsNone(req_obj.force_hosts)
+        self.assertIsNone(req_obj.force_nodes)
+        mock_reset.assert_called_once_with(['force_hosts', 'force_nodes'])
+
 
 class TestRequestSpecObject(test_objects._LocalTest,
                             _TestRequestSpecObject):