Merge "Fix DB deadlock in quotas" into stable/2023.2

Author: Zuul

octavia/db/repositories.py

@@ -40,6 +40,7 @@
 from octavia.common import data_models
 from octavia.common import exceptions
 from octavia.common import validate
+from octavia.db import api as db_api
 from octavia.db import models
 
 CONF = cfg.CONF
@@ -390,13 +391,8 @@ def check_quota_met(self, session, lock_session, _class, project_id,
         if not project_id:
             raise exceptions.MissingProjectID()
 
-        quotas = self.quotas.get(session, project_id=project_id)
-        if not quotas:
-            # Make sure we have a record to lock
-            self.quotas.update(
-                session,
-                project_id,
-                quota={})
+        self.quotas.ensure_project_exists(project_id)
+
         # Lock the project record in the database to block other quota checks
         #
         # Note: You cannot just use the current count as the in-use
@@ -1883,11 +1879,6 @@ def delete(self, session, id, **filters):
 class QuotasRepository(BaseRepository):
     model_class = models.Quotas
 
-    # Since this is for the initial quota record creation it locks the table
-    # which can lead to recoverable deadlocks. Thus we use the deadlock
-    # retry wrapper here. This may not be appropriate for other sessions
-    # and or queries. Use with caution.
-    @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True)
     def update(self, session, project_id, **model_kwargs):
         kwargs_quota = model_kwargs['quota']
         quotas = (
@@ -1904,6 +1895,19 @@ def update(self, session, project_id, **model_kwargs):
         session.flush()
         return self.get(session, project_id=project_id)
 
+    # Since this is for the initial quota record creation it locks the table
+    # which can lead to recoverable deadlocks. Thus we use the deadlock
+    # retry wrapper here. This may not be appropriate for other sessions
+    # and or queries. Use with caution.
+    @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True)
+    def ensure_project_exists(self, project_id):
+        with db_api.session().begin() as session:
+            quotas = self.get(session, project_id=project_id)
+            if not quotas:
+                # Make sure we have a record to lock
+                self.update(session, project_id, quota={})
+            session.commit()
+
     def delete(self, session, project_id):
         quotas = (
             session.query(self.model_class)

octavia/tests/functional/db/base.py

@@ -30,6 +30,8 @@
 
 class OctaviaDBTestBase(test_base.BaseTestCase):
 
+    facade = None
+
     def setUp(self, connection_string='sqlite://'):
         super().setUp()
 
@@ -73,11 +75,18 @@ def _get_db_engine_session(self):
                                                          sqlite_fk=True)
             engine = facade.get_engine()
             session = facade.get_session(expire_on_commit=True)
+            self.facade = facade
         else:
             engine = db_api.get_engine()
             session = db_api.get_session()
         return engine, session
 
+    def get_session(self):
+        if 'sqlite:///' in self.connection_string:
+            return self.facade.get_session(expire_on_commit=True)
+        else:
+            return db_api.get_session()
+
     def _seed_lookup_tables(self, session):
         self._seed_lookup_table(
             session, constants.SUPPORTED_PROVISIONING_STATUSES,

octavia/tests/functional/db/test_repositories.py

@@ -12,6 +12,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from concurrent.futures import ThreadPoolExecutor
 import datetime
 import random
 from unittest import mock
@@ -509,6 +510,47 @@ def test_sqlite_transactions_broken(self):
             project_id=project_id).all()
         self.assertEqual(1, len(lbs))  # After rollback: 1 (broken!)
 
+    def test_check_quota_met_check_deadlock(self):
+        # This test doesn't work with sqlite, using another backend is not
+        # straighforward, we need to update the connection_string passed to the
+        # __init__ func and also change some calls in the constructor (don't
+        # create the DB objects if we use a DB that was deployed for Octavia)
+        if 'sqlite://' in self.connection_string:
+            self.skipTest("The test for checking potential deadlocks "
+                          "doesn't work with the sqlite backend")
+
+        conf = self.useFixture(oslo_fixture.Config(cfg.CONF))
+        conf.config(group='api_settings', auth_strategy=constants.TESTING)
+        conf.config(group='quotas', default_load_balancer_quota=-1)
+
+        # Calling check_quota_met concurrently from many threads may
+        # have triggered a deadlock in the DB
+        # (Note: we run the test 8 times because it's not 100% reproducible)
+        # https://bugs.launchpad.net/octavia/+bug/2038798
+        for _ in range(8):
+            number_of_projects = 8
+            project_ids = (
+                uuidutils.generate_uuid()
+                for _ in range(number_of_projects))
+
+            with ThreadPoolExecutor(
+                    max_workers=number_of_projects) as executor:
+                def _test_check_quota_met(project_id):
+                    session = self.get_session()
+                    session.begin()
+                    self.assertFalse(self.repos.check_quota_met(
+                        session, data_models.LoadBalancer,
+                        project_id))
+                    session.commit()
+
+                futs = []
+                for project_id in project_ids:
+                    future = executor.submit(_test_check_quota_met, project_id)
+                    futs.append(future)
+
+                for fut in futs:
+                    fut.result()
+
     def test_check_quota_met(self):
 
         project_id = uuidutils.generate_uuid()