From 66e44c64297e77395195d77104017fb6fcea58d2 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Thu, 6 Dec 2018 11:17:15 -0500 Subject: [PATCH] Only construct SchedulerReportClient on first access from API With commit 5d1a50018510b2b281ad33895ae2d9555f5d5b05 each API/AggregateAPI class instance constructs a SchedulerReportClient which holds an in-memory lock during its initialization. With at least 34 API extensions constructing at least one of those two API classes, the accumulated affect of the SchedulerReportClient construction can slow down nova-api startup times, especially when running with multiple API workers, like in our tempest-full CI job (there are 2 workers, so 68 inits). This change simply defers constructing the SchedulerReportClient until it is used, which is only in a few spots in the API code, which should help with nova-api start times. The AggregateAPI also has to construct the SchedulerQueryClient separately because SchedulerClient creates both the query and report clients. Long-term we could consider making it a singleton in nova.compute.api if that is safe (the aggregate code might be relying on some caching aspects in the SchedulerReportClient). Change-Id: Idf6e548d725db0181629a451f46b6a3a5850d186 Closes-Bug: #1807219 --- nova/api/openstack/compute/services.py | 8 +++++- nova/compute/api.py | 27 +++++++++++------- nova/tests/unit/compute/test_compute.py | 31 ++++++++++++++++----- nova/tests/unit/compute/test_compute_api.py | 12 ++++++++ 4 files changed, 60 insertions(+), 18 deletions(-) diff --git a/nova/api/openstack/compute/services.py b/nova/api/openstack/compute/services.py index 7a35c5665fc7..955c40e3cf5d 100644 --- a/nova/api/openstack/compute/services.py +++ b/nova/api/openstack/compute/services.py @@ -42,7 +42,13 @@ class ServiceController(wsgi.Controller): self.actions = {"enable": self._enable, "disable": self._disable, "disable-log-reason": self._disable_log_reason} - self.placementclient = report.SchedulerReportClient() + self._placementclient = None # Lazy-load on first access. + + @property + def placementclient(self): + if self._placementclient is None: + self._placementclient = report.SchedulerReportClient() + return self._placementclient def _get_services(self, req): # The API services are filtered out since they are not RPC services diff --git a/nova/compute/api.py b/nova/compute/api.py index a43183d007f7..81a02a352513 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -76,7 +76,8 @@ from nova.policies import servers as servers_policies import nova.policy from nova import profiler from nova import rpc -from nova.scheduler import client as scheduler_client +from nova.scheduler.client import query as queryclient +from nova.scheduler.client import report as reportclient from nova.scheduler import utils as scheduler_utils from nova import servicegroup from nova import utils @@ -253,13 +254,7 @@ class API(base.Base): self.image_api = image_api or image.API() self.network_api = network_api or network.API() self.volume_api = volume_api or cinder.API() - # NOTE(mriedem): This looks a bit weird but we get the reportclient - # via SchedulerClient since it lazy-loads SchedulerReportClient on - # the first usage which helps to avoid a bunch of lockutils spam in - # the nova-api logs every time the service is restarted (remember - # that pretty much all of the REST API controllers construct this - # API class). - self.placementclient = scheduler_client.SchedulerClient().reportclient + self._placementclient = None # Lazy-load on first access. self.security_group_api = (security_group_api or openstack_driver.get_openstack_security_group_driver()) self.consoleauth_rpcapi = consoleauth_rpcapi.ConsoleAuthAPI() @@ -2136,6 +2131,12 @@ class API(base.Base): if 'id' in bdm: bdm.destroy() + @property + def placementclient(self): + if self._placementclient is None: + self._placementclient = reportclient.SchedulerReportClient() + return self._placementclient + def _local_delete(self, context, instance, bdms, delete_type, cb): if instance.vm_state == vm_states.SHELVED_OFFLOADED: LOG.info("instance is in SHELVED_OFFLOADED state, cleanup" @@ -5301,10 +5302,16 @@ class AggregateAPI(base.Base): """Sub-set of the Compute Manager API for managing host aggregates.""" def __init__(self, **kwargs): self.compute_rpcapi = compute_rpcapi.ComputeAPI() - self.scheduler_client = scheduler_client.SchedulerClient() - self.placement_client = self.scheduler_client.reportclient + self.scheduler_client = queryclient.SchedulerQueryClient() + self._placement_client = None # Lazy-load on first access. super(AggregateAPI, self).__init__(**kwargs) + @property + def placement_client(self): + if self._placement_client is None: + self._placement_client = reportclient.SchedulerReportClient() + return self._placement_client + @wrap_exception() def create_aggregate(self, context, aggregate_name, availability_zone): """Creates the model for the aggregate.""" diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 633e2efc21a2..fb6051064dc3 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -67,7 +67,6 @@ from nova.objects import block_device as block_device_obj from nova.objects import fields as obj_fields from nova.objects import instance as instance_obj from nova.objects import migrate_data as migrate_data_obj -from nova.scheduler import client as scheduler_client from nova import test from nova.tests import fixtures from nova.tests.unit.compute import eventlet_utils @@ -12288,6 +12287,18 @@ class ComputeAPIAggrTestCase(BaseTestCase): hosts = aggregate.hosts if 'hosts' in aggregate else None self.assertIn(values[0][1][0], hosts) + @mock.patch('nova.scheduler.client.report.SchedulerReportClient') + def test_placement_client_init(self, mock_report_client): + """Tests to make sure that the construction of the placement client + only happens once per AggregateAPI class instance. + """ + self.assertIsNone(self.api._placement_client) + # Access the property twice to make sure SchedulerReportClient is + # only loaded once. + for x in range(2): + self.api.placement_client + mock_report_client.assert_called_once_with() + class ComputeAPIAggrCallsSchedulerTestCase(test.NoDBTestCase): """This is for making sure that all Aggregate API methods which are @@ -12300,13 +12311,15 @@ class ComputeAPIAggrCallsSchedulerTestCase(test.NoDBTestCase): self.api = compute_api.AggregateAPI() self.context = context.RequestContext('fake', 'fake') - @mock.patch.object(scheduler_client.SchedulerClient, 'update_aggregates') + @mock.patch('nova.scheduler.client.query.SchedulerQueryClient.' + 'update_aggregates') def test_create_aggregate(self, update_aggregates): with mock.patch.object(objects.Aggregate, 'create'): agg = self.api.create_aggregate(self.context, 'fake', None) update_aggregates.assert_called_once_with(self.context, [agg]) - @mock.patch.object(scheduler_client.SchedulerClient, 'update_aggregates') + @mock.patch('nova.scheduler.client.query.SchedulerQueryClient.' + 'update_aggregates') def test_update_aggregate(self, update_aggregates): self.api.is_safe_to_update_az = mock.Mock() agg = objects.Aggregate() @@ -12315,7 +12328,8 @@ class ComputeAPIAggrCallsSchedulerTestCase(test.NoDBTestCase): self.api.update_aggregate(self.context, 1, {}) update_aggregates.assert_called_once_with(self.context, [agg]) - @mock.patch.object(scheduler_client.SchedulerClient, 'update_aggregates') + @mock.patch('nova.scheduler.client.query.SchedulerQueryClient.' + 'update_aggregates') def test_update_aggregate_metadata(self, update_aggregates): self.api.is_safe_to_update_az = mock.Mock() agg = objects.Aggregate() @@ -12325,7 +12339,8 @@ class ComputeAPIAggrCallsSchedulerTestCase(test.NoDBTestCase): self.api.update_aggregate_metadata(self.context, 1, {}) update_aggregates.assert_called_once_with(self.context, [agg]) - @mock.patch.object(scheduler_client.SchedulerClient, 'delete_aggregate') + @mock.patch('nova.scheduler.client.query.SchedulerQueryClient.' + 'delete_aggregate') def test_delete_aggregate(self, delete_aggregate): self.api.is_safe_to_update_az = mock.Mock() agg = objects.Aggregate(uuid=uuids.agg, name='fake-aggregate', @@ -12340,7 +12355,8 @@ class ComputeAPIAggrCallsSchedulerTestCase(test.NoDBTestCase): 'aggregate_add_host') @mock.patch('nova.compute.utils.notify_about_aggregate_action') @mock.patch('nova.compute.rpcapi.ComputeAPI.add_aggregate_host') - @mock.patch.object(scheduler_client.SchedulerClient, 'update_aggregates') + @mock.patch('nova.scheduler.client.query.SchedulerQueryClient.' + 'update_aggregates') def test_add_host_to_aggregate(self, update_aggregates, mock_add_agg, mock_notify, mock_add_host): self.api.is_safe_to_update_az = mock.Mock() @@ -12365,7 +12381,8 @@ class ComputeAPIAggrCallsSchedulerTestCase(test.NoDBTestCase): 'aggregate_remove_host') @mock.patch('nova.compute.utils.notify_about_aggregate_action') @mock.patch('nova.compute.rpcapi.ComputeAPI.remove_aggregate_host') - @mock.patch.object(scheduler_client.SchedulerClient, 'update_aggregates') + @mock.patch('nova.scheduler.client.query.SchedulerQueryClient.' + 'update_aggregates') def test_remove_host_from_aggregate(self, update_aggregates, mock_remove_agg, mock_notify, mock_remove_host): diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index dca76b475fa9..22c0289f73da 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -6718,6 +6718,18 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): self.assertTrue(hasattr(self.compute_api, 'host')) self.assertEqual(CONF.host, self.compute_api.host) + @mock.patch('nova.scheduler.client.report.SchedulerReportClient') + def test_placement_client_init(self, mock_report_client): + """Tests to make sure that the construction of the placement client + only happens once per API class instance. + """ + self.assertIsNone(self.compute_api._placementclient) + # Access the property twice to make sure SchedulerReportClient is + # only loaded once. + for x in range(2): + self.compute_api.placementclient + mock_report_client.assert_called_once_with() + class Cellsv1DeprecatedTestMixIn(object): @mock.patch.object(objects.BuildRequestList, 'get_by_filters')