From 9fe847bdca39627b4d1741d2c5807ebca7101d2e Mon Sep 17 00:00:00 2001 From: Surya Seetharaman Date: Wed, 21 Mar 2018 14:16:24 +0100 Subject: [PATCH] Cleanup RP and HM records while deleting a compute service. Currently when deleting a nova-compute service via the API, we will (soft) delete the service and compute_node records in the DB, but the placement resource provider and host mapping records will be orphaned. This patch deletes the resource provider and host_mapping records before deleting the service/compute node. Change-Id: I7b8622b178d5043ed1556d7bdceaf60f47e5ac80 Closes-Bug: #1756179 (cherry picked from commit 589c495c1ae62129e20ab5e2641e330541eee01f) (cherry picked from commit dede2de2bd482d0378a7acd81b65d93b1635e825) --- nova/api/openstack/compute/services.py | 9 +++++++ nova/tests/functional/api/client.py | 12 +++++++++- .../api_sample_tests/test_services.py | 24 +++++++++++++++++++ nova/tests/functional/wsgi/test_services.py | 17 ++++++------- nova/tests/unit/compute/test_host_api.py | 9 ++++++- 5 files changed, 61 insertions(+), 10 deletions(-) diff --git a/nova/api/openstack/compute/services.py b/nova/api/openstack/compute/services.py index 23de1670cd39..ac4c8d52e763 100644 --- a/nova/api/openstack/compute/services.py +++ b/nova/api/openstack/compute/services.py @@ -27,6 +27,7 @@ from nova import exception from nova.i18n import _ from nova import objects from nova.policies import services as services_policies +from nova.scheduler.client import report from nova import servicegroup from nova import utils @@ -42,6 +43,7 @@ class ServiceController(wsgi.Controller): self.actions = {"enable": self._enable, "disable": self._disable, "disable-log-reason": self._disable_log_reason} + self.placementclient = report.SchedulerReportClient() def _get_services(self, req): # The API services are filtered out since they are not RPC services @@ -234,6 +236,13 @@ class ServiceController(wsgi.Controller): self.aggregate_api.remove_host_from_aggregate(context, 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) + # remove the host_mapping of this host. + hm = objects.HostMapping.get_by_host(context, service.host) + hm.destroy() self.host_api.service_delete(context, id) except exception.ServiceNotFound: diff --git a/nova/tests/functional/api/client.py b/nova/tests/functional/api/client.py index 17656a20fc16..3b8510574593 100644 --- a/nova/tests/functional/api/client.py +++ b/nova/tests/functional/api/client.py @@ -56,7 +56,17 @@ class APIResponse(object): self.status = response.status_code self.content = response.content if self.content: - self.body = jsonutils.loads(self.content) + # The Compute API and Placement API handle error responses a bit + # differently so we need to check the content-type header to + # figure out what to do. + content_type = response.headers.get('content-type') + if 'application/json' in content_type: + self.body = response.json() + elif 'text/html' in content_type: + self.body = response.text + else: + raise ValueError('Unexpected response content-type: %s' % + content_type) self.headers = response.headers def __str__(self): diff --git a/nova/tests/functional/api_sample_tests/test_services.py b/nova/tests/functional/api_sample_tests/test_services.py index 0b92b52ea0fd..29e129e22fa0 100644 --- a/nova/tests/functional/api_sample_tests/test_services.py +++ b/nova/tests/functional/api_sample_tests/test_services.py @@ -18,6 +18,8 @@ from oslo_utils import fixture as utils_fixture from nova import exception from nova.tests.functional.api_sample_tests import api_sample_base from nova.tests.unit.api.openstack.compute import test_services +from nova.tests.unit.objects import test_compute_node +from nova.tests.unit.objects import test_host_mapping class ServicesJsonTest(api_sample_base.ApiSampleTestBaseV21): @@ -119,7 +121,29 @@ class ServicesV253JsonTest(ServicesV211JsonTest): if svc['uuid'] == service_uuid: return svc raise exception.ServiceNotFound(service_id=service_uuid) + + def fake_cn_get_all_by_host(context, host): + cn = test_compute_node.fake_compute_node + cn['uuid'] = test_services.FAKE_UUID_COMPUTE_HOST1 + cn['host'] = host + return [cn] + + def fake_hm_get_by_host(context, host): + hm = test_host_mapping.get_db_mapping() + hm['host'] = host + return hm + + def fake_hm_destroy(context, host): + return 1 + self.stub_out('nova.db.service_get_by_uuid', db_service_get_by_uuid) + self.stub_out('nova.db.compute_node_get_all_by_host', + fake_cn_get_all_by_host) + self.stub_out( + 'nova.objects.host_mapping.HostMapping._get_by_host_from_db', + fake_hm_get_by_host) + self.stub_out('nova.objects.host_mapping.HostMapping._destroy_in_db', + fake_hm_destroy) def test_service_enable(self): """Enable an existing service.""" diff --git a/nova/tests/functional/wsgi/test_services.py b/nova/tests/functional/wsgi/test_services.py index eb3902fa468c..7584749cfca1 100644 --- a/nova/tests/functional/wsgi/test_services.py +++ b/nova/tests/functional/wsgi/test_services.py @@ -13,6 +13,7 @@ import six from nova import context as nova_context +from nova import exception from nova import objects from nova.objects import fields as rc_fields from nova.tests.functional.api import client as api_client @@ -105,13 +106,13 @@ class TestServicesAPI(test_servers.ProviderUsageBaseTestCase): 'os-hypervisors?hypervisor_hostname_pattern=%s' % service['host'], check_response_status=[404]) - # FIXME(mriedem): This is bug 1756179 where the host mapping is not - # deleted. Once the bug is fixed, we should be able to change this - # to assert that HostMappingNotFound is raised. - objects.HostMapping.get_by_host(ctxt, service['host']) + # The host mapping should also be gone. + self.assertRaises(exception.HostMappingNotFound, + objects.HostMapping.get_by_host, + ctxt, service['host']) - # FIXME(mriedem): This is bug 1756179 where the compute node resource - # provider is not deleted. Once the bug is fixed, this should result - # in a 404 response. + # And finally, the resource provider should also be gone. The API + # will perform a cascading delete of the resource provider inventory + # and allocation information. resp = self.placement_api.get('/resource_providers/%s' % rp_uuid) - self.assertEqual(200, resp.status) + self.assertEqual(404, resp.status) diff --git a/nova/tests/unit/compute/test_host_api.py b/nova/tests/unit/compute/test_host_api.py index a0fe540c0f67..fbc944d95997 100644 --- a/nova/tests/unit/compute/test_host_api.py +++ b/nova/tests/unit/compute/test_host_api.py @@ -405,12 +405,18 @@ class ComputeHostAPITestCase(test.TestCase): self.assertFalse(service2.destroy.called) self.assertFalse(set_target.called) - def test_service_delete_compute_in_aggregate(self): + @mock.patch.object(objects.ComputeNodeList, 'get_all_by_host') + @mock.patch.object(objects.HostMapping, 'get_by_host') + def test_service_delete_compute_in_aggregate(self, mock_hm, mock_get_cn): compute = self.host_api.db.service_create(self.ctxt, {'host': 'fake-compute-host', 'binary': 'nova-compute', 'topic': 'compute', 'report_count': 0}) + # This is needed because of lazy-loading service.compute_node + cn = objects.ComputeNode(uuid=uuids.cn, host="fake-compute-host", + hypervisor_hostname="fake-compute-host") + mock_get_cn.return_value = [cn] aggregate = self.aggregate_api.create_aggregate(self.ctxt, 'aggregate', None) @@ -421,6 +427,7 @@ class ComputeHostAPITestCase(test.TestCase): result = self.aggregate_api.get_aggregate(self.ctxt, aggregate.id).hosts self.assertEqual([], result) + mock_hm.return_value.destroy.assert_called_once_with() @mock.patch('nova.db.compute_node_statistics') def test_compute_node_statistics(self, mock_cns):