From 8fc789deb7c774bcc5b5128d638c6c7e30bf0a54 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Tue, 22 Aug 2017 16:54:44 -0400 Subject: [PATCH] Restrict live migration to same cell We do not yet support live migrating an instance across cells. This change handles two cases for live migration: 1. The destination host is forced so the scheduler is bypassed. In this case we directly compare the source cell against the destination cell and fail if they are not the same with a MigrationPreCheckError. 2. If no destination host is specified, or it's not forced, we update the RequestSpec sent to the scheduler so it will restrict the compute nodes it pulls from the same cell that the instance lives in. If a host is requested in this case but it's in a different cell, it would result in a NoValidHost error from the scheduler. Change-Id: I66fc72d402ac118270a835cf929fe1ea387d78cd Closes-Bug: #1712008 --- nova/conductor/tasks/live_migrate.py | 55 +++++++++++++++++ .../unit/conductor/tasks/test_live_migrate.py | 59 +++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/nova/conductor/tasks/live_migrate.py b/nova/conductor/tasks/live_migrate.py index 16de56102fa6..e063b01991c8 100644 --- a/nova/conductor/tasks/live_migrate.py +++ b/nova/conductor/tasks/live_migrate.py @@ -117,6 +117,16 @@ class LiveMigrationTask(base.TaskBase): source_node, dest_node = self._check_compatible_with_source_hypervisor( self.destination) self._call_livem_checks_on_host(self.destination) + # Make sure the forced destination host is in the same cell that the + # instance currently lives in. + # NOTE(mriedem): This can go away if/when the forced destination host + # case calls select_destinations. + source_cell_mapping = self._get_source_cell_mapping() + dest_cell_mapping = self._get_destination_cell_mapping() + if source_cell_mapping.uuid != dest_cell_mapping.uuid: + raise exception.MigrationPreCheckError( + reason=(_('Unable to force live migrate instance %s ' + 'across cells.') % self.instance.uuid)) return source_node, dest_node def _claim_resources_on_destination(self, source_node, dest_node): @@ -246,6 +256,36 @@ class LiveMigrationTask(base.TaskBase): "%s") % destination raise exception.MigrationPreCheckError(msg) + def _get_source_cell_mapping(self): + """Returns the CellMapping for the cell in which the instance lives + + :returns: nova.objects.CellMapping record for the cell where + the instance currently lives. + :raises: MigrationPreCheckError - in case a mapping is not found + """ + try: + return objects.InstanceMapping.get_by_instance_uuid( + self.context, self.instance.uuid).cell_mapping + except exception.InstanceMappingNotFound: + raise exception.MigrationPreCheckError( + reason=(_('Unable to determine in which cell ' + 'instance %s lives.') % self.instance.uuid)) + + def _get_destination_cell_mapping(self): + """Returns the CellMapping for the destination host + + :returns: nova.objects.CellMapping record for the cell where + the destination host is mapped. + :raises: MigrationPreCheckError - in case a mapping is not found + """ + try: + return objects.HostMapping.get_by_host( + self.context, self.destination).cell_mapping + except exception.HostMappingNotFound: + raise exception.MigrationPreCheckError( + reason=(_('Unable to determine in which cell ' + 'destination host %s lives.') % self.destination)) + def _find_destination(self): # TODO(johngarbutt) this retry loop should be shared attempted_hosts = [self.source] @@ -269,6 +309,21 @@ class LiveMigrationTask(base.TaskBase): # is not forced to be the original host request_spec.reset_forced_destinations() scheduler_utils.setup_instance_group(self.context, request_spec) + + # We currently only support live migrating to hosts in the same + # cell that the instance lives in, so we need to tell the scheduler + # to limit the applicable hosts based on cell. + cell_mapping = self._get_source_cell_mapping() + LOG.debug('Requesting cell %(cell)s while live migrating', + {'cell': cell_mapping.identity}, + instance=self.instance) + if ('requested_destination' in request_spec and + request_spec.requested_destination): + request_spec.requested_destination.cell = cell_mapping + else: + request_spec.requested_destination = objects.Destination( + cell=cell_mapping) + host = None while host is None: self._check_not_over_max_retries(attempted_hosts) diff --git a/nova/tests/unit/conductor/tasks/test_live_migrate.py b/nova/tests/unit/conductor/tasks/test_live_migrate.py index 4c633c59054f..29a2ba407d19 100644 --- a/nova/tests/unit/conductor/tasks/test_live_migrate.py +++ b/nova/tests/unit/conductor/tasks/test_live_migrate.py @@ -12,6 +12,7 @@ import mock import oslo_messaging as messaging +import six from nova.compute import power_state from nova.compute import rpcapi as compute_rpcapi @@ -246,6 +247,33 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): mock.call(self.destination)], mock_get_info.call_args_list) + @mock.patch.object(objects.Service, 'get_by_compute_host') + @mock.patch.object(live_migrate.LiveMigrationTask, '_get_compute_info') + @mock.patch.object(servicegroup.API, 'service_is_up') + @mock.patch.object(compute_rpcapi.ComputeAPI, + 'check_can_live_migrate_destination') + @mock.patch.object(objects.HostMapping, 'get_by_host', + return_value=objects.HostMapping( + cell_mapping=objects.CellMapping( + uuid=uuids.different))) + def test_check_requested_destination_fails_different_cells( + self, mock_get_host_mapping, mock_check, mock_is_up, + mock_get_info, mock_get_host): + mock_get_host.return_value = "service" + mock_is_up.return_value = True + hypervisor_details = objects.ComputeNode( + hypervisor_type="a", + hypervisor_version=6.1, + free_ram_mb=513, + memory_mb=512, + ram_allocation_ratio=1.0) + mock_get_info.return_value = hypervisor_details + mock_check.return_value = "migrate_data" + + ex = self.assertRaises(exception.MigrationPreCheckError, + self.task._check_requested_destination) + self.assertIn('across cells', six.text_type(ex)) + def test_find_destination_works(self): self.mox.StubOutWithMock(utils, 'get_image_from_system_metadata') self.mox.StubOutWithMock(scheduler_utils, 'setup_instance_group') @@ -271,6 +299,10 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): self.mox.ReplayAll() self.assertEqual("host1", self.task._find_destination()) + # Make sure the request_spec was updated to include the cell + # mapping. + self.assertIsNotNone(self.fake_spec.requested_destination.cell) + def test_find_destination_works_with_no_request_spec(self): task = live_migrate.LiveMigrationTask( self.context, self.instance, self.destination, @@ -300,6 +332,9 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): setup_ig.assert_called_once_with(self.context, another_spec) select_dest.assert_called_once_with(self.context, another_spec, [self.instance.uuid]) + # Make sure the request_spec was updated to include the cell + # mapping. + self.assertIsNotNone(another_spec.requested_destination.cell) check_compat.assert_called_once_with("host1") call_livem_checks.assert_called_once_with("host1") do_test() @@ -577,3 +612,27 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): self.instance.project_id, self.instance.user_id) test() + + @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid', + side_effect=exception.InstanceMappingNotFound( + uuid=uuids.instance)) + def test_get_source_cell_mapping_not_found(self, mock_get): + """Negative test where InstanceMappingNotFound is raised and converted + to MigrationPreCheckError. + """ + self.assertRaises(exception.MigrationPreCheckError, + self.task._get_source_cell_mapping) + mock_get.assert_called_once_with( + self.task.context, self.task.instance.uuid) + + @mock.patch.object(objects.HostMapping, 'get_by_host', + side_effect=exception.HostMappingNotFound( + name='destination')) + def test_get_destination_cell_mapping_not_found(self, mock_get): + """Negative test where HostMappingNotFound is raised and converted + to MigrationPreCheckError. + """ + self.assertRaises(exception.MigrationPreCheckError, + self.task._get_destination_cell_mapping) + mock_get.assert_called_once_with( + self.task.context, self.task.destination)