Only construct SchedulerReportClient on first access from API

With commit 5d1a500185 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

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

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
+        self._placementclient = None  # Lazy-load on first access.
-        # 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.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.scheduler_client = queryclient.SchedulerQueryClient()
-        self.placement_client = self.scheduler_client.reportclient
+        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."""

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

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