openstack/ironic
Code Issues Proposed changes

API to pass fields to node object list

Browse Source 
This change modifies the nodes _get_nodes_collection method to
consider and pass in an explicit lisst of requested fields into
the node list method, while also including the required fields
for things like ownership/policy checking.

And slightly modifies node_convert_with_links method to simplify
it while enabling field validity to be checked, and specific
requisite field lists provided in based upon that value.

And also optionally builds the traits list as they are nolonger
*always* populated on all objects with fully populated objects
as only partially hydrated objects are provided back when specific
fields are requested.

Story: 2008885
Task: 42572
Change-Id: Ided419263d84184cab902944b6c518f98618c9d2
This commit is contained in:
Julia Kreger 2021-05-19 15:01:25 -07:00
parent 7b37e03b69
commit 87e42afb9e
5 changed files with 120 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

165
ironic/api/controllers/v1/node.py
View File

@ -1216,62 +1216,91 @@ class NodeTraitsController(rest.RestController):
chassis_uuid=chassis_uuid)
def _get_fields_for_node_query(fields=None):
valid_fields = ['automated_clean',
'bios_interface',
'boot_interface',
'clean_step',
'conductor_group',
'console_enabled',
'console_interface',
'deploy_interface',
'deploy_step',
'description',
'driver',
'driver_info',
'driver_internal_info',
'extra',
'fault',
'inspection_finished_at',
'inspection_started_at',
'inspect_interface',
'instance_info',
'instance_uuid',
'last_error',
'lessee',
'maintenance',
'maintenance_reason',
'management_interface',
'name',
'network_data',
'network_interface',
'owner',
'power_interface',
'power_state',
'properties',
'protected',
'protected_reason',
'provision_state',
'provision_updated_at',
'raid_config',
'raid_interface',
'rescue_interface',
'reservation',
'resource_class',
'retired',
'retired_reason',
'storage_interface',
'target_power_state',
'target_provision_state',
'target_raid_config',
'traits',
'vendor_interface']
if not fields:
return valid_fields
else:
object_fields = fields[:]
api_fulfilled_fields = ['allocation_uuid', 'chassis_uuid',
'conductor']
for api_field in api_fulfilled_fields:
if api_field in object_fields:
object_fields.remove(api_field)
query_fields = ['uuid', 'traits'] + api_fulfilled_fields \
+ valid_fields
for field in fields:
if field not in query_fields:
msg = 'Field %s is not a valid field.' % field
raise exception.Invalid(msg)
return object_fields
def node_convert_with_links(rpc_node, fields=None, sanitize=True):
# NOTE(TheJulia): This takes approximately 10% of the time to
# collect and return requests to API consumer, specifically
# for the nova sync query which is the most intense overhead
# an integrated deployment can really face.
node = api_utils.object_to_dict(
rpc_node,
link_resource='nodes',
fields=(
'automated_clean',
'bios_interface',
'boot_interface',
'clean_step',
'conductor_group',
'console_enabled',
'console_interface',
'deploy_interface',
'deploy_step',
'description',
'driver',
'driver_info',
'driver_internal_info',
'extra',
'fault',
'inspection_finished_at',
'inspection_started_at',
'inspect_interface',
'instance_info',
'instance_uuid',
'last_error',
'lessee',
'maintenance',
'maintenance_reason',
'management_interface',
'name',
'network_data',
'network_interface',
'owner',
'power_interface',
'power_state',
'properties',
'protected',
'protected_reason',
'provision_state',
'provision_updated_at',
'raid_config',
'raid_interface',
'rescue_interface',
'reservation',
'resource_class',
'retired',
'retired_reason',
'storage_interface',
'target_power_state',
'target_provision_state',
'target_raid_config',
'vendor_interface'
),
)
node['traits'] = rpc_node.traits.get_trait_names()
fields=_get_fields_for_node_query(fields))
if node.get('traits') is not None:
node['traits'] = rpc_node.traits.get_trait_names()
if (api_utils.allow_expose_conductors()
and (fields is None or 'conductor' in fields)):
@ -1285,6 +1314,8 @@ def node_convert_with_links(rpc_node, fields=None, sanitize=True):
'%(node)s.', {'node': rpc_node.uuid})
node['conductor'] = None
# If allocations ever become the primary use path, this absolutely
# needs to become a join. :\
if (api_utils.allow_allocations()
and (fields is None or 'allocation_uuid' in fields)):
node['allocation_uuid'] = None
@ -1296,7 +1327,6 @@ def node_convert_with_links(rpc_node, fields=None, sanitize=True):
node['allocation_uuid'] = allocation.uuid
except exception.AllocationNotFound:
pass
if fields is None or 'chassis_uuid' in fields:
node['chassis_uuid'] = _get_chassis_uuid(rpc_node)
@ -1356,6 +1386,10 @@ def node_sanitize(node, fields):
list of fields to preserve, or ``None`` to preserve them all
:type fields: list of str
"""
# FIXME(TheJulia): As of ironic 18.0, this method is about 88% of
# the time spent preparing to return a node to. If it takes us
# ~ 4.5 seconds to get 1000 nodes, we spend approximately 4 seconds
# PER 1000 in this call.
cdict = api.request.context.to_policy_values()
target_dict = dict(cdict)
owner = node.get('owner')
@ -1379,6 +1413,10 @@ def node_sanitize(node, fields):
show_driver_secrets = policy.check("show_password", cdict, target_dict)
show_instance_secrets = policy.check("show_instance_secrets",
cdict, target_dict)
# FIXME(TheJulia): The above two methods take approximately 20% of the
# present execution time. This needs to be more efficent as they do not
# need to be called for *every* node.
# TODO(TheJulia): The above checks need to be migrated in some direction,
# but until we have auditing clarity, it might not be a big deal.
@ -1782,9 +1820,26 @@ class NodesController(rest.RestController):
if value is not None:
filters[key] = value
if fields:
obj_fields = fields[:]
required_object_fields = ('allocation_id', 'chassis_id',
'uuid', 'owner', 'lessee',
'created_at', 'updated_at')
for req_field in required_object_fields:
if req_field not in obj_fields:
obj_fields.append(req_field)
else:
# map the name for the call, as we did not pickup a specific
# list of fields to return.
obj_fields = fields
# NOTE(TheJulia): When a data set of the nodeds list is being
# requested, this method takes approximately 3-3.5% of the time
# when requesting specific fields aligning with Nova's sync
# process. (Local DB though)
nodes = objects.Node.list(api.request.context, limit, marker_obj,
sort_key=sort_key, sort_dir=sort_dir,
filters=filters)
filters=filters, fields=obj_fields)
# Special filtering on results based on conductor field
if conductor:
@ -1800,6 +1855,7 @@ class NodesController(rest.RestController):
if detail is not None:
parameters['detail'] = detail
if instance_uuid:
# NOTE(rloo) if limit==1 and len(nodes)==1 (see
# Collection.has_next()), a 'next' link will
@ -1969,7 +2025,6 @@ class NodesController(rest.RestController):
fields = api_utils.get_request_return_fields(fields, detail,
_DEFAULT_RETURN_FIELDS)
extra_args = {'description_contains': description_contains}
return self._get_nodes_collection(chassis_uuid, instance_uuid,
associated, maintenance, retired,

4
ironic/objects/base.py
View File

@ -312,9 +312,7 @@ class IronicObject(object_base.VersionedObject):
objects.
:returns: A list of objects corresponding to the database entities
"""
# NOTE(TheJulia): Fields is used in a later patch in this series
# and tests are landed in an intermediate change.
return [cls._from_db_object(context, cls(), db_obj, fields=None)
return [cls._from_db_object(context, cls(), db_obj, fields=fields)
for db_obj in db_objects]
def do_version_changes_for_db(self):

2
ironic/objects/node.py
View File

@ -216,8 +216,6 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat):
use_fields = set(fields or self.fields) - {'traits'}
super(Node, self)._set_from_db_object(context, db_object, use_fields)
if not fields or 'traits' in fields:
# NOTE(TheJulia): No reason to do additional work on a field
# selected query, unless they are seeking the traits themselves.
self.traits = object_base.obj_make_list(
context, objects.TraitList(context),
objects.Trait, db_object['traits'],

1
ironic/tests/unit/objects/test_node.py
View File

@ -415,7 +415,6 @@ class TestNodeObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn):
self.assertEqual(self.fake_node['uuid'], nodes[0].uuid)
self.assertEqual(self.fake_node['provision_state'],
nodes[0].provision_state)
self.assertIsInstance(nodes[0].traits, objects.TraitList)
# Random assortment of fields which should not be present.
for field in ['power_state', 'instance_info', 'resource_class',
'automated_clean', 'properties', 'driver', 'traits']:

9
releasenotes/notes/db-field-overhead-reduction-40be1821e38b468c.yaml Normal file
View File

@ -0,0 +1,9 @@
---
fixes:
- |
Slow database retrieval of nodes has been addressed at the lower layer by
explicitly passing and handling only the requested fields. The result is
excess discarded work is not performed, making the overall process more
efficent.
This is particullarly beneficial for OpenStack Nova's syncronization with
Ironic.