From 644ba5d4bc69a029f4b02cbd47e9c542ae316748 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Wed, 19 May 2021 12:24:16 -0700 Subject: [PATCH] Only return the requested fields from the DB A node object has many fields, and a huge opportunity for performance improvement is to reduce the amount of work performed at the lower layers when it is not necessary. In particular, the easiest case to identify and handle is when a list of fields to be fulfilled is explicitly supplied. This is particularlly noteworthy when we collecting a list of nodes for synconization with Nova, where only 9 columns are really needed to supply Nova with the information required, and thus numerous fields are discarded. This is all particularlly compounded when traits are used, which presently uses a joined load pattern from SQL. With this patch, we explicitly load and convert only the fields requested at the lowest level, and then perform a different style of loading node traits which requires less overhead by SQLAlchemy to de-duplicate the returned result set to the application. This turns out to be far more efficent as we're reducing the amount of data/object conversion work by 360%, even before we consider populating traits, which is performed as a separate query when traits are requested. Story: 2008885 Task: 42434 Change-Id: Iac703d2a9b7b240a47477be8a64c7c33e28f692f --- ironic/db/api.py | 6 +- ironic/db/sqlalchemy/api.py | 61 +++++++++- ironic/objects/base.py | 8 +- ironic/objects/node.py | 23 +++- ironic/tests/unit/db/test_nodes.py | 149 +++++++++++++++++++++++++ ironic/tests/unit/objects/test_node.py | 11 ++ 6 files changed, 250 insertions(+), 8 deletions(-) diff --git a/ironic/db/api.py b/ironic/db/api.py index 0845fcd55e..697654d38b 100644 --- a/ironic/db/api.py +++ b/ironic/db/api.py @@ -74,7 +74,7 @@ class Connection(object, metaclass=abc.ABCMeta): @abc.abstractmethod def get_node_list(self, filters=None, limit=None, marker=None, - sort_key=None, sort_dir=None): + sort_key=None, sort_dir=None, fields=None): """Return a list of nodes. :param filters: Filters to apply. Defaults to None. @@ -94,6 +94,10 @@ class Connection(object, metaclass=abc.ABCMeta): :param sort_key: Attribute by which results should be sorted. :param sort_dir: direction in which results should be sorted. (asc, desc) + :param fields: Comma separated field list to return, to allow for + only specific fields to be returned to have maximum + API performance calls where not all columns are + needed from the database. """ @abc.abstractmethod diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 297b30d277..02cd5b27c3 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -32,6 +32,8 @@ from osprofiler import sqlalchemy as osp_sqlalchemy import sqlalchemy as sa from sqlalchemy.orm.exc import NoResultFound, MultipleResultsFound from sqlalchemy.orm import joinedload +from sqlalchemy.orm import Load +from sqlalchemy.orm import selectinload from sqlalchemy import sql from ironic.common import exception @@ -433,8 +435,63 @@ class Connection(api.Connection): sort_key, sort_dir, query) def get_node_list(self, filters=None, limit=None, marker=None, - sort_key=None, sort_dir=None): - query = _get_node_query_with_all() + sort_key=None, sort_dir=None, fields=None): + if not fields: + query = _get_node_query_with_all() + query = self._add_nodes_filters(query, filters) + return _paginate_query(models.Node, limit, marker, + sort_key, sort_dir, query) + else: + # Shunt to the proper method to return the limited list. + return self.get_node_list_columns(columns=fields, filters=filters, + limit=limit, marker=marker, + sort_key=sort_key, + sort_dir=sort_dir) + + def get_node_list_columns(self, columns=None, filters=None, limit=None, + marker=None, sort_key=None, sort_dir=None): + """Get a node list with specific fields/columns. + + :param columns: A list of columns to retrieve from the database + and populate into the object. + :param filters: The requested database field filters in the form of + a dictionary with the applicable key, and filter + value. + :param limit: Limit the number of returned nodes, default None. + :param marker: Starting marker to generate a paginated result + set for the consumer. + :param sort_key: Sort key to apply to the result set. + :param sort_dir: Sort direction to apply to the result set. + :returns: A list of Node objects based on the data model from + a SQLAlchemy result set, which the object layer can + use to convert the node into an Node object list. + """ + traits_found = False + use_columns = columns[:] + if 'traits' in columns: + # Traits is synthetic in the data model and not a direct + # table column. As such, a different query pattern is used + # with SQLAlchemy. + traits_found = True + use_columns.remove('traits') + + # Generate the column object list so SQLAlchemy only fulfills + # the requested columns. + use_columns = [getattr(models.Node, c) for c in use_columns] + + # In essence, traits (and anything else needed to generate the + # composite objects) need to be reconciled without using a join + # as multiple rows can be generated in the result set being returned + # from the database server. In this case, with traits, we use + # a selectinload pattern. + if traits_found: + query = model_query(models.Node).options( + Load(models.Node).load_only(*use_columns), + selectinload(models.Node.traits)) + else: + query = model_query(models.Node).options( + Load(models.Node).load_only(*use_columns)) + query = self._add_nodes_filters(query, filters) return _paginate_query(models.Node, limit, marker, sort_key, sort_dir, query) diff --git a/ironic/objects/base.py b/ironic/objects/base.py index a266215394..b4103dbe26 100644 --- a/ironic/objects/base.py +++ b/ironic/objects/base.py @@ -299,7 +299,7 @@ class IronicObject(object_base.VersionedObject): return obj @classmethod - def _from_db_object_list(cls, context, db_objects): + def _from_db_object_list(cls, context, db_objects, fields=None): """Returns objects corresponding to database entities. Returns a list of formal objects of this class that correspond to @@ -308,9 +308,13 @@ class IronicObject(object_base.VersionedObject): :param cls: the VersionedObject class of the desired object :param context: security context :param db_objects: A list of DB models of the object + :param fields: A list of field names to comprise lower level + objects. :returns: A list of objects corresponding to the database entities """ - return [cls._from_db_object(context, cls(), db_obj) + # 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) 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 3eb997c513..c7076b2869 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -313,7 +313,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): # @object_base.remotable_classmethod @classmethod def list(cls, context, limit=None, marker=None, sort_key=None, - sort_dir=None, filters=None): + sort_dir=None, filters=None, fields=None): """Return a list of Node objects. :param cls: the :class:`Node` @@ -323,13 +323,30 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): :param sort_key: column to sort results by. :param sort_dir: direction to sort. "asc" or "desc". :param filters: Filters to apply. + :param fields: Requested fields to be returned. Please note, some + fields are mandatory for the data model and are + automatically included. These are: id, version, + updated_at, created_at, owner, and lessee. :returns: a list of :class:`Node` object. """ + if fields: + # All requests must include version, updated_at, created_at + # owner, and lessee to support access controls and database + # version model updates. Driver and conductor_group are required + # for conductor mapping. + target_fields = ['id'] + fields[:] + ['version', 'updated_at', + 'created_at', 'owner', + 'lessee', 'driver', + 'conductor_group'] + else: + target_fields = None + db_nodes = cls.dbapi.get_node_list(filters=filters, limit=limit, marker=marker, sort_key=sort_key, - sort_dir=sort_dir) - return cls._from_db_object_list(context, db_nodes) + sort_dir=sort_dir, + fields=target_fields) + return cls._from_db_object_list(context, db_nodes, target_fields) # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable # methods can be used in the future to replace current explicit RPC calls. diff --git a/ironic/tests/unit/db/test_nodes.py b/ironic/tests/unit/db/test_nodes.py index 2ca7eb5e9e..92e315eb10 100644 --- a/ironic/tests/unit/db/test_nodes.py +++ b/ironic/tests/unit/db/test_nodes.py @@ -20,6 +20,7 @@ from unittest import mock from oslo_utils import timeutils from oslo_utils import uuidutils +from sqlalchemy.orm import exc as sa_exc from ironic.common import exception from ironic.common import states @@ -302,6 +303,20 @@ class DbNodeTestCase(base.DbTestCase): self.assertEqual([], r.tags) self.assertEqual([], r.traits) + def test_get_node_list_includes_traits(self): + uuids = [] + for i in range(1, 6): + node = utils.create_test_node(uuid=uuidutils.generate_uuid()) + uuids.append(str(node['uuid'])) + self.dbapi.set_node_traits(node.id, ['trait1', 'trait2'], '1.35') + + res = self.dbapi.get_node_list() + res_uuids = [r.uuid for r in res] + self.assertCountEqual(uuids, res_uuids) + for r in res: + self.assertEqual([], r.tags) + self.assertEqual(2, len(r.traits)) + def test_get_node_list_with_filters(self): ch1 = utils.create_test_chassis(uuid=uuidutils.generate_uuid()) ch2 = utils.create_test_chassis(uuid=uuidutils.generate_uuid()) @@ -446,6 +461,140 @@ class DbNodeTestCase(base.DbTestCase): self.dbapi.get_node_list, {'chassis_uuid': uuidutils.generate_uuid()}) + def test_get_node_list_requested_fields_with_traits(self): + # Checks to to ensure we're not returning a node object with all + # fields populated as this is a high overhead for SQLAlchemy to do + # all of the object conversions, when we have fields which were not + # requested nor required. + # Modeled after the nova query which is used to collect node state + uuids = [] + for i in range(1, 6): + node = utils.create_test_node(uuid=uuidutils.generate_uuid(), + provision_state=states.AVAILABLE, + power_state=states.POWER_OFF, + target_power_state=None, + target_provision_state=None, + last_error=None, + maintenance=False, + properties={'cpu': 'x86_64'}, + instance_uuid=None, + resource_class='CUSTOM_BAREMETAL', + # Code requires the fields below + owner='fred', + lessee='marsha', + # Fields that should not be + # present in the obejct. + driver_internal_info={ + 'cat': 'meow'}, + internal_info={'corgi': 'rocks'}, + deploy_interface='purring_machine') + # Add some traits for good measure + self.dbapi.set_node_traits(node.id, ['trait1', 'trait2'], '1.35') + uuids.append(str(node['uuid'])) + req_fields = ['uuid', + 'power_state', + 'target_power_state', + 'provision_state', + 'target_provision_state', + 'last_error', + 'maintenance', + 'properties', + 'instance_uuid', + 'resource_class', + 'traits', + 'version', + 'updated_at', + 'created_at'] + + res = self.dbapi.get_node_list(fields=req_fields) + res_uuids = [r.uuid for r in res] + self.assertCountEqual(uuids, res_uuids) + for r in res: + self.assertIsNotNone(r.traits) + self.assertIsNotNone(r.version) + self.assertEqual(states.AVAILABLE, r.provision_state) + self.assertEqual(states.POWER_OFF, r.power_state) + self.assertIsNone(r.target_power_state) + self.assertIsNone(r.target_provision_state) + self.assertIsNone(r.last_error) + self.assertFalse(r.maintenance) + self.assertIsNone(r.instance_uuid) + self.assertEqual('CUSTOM_BAREMETAL', r.resource_class) + self.assertEqual('trait1', r.traits[0]['trait']) + self.assertEqual('trait2', r.traits[1]['trait']) + # These always need to be returned, even if not requested. + # These should always be empty values as they are not populated + # due to the object not returning a value in the field to save on + # excess un-necessary data conversions. + + def _attempt_field_access(obj, field): + return obj[field] + + for field in ['driver_internal_info', 'internal_info', + 'deploy_interface', 'boot_interface', + 'driver', 'extra']: + try: + self.assertRaises(sa_exc.DetachedInstanceError, + _attempt_field_access, r, field) + except AttributeError: + pass + + def test_get_node_list_requested_fields_no_traits(self): + # The join for traits handling requires some special handling + # so in this case we execute without traits being joined in. + uuids = [] + for i in range(1, 3): + node = utils.create_test_node(uuid=uuidutils.generate_uuid(), + provision_state=states.AVAILABLE, + last_error=None, + maintenance=False, + resource_class='CUSTOM_BAREMETAL', + # Code requires the fields below + owner='fred', + lessee='marsha', + # Fields that should not be + # present in the object. + driver_internal_info={ + 'cat': 'meow'}, + internal_info={'corgi': 'rocks'}, + deploy_interface='purring_machine') + uuids.append(str(node['uuid'])) + req_fields = ['uuid', + 'provision_state', + 'last_error', + 'owner', + 'lessee', + 'version'] + + res = self.dbapi.get_node_list(fields=req_fields) + res_uuids = [r.uuid for r in res] + self.assertCountEqual(uuids, res_uuids) + for r in res: + self.assertIsNotNone(r.version) + self.assertEqual(states.AVAILABLE, r.provision_state) + self.assertIsNone(r.last_error) + # These always need to be returned, even if not requested. + self.assertEqual('fred', r.owner) + self.assertEqual('marsha', r.lessee) + # These should always be empty values as they are not populated + # due to the object not returning a value in the field to save on + # excess un-necessary data conversions. + + def _attempt_field_access(obj, field): + return obj[field] + + for field in ['driver_internal_info', 'internal_info', + 'deploy_interface', 'boot_interface', + 'driver', 'extra', 'power_state', + 'traits']: + try: + self.assertRaises(sa_exc.DetachedInstanceError, + _attempt_field_access, r, field) + except AttributeError: + # We expect an AttributeError, in addition to + # SQLAlchemy raising an exception. + pass + def test_get_node_by_instance(self): node = utils.create_test_node( instance_uuid='12345678-9999-0000-aaaa-123456789012') diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index ab2b9cec88..8d8b2a8e5f 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/ironic/tests/unit/objects/test_node.py @@ -397,6 +397,17 @@ class TestNodeObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): self.assertIsInstance(nodes[0], objects.Node) self.assertEqual(self.context, nodes[0]._context) + def test_list_with_fields(self): + with mock.patch.object(self.dbapi, 'get_node_list', + autospec=True) as mock_get_list: + mock_get_list.return_value = [self.fake_node] + objects.Node.list(self.context, fields=['name']) + mock_get_list.assert_called_with( + filters=None, limit=None, marker=None, sort_key=None, + sort_dir=None, + fields=['id', 'name', 'version', 'updated_at', 'created_at', + 'owner', 'lessee', 'driver', 'conductor_group']) + def test_reserve(self): with mock.patch.object(self.dbapi, 'reserve_node', autospec=True) as mock_reserve: