From 95df2a239c32f2ee5d00f06a59a9e91b59f3aca5 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 5 Apr 2019 15:36:00 -0400 Subject: [PATCH] Restrict RequestSpec to cell when evacuating When evacuating a server in a multi-cell environment we need to restrict the scheduling request during evacuate to the cell in which the instance already exists since we don't support cross-cell evacuate. This fixes the issue by restricting the RequestSpec to the instance's current cell during evacuate in the same way we do during unshelve. Note that this should also improve performance when rebuilding a server with a new image since we will only look for the ComputeNode from the targeted cell rather than iterate all enabled cells during scheduling. Change-Id: I497180fb81fd966d1d3d4b54ac66d2609347583e Closes-Bug: #1823370 --- nova/conductor/manager.py | 44 +++++++++++++------ .../test_instance.py | 6 ++- .../regressions/test_bug_1823370.py | 6 +-- nova/tests/unit/conductor/test_conductor.py | 4 ++ 4 files changed, 39 insertions(+), 21 deletions(-) diff --git a/nova/conductor/manager.py b/nova/conductor/manager.py index 82b125cd45..3a8cc875cf 100644 --- a/nova/conductor/manager.py +++ b/nova/conductor/manager.py @@ -813,6 +813,32 @@ class ComputeTaskManager(base.Base): 'instance(s).', timer.elapsed(), len(instance_uuids)) return host_lists + @staticmethod + def _restrict_request_spec_to_cell(context, instance, request_spec): + """Sets RequestSpec.requested_destination.cell for the move operation + + Move operations, e.g. evacuate and unshelve, must be restricted to the + cell in which the instance already exists, so this method is used to + target the RequestSpec, which is sent to the scheduler via the + _schedule_instances method, to the instance's current cell. + + :param context: nova auth RequestContext + """ + instance_mapping = \ + objects.InstanceMapping.get_by_instance_uuid( + context, instance.uuid) + LOG.debug('Requesting cell %(cell)s during scheduling', + {'cell': instance_mapping.cell_mapping.identity}, + instance=instance) + if ('requested_destination' in request_spec and + request_spec.requested_destination): + request_spec.requested_destination.cell = ( + instance_mapping.cell_mapping) + else: + request_spec.requested_destination = ( + objects.Destination( + cell=instance_mapping.cell_mapping)) + # TODO(mriedem): Make request_spec required in ComputeTaskAPI RPC v2.0. @targets_cell def unshelve_instance(self, context, instance, request_spec=None): @@ -871,20 +897,8 @@ class ComputeTaskManager(base.Base): # NOTE(cfriesen): Ensure that we restrict the scheduler to # the cell specified by the instance mapping. - instance_mapping = \ - objects.InstanceMapping.get_by_instance_uuid( - context, instance.uuid) - LOG.debug('Requesting cell %(cell)s while unshelving', - {'cell': instance_mapping.cell_mapping.identity}, - instance=instance) - if ('requested_destination' in request_spec and - request_spec.requested_destination): - request_spec.requested_destination.cell = ( - instance_mapping.cell_mapping) - else: - request_spec.requested_destination = ( - objects.Destination( - cell=instance_mapping.cell_mapping)) + self._restrict_request_spec_to_cell( + context, instance, request_spec) request_spec.ensure_project_and_user_id(instance) request_spec.ensure_network_metadata(instance) @@ -1053,6 +1067,8 @@ class ComputeTaskManager(base.Base): self._validate_image_traits_for_rebuild(context, instance, image_ref) + self._restrict_request_spec_to_cell( + context, instance, request_spec) request_spec.ensure_project_and_user_id(instance) request_spec.ensure_network_metadata(instance) compute_utils.heal_reqspec_is_bfv( diff --git a/nova/tests/functional/notification_sample_tests/test_instance.py b/nova/tests/functional/notification_sample_tests/test_instance.py index 100fd64137..76a2770c52 100644 --- a/nova/tests/functional/notification_sample_tests/test_instance.py +++ b/nova/tests/functional/notification_sample_tests/test_instance.py @@ -1258,7 +1258,8 @@ class TestInstanceNotificationSample( 'image.tags': [], 'scheduler_hints': {'_nova_check_type': ['rebuild']}, 'force_hosts': 'compute', - 'force_nodes': 'fake-mini'}) + 'force_nodes': 'fake-mini', + 'requested_destination': self._build_destination_payload()}) # 0. instance.rebuild_scheduled # 1. instance.exists @@ -1354,7 +1355,8 @@ class TestInstanceNotificationSample( 'image.tags': [], 'scheduler_hints': {'_nova_check_type': ['rebuild']}, 'force_hosts': 'compute', - 'force_nodes': 'fake-mini'}) + 'force_nodes': 'fake-mini', + 'requested_destination': self._build_destination_payload()}) # 0. instance.rebuild_scheduled # 1. instance.exists diff --git a/nova/tests/functional/regressions/test_bug_1823370.py b/nova/tests/functional/regressions/test_bug_1823370.py index 08c8b0513a..b865afbe54 100644 --- a/nova/tests/functional/regressions/test_bug_1823370.py +++ b/nova/tests/functional/regressions/test_bug_1823370.py @@ -83,8 +83,4 @@ class MultiCellEvacuateTestCase(integrated_helpers._IntegratedTestBase, self.api.post_server_action(server['id'], req) self._wait_for_migration_status(server, ['done']) server = self._wait_for_state_change(self.api, server, 'ACTIVE') - # FIXME(mriedem): This is bug 1823370 where conductor does not restrict - # the RequestSpec to the origin cell before calling the scheduler to - # pick a new host so the host (host2) in the other cell (cell2) is - # incorrectly picked. - self.assertEqual('host2', server['OS-EXT-SRV-ATTR:host']) + self.assertEqual('host3', server['OS-EXT-SRV-ATTR:host']) diff --git a/nova/tests/unit/conductor/test_conductor.py b/nova/tests/unit/conductor/test_conductor.py index 31bbb5d8fd..870bc82078 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/nova/tests/unit/conductor/test_conductor.py @@ -1687,6 +1687,10 @@ class _BaseTaskTestCase(object): reset_fd.assert_called_once_with() # The RequestSpec.ignore_hosts field should be overwritten. self.assertEqual([inst_obj.host], fake_spec.ignore_hosts) + # The RequestSpec.requested_destination.cell field should be set. + self.assertIn('requested_destination', fake_spec) + self.assertIn('cell', fake_spec.requested_destination) + self.assertIsNotNone(fake_spec.requested_destination.cell) select_dest_mock.assert_called_once_with(self.context, fake_spec, [inst_obj.uuid], return_objects=True, return_alternates=False)