From 05fcfef6ced908d7ccf9b2bc13b76b13f055d096 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Fri, 31 May 2024 16:14:32 -0400 Subject: [PATCH] Change to use selectin for DB load strategy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During a mailing list discussion on some OOM issues neutron has been seeing [0], Mike Bayer recommended we should change from using subquery to selectin DB load strategy. A full description of this strategy can be found here [1], but in short: - “subquery” loading incurs additional performance / complexity issues when used on a many-levels-deep eager load, as subqueries will be nested repeatedly. - "The subqueryload() eager loader is mostly legacy at this point, superseded by selectinload() - "The only scenario in which selectin eager loading is not feasible is when the model is using composite primary keys, and the backend database does not support tuples with IN, which currently includes SQL Server." So that does not apply to us. The plan agreed to at the neutron drivers meeting [2] was to make this change early in the cycle so we would be able to see if there were any issues through the D cycle. Added hacking checks so new code using subquery loads is not added back. [0] https://lists.openstack.org/archives/list/openstack-discuss@lists.openstack.org/thread/EHLQQXNG3NLLZYPDGG2ES3DINIJ7YT3N/ [1] https://docs.sqlalchemy.org/en/20/orm/queryguide/relationships.html#selectin-eager-loading [2] https://meetings.opendev.org/meetings/neutron_drivers/2024/neutron_drivers.2024-05-31-14.00.log.html#l-67 Closes-bug: #2067770 Depends-on: https://review.opendev.org/c/openstack/neutron-lib/+/920936 Change-Id: I6e40a15284da392a3d48d45205a7a5770c14c297 --- neutron/db/extra_dhcp_opt/models.py | 2 +- neutron/db/models/address_group.py | 2 +- neutron/db/models/allowed_address_pair.py | 2 +- neutron/db/models/conntrack_helper.py | 2 +- neutron/db/models/flavor.py | 2 +- neutron/db/models/l3.py | 6 +++--- neutron/db/models/metering.py | 4 ++-- neutron/db/models/ndp_proxy.py | 2 +- neutron/db/models/port_forwarding.py | 4 ++-- neutron/db/models/segment.py | 4 ++-- neutron/db/models/subnet_service_type.py | 2 +- neutron/db/models/tag.py | 2 +- neutron/db/models_v2.py | 16 ++++++++-------- neutron/db/network_dhcp_agent_binding/models.py | 2 +- neutron/hacking/checks.py | 17 +++++++++++++++++ neutron/plugins/ml2/drivers/l2pop/db.py | 4 ++-- neutron/plugins/ml2/models.py | 2 +- neutron/services/trunk/models.py | 2 +- neutron/tests/unit/hacking/test_checks.py | 11 +++++++++++ tox.ini | 1 + 20 files changed, 59 insertions(+), 30 deletions(-) diff --git a/neutron/db/extra_dhcp_opt/models.py b/neutron/db/extra_dhcp_opt/models.py index 9510b6f1e08..0b522964e0d 100644 --- a/neutron/db/extra_dhcp_opt/models.py +++ b/neutron/db/extra_dhcp_opt/models.py @@ -42,5 +42,5 @@ class ExtraDhcpOpt(model_base.BASEV2, model_base.HasId): # eagerly load extra_dhcp_opts bindings ports = orm.relationship( models_v2.Port, load_on_pending=True, - backref=orm.backref("dhcp_opts", lazy='subquery', cascade='delete')) + backref=orm.backref("dhcp_opts", lazy='selectin', cascade='delete')) revises_on_change = ('ports', ) diff --git a/neutron/db/models/address_group.py b/neutron/db/models/address_group.py index d810bb1de12..7ff31715d47 100644 --- a/neutron/db/models/address_group.py +++ b/neutron/db/models/address_group.py @@ -42,7 +42,7 @@ class AddressGroup(standard_attr.HasStandardAttributes, addresses = orm.relationship(AddressAssociation, backref=orm.backref('address_groups', load_on_pending=True), - lazy='subquery', + lazy='selectin', cascade='all, delete-orphan') rbac_entries = sa.orm.relationship(rbac_db_models.AddressGroupRBAC, backref='address_groups', diff --git a/neutron/db/models/allowed_address_pair.py b/neutron/db/models/allowed_address_pair.py index 5e7763aff59..50a6c5c2acb 100644 --- a/neutron/db/models/allowed_address_pair.py +++ b/neutron/db/models/allowed_address_pair.py @@ -27,5 +27,5 @@ class AllowedAddressPair(model_base.BASEV2): port = orm.relationship( models_v2.Port, load_on_pending=True, backref=orm.backref("allowed_address_pairs", - lazy="subquery", cascade="delete")) + lazy="selectin", cascade="delete")) revises_on_change = ('port', ) diff --git a/neutron/db/models/conntrack_helper.py b/neutron/db/models/conntrack_helper.py index 44f5bd39018..7be909375d8 100644 --- a/neutron/db/models/conntrack_helper.py +++ b/neutron/db/models/conntrack_helper.py @@ -39,7 +39,7 @@ class ConntrackHelper(model_base.BASEV2, model_base.HasId): router = orm.relationship(l3.Router, load_on_pending=True, backref=orm.backref("conntrack_helpers", - lazy='subquery', + lazy='selectin', uselist=True, cascade='delete')) revises_on_change = ('router', ) diff --git a/neutron/db/models/flavor.py b/neutron/db/models/flavor.py index 6b822e78d0b..7f0d4a15f0a 100644 --- a/neutron/db/models/flavor.py +++ b/neutron/db/models/flavor.py @@ -41,7 +41,7 @@ class FlavorServiceProfileBinding(model_base.BASEV2): flavor = orm.relationship(Flavor, backref=orm.backref( "service_profiles", - lazy='subquery', + lazy='selectin', cascade="all, delete-orphan")) service_profile_id = sa.Column(sa.String(36), sa.ForeignKey("serviceprofiles.id", diff --git a/neutron/db/models/l3.py b/neutron/db/models/l3.py index 1a9c5b900d7..8181bcce947 100644 --- a/neutron/db/models/l3.py +++ b/neutron/db/models/l3.py @@ -58,9 +58,9 @@ class Router(standard_attr.HasStandardAttributes, model_base.BASEV2, attached_ports = orm.relationship( RouterPort, backref=orm.backref('router', load_on_pending=True), - lazy='subquery') + lazy='selectin') l3_agents = orm.relationship( - 'Agent', lazy='subquery', viewonly=True, + 'Agent', lazy='selectin', viewonly=True, secondary=rb_model.RouterL3AgentBinding.__table__) api_collections = [l3_apidef.ROUTERS] collection_resource_map = {l3_apidef.ROUTERS: l3_apidef.ROUTER} @@ -120,6 +120,6 @@ class RouterRoute(model_base.BASEV2, models_v2.Route): router = orm.relationship(Router, load_on_pending=True, backref=orm.backref("route_list", - lazy='subquery', + lazy='selectin', cascade='delete')) revises_on_change = ('router', ) diff --git a/neutron/db/models/metering.py b/neutron/db/models/metering.py index b4466a72342..8a61d835d94 100644 --- a/neutron/db/models/metering.py +++ b/neutron/db/models/metering.py @@ -38,11 +38,11 @@ class MeteringLabel(model_base.BASEV2, name = sa.Column(sa.String(db_const.NAME_FIELD_SIZE)) description = sa.Column(sa.String(db_const.LONG_DESCRIPTION_FIELD_SIZE)) rules = orm.relationship(MeteringLabelRule, backref="label", - cascade="delete", lazy="subquery") + cascade="delete", lazy="selectin") routers = orm.relationship( l3_models.Router, primaryjoin="MeteringLabel.project_id==Router.project_id", foreign_keys='MeteringLabel.project_id', - lazy='subquery', + lazy='selectin', uselist=True) shared = sa.Column(sa.Boolean, default=False, server_default=sql.false()) diff --git a/neutron/db/models/ndp_proxy.py b/neutron/db/models/ndp_proxy.py index 7653018a90c..5cedd11d647 100644 --- a/neutron/db/models/ndp_proxy.py +++ b/neutron/db/models/ndp_proxy.py @@ -63,6 +63,6 @@ class RouterNDPProxyState(model_base.BASEV2): router = orm.relationship( l3.Router, load_on_pending=True, backref=orm.backref("ndp_proxy_state", - lazy='subquery', uselist=False, + lazy='selectin', uselist=False, cascade='delete') ) diff --git a/neutron/db/models/port_forwarding.py b/neutron/db/models/port_forwarding.py index 6708b20c73b..c02ad7c1f6a 100644 --- a/neutron/db/models/port_forwarding.py +++ b/neutron/db/models/port_forwarding.py @@ -58,13 +58,13 @@ class PortForwarding(standard_attr.HasStandardAttributes, models_v2.Port, load_on_pending=True, foreign_keys=internal_neutron_port_id, backref=orm.backref("port_forwardings", - lazy='subquery', uselist=True, + lazy='selectin', uselist=True, cascade='delete') ) floating_ip = orm.relationship( l3.FloatingIP, load_on_pending=True, backref=orm.backref("port_forwardings", - lazy='subquery', uselist=True, + lazy='selectin', uselist=True, cascade='delete') ) revises_on_change = ('floating_ip', 'port',) diff --git a/neutron/db/models/segment.py b/neutron/db/models/segment.py index 8855d341391..8d3e2ef780f 100644 --- a/neutron/db/models/segment.py +++ b/neutron/db/models/segment.py @@ -49,7 +49,7 @@ class NetworkSegment(standard_attr.HasStandardAttributes, nullable=True) network = orm.relationship(models_v2.Network, backref=orm.backref("segments", - lazy='subquery', + lazy='selectin', cascade='delete')) api_collections = [segment.COLLECTION_NAME] @@ -81,6 +81,6 @@ class SegmentHostMapping(model_base.BASEV2): network_segment = orm.relationship( NetworkSegment, load_on_pending=True, backref=orm.backref("segment_host_mapping", - lazy='subquery', + lazy='selectin', cascade='delete')) revises_on_change = ('network_segment', ) diff --git a/neutron/db/models/subnet_service_type.py b/neutron/db/models/subnet_service_type.py index a8cab1814f2..c2b2f3ff757 100644 --- a/neutron/db/models/subnet_service_type.py +++ b/neutron/db/models/subnet_service_type.py @@ -33,7 +33,7 @@ class SubnetServiceType(model_base.BASEV2): length=db_const.DEVICE_OWNER_FIELD_SIZE)) subnet = orm.relationship(models_v2.Subnet, load_on_pending=True, backref=orm.backref('service_types', - lazy='subquery', + lazy='selectin', cascade='all, delete-orphan', uselist=True)) __table_args__ = ( diff --git a/neutron/db/models/tag.py b/neutron/db/models/tag.py index 55195340d03..4346e5bc795 100644 --- a/neutron/db/models/tag.py +++ b/neutron/db/models/tag.py @@ -26,6 +26,6 @@ class Tag(model_base.BASEV2): tag = sa.Column(sa.String(255), nullable=False, primary_key=True) standard_attr = orm.relationship( 'StandardAttribute', load_on_pending=True, - backref=orm.backref('tags', lazy='subquery', viewonly=True), + backref=orm.backref('tags', lazy='selectin', viewonly=True), sync_backref=False) revises_on_change = ('standard_attr', ) diff --git a/neutron/db/models_v2.py b/neutron/db/models_v2.py index 3b1f2ece796..383c2bb6851 100644 --- a/neutron/db/models_v2.py +++ b/neutron/db/models_v2.py @@ -132,7 +132,7 @@ class Port(standard_attr.HasStandardAttributes, model_base.BASEV2, fixed_ips = orm.relationship(IPAllocation, backref=orm.backref('port', load_on_pending=True), - lazy='subquery', + lazy='selectin', cascade='all, delete-orphan', order_by=(IPAllocation.ip_address, IPAllocation.subnet_id)) @@ -217,24 +217,24 @@ class Subnet(standard_attr.HasStandardAttributes, model_base.BASEV2, cidr = sa.Column(sa.String(64), nullable=False) gateway_ip = sa.Column(sa.String(64)) network_standard_attr = orm.relationship( - 'StandardAttribute', lazy='subquery', viewonly=True, + 'StandardAttribute', lazy='selectin', viewonly=True, secondary='networks', uselist=False, load_on_pending=True) revises_on_change = ('network_standard_attr', ) allocation_pools = orm.relationship(IPAllocationPool, backref='subnet', - lazy="subquery", + lazy="selectin", cascade='delete') enable_dhcp = sa.Column(sa.Boolean()) dns_nameservers = orm.relationship(DNSNameServer, backref='subnet', cascade='all, delete, delete-orphan', order_by=DNSNameServer.order, - lazy='subquery') + lazy='selectin') routes = orm.relationship(SubnetRoute, backref='subnet', cascade='all, delete, delete-orphan', - lazy='subquery') + lazy='selectin') ipv6_ra_mode = sa.Column(sa.Enum(constants.IPV6_SLAAC, constants.DHCPV6_STATEFUL, constants.DHCPV6_STATELESS, @@ -299,7 +299,7 @@ class SubnetPool(standard_attr.HasStandardAttributes, model_base.BASEV2, prefixes = orm.relationship(SubnetPoolPrefix, backref='subnetpools', cascade='all, delete, delete-orphan', - lazy='subquery') + lazy='selectin') rbac_entries = sa.orm.relationship(rbac_db_models.SubnetPoolRBAC, backref='subnetpools', lazy='joined', @@ -317,7 +317,7 @@ class Network(standard_attr.HasStandardAttributes, model_base.BASEV2, name = sa.Column(sa.String(db_const.NAME_FIELD_SIZE)) subnets = orm.relationship( Subnet, - lazy="subquery") + lazy="selectin") status = sa.Column(sa.String(16)) admin_state_up = sa.Column(sa.Boolean) vlan_transparent = sa.Column(sa.Boolean, nullable=True) @@ -331,7 +331,7 @@ class Network(standard_attr.HasStandardAttributes, model_base.BASEV2, default=constants.DEFAULT_NETWORK_MTU, server_default=str(constants.DEFAULT_NETWORK_MTU)) dhcp_agents = orm.relationship( - 'Agent', lazy='subquery', viewonly=True, + 'Agent', lazy='selectin', viewonly=True, secondary=ndab_model.NetworkDhcpAgentBinding.__table__) api_collections = [net_def.COLLECTION_NAME] collection_resource_map = {net_def.COLLECTION_NAME: net_def.RESOURCE_NAME} diff --git a/neutron/db/network_dhcp_agent_binding/models.py b/neutron/db/network_dhcp_agent_binding/models.py index c0077aa548a..5d90b2b20b5 100644 --- a/neutron/db/network_dhcp_agent_binding/models.py +++ b/neutron/db/network_dhcp_agent_binding/models.py @@ -31,7 +31,7 @@ class NetworkDhcpAgentBinding(model_base.BASEV2): network_id = sa.Column(sa.String(36), sa.ForeignKey("networks.id", ondelete='CASCADE'), primary_key=True) - dhcp_agent = orm.relationship(agent_model.Agent, lazy='subquery') + dhcp_agent = orm.relationship(agent_model.Agent, lazy='selectin') dhcp_agent_id = sa.Column(sa.String(36), sa.ForeignKey("agents.id", ondelete='CASCADE'), diff --git a/neutron/hacking/checks.py b/neutron/hacking/checks.py index 1326a324d1d..66dd8addf0b 100644 --- a/neutron/hacking/checks.py +++ b/neutron/hacking/checks.py @@ -44,6 +44,9 @@ import_packaging = re.compile(r"\bimport[\s]+packaging\b") import_version_from_packaging = ( re.compile(r"\bfrom[\s]+packaging[\s]+import[\s]version\b")) +filter_lazy_subquery = re.compile(r".*lazy=.+subquery") +filter_subquery_load = re.compile(r".*subqueryload\(") + @core.flake8ext def check_assert_called_once_with(logical_line, filename): @@ -263,3 +266,17 @@ def check_no_import_packaging(logical_line, filename, noqa): for regex in import_packaging, import_version_from_packaging: if re.match(regex, logical_line): yield (0, msg) + + +@core.flake8ext +def check_no_sqlalchemy_lazy_subquery(logical_line): + """N350 - Use selectin DB load strategy instead of subquery.""" + + msg = ("N350: Use selectin DB load strategy instead of " + "subquery with sqlalchemy.") + + if filter_lazy_subquery.match(logical_line): + yield (0, msg) + + if filter_subquery_load.match(logical_line): + yield (0, msg) diff --git a/neutron/plugins/ml2/drivers/l2pop/db.py b/neutron/plugins/ml2/drivers/l2pop/db.py index f166c6140aa..172bbfded57 100644 --- a/neutron/plugins/ml2/drivers/l2pop/db.py +++ b/neutron/plugins/ml2/drivers/l2pop/db.py @@ -79,7 +79,7 @@ def _get_active_network_ports(context, network_id): agent_model.Agent, agent_model.Agent.host == ml2_models.PortBinding.host) query = query.join(models_v2.Port) - query = query.options(orm.subqueryload(ml2_models.PortBinding.port)) + query = query.options(orm.selectinload(ml2_models.PortBinding.port)) query = query.filter(models_v2.Port.network_id == network_id, models_v2.Port.status == const.PORT_STATUS_ACTIVE) return query @@ -126,7 +126,7 @@ def _get_dvr_active_network_ports(context, network_id): ml2_models.DistributedPortBinding.host) query = query.join(models_v2.Port) query = query.options( - orm.subqueryload(ml2_models.DistributedPortBinding.port)) + orm.selectinload(ml2_models.DistributedPortBinding.port)) query = query.filter(models_v2.Port.network_id == network_id, models_v2.Port.status == const.PORT_STATUS_ACTIVE, models_v2.Port.device_owner == diff --git a/neutron/plugins/ml2/models.py b/neutron/plugins/ml2/models.py index 6061eaa1bd6..58d3b84f67f 100644 --- a/neutron/plugins/ml2/models.py +++ b/neutron/plugins/ml2/models.py @@ -89,7 +89,7 @@ class PortBindingLevel(model_base.BASEV2): port = orm.relationship( models_v2.Port, load_on_pending=True, - backref=orm.backref("binding_levels", lazy='subquery', + backref=orm.backref("binding_levels", lazy='selectin', cascade='delete')) segment = orm.relationship( segment_models.NetworkSegment, diff --git a/neutron/services/trunk/models.py b/neutron/services/trunk/models.py index 5a678b9f46a..625ac2dbd41 100644 --- a/neutron/services/trunk/models.py +++ b/neutron/services/trunk/models.py @@ -46,7 +46,7 @@ class Trunk(standard_attr.HasStandardAttributes, model_base.BASEV2, cascade='delete')) sub_ports = sa.orm.relationship( - 'SubPort', lazy='subquery', uselist=True, cascade="all, delete-orphan") + 'SubPort', lazy='selectin', uselist=True, cascade="all, delete-orphan") api_collections = ['trunks'] collection_resource_map = {'trunks': 'trunk'} tag_support = True diff --git a/neutron/tests/unit/hacking/test_checks.py b/neutron/tests/unit/hacking/test_checks.py index 336c38aacce..a43825bdc03 100644 --- a/neutron/tests/unit/hacking/test_checks.py +++ b/neutron/tests/unit/hacking/test_checks.py @@ -279,3 +279,14 @@ class HackingTestCase(base.BaseTestCase): _pass(["_('foo')"], "neutron/_i18n.py") _pass(["_('foo')"], "neutron/i18n.py") _pass(["_('foo')"], "neutron/foo.py", noqa=True) + + def test_check_no_sqlalchemy_lazy_subquery(self): + f = checks.check_no_sqlalchemy_lazy_subquery + self.assertLineFails('N350', f, + "backref=orm.backref('tags', lazy='subquery', viewonly=True),") + self.assertLineFails('N350', f, + "query.options(orm.subqueryload(ml2_models.PortBinding.port))") + self.assertLinePasses(f, + "backref=orm.backref('tags', lazy='selectin', viewonly=True),") + self.assertLinePasses(f, + "query.options(orm.selectinload(ml2_models.PortBinding.port))") diff --git a/tox.ini b/tox.ini index 1a295658a38..52edf322f4c 100644 --- a/tox.ini +++ b/tox.ini @@ -239,6 +239,7 @@ extension = N346 = neutron.hacking.checks:check_no_sqlalchemy_event_import N348 = neutron.hacking.checks:check_no_import_six N349 = neutron.hacking.checks:check_no_import_packaging + N350 = neutron.hacking.checks:check_no_sqlalchemy_lazy_subquery # Checks from neutron-lib N521 = neutron_lib.hacking.checks:use_jsonutils N524 = neutron_lib.hacking.checks:check_no_contextlib_nested