Delete resource providers for all nodes when deleting compute service

Change I7b8622b178d5043ed1556d7bdceaf60f47e5ac80 started deleting the
compute node resource provider associated with a compute node when
deleting a nova-compute service. However, it would only delete the
first compute node associated with the service which means for an
ironic compute service that is managing multiple nodes, the resource
providers were not cleaned up in placement. This fixes the issue by
iterating all the compute nodes and cleaning up their providers.
Note this could be potentially a lot of nodes, but we don't really
have many good options here but to iterate them and clean them up
one at a time.

Note that this is best-effort but because of how the
SchedulerReportClient.delete_resource_provider method ignores
ResourceProviderInUse errors, and we could have stale allocations
on the host for which delete_resource_provider is not accounting,
namely allocations from evacuated instances (or incomplete migrations
though you can't migrate baremetal instances today), we could still
delete the compute service and orphan those in-use providers. That,
however, is no worse than before this change where we did not try
to cleanup all providers. The issue described above is being tracked
with bug 1829479 and will be dealt with separately.

Change-Id: I9e852e25ea89f32bf19cdaeb1f5dac8f749f5dbc
Closes-Bug: #1811726
(cherry picked from commit 650fe11)

Author: mriedem

nova/api/openstack/compute/services.py

@@ -266,9 +266,14 @@ def delete(self, req, id):
                                                                   ag.id,
                                                                   service.host)
                 # remove the corresponding resource provider record from
-                # placement for this compute node
-                self.placementclient.delete_resource_provider(
-                    context, service.compute_node, cascade=True)
+                # placement for the compute nodes managed by this service;
+                # remember that an ironic compute service can manage multiple
+                # nodes
+                compute_nodes = objects.ComputeNodeList.get_all_by_host(
+                    context, service.host)
+                for compute_node in compute_nodes:
+                    self.placementclient.delete_resource_provider(
+                        context, compute_node, cascade=True)
                 # remove the host_mapping of this host.
                 try:
                     hm = objects.HostMapping.get_by_host(context, service.host)

nova/tests/unit/api/openstack/compute/test_services.py

@@ -713,25 +713,33 @@ def test_compute_service_delete_host_mapping_not_found(
         """Tests that we are still able to successfully delete a nova-compute
         service even if the HostMapping is not found.
         """
+        @mock.patch('nova.objects.ComputeNodeList.get_all_by_host',
+                    return_value=objects.ComputeNodeList(objects=[
+                        objects.ComputeNode(host='host1',
+                                            hypervisor_hostname='node1'),
+                        objects.ComputeNode(host='host1',
+                                            hypervisor_hostname='node2')]))
         @mock.patch.object(self.controller.host_api, 'service_get_by_id',
                            return_value=objects.Service(
-                               host='host1', binary='nova-compute',
-                               compute_node=objects.ComputeNode()))
+                               host='host1', binary='nova-compute'))
         @mock.patch.object(self.controller.aggregate_api,
                            'get_aggregates_by_host',
                            return_value=objects.AggregateList())
         @mock.patch.object(self.controller.placementclient,
                            'delete_resource_provider')
         def _test(delete_resource_provider,
-                  get_aggregates_by_host, service_get_by_id):
+                  get_aggregates_by_host, service_get_by_id,
+                  cn_get_all_by_host):
             self.controller.delete(self.req, 2)
             ctxt = self.req.environ['nova.context']
             service_get_by_id.assert_called_once_with(ctxt, 2)
             get_instances.assert_called_once_with(ctxt, 'host1')
             get_aggregates_by_host.assert_called_once_with(ctxt, 'host1')
-            delete_resource_provider.assert_called_once_with(
-                ctxt, service_get_by_id.return_value.compute_node,
-                cascade=True)
+            self.assertEqual(2, delete_resource_provider.call_count)
+            nodes = cn_get_all_by_host.return_value
+            delete_resource_provider.assert_has_calls([
+                mock.call(ctxt, node, cascade=True) for node in nodes
+            ], any_order=True)
             get_hm.assert_called_once_with(ctxt, 'host1')
             service_delete.assert_called_once_with()
         _test()

releasenotes/notes/bug-1811726-multi-node-delete-2ba17f02c6171fbb.yaml

@@ -0,0 +1,10 @@
+---
+fixes:
+  - |
+    `Bug 1811726`_ is fixed by deleting the resource provider (in placement)
+    associated with each compute node record managed by a ``nova-compute``
+    service when that service is deleted via the
+    ``DELETE /os-services/{service_id}`` API. This is particularly important
+    for compute services managing ironic baremetal nodes.
+
+    .. _Bug 1811726: https://bugs.launchpad.net/nova/+bug/1811726