From 62878ef5a6c9f6a2c727c91397830387425c4d37 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Thu, 27 Apr 2017 07:55:40 -0700 Subject: [PATCH] re-Allow adding computes with no ComputeNodes to aggregates After the cellification of the Aggregates API, we introduced a requirement that the service must have a HostMapping record, so that we know which cell the service is in. This is normally fine, but for weird drivers such as ironic, there may be valid cases (i.e. during setup) where no ironic nodes are present, thus one or more services may not have any compute node records, and thus cannot be added to aggregates. This adds a cell scan, only if necessary, to find the desired service so that the operation may proceed as it did before. To do this, we refactor the _find_service() helper to a more generic utility and use that if we don't find a HostMapping during the add operation. Change-Id: Idc97126d63684e7d638b974d7226ff210c744404 Closes-Bug: #1686744 --- nova/compute/api.py | 130 ++++++++++++-------- nova/tests/functional/api/client.py | 7 ++ nova/tests/functional/test_aggregates.py | 41 ++++++ nova/tests/unit/compute/test_compute.py | 4 +- nova/tests/unit/compute/test_compute_api.py | 4 + 5 files changed, 131 insertions(+), 55 deletions(-) create mode 100644 nova/tests/functional/test_aggregates.py diff --git a/nova/compute/api.py b/nova/compute/api.py index c12579c4df20..7a27ae352d8e 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -4304,6 +4304,65 @@ def target_host_cell(fn): return targeted +def _find_service_in_cell(context, service_id=None, service_host=None): + """Find a service by id or hostname by searching all cells. + + If one matching service is found, return it. If none or multiple + are found, raise an exception. + + :param context: A context.RequestContext + :param service_id: If not none, the DB ID of the service to find + :param service_host: If not None, the hostname of the service to find + :returns: An objects.Service + :raises: ServiceNotUnique if multiple matching IDs are found + :raises: NotFound if no matches are found + :raises: NovaException if called with neither search option + """ + + load_cells() + service = None + found_in_cell = None + + is_uuid = False + if service_id is not None: + is_uuid = uuidutils.is_uuid_like(service_id) + if is_uuid: + lookup_fn = lambda c: objects.Service.get_by_uuid(c, service_id) + else: + lookup_fn = lambda c: objects.Service.get_by_id(c, service_id) + elif service_host is not None: + lookup_fn = lambda c: ( + objects.Service.get_by_compute_host(c, service_host)) + else: + LOG.exception('_find_service_in_cell called with no search parameters') + # This is intentionally cryptic so we don't leak implementation details + # out of the API. + raise exception.NovaException() + + for cell in CELLS: + # NOTE(danms): Services can be in cell0, so don't skip it here + try: + with nova_context.target_cell(context, cell): + cell_service = lookup_fn(context) + except exception.NotFound: + # NOTE(danms): Keep looking in other cells + continue + if service and cell_service: + raise exception.ServiceNotUnique() + service = cell_service + found_in_cell = cell + if service and is_uuid: + break + + if service: + # NOTE(danms): Set the cell on the context so it remains + # when we return to our caller + nova_context.set_target_cell(context, found_in_cell) + return service + else: + raise exception.NotFound() + + class HostAPI(base.Base): """Sub-set of the Compute Manager API for managing host operations.""" @@ -4418,57 +4477,12 @@ class HostAPI(base.Base): ret_services.append(service) return ret_services - def _find_service(self, context, service_id): - """Find a service by id by searching all cells. - - If one matching service is found, return it. If none or multiple - are found, raise an exception. - - :param context: A context.RequestContext - :param service_id: The DB ID (or UUID) of the service to find - :returns: An objects.Service - :raises: ServiceNotUnique if multiple matches are found - :raises: ServiceNotFound if no matches are found - """ - - load_cells() - # NOTE(danms): Unfortunately this API exposes database identifiers - # which means we really can't do something efficient here - service = None - found_in_cell = None - is_uuid = uuidutils.is_uuid_like(service_id) - for cell in CELLS: - # NOTE(danms): Services can be in cell0, so don't skip it here - try: - with nova_context.target_cell(context, cell) as cctxt: - if is_uuid: - service = objects.Service.get_by_uuid(cctxt, - service_id) - found_in_cell = cell - # Service uuids are unique so we can break the loop now - break - else: - cell_service = objects.Service.get_by_id(cctxt, - service_id) - except exception.ServiceNotFound: - # NOTE(danms): Keep looking in other cells - continue - if service and cell_service: - raise exception.ServiceNotUnique() - service = cell_service - found_in_cell = cell - - if service: - # NOTE(danms): Set the cell on the context so it remains - # when we return to our caller - nova_context.set_target_cell(context, found_in_cell) - return service - else: - raise exception.ServiceNotFound(service_id=service_id) - def service_get_by_id(self, context, service_id): """Get service entry for the given service id or uuid.""" - return self._find_service(context, service_id) + try: + return _find_service_in_cell(context, service_id=service_id) + except exception.NotFound: + raise exception.ServiceNotFound(service_id=service_id) @target_host_cell def service_get_by_compute_host(self, context, host_name): @@ -4494,7 +4508,10 @@ class HostAPI(base.Base): def _service_delete(self, context, service_id): """Performs the actual Service deletion operation.""" - service = self._find_service(context, service_id) + try: + service = _find_service_in_cell(context, service_id=service_id) + except exception.NotFound: + raise exception.ServiceNotFound(service_id=service_id) service.destroy() def service_delete(self, context, service_id): @@ -4761,9 +4778,16 @@ class AggregateAPI(base.Base): aggregate_payload) # validates the host; HostMappingNotFound or ComputeHostNotFound # is raised if invalid - mapping = objects.HostMapping.get_by_host(context, host_name) - nova_context.set_target_cell(context, mapping.cell_mapping) - objects.Service.get_by_compute_host(context, host_name) + try: + mapping = objects.HostMapping.get_by_host(context, host_name) + nova_context.set_target_cell(context, mapping.cell_mapping) + objects.Service.get_by_compute_host(context, host_name) + except exception.HostMappingNotFound: + try: + # NOTE(danms): This targets our cell + _find_service_in_cell(context, service_host=host_name) + except exception.NotFound: + raise exception.ComputeHostNotFound(host=host_name) aggregate = objects.Aggregate.get_by_id(context, aggregate_id) self.is_safe_to_update_az(context, aggregate.metadata, diff --git a/nova/tests/functional/api/client.py b/nova/tests/functional/api/client.py index 9bf9ecd60acb..1abe5008e8d8 100644 --- a/nova/tests/functional/api/client.py +++ b/nova/tests/functional/api/client.py @@ -398,6 +398,10 @@ class TestOpenStackClient(object): def delete_aggregate(self, aggregate_id): self.api_delete('/os-aggregates/%s' % aggregate_id) + def add_host_to_aggregate(self, aggregate_id, host): + return self.api_post('/os-aggregates/%s/action' % aggregate_id, + {'add_host': {'host': host}}) + def get_limits(self): return self.api_get('/limits').body['limits'] @@ -416,3 +420,6 @@ class TestOpenStackClient(object): def detach_interface(self, server_id, port_id): return self.api_delete('/servers/%s/os-interface/%s' % (server_id, port_id)) + + def get_services(self): + return self.api_get('/os-services').body['services'] diff --git a/nova/tests/functional/test_aggregates.py b/nova/tests/functional/test_aggregates.py new file mode 100644 index 000000000000..06d6ae441a7d --- /dev/null +++ b/nova/tests/functional/test_aggregates.py @@ -0,0 +1,41 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from nova.tests.functional import integrated_helpers + + +class AggregatesTest(integrated_helpers._IntegratedTestBase): + api_major_version = 'v2' + ADMIN_API = True + + def _add_hosts_to_aggregate(self): + """List all compute services and add them all to an aggregate.""" + + compute_services = [s for s in self.api.get_services() + if s['binary'] == 'nova-compute'] + agg = {'aggregate': {'name': 'test-aggregate'}} + agg = self.api.post_aggregate(agg) + for service in compute_services: + self.api.add_host_to_aggregate(agg['id'], service['host']) + return len(compute_services) + + def test_add_hosts(self): + # Default case with one compute, mapped for us + self.assertEqual(1, self._add_hosts_to_aggregate()) + + def test_add_unmapped_host(self): + """Ensure that hosts without mappings are still found and added""" + + # Add another compute, but nuke its HostMapping + self.start_service('compute', host='compute2') + self.host_mappings['compute2'].destroy() + self.assertEqual(2, self._add_hosts_to_aggregate()) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 64b10d50e064..5fcbfbeadac1 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -11354,11 +11354,11 @@ class ComputeAPIAggrTestCase(BaseTestCase): self.assertEqual(len(aggr.hosts), len(values[0][1])) def test_add_host_to_aggregate_raise_not_found(self): - # Ensure HostMappingNotFound is raised when adding invalid host. + # Ensure ComputeHostNotFound is raised when adding invalid host. aggr = self.api.create_aggregate(self.context, 'fake_aggregate', 'fake_zone') fake_notifier.NOTIFICATIONS = [] - self.assertRaises(exception.HostMappingNotFound, + self.assertRaises(exception.ComputeHostNotFound, self.api.add_host_to_aggregate, self.context, aggr.id, 'invalid_host') self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index d1bd018238df..d7fab7136ca0 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -5100,6 +5100,10 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): self.assertRaises(exception.CannotResizeToSameFlavor, self._test_resize, same_flavor=True) + def test_find_service_in_cell_error_case(self): + self.assertRaises(exception.NovaException, + compute_api._find_service_in_cell, self.context) + def test_validate_and_build_base_options_translate_neutron_secgroup(self): """Tests that _check_requested_secgroups will return a uuid for a requested Neutron security group and that will be returned from