From 87e42afb9e98204a683baff22c67171dca2100fe Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Wed, 19 May 2021 15:01:25 -0700 Subject: [PATCH] API to pass fields to node object list 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 --- ironic/api/controllers/v1/node.py | 165 ++++++++++++------ ironic/objects/base.py | 4 +- ironic/objects/node.py | 2 - ironic/tests/unit/objects/test_node.py | 1 - ...d-overhead-reduction-40be1821e38b468c.yaml | 9 + 5 files changed, 120 insertions(+), 61 deletions(-) create mode 100644 releasenotes/notes/db-field-overhead-reduction-40be1821e38b468c.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 760ef82611..7aad682862 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -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, diff --git a/ironic/objects/base.py b/ironic/objects/base.py index b4103dbe26..e75e5ad3be 100644 --- a/ironic/objects/base.py +++ b/ironic/objects/base.py @@ -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): diff --git a/ironic/objects/node.py b/ironic/objects/node.py index 3c39961964..bcd34abdef 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -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'], diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index 6cbda0ba5a..313495161c 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/ironic/tests/unit/objects/test_node.py @@ -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']: diff --git a/releasenotes/notes/db-field-overhead-reduction-40be1821e38b468c.yaml b/releasenotes/notes/db-field-overhead-reduction-40be1821e38b468c.yaml new file mode 100644 index 0000000000..5aee33b20c --- /dev/null +++ b/releasenotes/notes/db-field-overhead-reduction-40be1821e38b468c.yaml @@ -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.