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

@@ -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:

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()