From 29f22b3b5156e0edb269a7792c11226e66223e66 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Sun, 10 Nov 2019 09:42:50 -0500 Subject: [PATCH] Resolve TODO in _remove_host_allocations The Selection object returned by select_destinations has a compute node UUID in it so we don't have to look up the compute node object by host/nodename when reverting allocations during live migration. Change-Id: I0156cda8543f847d50e16683e1eb29fbdd556d27 --- nova/conductor/tasks/live_migrate.py | 25 ++++------------- .../unit/conductor/tasks/test_live_migrate.py | 27 +++++++++---------- 2 files changed, 17 insertions(+), 35 deletions(-) diff --git a/nova/conductor/tasks/live_migrate.py b/nova/conductor/tasks/live_migrate.py index 2a6d6e77c2fe..46cdd31b24c7 100644 --- a/nova/conductor/tasks/live_migrate.py +++ b/nova/conductor/tasks/live_migrate.py @@ -489,38 +489,23 @@ class LiveMigrationTask(base.TaskBase): # The scheduler would have created allocations against the # selected destination host in Placement, so we need to remove # those before moving on. - self._remove_host_allocations(host, selection.nodename) + self._remove_host_allocations(selection.compute_node_uuid) host = None # TODO(artom) We should probably just return the whole selection object # at this point. return (selection.service_host, selection.nodename, selection.limits) - def _remove_host_allocations(self, host, node): - """Removes instance allocations against the given host from Placement + def _remove_host_allocations(self, compute_node_uuid): + """Removes instance allocations against the given node from Placement - :param host: The name of the host. - :param node: The name of the node. + :param compute_node_uuid: UUID of ComputeNode resource provider """ - # Get the compute node object since we need the UUID. - # TODO(mriedem): If the result of select_destinations eventually - # returns the compute node uuid, we wouldn't need to look it - # up via host/node and we can save some time. - try: - compute_node = objects.ComputeNode.get_by_host_and_nodename( - self.context, host, node) - except exception.ComputeHostNotFound: - # This shouldn't happen, but we're being careful. - LOG.info('Unable to remove instance allocations from host %s ' - 'and node %s since it was not found.', host, node, - instance=self.instance) - return - # Now remove the allocations for our instance against that node. # Note that this does not remove allocations against any other node # or shared resource provider, it's just undoing what the scheduler # allocated for the given (destination) node. self.report_client.remove_provider_tree_from_instance_allocation( - self.context, self.instance.uuid, compute_node.uuid) + self.context, self.instance.uuid, compute_node_uuid) def _check_not_over_max_retries(self, attempted_hosts): if CONF.migrate_max_retries == -1: diff --git a/nova/tests/unit/conductor/tasks/test_live_migrate.py b/nova/tests/unit/conductor/tasks/test_live_migrate.py index 6a77da616ec3..b1cf177bea56 100644 --- a/nova/tests/unit/conductor/tasks/test_live_migrate.py +++ b/nova/tests/unit/conductor/tasks/test_live_migrate.py @@ -33,10 +33,12 @@ from nova.tests.unit import fake_instance fake_limits1 = objects.SchedulerLimits() fake_selection1 = objects.Selection(service_host="host1", nodename="node1", - cell_uuid=uuids.cell, limits=fake_limits1) + cell_uuid=uuids.cell, limits=fake_limits1, + compute_node_uuid=uuids.compute_node1) fake_limits2 = objects.SchedulerLimits() fake_selection2 = objects.Selection(service_host="host2", nodename="node2", - cell_uuid=uuids.cell, limits=fake_limits2) + cell_uuid=uuids.cell, limits=fake_limits2, + compute_node_uuid=uuids.compute_node2) class LiveMigrationTaskTestCase(test.NoDBTestCase): @@ -476,7 +478,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): self.assertEqual(("host2", "node2", fake_limits2), self.task._find_destination()) # Should have removed allocations for the first host. - mock_remove.assert_called_once_with('host1', 'node1') + mock_remove.assert_called_once_with(fake_selection1.compute_node_uuid) mock_setup.assert_called_once_with(self.context, self.fake_spec) mock_select.assert_has_calls([ mock.call(self.context, self.fake_spec, [self.instance.uuid], @@ -511,7 +513,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): self.assertEqual(("host2", "node2", fake_limits2), self.task._find_destination()) # Should have removed allocations for the first host. - mock_remove.assert_called_once_with('host1', 'node1') + mock_remove.assert_called_once_with(fake_selection1.compute_node_uuid) mock_setup.assert_called_once_with(self.context, self.fake_spec) mock_select.assert_has_calls([ mock.call(self.context, self.fake_spec, [self.instance.uuid], @@ -539,7 +541,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): self.assertEqual(("host2", "node2", fake_limits2), self.task._find_destination()) # Should have removed allocations for the first host. - mock_remove.assert_called_once_with('host1', 'node1') + mock_remove.assert_called_once_with(fake_selection1.compute_node_uuid) mock_setup.assert_called_once_with(self.context, self.fake_spec) mock_select.assert_has_calls([ mock.call(self.context, self.fake_spec, [self.instance.uuid], @@ -567,7 +569,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): self.assertEqual('failed', self.task.migration.status) mock_save.assert_called_once_with() # Should have removed allocations for the first host. - mock_remove.assert_called_once_with('host1', 'node1') + mock_remove.assert_called_once_with(fake_selection1.compute_node_uuid) mock_setup.assert_called_once_with(self.context, self.fake_spec) mock_select.assert_called_once_with( self.context, self.fake_spec, [self.instance.uuid], @@ -673,17 +675,12 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): mock_get.assert_called_once_with( self.task.context, self.task.destination) - @mock.patch.object(objects.ComputeNode, 'get_by_host_and_nodename', - side_effect=exception.ComputeHostNotFound(host='host')) @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'remove_provider_tree_from_instance_allocation') - def test_remove_host_allocations_compute_host_not_found( - self, remove_provider, get_cn): - """Tests that failing to find a ComputeNode will not blow up - the _remove_host_allocations method. - """ - self.task._remove_host_allocations('host', 'node') - remove_provider.assert_not_called() + def test_remove_host_allocations(self, remove_provider): + self.task._remove_host_allocations(uuids.cn) + remove_provider.assert_called_once_with( + self.task.context, self.task.instance.uuid, uuids.cn) def test_check_can_migrate_pci(self): """Tests that _check_can_migrate_pci() allows live-migration if