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
This commit is contained in:
parent
b63177470f
commit
446d15568e
|
@ -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
|
||||
|
|
|
@ -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:
|
||||
|
|
|
@ -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):
|
||||
|
|
|
@ -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'}])
|
||||
|
|
|
@ -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,
|
||||
|
|
|
@ -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):
|
||||
|
|
Loading…
Reference in New Issue