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
This commit is contained in:
parent
910574a5cc
commit
e98a268de4
@ -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.
|
||||
|
@ -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)
|
||||
|
@ -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)))
|
||||
|
||||
|
Loading…
x
Reference in New Issue
Block a user