From 935ddf7c7211c26141863d4579f40a04e5e3614e Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 15 Mar 2021 17:00:31 +0100 Subject: [PATCH] Prevent quota and reservations to go into negative Our current quota usage code doesn't prevent our usage or reservations to go into negative, though it warns at reservation time. This patch makes sure that we don't go into negative, setting it to 0 in case it would be negative. Change-Id: I487157d7092fa4cee1cd72cf01ce7031c94f553f --- cinder/db/sqlalchemy/api.py | 21 ++++--- cinder/tests/unit/test_db_api.py | 95 ++++++++++++++++++++++++++++++-- 2 files changed, 105 insertions(+), 11 deletions(-) diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index ae2c6671e17..738047aeb6a 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/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 " - "resources: %s", unders) + LOG.warning("Reservation would make usage less than 0 for the " + "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: - usage.reserved -= reservation.delta - usage.in_use += reservation.delta + delta = reservation.delta + if delta >= 0: + 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) diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index 27ed9e9b443..926341cd558 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/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')): - quota_obj = db.quota_create(context, project_id, resource, i + 1) + for resource, value in resource_dict.items(): + 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."""