Merge "Prevent quota and reservations to go into negative"

cinder/db/sqlalchemy/api.py

@@ -1289,8 +1289,9 @@ def quota_reserve(context, resources, quotas, deltas, expire,
                     usages[resource].reserved += delta
 
         if unders:
-            LOG.warning("Change will make usage less than 0 for the following "
+            LOG.warning("Reservation would make usage less than 0 for the "
-                        "resources: %s", unders)
+                        "following resources, so on commit they will be "
+                        "limited to prevent going below 0: %s", unders)
         if overs:
             usages = {k: dict(in_use=v.in_use, reserved=v.reserved)
                       for k, v in usages.items()}
@@ -1345,9 +1346,14 @@ def reservation_commit(context, reservations, project_id=None):
 
         for reservation in _quota_reservations(session, context, reservations):
             usage = usages[reservation.usage_id]
-            if reservation.delta >= 0:
+            delta = reservation.delta
-                usage.reserved -= reservation.delta
+            if delta >= 0:
-            usage.in_use += reservation.delta
+                usage.reserved -= min(delta, usage.reserved)
+            # For negative deltas make sure we never go into negative usage
+            elif -delta > usage.in_use:
+                delta = -usage.in_use
+
+            usage.in_use += delta
 
             reservation.delete(session=session)
 
@@ -1365,7 +1371,7 @@ def reservation_rollback(context, reservations, project_id=None):
         for reservation in _quota_reservations(session, context, reservations):
             usage = usages[reservation.usage_id]
             if reservation.delta >= 0:
-                usage.reserved -= reservation.delta
+                usage.reserved -= min(reservation.delta, usage.reserved)
 
             reservation.delete(session=session)
 
@@ -1434,7 +1440,8 @@ def reservation_expire(context):
         if results:
             for reservation in results:
                 if reservation.delta >= 0:
-                    reservation.usage.reserved -= reservation.delta
+                    reservation.usage.reserved -= min(
+                        reservation.delta, reservation.usage.reserved)
                     reservation.usage.save(session=session)
 
                 reservation.delete(session=session)

cinder/tests/unit/test_db_api.py

@@ -45,7 +45,7 @@ ONE_HUNDREDS = 100
 UTC_NOW = timeutils.utcnow()
 
 
-def _quota_reserve(context, project_id):
+def _quota_reserve(context, project_id, **resource_dict):
     """Create sample Quota, QuotaUsage and Reservation objects.
 
     There is no method db.quota_usage_create(), so we have to use
@@ -58,15 +58,19 @@ def _quota_reserve(context, project_id):
         def sync(elevated, project_id, session):
             return {resource: usage}
         return sync
+
+    if not resource_dict:
+        resource_dict = {'volumes': 1, 'gigabytes': 2}
+
     quotas = {}
     resources = {}
     deltas = {}
-    for i, resource in enumerate(('volumes', 'gigabytes')):
+    for resource, value in resource_dict.items():
-        quota_obj = db.quota_create(context, project_id, resource, i + 1)
+        quota_obj = db.quota_create(context, project_id, resource, value)
         quotas[resource] = quota_obj.hard_limit
         resources[resource] = quota.ReservableResource(resource,
                                                        '_sync_%s' % resource)
-        deltas[resource] = i + 1
+        deltas[resource] = value
     return db.quota_reserve(
         context, resources, quotas, deltas,
         datetime.datetime.utcnow(), until_refresh=None,
@@ -2530,6 +2534,47 @@ class DBAPIReservationTestCase(BaseTest):
                              self.ctxt,
                              'project1'))
 
+    def test_reservation_commit_negative_reservation(self):
+        """Verify we can't make reservations negative on commit."""
+        project = 'project1'
+        reservations = _quota_reserve(self.ctxt, project, volumes=2)
+
+        # Force a smaller reserved value in quota_usages table
+        session = sqlalchemy_api.get_session()
+        with session.begin():
+            vol_usage = db.quota_usage_get(self.ctxt, project, 'volumes')
+            vol_usage.reserved -= 1
+            vol_usage.save(session=session)
+
+        # When committing 2 volumes from reserved to used reserved should not
+        # go from 1 to -1 but from 1 to 0, but in-use should still increase by
+        # 2
+        db.reservation_commit(self.ctxt, reservations, project)
+        expected = {'project_id': project,
+                    'volumes': {'reserved': 0, 'in_use': 2}}
+        self.assertEqual(expected,
+                         db.quota_usage_get_all_by_project(self.ctxt, project))
+
+    def test_reservation_commit_negative_in_use(self):
+        """Verify we can't make in-use negative on commit."""
+        project = 'project1'
+        reservations = _quota_reserve(self.ctxt, project, volumes=-2)
+
+        # Force a smaller in_use than the one the reservation will decrease
+        session = sqlalchemy_api.get_session()
+        with session.begin():
+            vol_usage = db.quota_usage_get(self.ctxt, 'project1', 'volumes')
+            vol_usage.in_use = 1
+            vol_usage.save(session=session)
+
+        # When committing -2 volumes from reserved to in-use they should not
+        # make in-use go from 1 to -1, but from 1 to 0
+        db.reservation_commit(self.ctxt, reservations, project)
+        expected = {'project_id': project,
+                    'volumes': {'reserved': 0, 'in_use': 0}}
+        self.assertEqual(expected,
+                         db.quota_usage_get_all_by_project(self.ctxt, project))
+
     def test_reservation_rollback(self):
         reservations = _quota_reserve(self.ctxt, 'project1')
         expected = {'project_id': 'project1',
@@ -2550,6 +2595,27 @@ class DBAPIReservationTestCase(BaseTest):
                              self.ctxt,
                              'project1'))
 
+    def test_reservation_rollback_negative(self):
+        """Verify we can't make reservations negative on rollback."""
+        project = 'project1'
+        reservations = _quota_reserve(self.ctxt, project, volumes=2)
+
+        # Force a smaller reserved value in quota_usages table
+        session = sqlalchemy_api.get_session()
+        with session.begin():
+            vol_usage = db.quota_usage_get(self.ctxt, project, 'volumes')
+            vol_usage.reserved -= 1
+            vol_usage.save(session=session)
+
+        # When rolling back 2 volumes from reserved when there's only 1 in the
+        # quota usage's reserved field, reserved should not go from 1 to -1
+        # but from 1 to 0
+        db.reservation_rollback(self.ctxt, reservations, project)
+        expected = {'project_id': project,
+                    'volumes': {'reserved': 0, 'in_use': 0}}
+        self.assertEqual(expected,
+                         db.quota_usage_get_all_by_project(self.ctxt, project))
+
     def test_reservation_expire(self):
         self.values['expire'] = datetime.datetime.utcnow() + \
             datetime.timedelta(days=1)
@@ -2564,6 +2630,27 @@ class DBAPIReservationTestCase(BaseTest):
                              self.ctxt,
                              'project1'))
 
+    def test_reservation_expire_negative(self):
+        """Verify we can't make reservation negative on expiration."""
+        project = 'project1'
+        _quota_reserve(self.ctxt, project, volumes=2)
+
+        # Force a smaller reserved value in quota_usages table
+        session = sqlalchemy_api.get_session()
+        with session.begin():
+            vol_usage = db.quota_usage_get(self.ctxt, project, 'volumes')
+            vol_usage.reserved -= 1
+            vol_usage.save(session=session)
+
+        # When expiring 2 volumes from reserved when there's only 1 in the
+        # quota usage's reserved field, reserved should not go from 1 to -1
+        # but from 1 to 0
+        db.reservation_expire(self.ctxt)
+        expected = {'project_id': project,
+                    'volumes': {'reserved': 0, 'in_use': 0}}
+        self.assertEqual(expected,
+                         db.quota_usage_get_all_by_project(self.ctxt, project))
+
     @mock.patch('time.sleep', mock.Mock())
     def test_quota_reserve_create_usages_race(self):
         """Test we retry when there is a race in creation."""