From 14fed95f61e66cb63818eb1e2c856d6f9b3b6211 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 4 Feb 2020 12:28:32 +0000 Subject: [PATCH] trivial: Fetch 'Service' objects once when building AZs As noted in a TODO by mriedem, we were making two trips to the DB when retrieving 'Service' objects in order to build a list of availability zones. Optimize things so that this is no longer the case. Change-Id: Id0d8c52cba8b548a0e6436d94765b84f040efee5 Signed-off-by: Stephen Finucane --- .../openstack/compute/availability_zone.py | 14 ++- nova/availability_zones.py | 97 +++++++++++-------- nova/tests/fixtures.py | 2 +- .../compute/test_availability_zone.py | 81 ++++++++-------- nova/tests/unit/test_availability_zones.py | 2 +- 5 files changed, 104 insertions(+), 92 deletions(-) diff --git a/nova/api/openstack/compute/availability_zone.py b/nova/api/openstack/compute/availability_zone.py index d41e493ab0a9..77b720e31555 100644 --- a/nova/api/openstack/compute/availability_zone.py +++ b/nova/api/openstack/compute/availability_zone.py @@ -44,9 +44,9 @@ class AvailabilityZoneController(wsgi.Controller): def _describe_availability_zones(self, context, **kwargs): ctxt = context.elevated() - available_zones, not_available_zones = \ + available_zones, not_available_zones = ( availability_zones.get_availability_zones( - ctxt, self.host_api) + ctxt, self.host_api)) filtered_available_zones = \ self._get_filtered_availability_zones(available_zones, True) @@ -58,18 +58,16 @@ class AvailabilityZoneController(wsgi.Controller): def _describe_availability_zones_verbose(self, context, **kwargs): ctxt = context.elevated() - # Available services - enabled_services = self.host_api.service_get_all( - context, {'disabled': False}, set_zones=True, all_cells=True) - + services = self.host_api.service_get_all( + context, set_zones=True, all_cells=True) available_zones, not_available_zones = ( availability_zones.get_availability_zones( - ctxt, self.host_api, enabled_services=enabled_services)) + ctxt, self.host_api, services=services)) zone_hosts = {} host_services = {} api_services = ('nova-osapi_compute', 'nova-metadata') - for service in enabled_services: + for service in filter(lambda x: not x.disabled, services): if service.binary in api_services: # Skip API services in the listing since they are not # maintained in the same way as other services diff --git a/nova/availability_zones.py b/nova/availability_zones.py index d5651f75b883..dfb6358c8dd8 100644 --- a/nova/availability_zones.py +++ b/nova/availability_zones.py @@ -110,7 +110,7 @@ def update_host_availability_zone_cache(context, host, availability_zone=None): def get_availability_zones(context, hostapi, get_only_available=False, - with_hosts=False, enabled_services=None): + with_hosts=False, services=None): """Return available and unavailable zones on demand. :param context: nova auth RequestContext @@ -121,50 +121,65 @@ def get_availability_zones(context, hostapi, get_only_available=False, available zones only :param with_hosts: whether to return hosts part of the AZs :type with_hosts: bool - :param enabled_services: list of enabled services to use; if None - enabled services will be retrieved from all cells with zones set + :param services: list of services to use; if None, enabled services will be + retrieved from all cells with zones set """ - if enabled_services is None: - enabled_services = hostapi.service_get_all( - context, {'disabled': False}, set_zones=True, all_cells=True) + if services is None: + services = hostapi.service_get_all( + context, set_zones=True, all_cells=True) - available_zones = [] - for (zone, host) in [(service['availability_zone'], service['host']) - for service in enabled_services]: - if not with_hosts and zone not in available_zones: - available_zones.append(zone) - elif with_hosts: - _available_zones = dict(available_zones) - zone_hosts = _available_zones.setdefault(zone, set()) - zone_hosts.add(host) - # .items() returns a view in Py3, casting it to list for Py2 compat - available_zones = list(_available_zones.items()) + enabled_services = [] + disabled_services = [] + for service in services: + if not service.disabled: + enabled_services.append(service) + else: + disabled_services.append(service) - if not get_only_available: - # TODO(mriedem): We could probably optimize if we know that we're going - # to get both enabled and disabled services and just pull them all from - # the cell DBs at the same time and then filter into enabled/disabled - # lists in python. - disabled_services = hostapi.service_get_all( - context, {'disabled': True}, set_zones=True, all_cells=True) - not_available_zones = [] - azs = available_zones if not with_hosts else dict(available_zones) - zones = [(service['availability_zone'], service['host']) - for service in disabled_services - if service['availability_zone'] not in azs] - for (zone, host) in zones: - if not with_hosts and zone not in not_available_zones: - not_available_zones.append(zone) - elif with_hosts: - _not_available_zones = dict(not_available_zones) - zone_hosts = _not_available_zones.setdefault(zone, set()) - zone_hosts.add(host) - # .items() returns a view in Py3, casting it to list for Py2 - # compat - not_available_zones = list(_not_available_zones.items()) - return (available_zones, not_available_zones) + if with_hosts: + return _get_availability_zones_with_hosts( + enabled_services, disabled_services, get_only_available) else: - return available_zones + return _get_availability_zones( + enabled_services, disabled_services, get_only_available) + + +def _get_availability_zones( + enabled_services, disabled_services, get_only_available=False): + + available_zones = { + service['availability_zone'] for service in enabled_services + } + + if get_only_available: + return sorted(available_zones) + + not_available_zones = { + service['availability_zone'] for service in disabled_services + if service['availability_zone'] not in available_zones + } + + return sorted(available_zones), sorted(not_available_zones) + + +def _get_availability_zones_with_hosts( + enabled_services, disabled_services, get_only_available=False): + + available_zones = collections.defaultdict(set) + for service in enabled_services: + available_zones[service['availability_zone']].add(service['host']) + + if get_only_available: + return sorted(available_zones.items()) + + not_available_zones = collections.defaultdict(set) + for service in disabled_services: + if service['availability_zone'] in available_zones: + continue + + not_available_zones[service['availability_zone']].add(service['host']) + + return sorted(available_zones.items()), sorted(not_available_zones.items()) def get_instance_availability_zone(context, instance): diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index b55ee4fc8623..99a5750ba736 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -2401,7 +2401,7 @@ class AvailabilityZoneFixture(fixtures.Fixture): def fake_get_availability_zones( ctxt, hostapi, get_only_available=False, - with_hosts=False, enabled_services=None): + with_hosts=False, services=None): # A 2-item tuple is returned if get_only_available=False. if not get_only_available: return self.zones, [] diff --git a/nova/tests/unit/api/openstack/compute/test_availability_zone.py b/nova/tests/unit/api/openstack/compute/test_availability_zone.py index e142bb779ff8..da49f699e116 100644 --- a/nova/tests/unit/api/openstack/compute/test_availability_zone.py +++ b/nova/tests/unit/api/openstack/compute/test_availability_zone.py @@ -38,7 +38,6 @@ FAKE_UUID = fakes.FAKE_UUID def fake_service_get_all(context, filters=None, **kwargs): - disabled = filters.get('disabled') if filters else None def __fake_service(binary, availability_zone, created_at, updated_at, host, disabled): @@ -54,30 +53,32 @@ def fake_service_get_all(context, filters=None, **kwargs): db_s.pop('version', None) return objects.Service(context, **db_s) - if disabled: - svcs = [__fake_service("nova-compute", "zone-2", - datetime.datetime(2012, 11, 14, 9, 53, 25, 0), - datetime.datetime(2012, 12, 26, 14, 45, 25, 0), - "fake_host-1", True), - __fake_service("nova-scheduler", "internal", - datetime.datetime(2012, 11, 14, 9, 57, 3, 0), - datetime.datetime(2012, 12, 26, 14, 45, 25, 0), - "fake_host-1", True)] - else: - svcs = [__fake_service("nova-compute", "zone-1", - datetime.datetime(2012, 11, 14, 9, 53, 25, 0), - datetime.datetime(2012, 12, 26, 14, 45, 25, 0), - "fake_host-1", False), - __fake_service("nova-sched", "internal", - datetime.datetime(2012, 11, 14, 9, 57, 3, 0), - datetime.datetime(2012, 12, 26, 14, 45, 25, 0), - "fake_host-1", False), - # nova-conductor is in the same zone and host as nova-sched - # and is here to make sure /detail filters out duplicates. - __fake_service("nova-conductor", "internal", - datetime.datetime(2012, 11, 14, 9, 57, 3, 0), - datetime.datetime(2012, 12, 26, 14, 45, 25, 0), - "fake_host-1", False)] + svcs = [__fake_service("nova-compute", "zone-2", + datetime.datetime(2012, 11, 14, 9, 53, 25, 0), + datetime.datetime(2012, 12, 26, 14, 45, 25, 0), + "fake_host-1", True), + __fake_service("nova-scheduler", "internal", + datetime.datetime(2012, 11, 14, 9, 57, 3, 0), + datetime.datetime(2012, 12, 26, 14, 45, 25, 0), + "fake_host-1", True), + __fake_service("nova-compute", "zone-1", + datetime.datetime(2012, 11, 14, 9, 53, 25, 0), + datetime.datetime(2012, 12, 26, 14, 45, 25, 0), + "fake_host-1", False), + __fake_service("nova-sched", "internal", + datetime.datetime(2012, 11, 14, 9, 57, 3, 0), + datetime.datetime(2012, 12, 26, 14, 45, 25, 0), + "fake_host-1", False), + # nova-conductor is in the same zone and host as nova-sched + # and is here to make sure /detail filters out duplicates. + __fake_service("nova-conductor", "internal", + datetime.datetime(2012, 11, 14, 9, 57, 3, 0), + datetime.datetime(2012, 12, 26, 14, 45, 25, 0), + "fake_host-1", False)] + + if filters and 'disabled' in filters: + svcs = [svc for svc in svcs if svc.disabled == filters['disabled']] + return objects.ServiceList(objects=svcs) @@ -136,19 +137,6 @@ class AvailabilityZoneApiTestV21(test.NoDBTestCase): self.assertEqual(len(zones), 3) timestamp = iso8601.parse_date("2012-12-26T14:45:25Z") expected = [ - { - 'zoneName': 'zone-1', - 'zoneState': {'available': True}, - 'hosts': { - 'fake_host-1': { - 'nova-compute': { - 'active': True, - 'available': True, - 'updated_at': timestamp - } - } - } - }, { 'zoneName': 'internal', 'zoneState': {'available': True}, @@ -167,6 +155,19 @@ class AvailabilityZoneApiTestV21(test.NoDBTestCase): } } }, + { + 'zoneName': 'zone-1', + 'zoneState': {'available': True}, + 'hosts': { + 'fake_host-1': { + 'nova-compute': { + 'active': True, + 'available': True, + 'updated_at': timestamp + } + } + } + }, { 'zoneName': 'zone-2', 'zoneState': {'available': False}, @@ -174,9 +175,7 @@ class AvailabilityZoneApiTestV21(test.NoDBTestCase): } ] self.assertEqual(expected, zones) - # We get both enabled and disabled services per cell (just one in this - # test case) so we'll query the services table twice. - self.assertEqual(2, self.mock_service_get_all.call_count, + self.assertEqual(1, self.mock_service_get_all.call_count, self.mock_service_get_all.call_args_list) @mock.patch.object(availability_zones, 'get_availability_zones', diff --git a/nova/tests/unit/test_availability_zones.py b/nova/tests/unit/test_availability_zones.py index 94cbca9fc37a..6252ef10ac23 100644 --- a/nova/tests/unit/test_availability_zones.py +++ b/nova/tests/unit/test_availability_zones.py @@ -215,7 +215,7 @@ class AvailabilityZoneTestCases(test.TestCase): zones, not_zones = az.get_availability_zones(self.context, host_api) self.assertEqual(['nova-test', 'nova-test2'], zones) - self.assertEqual(['nova-test3', 'nova'], not_zones) + self.assertEqual(['nova', 'nova-test3'], not_zones) zones = az.get_availability_zones(self.context, host_api, get_only_available=True)