From 9aafd5f131aad6c885ef2eafdfedb8aea14967d4 Mon Sep 17 00:00:00 2001 From: Doug Wiegley Date: Fri, 11 Jan 2019 15:14:52 -0700 Subject: [PATCH] Fix slow SG api calls when limiting fields Relating to the issue with creating rules, when reading security groups, it is very slow. Even when limiting to id/name, it pulls in all rules before returning just id/name. This change looks at the fields requested, and if no "synthetic" fields are in the list, skips initializing those. Co-Authored-By: Hongbin Lu Closes-Bug: #1810563 Change-Id: Id6870633e3943666e9b7fb900ad2d0894ee2715d --- neutron/db/securitygroups_db.py | 31 ++++++++------ neutron/objects/base.py | 41 ++++++++++++++----- neutron/objects/securitygroup.py | 6 ++- .../tests/unit/objects/test_securitygroup.py | 32 +++++++++++++++ 4 files changed, 86 insertions(+), 24 deletions(-) diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py index 53c74d50739..95e80730fe9 100644 --- a/neutron/db/securitygroups_db.py +++ b/neutron/db/securitygroups_db.py @@ -161,7 +161,8 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): sorts=sorts, limit=limit, marker=marker, page_reverse=page_reverse) sg_objs = sg_obj.SecurityGroup.get_objects( - context, _pager=pager, validate_filters=False, **filters) + context, _pager=pager, validate_filters=False, + fields=fields, **filters) return [self._make_security_group_dict(obj, fields) for obj in sg_objs] @@ -176,7 +177,6 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): """Tenant id is given to handle the case when creating a security group rule on behalf of another use. """ - if tenant_id: tmp_context_tenant_id = context.tenant_id context.tenant_id = tenant_id @@ -184,16 +184,22 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): try: with db_api.context_manager.reader.using(context): ret = self._make_security_group_dict(self._get_security_group( - context, id), fields) - ret['security_group_rules'] = self.get_security_group_rules( - context, {'security_group_id': [id]}) + context, id, + fields=fields), + fields) + if (fields is None or len(fields) == 0 or + 'security_group_rules' in fields): + rules = self.get_security_group_rules(context, + {'security_group_id': [id]}) + ret['security_group_rules'] = rules + finally: if tenant_id: context.tenant_id = tmp_context_tenant_id return ret - def _get_security_group(self, context, id): - sg = sg_obj.SecurityGroup.get_object(context, id=id) + def _get_security_group(self, context, id, fields=None): + sg = sg_obj.SecurityGroup.get_object(context, fields=fields, id=id) if sg is None: raise ext_sg.SecurityGroupNotFound(id=id) return sg @@ -218,7 +224,7 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): if ports: raise ext_sg.SecurityGroupInUse(id=id) # confirm security group exists - sg = self._get_security_group(context, id) + sg = self._get_security_group(context, id, fields=['id', 'name']) if sg['name'] == 'default' and not context.is_admin: raise ext_sg.SecurityGroupCannotRemoveDefault() @@ -291,10 +297,11 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase): 'name': security_group['name'], 'tenant_id': security_group['tenant_id'], 'description': security_group['description']} - res['security_group_rules'] = [ - self._make_security_group_rule_dict(r.db_obj) - for r in security_group.rules - ] + if security_group.rules: + res['security_group_rules'] = [ + self._make_security_group_rule_dict(r.db_obj) + for r in security_group.rules + ] resource_extend.apply_funcs(ext_sg.SECURITYGROUPS, res, security_group.db_obj) return db_utils.resource_fields(res, fields) diff --git a/neutron/objects/base.py b/neutron/objects/base.py index f841b5bad6b..a0f0c5974a7 100644 --- a/neutron/objects/base.py +++ b/neutron/objects/base.py @@ -137,6 +137,7 @@ class NeutronObject(obj_base.VersionedObject, def __init__(self, context=None, **kwargs): super(NeutronObject, self).__init__(context, **kwargs) + self._load_synthetic_fields = True self.obj_set_defaults() def _synthetic_fields_items(self): @@ -223,7 +224,7 @@ class NeutronObject(obj_base.VersionedObject, return obj @classmethod - def get_object(cls, context, **kwargs): + def get_object(cls, context, fields=None, **kwargs): raise NotImplementedError() @classmethod @@ -252,7 +253,7 @@ class NeutronObject(obj_base.VersionedObject, @classmethod @abc.abstractmethod def get_objects(cls, context, _pager=None, validate_filters=True, - **kwargs): + fields=None, **kwargs): raise NotImplementedError() @classmethod @@ -443,7 +444,8 @@ class NeutronDbObject(NeutronObject): for field in self.fields: if field in fields and not self.is_synthetic(field): setattr(self, field, fields[field]) - self.load_synthetic_db_fields(db_obj) + if self._load_synthetic_fields: + self.load_synthetic_db_fields(db_obj) self._captured_db_model = db_obj self.obj_reset_changes() @@ -497,8 +499,13 @@ class NeutronDbObject(NeutronObject): return result @classmethod - def _load_object(cls, context, db_obj): + def _load_object(cls, context, db_obj, fields=None): obj = cls(context) + + if fields is not None and len(fields) != 0: + if len(set(fields).intersection(set(cls.synthetic_fields))) == 0: + obj._load_synthetic_fields = False + obj.from_db_object(db_obj) # detach the model so that consequent fetches don't reuse it context.session.expunge(obj.db_obj) @@ -533,12 +540,18 @@ class NeutronDbObject(NeutronObject): return db_api.autonested_transaction(context.session) @classmethod - def get_object(cls, context, **kwargs): - """ + def get_object(cls, context, fields=None, **kwargs): + """Fetch a single object + Return the first result of given context or None if the result doesn't contain any row. Next, convert it to a versioned object. :param context: + :param fields: indicate which fields the caller is interested in + using. Note that currently this is limited to + avoid loading synthetic fields when possible, and + does not affect db queries. Default is None, which + is the same as []. Example: ['id', 'name'] :param kwargs: multiple keys defined by key=value pairs :return: single object of NeutronDbObject class or None """ @@ -553,12 +566,13 @@ class NeutronDbObject(NeutronObject): db_obj = obj_db_api.get_object( cls, context, **cls.modify_fields_to_db(kwargs)) if db_obj: - return cls._load_object(context, db_obj) + return cls._load_object(context, db_obj, fields=fields) @classmethod def get_objects(cls, context, _pager=None, validate_filters=True, - **kwargs): - """ + fields=None, **kwargs): + """Fetch a list of objects + Fetch all results from DB and convert them to versioned objects. :param context: @@ -566,6 +580,11 @@ class NeutronDbObject(NeutronObject): criteria :param validate_filters: Raises an error in case of passing an unknown filter + :param fields: indicate which fields the caller is interested in + using. Note that currently this is limited to + avoid loading synthetic fields when possible, and + does not affect db queries. Default is None, which + is the same as []. Example: ['id', 'name'] :param kwargs: multiple keys defined by key=value pairs :return: list of objects of NeutronDbObject class or empty list """ @@ -574,7 +593,9 @@ class NeutronDbObject(NeutronObject): with cls.db_context_reader(context): db_objs = obj_db_api.get_objects( cls, context, _pager=_pager, **cls.modify_fields_to_db(kwargs)) - return [cls._load_object(context, db_obj) for db_obj in db_objs] + + return [cls._load_object(context, db_obj, fields=fields) + for db_obj in db_objs] @classmethod def update_object(cls, context, values, validate_filters=True, **kwargs): diff --git a/neutron/objects/securitygroup.py b/neutron/objects/securitygroup.py index 79cb5ca9c13..f807051130c 100644 --- a/neutron/objects/securitygroup.py +++ b/neutron/objects/securitygroup.py @@ -59,8 +59,10 @@ class SecurityGroup(base.NeutronDbObject): def from_db_object(self, db_obj): super(SecurityGroup, self).from_db_object(db_obj) - setattr(self, 'is_default', bool(db_obj.get('default_security_group'))) - self.obj_reset_changes(['is_default']) + if self._load_synthetic_fields: + setattr(self, 'is_default', + bool(db_obj.get('default_security_group'))) + self.obj_reset_changes(['is_default']) @base.NeutronObjectRegistry.register diff --git a/neutron/tests/unit/objects/test_securitygroup.py b/neutron/tests/unit/objects/test_securitygroup.py index b99d804a14e..54b8a04d43b 100644 --- a/neutron/tests/unit/objects/test_securitygroup.py +++ b/neutron/tests/unit/objects/test_securitygroup.py @@ -97,6 +97,38 @@ class SecurityGroupDbObjTestCase(test_base.BaseDbObjectTestCase, # follow-up patch. pass + def test_get_object_no_synth(self): + fields = self.obj_fields[0].copy() + sg_obj = self._make_object(fields) + sg_obj.is_default = True + sg_obj.create() + + listed_obj = securitygroup.SecurityGroup.get_object( + self.context, + fields=['id', 'name'], + id=sg_obj.id, + project_id=sg_obj.project_id + ) + self.assertIsNotNone(listed_obj) + self.assertEqual(len(sg_obj.rules), 0) + self.assertIsNone(listed_obj.rules) + + def test_get_objects_no_synth(self): + fields = self.obj_fields[0].copy() + sg_obj = self._make_object(fields) + sg_obj.is_default = True + sg_obj.create() + + listed_objs = securitygroup.SecurityGroup.get_objects( + self.context, + fields=['id', 'name'], + id=sg_obj.id, + project_id=sg_obj.project_id + ) + self.assertEqual(len(listed_objs), 1) + self.assertEqual(len(sg_obj.rules), 0) + self.assertIsNone(listed_objs[0].rules) + class DefaultSecurityGroupIfaceObjTestCase(test_base.BaseObjectIfaceTestCase):