openstack
/
nova
Code Issues Proposed changes

Fix policy check performance in 2.47+ 

In 2.47 we introduced inlined flavors, which include extra_specs if so
authorized. The code was doing a policy check deep in the per-instance
handler that generated this blob. For queries with a lot of instances,
this policy check is a lot of overhead, especially as it introduces a
check of the policy file on disk, in case it has changed.

This patch makes us calculate the permission flag once per list operation
and pass it down to the lower layers to honor.

Closes-Bug: #1719966
Change-Id: I036623ae7409c2e6c6a754b4c8d5c9120f777774
(cherry picked from commit b1591d1080)
This commit is contained in:
Dan Smith 2017-09-27 11:46:34 -07:00 committed by Matt Riedemann
parent edd59ae12d
commit bdd8b58351
2 changed files with 59 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
nova/api/openstack/compute/views/servers.py
View File

@ -80,7 +80,7 @@ class ViewBuilder(common.ViewBuilder):
},
}
def basic(self, request, instance):
def basic(self, request, instance, show_extra_specs=False):
"""Generic, non-detailed view of an instance."""
return {
"server": {
@ -112,11 +112,21 @@ class ViewBuilder(common.ViewBuilder):
# results.
return sorted(list(set(self._show_expected_attrs + expected_attrs)))
def show(self, request, instance, extend_address=True):
def show(self, request, instance, extend_address=True,
show_extra_specs=None):
"""Detailed view of a single instance."""
ip_v4 = instance.get('access_ip_v4')
ip_v6 = instance.get('access_ip_v6')
if show_extra_specs is None:
# detail will pre-calculate this for us. If we're doing show,
# then figure it out here.
show_extra_specs = False
if api_version_request.is_supported(request, min_version='2.47'):
context = request.environ['nova.context']
show_extra_specs = context.can(
fes_policies.POLICY_ROOT % 'index', fatal=False)
server = {
"server": {
"id": instance["uuid"],
@ -127,7 +137,8 @@ class ViewBuilder(common.ViewBuilder):
"metadata": self._get_metadata(instance),
"hostId": self._get_host_id(instance) or "",
"image": self._get_image(request, instance),
"flavor": self._get_flavor(request, instance),
"flavor": self._get_flavor(request, instance,
show_extra_specs),
"created": utils.isotime(instance["created_at"]),
"updated": utils.isotime(instance["updated_at"]),
"addresses": self._get_addresses(request, instance,
@ -168,14 +179,26 @@ class ViewBuilder(common.ViewBuilder):
def index(self, request, instances):
"""Show a list of servers without many details."""
coll_name = self._collection_name
return self._list_view(self.basic, request, instances, coll_name)
return self._list_view(self.basic, request, instances, coll_name,
False)
def detail(self, request, instances):
"""Detailed view of a list of instance."""
coll_name = self._collection_name + '/detail'
return self._list_view(self.show, request, instances, coll_name)
def _list_view(self, func, request, servers, coll_name):
if api_version_request.is_supported(request, min_version='2.47'):
# Determine if we should show extra_specs in the inlined flavor
# once before we iterate the list of instances
context = request.environ['nova.context']
show_extra_specs = context.can(fes_policies.POLICY_ROOT % 'index',
fatal=False)
else:
show_extra_specs = False
return self._list_view(self.show, request, instances, coll_name,
show_extra_specs)
def _list_view(self, func, request, servers, coll_name, show_extra_specs):
"""Provide a view for a list of servers.
:param func: Function used to format the server data
@ -185,7 +208,9 @@ class ViewBuilder(common.ViewBuilder):
for a pagination query
:returns: Server data in dictionary format
"""
server_list = [func(request, server)["server"] for server in servers]
server_list = [func(request, server,
show_extra_specs=show_extra_specs)["server"]
for server in servers]
servers_links = self._get_collection_links(request,
servers,
coll_name)
@ -245,7 +270,7 @@ class ViewBuilder(common.ViewBuilder):
else:
return ""
def _get_flavor_dict(self, request, instance_type):
def _get_flavor_dict(self, request, instance_type, show_extra_specs):
flavordict = {
"vcpus": instance_type.vcpus,
"ram": instance_type.memory_mb,
@ -254,12 +279,11 @@ class ViewBuilder(common.ViewBuilder):
"swap": instance_type.swap,
"original_name": instance_type.name
}
context = request.environ['nova.context']
if context.can(fes_policies.POLICY_ROOT % 'index', fatal=False):
if show_extra_specs:
flavordict['extra_specs'] = instance_type.extra_specs
return flavordict
def _get_flavor(self, request, instance):
def _get_flavor(self, request, instance, show_extra_specs):
instance_type = instance.get_flavor()
if not instance_type:
LOG.warning("Instance has had its instance_type removed "
@ -267,7 +291,8 @@ class ViewBuilder(common.ViewBuilder):
return {}
if api_version_request.is_supported(request, min_version="2.47"):
return self._get_flavor_dict(request, instance_type)
return self._get_flavor_dict(request, instance_type,
show_extra_specs)
flavor_id = instance_type["flavorid"]
flavor_bookmark = self._flavor_builder._get_bookmark_link(request,

24
nova/tests/unit/api/openstack/compute/test_serversV21.py
View File

@ -1690,7 +1690,26 @@ class ServerControllerTestV247(ControllerTest):
req = fakes.HTTPRequest.blank('/fake/servers/detail',
version=self.wsgi_api_version)
res_dict = self.controller.detail(req)
hits = []
real_auth = policy.authorize
# Wrapper for authorize to count the number of times
# we authorize for extra-specs
def fake_auth(context, action, target):
if 'extra-specs' in action:
hits.append(1)
return real_auth(context, action, target)
with mock.patch('nova.policy.authorize') as mock_auth:
mock_auth.side_effect = fake_auth
res_dict = self.controller.detail(req)
# We should have found more than one servers, but only hit the
# policy check once
self.assertGreater(len(res_dict['servers']), 1)
self.assertEqual(1, len(hits))
for i, s in enumerate(res_dict['servers']):
self.assertEqual(s['flavor'], expected_flavor)
@ -4086,7 +4105,8 @@ class ServersViewBuilderTest(test.TestCase):
expected = {"id": "1",
"links": [{"rel": "bookmark",
"href": flavor_bookmark}]}
result = self.view_builder._get_flavor(self.request, self.instance)
result = self.view_builder._get_flavor(self.request, self.instance,
False)
self.assertEqual(result, expected)
def test_build_server(self):