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
This commit is contained in:
Dan Smith 2017-04-27 07:55:40 -07:00
parent 65402e1aeb
commit 62878ef5a6
5 changed files with 131 additions and 55 deletions

View File

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

View File

@ -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']

View File

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

View File

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

View File

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