openstack
/
nova
Code Issues Proposed changes

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
This commit is contained in:
Matt Riedemann 2018-12-06 11:17:15 -05:00
parent c72dafad80
commit 66e44c6429
4 changed files with 60 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
nova/api/openstack/compute/services.py
View File

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

27
nova/compute/api.py
View File

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

31
nova/tests/unit/compute/test_compute.py
View File

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

12
nova/tests/unit/compute/test_compute_api.py
View File

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