From 1abac25fd2967be00c4d584c06ba15ca4e8d04cc Mon Sep 17 00:00:00 2001 From: Danil Akhmetov Date: Fri, 29 Apr 2016 11:45:28 +0300 Subject: [PATCH] Remove compute host from all host aggregates when compute service is deleted Nova currently does not check if compute host included in host-aggregates when user deletes compute service. It leads to inconsistency in nova host aggregates, impossibility to remove compute host from host-aggregate or remove host aggregate with invalid compute host. Change-Id: I8034da3827e47f3cd575e1f6ddf0e4be2f7dfecd Closes-Bug: #1470341 --- nova/api/openstack/compute/services.py | 11 +++++++ nova/compute/api.py | 8 +++++ .../api/openstack/compute/test_services.py | 16 +++++++--- nova/tests/unit/compute/test_host_api.py | 29 +++++++++++++++++++ 4 files changed, 60 insertions(+), 4 deletions(-) diff --git a/nova/api/openstack/compute/services.py b/nova/api/openstack/compute/services.py index 4fd666bbc9ca..52f4400373d1 100644 --- a/nova/api/openstack/compute/services.py +++ b/nova/api/openstack/compute/services.py @@ -33,6 +33,7 @@ class ServiceController(wsgi.Controller): def __init__(self): self.host_api = compute.HostAPI() + self.aggregate_api = compute.api.AggregateAPI() self.servicegroup_api = servicegroup.API() self.actions = {"enable": self._enable, "disable": self._disable, @@ -178,7 +179,17 @@ class ServiceController(wsgi.Controller): raise webob.exc.HTTPBadRequest(explanation=exc.format_message()) try: + service = self.host_api.service_get_by_id(context, id) + # remove the service from all the aggregates in which it's included + if service.binary == 'nova-compute': + aggrs = self.aggregate_api.get_aggregates_by_host(context, + service.host) + for ag in aggrs: + self.aggregate_api.remove_host_from_aggregate(context, + ag.id, + service.host) self.host_api.service_delete(context, id) + except exception.ServiceNotFound: explanation = _("Service %s not found.") % id raise webob.exc.HTTPNotFound(explanation=explanation) diff --git a/nova/compute/api.py b/nova/compute/api.py index 2b44137f5f8d..13372f954f49 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -3619,6 +3619,10 @@ class HostAPI(base.Base): ret_services.append(service) return ret_services + def service_get_by_id(self, context, service_id): + """Get service entry for the given service id.""" + return objects.Service.get_by_id(context, service_id) + def service_get_by_compute_host(self, context, host_name): """Get service entry for the given compute hostname.""" return objects.Service.get_by_compute_host(context, host_name) @@ -3721,6 +3725,10 @@ class AggregateAPI(base.Base): """Get all the aggregates.""" return objects.AggregateList.get_all(context) + def get_aggregates_by_host(self, context, compute_host): + """Get all the aggregates where the given host is presented.""" + return objects.AggregateList.get_by_host(context, compute_host) + @wrap_exception() def update_aggregate(self, context, aggregate_id, values): """Update the properties of an aggregate.""" diff --git a/nova/tests/unit/api/openstack/compute/test_services.py b/nova/tests/unit/api/openstack/compute/test_services.py index a08461b7bce6..9a18e0aa5245 100644 --- a/nova/tests/unit/api/openstack/compute/test_services.py +++ b/nova/tests/unit/api/openstack/compute/test_services.py @@ -27,7 +27,7 @@ from nova.api.openstack import extensions from nova.api.openstack import wsgi as os_wsgi from nova import availability_zones from nova.cells import utils as cells_utils -from nova.compute import cells_api +from nova import compute from nova import context from nova import exception from nova import objects @@ -197,6 +197,8 @@ class ServicesTestV21(test.TestCase): self.ext_mgr = extensions.ExtensionManager() self.ext_mgr.extensions = {} + self.ctxt = context.get_admin_context() + self.host_api = compute.HostAPI() self._set_up_controller() self.controller.host_api.service_get_all = ( mock.Mock(side_effect=fake_service_get_all(fake_services_list))) @@ -552,11 +554,17 @@ class ServicesTestV21(test.TestCase): def test_services_delete(self): self.ext_mgr.extensions['os-extended-services-delete'] = True + compute = self.host_api.db.service_create(self.ctxt, + {'host': 'fake-compute-host', + 'binary': 'nova-compute', + 'topic': 'compute', + 'report_count': 0}) + with mock.patch.object(self.controller.host_api, 'service_delete') as service_delete: - self.controller.delete(self.req, '1') + self.controller.delete(self.req, compute.id) service_delete.assert_called_once_with( - self.req.environ['nova.context'], '1') + self.req.environ['nova.context'], compute.id) self.assertEqual(self.controller.delete.wsgi_code, 204) def test_services_delete_not_found(self): @@ -863,7 +871,7 @@ class ServicesCellsTestV21(test.TestCase): def setUp(self): super(ServicesCellsTestV21, self).setUp() - host_api = cells_api.HostAPI() + host_api = compute.cells_api.HostAPI() self.ext_mgr = extensions.ExtensionManager() self.ext_mgr.extensions = {} diff --git a/nova/tests/unit/compute/test_host_api.py b/nova/tests/unit/compute/test_host_api.py index b73a3b8621dc..3f20816850b5 100644 --- a/nova/tests/unit/compute/test_host_api.py +++ b/nova/tests/unit/compute/test_host_api.py @@ -19,24 +19,31 @@ import copy import mock from oslo_serialization import jsonutils +from nova.api.openstack.compute import services from nova.cells import utils as cells_utils from nova import compute +from nova.compute import api as compute_api from nova import context from nova import exception from nova import objects from nova import test +from nova.tests.unit.api.openstack import fakes from nova.tests.unit import fake_notifier from nova.tests.unit.objects import test_objects from nova.tests.unit.objects import test_service +import testtools class ComputeHostAPITestCase(test.TestCase): def setUp(self): super(ComputeHostAPITestCase, self).setUp() self.host_api = compute.HostAPI() + self.aggregate_api = compute_api.AggregateAPI() self.ctxt = context.get_admin_context() fake_notifier.stub_notifier(self) self.addCleanup(fake_notifier.reset) + self.req = fakes.HTTPRequest.blank('') + self.controller = services.ServiceController() def _compare_obj(self, obj, db_obj): test_objects.compare_obj(self, obj, db_obj, @@ -305,6 +312,23 @@ class ComputeHostAPITestCase(test.TestCase): get_by_id.assert_called_once_with(self.ctxt, 1) destroy.assert_called_once_with() + def test_service_delete_compute_in_aggregate(self): + compute = self.host_api.db.service_create(self.ctxt, + {'host': 'fake-compute-host', + 'binary': 'nova-compute', + 'topic': 'compute', + 'report_count': 0}) + aggregate = self.aggregate_api.create_aggregate(self.ctxt, + 'aggregate', + None) + self.aggregate_api.add_host_to_aggregate(self.ctxt, + aggregate.id, + 'fake-compute-host') + self.controller.delete(self.req, compute.id) + result = self.aggregate_api.get_aggregate(self.ctxt, + aggregate.id).hosts + self.assertEqual([], result) + class ComputeHostAPICellsTestCase(ComputeHostAPITestCase): def setUp(self): @@ -411,6 +435,11 @@ class ComputeHostAPICellsTestCase(ComputeHostAPITestCase): service_delete.assert_called_once_with( self.ctxt, cell_service_id) + @testtools.skip('cells do not support host aggregates') + def test_service_delete_compute_in_aggregate(self): + # this test is not valid for cell + pass + @mock.patch.object(objects.InstanceList, 'get_by_host') def test_instance_get_all_by_host(self, mock_get): instances = [dict(id=1, cell_name='cell1', host='host1'),