openstack/ironic
Code Issues Proposed changes

Set stage for objects to handle selected field lists.

Browse Source 
Prior to this change, ironic would not pass the API consumer request
for a list of nodes down to the underlying layers with the list of
specific fields being requested.

This resulted in full objects being returned from the database where
in essence the entire database would be downloaded to construct the
objects, and in the case of joined views, whole roles would largely
be duplicated.

In order to be more efficent, we need to hand the user desired
fields down to the database, in order to return only that data,
and thus transform it.

In this case, we already have testing that handles the conversion
of objects at the lower layer, and in this case, the db object
conversion handler already understood fields, so we're just kind
of completing the awareness further downward for increased efficency.

This change only sets the stage, the final change to this is aligned
with API change to leverage this as the API is coded such that the
field does not include all of the required fields needed by the API
to render replies, which is fixed in the API patch to leverage this
with the API.

Story: 2008885
Task: 42495
Change-Id: I6283c4cc1b1ff608c4be24a6c41eb7b430a5ad68
This commit is contained in:
Julia Kreger 2021-05-19 14:29:03 -07:00
parent 644ba5d4bc
commit 7b37e03b69
3 changed files with 96 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
ironic/objects/node.py
View File

@ -213,12 +213,16 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat):
raise exception.InvalidParameterValue(msg)
def _set_from_db_object(self, context, db_object, fields=None):
fields = set(fields or self.fields) - {'traits'}
super(Node, self)._set_from_db_object(context, db_object, fields)
self.traits = object_base.obj_make_list(
context, objects.TraitList(context),
objects.Trait, db_object['traits'])
self.traits.obj_reset_changes()
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'],
fields=['trait', 'version'])
self.traits.obj_reset_changes()
# 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.
@ -328,7 +332,6 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat):
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

32
ironic/tests/unit/api/controllers/v1/test_node.py
View File

@ -498,6 +498,38 @@ class TestListNodes(test_api_base.BaseApiTest):
# We always append "links"
self.assertCountEqual(['uuid', 'instance_info', 'links'], node)
def test_get_collection_fields_for_nova(self):
# Unit test which explicitly attempts to request traits along with
# all columns which nova would normally request as part of it's sync
# process.
fields = ('uuid,power_state,target_power_state,provision_state,'
'target_provision_state,last_error,maintenance,'
'instance_uuid,traits,resource_class')
trait_list = ['CUSTOM_TRAIT1', 'CUSTOM_RAID5']
for i in range(3):
node = obj_utils.create_test_node(
self.context, uuid=uuidutils.generate_uuid(),
instance_uuid=uuidutils.generate_uuid())
self.dbapi.set_node_traits(node.id, trait_list, '1.0')
data = self.get_json(
'/nodes?fields=%s' % fields,
headers={api_base.Version.string: str(api_v1.max_version())})
self.assertEqual(3, len(data['nodes']))
for node in data['nodes']:
# We always append "links"
self.assertCountEqual([
'uuid', 'power_state', 'target_power_state',
'provision_state', 'target_provision_state', 'last_error',
'maintenance', 'instance_uuid', 'traits', 'resource_class',
'links'], node)
# Ensure traits exists that have been created on the node.
# Sorted traits list is an artifact of the data being inserted
# as distinct rows and then collected back into a list.
# There is no contract around ordering for the results of traits.
self.assertEqual(sorted(trait_list, reverse=False), node['traits'])
def test_get_custom_fields_invalid_fields(self):
node = obj_utils.create_test_node(self.context,
chassis_id=self.chassis.id)

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

@ -396,17 +396,68 @@ class TestNodeObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn):
self.assertThat(nodes, matchers.HasLength(1))
self.assertIsInstance(nodes[0], objects.Node)
self.assertEqual(self.context, nodes[0]._context)
self.assertIsInstance(nodes[0].traits, objects.TraitList)
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'])
nodes = objects.Node.list(self.context,
fields=['name', 'uuid',
'provision_state'])
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'])
fields=['id', 'name', 'uuid', 'provision_state', 'version',
'updated_at', 'created_at', 'owner', 'lessee',
'driver', 'conductor_group'])
self.assertThat(nodes, matchers.HasLength(1))
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']:
self.assertFalse(field not in dir(nodes[0]))
def test_list_with_fields_traits(self):
with mock.patch.object(self.dbapi, 'get_node_list',
autospec=True) as mock_get_list:
# Trait objects and ultimately rows have an underlying
# version. Since we've never changed it, it should just
# be one, but this is required for the oslo versioned
# objects code path to handle objects properly and
# navigate mid-upgrade cases.
self.fake_node['traits'] = [{
'trait': 'traitx', 'version': "1"}]
mock_get_list.return_value = [self.fake_node]
nodes = objects.Node.list(self.context,
fields=['uuid', 'provision_state',
'traits'])
self.assertThat(nodes, matchers.HasLength(1))
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)
self.assertEqual('traitx', nodes[0].traits[0].trait)
for field in ['power_state', 'instance_info', 'resource_class',
'automated_clean', 'properties', 'driver']:
self.assertFalse(field not in dir(nodes[0]))
def test_list_with_fields_empty_trait_present(self):
with mock.patch.object(self.dbapi, 'get_node_list',
autospec=True) as mock_get_list:
mock_get_list.return_value = [self.fake_node]
nodes = objects.Node.list(self.context,
fields=['uuid', 'provision_state',
'traits'])
self.assertThat(nodes, matchers.HasLength(1))
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)
self.assertIn('traits', nodes[0])
def test_reserve(self):
with mock.patch.object(self.dbapi, 'reserve_node',