From d075028146a6fcec57e05fc4952682bb2ad49b58 Mon Sep 17 00:00:00 2001 From: Doug Wiegley Date: Sun, 3 Mar 2019 10:42:16 -0700 Subject: [PATCH] Use dynamic lazy mode for fetching security group rules In conjunction with the prior fix to only get a subset of fields when needed, this makes the querying of non-rules SG objects very very fast. Before the two fixes, if you have about ten security groups with 2000 rules each: list all: 14s list all, just 'id' field: 14s list one: 0.6s list one, just 'id' field: 0.6s With just the previous partial fix: list all: 14s list all, just 'id' field: 6s list one: 0.6s list one, just 'id' field: 0.2s Now with this change: list all: 14s list all, just 'id' field: 0.04s list one: 0.6s list one, just 'id' field: 0.03s Closes-Bug: #1810563 Change-Id: I15df276ba7dbcb3763ab20b63b26cddf2d594954 (cherry picked from commit 1e9086f6e269f32d1cb19a0f55eb1f5dd73ca651) --- neutron/db/models/securitygroup.py | 2 +- neutron/objects/base.py | 61 +++++++++++++++++++++++-- neutron/objects/common_types.py | 23 ++++++++++ neutron/objects/securitygroup.py | 4 +- neutron/tests/unit/objects/test_base.py | 13 +++++- 5 files changed, 94 insertions(+), 9 deletions(-) diff --git a/neutron/db/models/securitygroup.py b/neutron/db/models/securitygroup.py index 8781d9fc42b..4875492d077 100644 --- a/neutron/db/models/securitygroup.py +++ b/neutron/db/models/securitygroup.py @@ -88,7 +88,7 @@ class SecurityGroupRule(standard_attr.HasStandardAttributes, model_base.BASEV2, remote_ip_prefix = sa.Column(sa.String(255)) security_group = orm.relationship( SecurityGroup, load_on_pending=True, - backref=orm.backref('rules', cascade='all,delete', lazy='subquery'), + backref=orm.backref('rules', cascade='all,delete', lazy='dynamic'), primaryjoin="SecurityGroup.id==SecurityGroupRule.security_group_id") source_group = orm.relationship( SecurityGroup, diff --git a/neutron/objects/base.py b/neutron/objects/base.py index 8ecca260a29..7c881c00b7d 100644 --- a/neutron/objects/base.py +++ b/neutron/objects/base.py @@ -32,6 +32,7 @@ from sqlalchemy import orm from neutron._i18n import _ from neutron.db import api as db_api from neutron.db import standard_attr +from neutron.objects import common_types from neutron.objects.db import api as obj_db_api from neutron.objects.extensions import standardattributes @@ -62,6 +63,25 @@ def register_filter_hook_on_model(model, filter_name): obj_class.add_extra_filter_name(filter_name) +class LazyQueryIterator(six.Iterator): + def __init__(self, obj_class, lazy_query): + self.obj_class = obj_class + self.context = None + self.query = lazy_query + + def __iter__(self): + self.results = self.query.all() + self.i = 0 + return self + + def __next__(self): + if self.i >= len(self.results): + raise StopIteration() + item = self.obj_class._load_object(self.context, self.results[self.i]) + self.i += 1 + return item + + class Pager(object): ''' This class represents a pager object. It is consumed by get_objects to @@ -136,6 +156,11 @@ class NeutronObject(obj_base.VersionedObject, synthetic_fields = [] extra_filter_names = set() + # To use lazy queries for child objects, you must set the ORM + # relationship in the db model to 'dynamic'. By default, all + # children are eager loaded. + lazy_fields = set() + def __init__(self, context=None, **kwargs): super(NeutronObject, self).__init__(context, **kwargs) self._load_synthetic_fields = True @@ -161,7 +186,8 @@ class NeutronObject(obj_base.VersionedObject, dict_[name] = value for field_name, value in self._synthetic_fields_items(): field = self.fields[field_name] - if isinstance(field, obj_fields.ListOfObjectsField): + if (isinstance(field, obj_fields.ListOfObjectsField) or + isinstance(field, common_types.ListOfObjectsField)): dict_[field_name] = [obj.to_dict() for obj in value] elif isinstance(field, obj_fields.ObjectField): dict_[field_name] = ( @@ -177,7 +203,8 @@ class NeutronObject(obj_base.VersionedObject, @classmethod def is_object_field(cls, field): return (isinstance(cls.fields[field], obj_fields.ListOfObjectsField) or - isinstance(cls.fields[field], obj_fields.ObjectField)) + isinstance(cls.fields[field], common_types.ListOfObjectsField) or + isinstance(cls.fields[field], obj_fields.ObjectField)) @classmethod def obj_class_from_name(cls, objname, objver): @@ -440,8 +467,15 @@ class NeutronDbObject(NeutronObject): '''Return a database model that persists object data.''' return self._captured_db_model + def _set_lazy_contexts(self, fields, context): + for field in self.lazy_fields.intersection(fields): + if isinstance(fields[field], LazyQueryIterator): + fields[field].context = context + def from_db_object(self, db_obj): fields = self.modify_fields_from_db(db_obj) + if self.lazy_fields: + self._set_lazy_contexts(fields, self.obj_context) for field in self.fields: if field in fields and not self.is_synthetic(field): setattr(self, field, fields[field]) @@ -470,12 +504,23 @@ class NeutronDbObject(NeutronObject): :param fields: dict of fields from NeutronDbObject :return: modified dict of fields """ + for k, v in fields.items(): + if isinstance(v, LazyQueryIterator): + fields[k] = list(v) result = copy.deepcopy(dict(fields)) for field, field_db in cls.fields_need_translation.items(): if field in result: result[field_db] = result.pop(field) return result + @classmethod + def _get_lazy_iterator(cls, field, appender_query): + if field not in cls.lazy_fields: + raise KeyError(_('Field %s is not a lazy query field') % field) + n_obj_classes = NeutronObjectRegistry.obj_classes() + n_obj = n_obj_classes.get(cls.fields[field].objname) + return LazyQueryIterator(n_obj[0], appender_query) + @classmethod def modify_fields_from_db(cls, db_obj): """Modify the fields after data were fetched from DB. @@ -501,6 +546,8 @@ class NeutronDbObject(NeutronObject): # don't allow sqlalchemy lists to propagate outside if isinstance(v, orm.collections.InstrumentedList): result[k] = list(v) + if isinstance(v, orm.dynamic.AppenderQuery): + result[k] = cls._get_lazy_iterator(k, v) return result @classmethod @@ -513,7 +560,8 @@ class NeutronDbObject(NeutronObject): obj.from_db_object(db_obj) # detach the model so that consequent fetches don't reuse it - context.session.expunge(obj.db_obj) + if context is not None: + context.session.expunge(obj.db_obj) return obj def obj_load_attr(self, attrname): @@ -744,8 +792,9 @@ class NeutronDbObject(NeutronObject): # subclasses=True for field in self.synthetic_fields: try: + field_def = self.fields[field] objclasses = NeutronObjectRegistry.obj_classes( - ).get(self.fields[field].objname) + ).get(field_def.objname) except AttributeError: # NOTE(rossella_s) this is probably because this field is not # an ObjectField @@ -772,7 +821,9 @@ class NeutronDbObject(NeutronObject): # synth_db_objs can be list, empty list or None, that is why # we need 'is not None', because [] is valid case for 'True' if synth_db_objs is not None: - if not isinstance(synth_db_objs, list): + if isinstance(synth_db_objs, orm.dynamic.AppenderQuery): + synth_db_objs = list(synth_db_objs) + elif not isinstance(synth_db_objs, list): synth_db_objs = [synth_db_objs] synth_objs = [objclass._load_object(self.obj_context, obj) for obj in synth_db_objs] diff --git a/neutron/objects/common_types.py b/neutron/objects/common_types.py index b96cb80e420..1ce60e6e403 100644 --- a/neutron/objects/common_types.py +++ b/neutron/objects/common_types.py @@ -11,6 +11,7 @@ # License for the specific language governing permissions and limitations # under the License. +import collections import itertools import uuid @@ -313,3 +314,25 @@ class FloatingIPStatusEnumField(obj_fields.AutoTypedField): class RouterStatusEnumField(obj_fields.AutoTypedField): AUTO_TYPE = obj_fields.Enum(valid_values=constants.VALID_ROUTER_STATUS) + + +# Duplicate some fixes in later oslo.versionedobjects, so we can backport +# fixes without modifying requirements. +class List(obj_fields.List): + def coerce(self, obj, attr, value): + if (not isinstance(value, collections.Iterable) or + isinstance(value, six.string_types + (collections.Mapping,))): + raise ValueError(_('A list is required in field %(attr)s, ' + 'not a %(type)s') % + {'attr': attr, 'type': type(value).__name__}) + coerced_list = obj_fields.CoercedList() + coerced_list.enable_coercing(self._element_type, obj, attr) + coerced_list.extend(value) + return coerced_list + + +class ListOfObjectsField(obj_fields.AutoTypedField): + def __init__(self, objtype, subclasses=False, **kwargs): + self.AUTO_TYPE = List(obj_fields.Object(objtype, subclasses)) + self.objname = objtype + super(ListOfObjectsField, self).__init__(**kwargs) diff --git a/neutron/objects/securitygroup.py b/neutron/objects/securitygroup.py index f807051130c..f76093f7539 100644 --- a/neutron/objects/securitygroup.py +++ b/neutron/objects/securitygroup.py @@ -30,7 +30,7 @@ class SecurityGroup(base.NeutronDbObject): 'name': obj_fields.StringField(nullable=True), 'project_id': obj_fields.StringField(nullable=True), 'is_default': obj_fields.BooleanField(default=False), - 'rules': obj_fields.ListOfObjectsField( + 'rules': common_types.ListOfObjectsField( 'SecurityGroupRule', nullable=True ), # NOTE(ihrachys): we don't include source_rules that is present in the @@ -43,6 +43,8 @@ class SecurityGroup(base.NeutronDbObject): extra_filter_names = {'is_default'} + lazy_fields = set(['rules']) + def create(self): # save is_default before super() resets it to False is_default = self.is_default diff --git a/neutron/tests/unit/objects/test_base.py b/neutron/tests/unit/objects/test_base.py index a538deebe2a..716c7d8d3d5 100644 --- a/neutron/tests/unit/objects/test_base.py +++ b/neutron/tests/unit/objects/test_base.py @@ -29,6 +29,7 @@ from oslo_utils import uuidutils from oslo_versionedobjects import base as obj_base from oslo_versionedobjects import exception from oslo_versionedobjects import fields as obj_fields +from sqlalchemy import orm import testtools from neutron.db import _model_query as model_query @@ -484,6 +485,7 @@ FIELD_TYPE_VALUE_GENERATOR_MAP = { common_types.IPVersionEnumField: tools.get_random_ip_version, common_types.IpProtocolEnumField: tools.get_random_ip_protocol, common_types.ListOfIPNetworksField: get_list_of_random_networks, + common_types.ListOfObjectsField: lambda: [], common_types.MACAddressField: tools.get_random_EUI, common_types.PortBindingStatusEnumField: tools.get_random_port_binding_statuses, @@ -1217,7 +1219,10 @@ class BaseObjectIfaceTestCase(_BaseObjectTestCase, test_base.BaseTestCase): self.get_random_db_fields(obj_cls=objclass)) ) child_dict = child.to_dict() - if isinstance(cls_.fields[field], obj_fields.ListOfObjectsField): + if (isinstance(cls_.fields[field], + obj_fields.ListOfObjectsField) or + isinstance(cls_.fields[field], + common_types.ListOfObjectsField)): setattr(obj, field, [child]) dict_ = obj.to_dict() self.assertEqual([child_dict], dict_[field]) @@ -1986,7 +1991,11 @@ class BaseDbObjectTestCase(_BaseObjectTestCase, obj.update() self.assertIsNotNone(obj.db_obj) for k, v in obj.modify_fields_to_db(fields_to_update).items(): - self.assertEqual(v, obj.db_obj[k], '%s attribute differs' % k) + if isinstance(obj.db_obj[k], orm.dynamic.AppenderQuery): + self.assertIsInstance(v, list) + else: + self.assertEqual(v, obj.db_obj[k], + '%s attribute differs' % k) obj.delete() self.assertIsNone(obj.db_obj)