Merge "API to pass fields to node object list"

5 files changed, 120 insertions, 61 deletions

diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py
index 760ef8261..7aad68286 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 b4103dbe2..e75e5ad3b 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 3c3996196..bcd34abde 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 6cbda0ba5..313495161 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 000000000..5aee33b20
--- /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.