Merge "Reduce lock contention on subnets"

5 files changed, 82 insertions, 18 deletions

diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py
index dbbe94de7a..d80e7b3fc7 100644
--- a/neutron/db/db_base_plugin_v2.py
+++ b/neutron/db/db_base_plugin_v2.py
@@ -76,7 +76,7 @@ LOG = logging.getLogger(__name__)
 
 
 def _ensure_subnet_not_used(context, subnet_id):
-    models_v2.Subnet.lock_register(
+    models_v2.Subnet.write_lock_register(
         context, exc.SubnetInUse(subnet_id=subnet_id), id=subnet_id)
     try:
         registry.publish(
diff --git a/neutron/db/ipam_backend_mixin.py b/neutron/db/ipam_backend_mixin.py
index e734996900..9c245ae689 100644
--- a/neutron/db/ipam_backend_mixin.py
+++ b/neutron/db/ipam_backend_mixin.py
@@ -683,7 +683,7 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon):
             msg = ('This subnet is being modified by another concurrent '
                    'operation')
             for subnet in subnets:
-                subnet.lock_register(
+                subnet.read_lock_register(
                     context, exc.SubnetInUse(subnet_id=subnet.id, reason=msg),
                     id=subnet.id)
             subnet_dicts = [self._make_subnet_dict(subnet, context=context)
diff --git a/neutron/db/migration/alembic_migrations/versions/2023.2/expand/93f394357a27_remove_in_use_on_subnets.py b/neutron/db/migration/alembic_migrations/versions/2023.2/expand/93f394357a27_remove_in_use_on_subnets.py
new file mode 100644
index 0000000000..0e9ceb6071
--- /dev/null
+++ b/neutron/db/migration/alembic_migrations/versions/2023.2/expand/93f394357a27_remove_in_use_on_subnets.py
@@ -0,0 +1,42 @@
+# Copyright 2023 OpenStack Foundation
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+#
+
+from alembic import op
+import sqlalchemy as sa
+
+
+"""remove in_use from subnet
+
+Revision ID: 93f394357a27
+Revises: fc153938cdc1
+Create Date: 2023-03-07 14:48:15.763633
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '93f394357a27'
+down_revision = 'fc153938cdc1'
+
+
+def upgrade():
+    op.drop_column('subnets', 'in_use')
+
+
+def expand_drop_exceptions():
+    """Support dropping 'in_use' column in table 'subnets'"""
+
+    return {
+        sa.Column: ['subnets.in_use']
+    }
diff --git a/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD
index 961bd474e5..88323e719e 100644
--- a/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD
+++ b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD
@@ -1 +1 @@
-fc153938cdc1
+93f394357a27
diff --git a/neutron/db/models_v2.py b/neutron/db/models_v2.py
index 9a03b46763..8ab9707b17 100644
--- a/neutron/db/models_v2.py
+++ b/neutron/db/models_v2.py
@@ -33,25 +33,47 @@ from neutron.db import rbac_db_models
 class HasInUse(object):
     """NeutronBaseV2 mixin, to add the flag "in_use" to a DB model.
 
-    The content of this flag (boolean) parameter is not relevant. The goal of
-    this field is to be used in a write transaction to mark a DB register as
-    "in_use". Writing any value on this DB parameter will lock the container
-    register. At the end of the DB transaction, the DB engine will check if
-    this register was modified or deleted. In such case, the transaction will
-    fail and won't be committed.
-
-    "lock_register" is the method to write the register "in_use" column.
-    Because the lifespan of this DB lock is the DB transaction, there isn't an
-    unlock method. The lock will finish once the transaction ends.
+    The goal of this class is to allow users lock specific database rows with
+    a shared or exclusive lock (without necessarily introducing a change in
+    the table itself). Having these locks allows the DB engine to prevent
+    concurrent modifications (e.g. the deletion of a resource while we are
+    currently adding a new dependency on the resource).
+
+    "read_lock_register" takes a shared DB lock on the row specified by the
+    filters. The lock is automatically released once the transaction ends.
+    You can have any number of parallel read locks on the same DB row. But
+    you can not have any write lock in parallel.
+
+    "write_lock_register" takes an exclusive DB lock on the row specified by
+    the filters. The lock is automatically released on transaction commit.
+    You may only have one write lock on each row at a time. It therefor
+    blocks all other read and write locks to this row.
     """
-    in_use = sa.Column(sa.Boolean(), nullable=False,
-                       server_default=sql.false(), default=False)
 
     @classmethod
-    def lock_register(cls, context, exception, **filters):
+    def write_lock_register(cls, context, exception, **filters):
+        # we use `with_for_update()` to include `FOR UPDATE` in the sql
+        # statement.
+        # we need to set `enable_eagerloads(False)` so that we do not try to
+        # load attached resources (e.g. standardattributes) as this breaks the
+        # `FOR UPDATE` statement.
         num_reg = context.session.query(
-            cls).filter_by(**filters).update({'in_use': True})
-        if num_reg != 1:
+            cls).filter_by(**filters).enable_eagerloads(
+                False).with_for_update().first()
+        if num_reg is None:
+            raise exception
+
+    @classmethod
+    def read_lock_register(cls, context, exception, **filters):
+        # we use `with_for_update(read=True)` to include `LOCK IN SHARE MODE`
+        # in the sql statement.
+        # we need to set `enable_eagerloads(False)` so that we do not try to
+        # load attached resources (e.g. standardattributes) as this breaks the
+        # `LOCK IN SHARE MODE` statement.
+        num_reg = context.session.query(
+            cls).filter_by(**filters).enable_eagerloads(
+                False).with_for_update(read=True).first()
+        if num_reg is None:
             raise exception