Remove the expired reservations in a separate DB transaction

In "DbQuotaNoLockDriver", when a new reservation is being made,
first the expired reservations are removed. That guarantees the
freshness of the existing reservations.

In systems with high concurrency of operations, the
"DbQuotaNoLockDriver.make_reservation" method will be called in
parallel. The expired reservations removal implies a deletion
on the "reservation" table that could be executed by several
workers at the same time (in the same controller or not). That
could lead to a "DBDeadlock" exception if multiple workers want
to delete the same registers.

In case an API worker receives this exception, it should continue
as the expired reservations have been deleted by other worker. It
should not retry this operation.

If the reservations are not deleted, the quota engine will filter
out those expired reservations when counting the current number of
reservations [1][2][3]. That means even if in a particular request
the expired reservations are not deleted, these won't count in the
resource quota calculation.

The default reservation expiration timeout is set to 120 seconds
(as it should have been initially set) that is the default
expiration delta for a reservation since 2015.

[1]e99d9a9d06/neutron/quota/resource.py (L340)
[2]e99d9a9d06/neutron/db/quota/api.py (L226)
[3]e99d9a9d06/neutron/objects/quota.py (L100-L101)

Closes-Bug: #1954662
Change-Id: I8af6565d2537db7f0df2e8e567ea046a0a6e003a

neutron/db/quota/api.py

@@ -23,7 +23,7 @@ from neutron.common import utils
 from neutron.objects import quota as quota_obj
 
 
-RESERVATION_EXPIRATION_TIMEOUT = 20  # seconds
+RESERVATION_EXPIRATION_TIMEOUT = 120  # seconds
 UNLIMITED_QUOTA = -1
 
 
@@ -175,7 +175,8 @@ def set_all_quota_usage_dirty(context, resource, dirty=True):
 def create_reservation(context, project_id, deltas, expiration=None):
     # This method is usually called from within another transaction.
     # Consider using begin_nested
-    expiration = expiration or (utcnow() + datetime.timedelta(0, 120))
+    expiration = expiration or (
+            utcnow() + datetime.timedelta(0, RESERVATION_EXPIRATION_TIMEOUT))
     delta_objs = []
     for (resource, delta) in deltas.items():
         delta_objs.append(quota_obj.ResourceDelta(
@@ -241,7 +242,6 @@ def get_reservations_for_resources(context, project_id, resources,
         context, utcnow(), project_id, resources, expired)
 
 
-@db_api.retry_if_session_inactive()
 @db_api.CONTEXT_WRITER
 def remove_expired_reservations(context, project_id=None, timeout=None):
     expiring_time = utcnow()

neutron/db/quota/driver_nolock.py

@@ -15,8 +15,10 @@
 
 from neutron_lib.db import api as db_api
 from neutron_lib import exceptions
+from oslo_db import exception as db_exc
 from oslo_log import log
 
+from neutron.common import utils
 from neutron.db.quota import api as quota_api
 from neutron.db.quota import driver as quota_driver
 
@@ -40,8 +42,26 @@ class DbQuotaNoLockDriver(quota_driver.DbQuotaDriver):
     method is to be fast enough to avoid the concurrency when counting the
     resources while not blocking concurrent API operations.
     """
+    @utils.skip_exceptions(db_exc.DBError)
+    def _remove_expired_reservations(self, context, project_id, timeout):
+        """Remove expired reservations
+
+        Any DB exception will be catch and dismissed. This operation can have
+        been successfully executed by another concurrent request. There is no
+        need to fail or retry it.
+        """
+        quota_api.remove_expired_reservations(context, project_id=project_id,
+                                              timeout=timeout)
+
     @db_api.retry_if_session_inactive()
     def make_reservation(self, context, project_id, resources, deltas, plugin):
+        # Delete expired reservations before counting valid ones. This
+        # operation is fast and by calling it before making any
+        # reservation, we ensure the freshness of the reservations.
+        self._remove_expired_reservations(
+            context, project_id=project_id,
+            timeout=quota_api.RESERVATION_EXPIRATION_TIMEOUT)
+
         resources_over_limit = []
         with db_api.CONTEXT_WRITER.using(context):
             # Filter out unlimited resources.
@@ -50,13 +70,6 @@ class DbQuotaNoLockDriver(quota_driver.DbQuotaDriver):
                                        limits.items() if limit < 0])
             requested_resources = (set(deltas.keys()) - unlimited_resources)
 
-            # Delete expired reservations before counting valid ones. This
-            # operation is fast and by calling it before making any
-            # reservation, we ensure the freshness of the reservations.
-            quota_api.remove_expired_reservations(
-                context, project_id=project_id,
-                timeout=quota_api.RESERVATION_EXPIRATION_TIMEOUT)
-
             # Count the number of (1) used and (2) reserved resources for this
             # project_id. If any resource limit is exceeded, raise exception.
             for resource_name in requested_resources:

neutron/tests/unit/db/quota/test_driver.py

@@ -95,6 +95,8 @@ class TestDbQuotaDriver(testlib_api.SqlTestCase,
         self.quota_driver = driver.DbQuotaDriver()
         self.project_1, self.project_2 = 'prj_test_1', 'prj_test_2'
         self.resource_1, self.resource_2 = 'res_test_1', 'res_test_2'
+        self.projects = (self.project_1, self.project_2)
+        self.resources = (self.resource_1, self.resource_2)
 
     def test_create_quota_limit(self):
         defaults = {RESOURCE: TestResource(RESOURCE, 4)}

neutron/tests/unit/db/quota/test_driver_nolock.py

@@ -13,7 +13,13 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import itertools
+
+from neutron_lib.db import api as db_api
+
+from neutron.db.quota import api as quota_api
 from neutron.db.quota import driver_nolock
+from neutron.objects import quota as quota_obj
 from neutron.tests.unit.db.quota import test_driver
 
 
@@ -22,3 +28,32 @@ class TestDbQuotaDriverNoLock(test_driver.TestDbQuotaDriver):
     def setUp(self):
         super(TestDbQuotaDriverNoLock, self).setUp()
         self.quota_driver = driver_nolock.DbQuotaNoLockDriver()
+
+    def test__remove_expired_reservations(self):
+        for project, resource in itertools.product(self.projects,
+                                                   self.resources):
+            deltas = {resource: 1}
+            with db_api.CONTEXT_WRITER.using(self.context):
+                quota_api.create_reservation(self.context, project, deltas)
+
+        # Initial check: the reservations are correctly created.
+        for project in self.projects:
+            for res in quota_obj.Reservation.get_objects(self.context,
+                                                         project_id=project):
+                self.assertEqual(1, len(res.resource_deltas))
+                delta = res.resource_deltas[0]
+                self.assertEqual(1, delta.amount)
+                self.assertIn(delta.resource, self.resources)
+
+        # Delete the expired reservations and check.
+        for project in self.projects:
+            # NOTE(ralonsoh): the timeout is set to -121 to force the deletion
+            # of all created reservations, including those ones created in this
+            # test. The value of 121 overcomes the 120 seconds of default
+            # expiration time a reservation has.
+            time_delta = quota_api.RESERVATION_EXPIRATION_TIMEOUT + 1
+            self.quota_driver._remove_expired_reservations(
+                self.context, project, -time_delta)
+            res = quota_obj.Reservation.get_objects(self.context,
+                                                    project_id=project)
+            self.assertEqual([], res)