From e98a268de4316c4f889925a3da6266e1c70306aa Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 3 Aug 2020 11:08:19 -0400 Subject: [PATCH] Propose replacement of ORM from_self() The from_self() method in SQLAlchemy is currently being considered for removal from the library, with a deprecation phase throughout 1.4 and then removal by SQLAlchemy 2.0. The from_self() method takes an ORM query object, turns it into a subquery, then returns a new query object that will SELECT from that subquery, while transparently altering subsequent criteria added to the query to be stated in terms of the subquery. The current design direction of SQLAlchemy hopes to de-emphasize the "transparently altering criteria" part of the above use case, and to move users towards a more explicit and model of usage where a subquery should be created and used explicitly using the aliased() construct, which is now very mature and can be used in ways that were not available when from_self() was first introduced. On the SQLAlchemy side, from_self() has proven to be one of the most difficult features to maintain and test as it can easily lead to extremely complicated scenarios, and while I am also experimenting with some alternatives that may still retain some of the "automatic translation" features, those features are still proving to add similar internal complexity which is having me lean towards the original plan of removing open-ended "entity translation" features like that of from_self() at least through the start of the 2.0 series. A code search for all of Openstack shows that the two files modified here are the only usages of the from_self() method throughout all of searchable Openstack code. This speaks to the general obscurity of this method, although neutron's Subnet code is actually using this method as intended. The new approach necessarily changes some of the method signatures here so that the explicit "subquery" entity can be passed; code searches again show that these methods are not being called anywhere outside, so the query_filter_service_subnets method becomes the private _query_entity_service_subnets method. References: https://github.com/sqlalchemy/sqlalchemy/issues/5368 Closes-Bug: #2004263 Change-Id: Icec998873221ac8e6a1566a157b2044c1f6cd7f3 --- neutron/objects/subnet.py | 44 ++++++++++++------- neutron/plugins/ml2/drivers/l2pop/db.py | 15 +++++-- .../unit/plugins/ml2/drivers/l2pop/test_db.py | 6 +-- 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/neutron/objects/subnet.py b/neutron/objects/subnet.py index 9853556d9ac..3b896151ea5 100644 --- a/neutron/objects/subnet.py +++ b/neutron/objects/subnet.py @@ -21,6 +21,7 @@ from oslo_log import log as logging from oslo_utils import versionutils from oslo_versionedobjects import fields as obj_fields from sqlalchemy import and_, or_ +from sqlalchemy import orm from sqlalchemy.sql import exists from neutron.db.models import dns as dns_models @@ -168,7 +169,7 @@ class SubnetServiceType(base.NeutronDbObject): } @classmethod - def query_filter_service_subnets(cls, query, service_type): + def _query_entity_service_subnets(cls, query, service_type): # TODO(tuanvu): find OVO-like solution for handling "join queries" Subnet = models_v2.Subnet ServiceType = subnet_service_type.SubnetServiceType @@ -181,7 +182,9 @@ class SubnetServiceType(base.NeutronDbObject): # service type when DHCP is enabled on the subnet. and_(Subnet.enable_dhcp.is_(True), service_type == const.DEVICE_OWNER_DHCP))) - return query.from_self(Subnet) + + entity = orm.aliased(Subnet, query.subquery()) + return entity # RBAC metaclass is not applied here because 'shared' attribute of Subnet @@ -325,9 +328,12 @@ class Subnet(base.NeutronDbObject): distributed_service=False): """Find canditate subnets for the network, host, and service_type""" query = cls.query_subnets_on_network(context, network_id) - query = SubnetServiceType.query_filter_service_subnets( + + subnet_entity = SubnetServiceType._query_entity_service_subnets( query, service_type) + query = context.session.query(subnet_entity) + # Select candidate subnets and return them if not cls.is_host_set(host): if fixed_configured: @@ -336,14 +342,18 @@ class Subnet(base.NeutronDbObject): # on port update with binding:host_id set. Allocation _cannot_ # be deferred as requested fixed_ips would then be lost. return cls._query_filter_by_fixed_ips_segment( - query, fixed_ips, + query, fixed_ips, subnet_entity, allow_multiple_segments=distributed_service).all() # If the host isn't known, we can't allocate on a routed network. # So, exclude any subnets attached to segments. - return cls._query_exclude_subnets_on_segments(query).all() + return cls._query_exclude_subnets_on_segments( + query, subnet_entity + ).all() # The host is known. Consider both routed and non-routed networks - results = cls._query_filter_by_segment_host_mapping(query, host).all() + results = cls._query_filter_by_segment_host_mapping( + query, subnet_entity, host + ).all() # For now, we know that OVS agent is supporting multi-segments. segment_ids = {subnet.segment_id @@ -359,6 +369,7 @@ class Subnet(base.NeutronDbObject): @classmethod def _query_filter_by_fixed_ips_segment(cls, query, fixed_ips, + subnet_entity, allow_multiple_segments=False): """Excludes subnets not on the same segment as fixed_ips @@ -366,7 +377,6 @@ class Subnet(base.NeutronDbObject): """ segment_ids = [] subnets = query.all() - for fixed_ip in fixed_ips: subnet = None if 'subnet_id' in fixed_ip: @@ -404,10 +414,10 @@ class Subnet(base.NeutronDbObject): return query segment_id = None if not segment_ids else segment_ids[0] - return query.filter(cls.db_model.segment_id == segment_id) + return query.filter(subnet_entity.segment_id == segment_id) @classmethod - def _query_filter_by_segment_host_mapping(cls, query, host): + def _query_filter_by_segment_host_mapping(cls, query, subnet_entity, host): # TODO(tuanvu): find OVO-like solution for handling "join queries" and # write unit test for this function """Excludes subnets on segments not reachable by the host @@ -427,13 +437,13 @@ class Subnet(base.NeutronDbObject): query = query.add_entity(SegmentHostMapping) query = query.outerjoin( SegmentHostMapping, - and_(cls.db_model.segment_id == SegmentHostMapping.segment_id, + and_(subnet_entity.segment_id == SegmentHostMapping.segment_id, SegmentHostMapping.host == host)) # Essentially "segment_id IS NULL XNOR host IS NULL" - query = query.filter(or_(and_(cls.db_model.segment_id.isnot(None), + query = query.filter(or_(and_(subnet_entity.segment_id.isnot(None), SegmentHostMapping.host.isnot(None)), - and_(cls.db_model.segment_id.is_(None), + and_(subnet_entity.segment_id.is_(None), SegmentHostMapping.host.is_(None)))) return query @@ -443,14 +453,16 @@ class Subnet(base.NeutronDbObject): return query.filter(cls.db_model.network_id == network_id) @classmethod - def _query_exclude_subnets_on_segments(cls, query): + def _query_exclude_subnets_on_segments(cls, query, subnet_entity): """Excludes all subnets associated with segments For the case where the host is not known, we don't consider any subnets that are on segments. But, we still consider subnets that are not associated with any segment (i.e. for non-routed networks). """ - return query.filter(cls.db_model.segment_id.is_(None)) + # here, we assume this assertion is true + # assert cls.db_model is models_v2.Subnet + return query.filter(subnet_entity.segment_id.is_(None)) @classmethod def is_host_set(cls, host): @@ -478,8 +490,10 @@ class Subnet(base.NeutronDbObject): return True # Does filtering ineligible service subnets makes the list empty? - query = SubnetServiceType.query_filter_service_subnets( + subnet_entity = SubnetServiceType._query_entity_service_subnets( query, service_type) + + query = context.session.query(subnet_entity) if query.limit(1).count(): # No, must be a deferred IP port because there are matching # subnets. Happens on routed networks when host isn't known. diff --git a/neutron/plugins/ml2/drivers/l2pop/db.py b/neutron/plugins/ml2/drivers/l2pop/db.py index 38c22ac4f19..f166c6140aa 100644 --- a/neutron/plugins/ml2/drivers/l2pop/db.py +++ b/neutron/plugins/ml2/drivers/l2pop/db.py @@ -96,9 +96,12 @@ def _ha_router_interfaces_on_network_query(context, network_id): models_v2.Port.device_owner.in_(HA_ROUTER_PORTS)) -def _get_ha_router_interface_ids(context, network_id): +def _get_ha_router_interface_ids_subquery(context, network_id): query = _ha_router_interfaces_on_network_query(context, network_id) - return query.from_self(models_v2.Port.id).distinct() + + port_entity = orm.aliased(models_v2.Port, query.subquery()) + + return context.session.query(port_entity.id).distinct() @db_api.CONTEXT_READER @@ -107,7 +110,9 @@ def get_nondistributed_active_network_ports(context, network_id): # Exclude DVR and HA router interfaces query = query.filter(models_v2.Port.device_owner != const.DEVICE_OWNER_DVR_INTERFACE) - ha_iface_ids_query = _get_ha_router_interface_ids(context, network_id) + ha_iface_ids_query = _get_ha_router_interface_ids_subquery( + context, network_id + ) query = query.filter(models_v2.Port.id.notin_(ha_iface_ids_query)) return [(bind, agent) for bind, agent in query.all() if get_agent_ip(agent)] @@ -164,7 +169,9 @@ def get_agent_network_active_port_count(context, agent_host, const.DEVICE_OWNER_DVR_INTERFACE, ml2_models.PortBinding.host == agent_host) - ha_iface_ids_query = _get_ha_router_interface_ids(context, network_id) + ha_iface_ids_query = _get_ha_router_interface_ids_subquery( + context, network_id + ) query1 = query1.filter(models_v2.Port.id.notin_(ha_iface_ids_query)) ha_port_count = _get_ha_router_active_port_count( context, agent_host, network_id) diff --git a/neutron/tests/unit/plugins/ml2/drivers/l2pop/test_db.py b/neutron/tests/unit/plugins/ml2/drivers/l2pop/test_db.py index 7a1d3416be7..001b8ebbbf5 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/l2pop/test_db.py +++ b/neutron/tests/unit/plugins/ml2/drivers/l2pop/test_db.py @@ -189,7 +189,7 @@ class TestL2PopulationDBTestCase(testlib_api.SqlTestCase): self._setup_port_binding( device_owner=constants.DEVICE_OWNER_ROUTER_SNAT, device_id=TEST_ROUTER_ID) - ha_iface_ids = l2pop_db._get_ha_router_interface_ids( + ha_iface_ids = l2pop_db._get_ha_router_interface_ids_subquery( self.ctx, TEST_NETWORK_ID) self.assertEqual(1, len(list(ha_iface_ids))) @@ -201,7 +201,7 @@ class TestL2PopulationDBTestCase(testlib_api.SqlTestCase): self._setup_port_binding( device_owner=constants.DEVICE_OWNER_HA_REPLICATED_INT, device_id=TEST_ROUTER_ID) - ha_iface_ids = l2pop_db._get_ha_router_interface_ids( + ha_iface_ids = l2pop_db._get_ha_router_interface_ids_subquery( self.ctx, TEST_NETWORK_ID) self.assertEqual(1, len(list(ha_iface_ids))) @@ -210,7 +210,7 @@ class TestL2PopulationDBTestCase(testlib_api.SqlTestCase): self._setup_port_binding( device_owner=constants.DEVICE_OWNER_ROUTER_SNAT, device_id=TEST_ROUTER_ID) - ha_iface_ids = l2pop_db._get_ha_router_interface_ids( + ha_iface_ids = l2pop_db._get_ha_router_interface_ids_subquery( self.ctx, TEST_NETWORK_ID) self.assertEqual(0, len(list(ha_iface_ids)))