Prevent deletion of a compute node belonging to another host

There is a race condition in nova-compute with the ironic virt driver as
nodes get rebalanced. It can lead to compute nodes being removed in the
DB and not repopulated. Ultimately this prevents these nodes from being
scheduled to.

The main race condition involved is in update_available_resources in
the compute manager. When the list of compute nodes is queried, there is
a compute node belonging to the host that it does not expect to be
managing, i.e. it is an orphan. Between that time and deleting the
orphan, the real owner of the compute node takes ownership of it ( in
the resource tracker). However, the node is still deleted as the first
host is unaware of the ownership change.

This change prevents this from occurring by filtering on the host when
deleting a compute node. If another compute host has taken ownership of
a node, it will have updated the host field and this will prevent
deletion from occurring. The first host sees this has happened via the
ComputeHostNotFound exception, and avoids deleting its resource
provider.

Closes-Bug: #1853009
Related-Bug: #1841481
Change-Id: I260c1fded79a85d4899e94df4d9036a1ee437f02

Author: markgoddard

nova/compute/manager.py

@@ -8051,13 +8051,27 @@ def update_available_resource(self, context, startup=False):
                          "nodes are %(nodes)s",
                          {'id': cn.id, 'hh': cn.hypervisor_hostname,
                           'nodes': nodenames})
-                cn.destroy()
-                rt.remove_node(cn.hypervisor_hostname)
-                # Delete the corresponding resource provider in placement,
-                # along with any associated allocations and inventory.
-                # TODO(cdent): Move use of reportclient into resource tracker.
-                self.scheduler_client.reportclient.delete_resource_provider(
-                    context, cn, cascade=True)
+                try:
+                    cn.destroy()
+                except exception.ComputeHostNotFound:
+                    # NOTE(mgoddard): it's possible that another compute
+                    # service took ownership of this compute node since we
+                    # queried it due to a rebalance, and this will cause the
+                    # deletion to fail. Ignore the error in that case.
+                    LOG.info("Ignoring failure to delete orphan compute node "
+                             "%(id)s on hypervisor host %(hh)s due to "
+                             "possible node rebalance",
+                             {'id': cn.id, 'hh': cn.hypervisor_hostname})
+                    rt.remove_node(cn.hypervisor_hostname)
+                else:
+                    rt.remove_node(cn.hypervisor_hostname)
+                    # Delete the corresponding resource provider in placement,
+                    # along with any associated allocations.
+                    # TODO(cdent): Move use of reportclient into resource
+                    # tracker.
+                    reportclient = self.scheduler_client.reportclient
+                    reportclient.delete_resource_provider(
+                        context, cn, cascade=True)
 
         for nodename in nodenames:
             self._update_available_resource_for_node(context, nodename)

nova/db/api.py

@@ -330,15 +330,16 @@ def compute_node_update(context, compute_id, values):
     return IMPL.compute_node_update(context, compute_id, values)
 
 
-def compute_node_delete(context, compute_id):
+def compute_node_delete(context, compute_id, compute_host):
     """Delete a compute node from the database.
 
     :param context: The security context
     :param compute_id: ID of the compute node
+    :param compute_host: Hostname of the compute service
 
     Raises ComputeHostNotFound if compute node with the given ID doesn't exist.
     """
-    return IMPL.compute_node_delete(context, compute_id)
+    return IMPL.compute_node_delete(context, compute_id, compute_host)
 
 
 def compute_node_statistics(context):

nova/db/sqlalchemy/api.py

@@ -758,10 +758,10 @@ def compute_node_update(context, compute_id, values):
 
 
 @pick_context_manager_writer
-def compute_node_delete(context, compute_id):
+def compute_node_delete(context, compute_id, compute_host):
     """Delete a ComputeNode record."""
     result = model_query(context, models.ComputeNode).\
-             filter_by(id=compute_id).\
+             filter_by(id=compute_id, host=compute_host).\
              soft_delete(synchronize_session=False)
 
     if not result:

nova/objects/compute_node.py

@@ -337,7 +337,7 @@ def save(self, prune_stats=False):
 
     @base.remotable
     def destroy(self):
-        db.compute_node_delete(self._context, self.id)
+        db.compute_node_delete(self._context, self.id, self.host)
 
     def update_from_virt_driver(self, resources):
         # NOTE(pmurray): the virt driver provides a dict of values that

nova/tests/functional/regressions/test_bug_1853009.py

@@ -123,22 +123,32 @@ def test_node_rebalance_deleted_compute_node_race(self):
         # Mock out the compute node query to simulate a race condition where
         # the list includes an orphan compute node that is taken ownership of
         # by host1 by the time host2 deletes it.
-        # FIXME(mgoddard): Ideally host2 would not delete a node that does not
-        # belong to it. See https://bugs.launchpad.net/nova/+bug/1853009.
         with mock.patch('nova.compute.manager.ComputeManager.'
                         '_get_compute_nodes_in_db') as mock_get:
             mock_get.return_value = [cn]
             host2.manager.update_available_resource(ctxt)
 
-        # Verify that the node was deleted.
+        # Verify that the node was almost deleted, but saved by the host check.
         self.assertIn("Deleting orphan compute node %s hypervisor host "
                       "is fake-node, nodes are" % cn.id,
                       self.stdlog.logger.output)
-        hypervisors = self.api.api_get(
-            '/os-hypervisors/detail').body['hypervisors']
-        self.assertEqual(0, len(hypervisors), hypervisors)
+        self.assertIn("Ignoring failure to delete orphan compute node %s on "
+                      "hypervisor host fake-node" % cn.id,
+                      self.stdlog.logger.output)
+        self._assert_hypervisor_api(nodename, 'host1')
         rps = self._get_all_providers()
-        self.assertEqual(0, len(rps), rps)
+        self.assertEqual(1, len(rps), rps)
+        self.assertEqual(nodename, rps[0]['name'])
+
+        # Simulate deletion of an orphan by host2. It shouldn't happen anymore,
+        # but let's assume it already did.
+        cn = objects.ComputeNode.get_by_host_and_nodename(
+            ctxt, 'host1', nodename)
+        cn.destroy()
+        rt2 = host2.manager._get_resource_tracker()
+        rt2.remove_node(cn.hypervisor_hostname)
+        host2.manager.reportclient.delete_resource_provider(
+            ctxt, cn, cascade=True)
 
         # host1[3]: Should recreate compute node and resource provider.
         host1.manager.update_available_resource(ctxt)

nova/tests/unit/compute/test_compute.py

@@ -207,8 +207,10 @@ def fake_get_compute_nodes_in_db(self, context, **kwargs):
                         context, objects.ComputeNode(), cn)
                     for cn in fake_compute_nodes]
 
-        def fake_compute_node_delete(context, compute_node_id):
+        def fake_compute_node_delete(context, compute_node_id,
+                                     compute_node_host):
             self.assertEqual(2, compute_node_id)
+            self.assertEqual('fake_phyp1', compute_node_host)
 
         self.stub_out(
             'nova.compute.manager.ComputeManager._get_compute_nodes_in_db',

nova/tests/unit/compute/test_compute_mgr.py

@@ -342,6 +342,32 @@ def test_update_available_resource_not_ready(self, get_db_nodes,
         update_mock.assert_not_called()
         del_rp_mock.assert_not_called()
 
+    @mock.patch.object(manager.ComputeManager, '_get_resource_tracker')
+    @mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
+                'delete_resource_provider')
+    @mock.patch.object(manager.ComputeManager,
+                       '_update_available_resource_for_node')
+    @mock.patch.object(fake_driver.FakeDriver, 'get_available_nodes')
+    @mock.patch.object(manager.ComputeManager, '_get_compute_nodes_in_db')
+    def test_update_available_resource_destroy_rebalance(
+            self, get_db_nodes, get_avail_nodes, update_mock, del_rp_mock,
+            mock_get_rt):
+        db_nodes = [self._make_compute_node('node1', 1)]
+        get_db_nodes.return_value = db_nodes
+        # Destroy can fail if nodes were rebalanced between getting the node
+        # list and calling destroy.
+        db_nodes[0].destroy.side_effect = exception.ComputeHostNotFound(
+            'node1')
+        get_avail_nodes.return_value = set()
+        self.compute.update_available_resource(self.context)
+        get_db_nodes.assert_called_once_with(self.context, use_slave=True,
+                                             startup=False)
+        self.assertEqual(0, update_mock.call_count)
+
+        db_nodes[0].destroy.assert_called_once_with()
+        self.assertEqual(0, del_rp_mock.call_count)
+        mock_get_rt.return_value.remove_node.assert_called_once_with('node1')
+
     @mock.patch('nova.context.get_admin_context')
     def test_pre_start_hook(self, get_admin_context):
         """Very simple test just to make sure update_available_resource is

nova/tests/unit/db/test_db_api.py

@@ -7034,7 +7034,7 @@ def test_compute_node_get_all_deleted_compute_node(self):
 
             # Now delete the newly-created compute node to ensure the related
             # compute node stats are wiped in a cascaded fashion
-            db.compute_node_delete(self.ctxt, node['id'])
+            db.compute_node_delete(self.ctxt, node['id'], node['host'])
 
             # Clean up the service
             db.service_destroy(self.ctxt, service['id'])
@@ -7186,10 +7186,18 @@ def test_compute_node_update(self):
 
     def test_compute_node_delete(self):
         compute_node_id = self.item['id']
-        db.compute_node_delete(self.ctxt, compute_node_id)
+        compute_node_host = self.item['host']
+        db.compute_node_delete(self.ctxt, compute_node_id, compute_node_host)
         nodes = db.compute_node_get_all(self.ctxt)
         self.assertEqual(len(nodes), 0)
 
+    def test_compute_node_delete_different_host(self):
+        compute_node_id = self.item['id']
+        compute_node_host = 'invalid-host'
+        self.assertRaises(exception.ComputeHostNotFound,
+                          db.compute_node_delete,
+                          self.ctxt, compute_node_id, compute_node_host)
+
     def test_compute_node_search_by_hypervisor(self):
         nodes_created = []
         new_service = copy.copy(self.service_dict)

nova/tests/unit/objects/test_compute_node.py

@@ -388,8 +388,9 @@ def test_set_id_failure(self, mock_get, db_mock):
     def test_destroy(self, mock_delete):
         compute = compute_node.ComputeNode(context=self.context)
         compute.id = 123
+        compute.host = 'fake'
         compute.destroy()
-        mock_delete.assert_called_once_with(self.context, 123)
+        mock_delete.assert_called_once_with(self.context, 123, 'fake')
 
     @mock.patch.object(db, 'compute_node_get_all')
     def test_get_all(self, mock_get_all):

releasenotes/notes/bug-1853009-99414e14d1491b5f.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fixes an issue with multiple ``nova-compute`` services used with Ironic,
+    where a rebalance operation could result in a compute node being deleted
+    from the database and not recreated. See `bug 1853009
+    <https://bugs.launchpad.net/nova/+bug/1853009>`__ for details.