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

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

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."""

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 = {}

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'),