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: