From 418fc93a10fe18de27c75b522a6afdc15e1c49f2 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Wed, 10 Oct 2018 17:37:38 -0400 Subject: [PATCH] Skip _remove_deleted_instances_allocations if compute is new If this is the first start of the compute service and the compute node record does not exist, the resource provider won't exist either. So when the ResourceTracker._remove_deleted_instances_allocations method is called it's going to log an ERROR because get_allocations_for_resource_provider will raise an error since the resource provider doesn't yet exist (that happens later during RT._update() on the new compute node record). We can avoid calling _remove_deleted_instances_allocations if we know the compute node is newly created, so this adds handling for that case. Tests are updated and an unnecessary mock is removed along the way. Change-Id: I37e8ad5b14262d801702411c2c87e73550adda70 Closes-Bug: #1789998 --- nova/compute/resource_tracker.py | 21 ++++--- .../unit/compute/test_resource_tracker.py | 59 +++++++++++++++---- 2 files changed, 63 insertions(+), 17 deletions(-) diff --git a/nova/compute/resource_tracker.py b/nova/compute/resource_tracker.py index 2cf6a48cdd9..c3ee7827633 100644 --- a/nova/compute/resource_tracker.py +++ b/nova/compute/resource_tracker.py @@ -554,6 +554,8 @@ def _init_compute_node(self, context, resources): :param context: security context :param resources: initial values + :returns: True if a new compute_nodes table record was created, + False otherwise """ nodename = resources['hypervisor_hostname'] @@ -563,7 +565,7 @@ def _init_compute_node(self, context, resources): cn = self.compute_nodes[nodename] self._copy_resources(cn, resources) self._setup_pci_tracker(context, cn, resources) - return + return False # now try to get the compute node record from the # database. If we get one we use resources to initialize @@ -572,10 +574,10 @@ def _init_compute_node(self, context, resources): self.compute_nodes[nodename] = cn self._copy_resources(cn, resources) self._setup_pci_tracker(context, cn, resources) - return + return False if self._check_for_nodes_rebalance(context, resources, nodename): - return + return False # there was no local copy and none in the database # so we need to create a new compute node. This needs @@ -590,6 +592,7 @@ def _init_compute_node(self, context, resources): {'host': self.host, 'node': nodename, 'uuid': cn.uuid}) self._setup_pci_tracker(context, cn, resources) + return True def _setup_pci_tracker(self, context, compute_node, resources): if not self.pci_tracker: @@ -745,7 +748,7 @@ def _update_available_resource(self, context, resources, startup=False): # initialize the compute node object, creating it # if it does not already exist. - self._init_compute_node(context, resources) + is_new_compute_node = self._init_compute_node(context, resources) nodename = resources['hypervisor_hostname'] @@ -772,9 +775,13 @@ def _update_available_resource(self, context, resources, startup=False): self._pair_instances_to_migrations(migrations, instance_by_uuid) self._update_usage_from_migrations(context, migrations, nodename) - self._remove_deleted_instances_allocations( - context, self.compute_nodes[nodename], migrations, - instance_by_uuid) + # A new compute node means there won't be a resource provider yet since + # that would be created via the _update() call below, and if there is + # no resource provider then there are no allocations against it. + if not is_new_compute_node: + self._remove_deleted_instances_allocations( + context, self.compute_nodes[nodename], migrations, + instance_by_uuid) # Detect and account for orphaned instances that may exist on the # hypervisor, but are not in the DB: diff --git a/nova/tests/unit/compute/test_resource_tracker.py b/nova/tests/unit/compute/test_resource_tracker.py index 84bbd8801f9..3bcb813e37e 100644 --- a/nova/tests/unit/compute/test_resource_tracker.py +++ b/nova/tests/unit/compute/test_resource_tracker.py @@ -585,11 +585,14 @@ def test_no_instances_no_migrations_no_reserved(self, get_mock, migr_mock, @mock.patch('nova.objects.ComputeNode.get_by_host_and_nodename') @mock.patch('nova.objects.MigrationList.get_in_progress_by_host_and_node') @mock.patch('nova.objects.InstanceList.get_by_host_and_node') - def test_startup_makes_it_through(self, get_mock, migr_mock, get_cn_mock, - pci_mock, instance_pci_mock): + @mock.patch('nova.compute.resource_tracker.ResourceTracker.' + '_remove_deleted_instances_allocations') + def test_startup_makes_it_through(self, rdia, get_mock, migr_mock, + get_cn_mock, pci_mock, + instance_pci_mock): """Just make sure the startup kwarg makes it from _update_available_resource all the way down the call stack to - _update. + _update. In this case a compute node record already exists. """ self._setup_rt() @@ -599,6 +602,40 @@ def test_startup_makes_it_through(self, get_mock, migr_mock, get_cn_mock, update_mock = self._update_available_resources(startup=True) update_mock.assert_called_once_with(mock.ANY, mock.ANY, startup=True) + rdia.assert_called_once_with( + mock.ANY, get_cn_mock.return_value, + [], {}) + + @mock.patch('nova.objects.InstancePCIRequests.get_by_instance', + return_value=objects.InstancePCIRequests(requests=[])) + @mock.patch('nova.objects.PciDeviceList.get_by_compute_node', + return_value=objects.PciDeviceList()) + @mock.patch('nova.compute.resource_tracker.ResourceTracker.' + '_init_compute_node', return_value=True) + @mock.patch('nova.objects.MigrationList.get_in_progress_by_host_and_node') + @mock.patch('nova.objects.InstanceList.get_by_host_and_node') + @mock.patch('nova.compute.resource_tracker.ResourceTracker.' + '_remove_deleted_instances_allocations') + def test_startup_new_compute(self, rdia, get_mock, migr_mock, init_cn_mock, + pci_mock, instance_pci_mock): + """Just make sure the startup kwarg makes it from + _update_available_resource all the way down the call stack to + _update. In this case a new compute node record is created. + """ + self._setup_rt() + cn = _COMPUTE_NODE_FIXTURES[0] + self.rt.compute_nodes[cn.hypervisor_hostname] = cn + mock_pci_tracker = mock.MagicMock() + mock_pci_tracker.stats.to_device_pools_obj.return_value = ( + objects.PciDevicePoolList()) + self.rt.pci_tracker = mock_pci_tracker + + get_mock.return_value = [] + migr_mock.return_value = [] + + update_mock = self._update_available_resources(startup=True) + update_mock.assert_called_once_with(mock.ANY, mock.ANY, startup=True) + rdia.assert_not_called() @mock.patch('nova.objects.InstancePCIRequests.get_by_instance', return_value=objects.InstancePCIRequests(requests=[])) @@ -1025,7 +1062,8 @@ def test_no_op_init_compute_node(self, update_mock, get_mock, service_mock, compute_node = copy.deepcopy(_COMPUTE_NODE_FIXTURES[0]) self.rt.compute_nodes[_NODENAME] = compute_node - self.rt._init_compute_node(mock.sentinel.ctx, resources) + self.assertFalse( + self.rt._init_compute_node(mock.sentinel.ctx, resources)) self.assertFalse(service_mock.called) self.assertFalse(get_mock.called) @@ -1050,7 +1088,8 @@ def fake_get_node(_ctx, host, node): get_mock.side_effect = fake_get_node resources = copy.deepcopy(_VIRT_DRIVER_AVAIL_RESOURCES) - self.rt._init_compute_node(mock.sentinel.ctx, resources) + self.assertFalse( + self.rt._init_compute_node(mock.sentinel.ctx, resources)) get_mock.assert_called_once_with(mock.sentinel.ctx, _HOSTNAME, _NODENAME) @@ -1078,7 +1117,8 @@ def fake_get_all(_ctx, nodename): get_by_hypervisor_mock.side_effect = fake_get_all resources = copy.deepcopy(_VIRT_DRIVER_AVAIL_RESOURCES) - self.rt._init_compute_node(mock.sentinel.ctx, resources) + self.assertFalse( + self.rt._init_compute_node(mock.sentinel.ctx, resources)) get_mock.assert_called_once_with(mock.sentinel.ctx, _HOSTNAME, _NODENAME) @@ -1206,7 +1246,8 @@ def set_cn_id(): cn.uuid = uuids.compute_node_uuid create_mock.side_effect = set_cn_id - self.rt._init_compute_node(mock.sentinel.ctx, resources) + self.assertTrue( + self.rt._init_compute_node(mock.sentinel.ctx, resources)) cn = self.rt.compute_nodes[_NODENAME] get_mock.assert_called_once_with(mock.sentinel.ctx, _HOSTNAME, @@ -3091,12 +3132,10 @@ def test_update_usage_from_instances_goes_negative(self): cn = objects.ComputeNode(memory_mb=1024, local_gb=10) self.rt.compute_nodes['foo'] = cn - @mock.patch.object(self.rt, - '_remove_deleted_instances_allocations') @mock.patch.object(self.rt, '_update_usage_from_instance') @mock.patch('nova.objects.Service.get_minimum_version', return_value=22) - def test(version_mock, uufi, rdia): + def test(version_mock, uufi): self.rt._update_usage_from_instances('ctxt', [], 'foo') test()