Merge "Don't lock whole project's resource when reserve/commit"

cinder/db/sqlalchemy/api.py

@@ -42,7 +42,7 @@ import six
 import sqlalchemy
 from sqlalchemy import MetaData
 from sqlalchemy import or_, and_, case
-from sqlalchemy.orm import joinedload, joinedload_all, undefer_group
+from sqlalchemy.orm import joinedload, joinedload_all, undefer_group, load_only
 from sqlalchemy.orm import RelationshipProperty
 from sqlalchemy import sql
 from sqlalchemy.sql.expression import bindparam
@@ -1106,15 +1106,16 @@ def _reservation_create(context, uuid, usage, project_id, resource, delta,
 # code always acquires the lock on quota_usages before acquiring the lock
 # on reservations.
 
-def _get_quota_usages(context, session, project_id):
+def _get_quota_usages(context, session, project_id, resources=None):
     # Broken out for testability
-    rows = model_query(context, models.QuotaUsage,
+    query = model_query(context, models.QuotaUsage,
-                       read_deleted="no",
+                        read_deleted="no",
-                       session=session).\
+                        session=session).filter_by(project_id=project_id)
-        filter_by(project_id=project_id).\
+    if resources:
-        order_by(models.QuotaUsage.id.asc()).\
+        query = query.filter(models.QuotaUsage.resource.in_(list(resources)))
-        with_lockmode('update').\
+    rows = query.order_by(models.QuotaUsage.id.asc()).\
-        all()
+        with_for_update().all()
+
     return {row.resource: row for row in rows}
 
 
@@ -1124,7 +1125,7 @@ def _get_quota_usages_by_resource(context, session, resource):
                        session=session).\
         filter_by(resource=resource).\
         order_by(models.QuotaUsage.id.asc()).\
-        with_lockmode('update').\
+        with_for_update().\
         all()
     return rows
 
@@ -1152,7 +1153,8 @@ def quota_reserve(context, resources, quotas, deltas, expire,
             project_id = context.project_id
 
         # Get the current usages
-        usages = _get_quota_usages(context, session, project_id)
+        usages = _get_quota_usages(context, session, project_id,
+                                   resources=deltas.keys())
         allocated = quota_allocated_get_all_by_project(context, project_id,
                                                        session=session)
         allocated.pop('project_id')
@@ -1317,6 +1319,18 @@ def _quota_reservations(session, context, reservations):
         all()
 
 
+def _get_reservation_resources(session, context, reservation_ids):
+    """Return the relevant resources by reservations."""
+
+    reservations = model_query(context, models.Reservation,
+                               read_deleted="no",
+                               session=session).\
+        options(load_only('resource')).\
+        filter(models.Reservation.uuid.in_(reservation_ids)).\
+        all()
+    return {r.resource for r in reservations}
+
+
 def _dict_with_usage_id(usages):
     return {row.id: row for row in usages.values()}
 
@@ -1326,7 +1340,10 @@ def _dict_with_usage_id(usages):
 def reservation_commit(context, reservations, project_id=None):
     session = get_session()
     with session.begin():
-        usages = _get_quota_usages(context, session, project_id)
+        usages = _get_quota_usages(
+            context, session, project_id,
+            resources=_get_reservation_resources(session, context,
+                                                 reservations))
         usages = _dict_with_usage_id(usages)
 
         for reservation in _quota_reservations(session, context, reservations):
@@ -1345,7 +1362,10 @@ def reservation_commit(context, reservations, project_id=None):
 def reservation_rollback(context, reservations, project_id=None):
     session = get_session()
     with session.begin():
-        usages = _get_quota_usages(context, session, project_id)
+        usages = _get_quota_usages(
+            context, session, project_id,
+            resources=_get_reservation_resources(session, context,
+                                                 reservations))
         usages = _dict_with_usage_id(usages)
         for reservation in _quota_reservations(session, context, reservations):
             if reservation.allocated_id:

cinder/db/sqlalchemy/migrate_repo/versions/111_add_project_resource_idx_for_quota_usage.py

@@ -0,0 +1,31 @@
+#    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 oslo_db.sqlalchemy import utils
+from oslo_log import log as logging
+from sqlalchemy import MetaData
+
+LOG = logging.getLogger(__name__)
+
+
+def upgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+    index_name = 'quota_usage_project_resource_idx'
+    columns = ['project_id', 'resource']
+    if utils.index_exists_on_columns(migrate_engine, 'quota_usages', columns):
+        LOG.info(
+            'Skipped adding %s because an equivalent index already exists.',
+            index_name
+        )
+    else:
+        utils.add_index(migrate_engine, 'quota_usages', index_name, columns)

cinder/db/sqlalchemy/models.py

@@ -25,7 +25,7 @@ from oslo_db.sqlalchemy import models
 from oslo_utils import timeutils
 from sqlalchemy import and_, func, select
 from sqlalchemy import bindparam
-from sqlalchemy import Column, Integer, String, Text, schema
+from sqlalchemy import Column, Integer, String, Text, schema, Index
 from sqlalchemy.ext.declarative import declarative_base
 from sqlalchemy import ForeignKey, DateTime, Boolean, UniqueConstraint
 from sqlalchemy.orm import backref, column_property, relationship, validates
@@ -609,6 +609,8 @@ class QuotaUsage(BASE, CinderBase):
     """Represents the current usage for a given resource."""
 
     __tablename__ = 'quota_usages'
+    __table_args__ = (Index('quota_usage_project_resource_idx', 'project_id',
+                            'resource'), CinderBase.__table_args__)
     id = Column(Integer, primary_key=True)
 
     project_id = Column(String(255), index=True)

cinder/tests/unit/api/contrib/test_quotas.py

@@ -1035,7 +1035,8 @@ class QuotaSetsControllerNestedQuotasTest(QuotaSetsControllerTestBase):
                     self.until_refresh = None
                     self.total = self.reserved + self.in_use
 
-        def _fake__get_quota_usages(context, session, project_id):
+        def _fake__get_quota_usages(context, session, project_id,
+                                    resources=None):
             if not project_id:
                 return {}
             return {'volumes': FakeUsage(fake_usages[project_id], 0)}

cinder/tests/unit/test_db_api.py

@@ -2216,6 +2216,13 @@ class DBAPIReservationTestCase(BaseTest):
             'usage': {'id': 1}
         }
 
+    def test__get_reservation_resources(self):
+        reservations = _quota_reserve(self.ctxt, 'project1')
+        expected = ['gigabytes', 'volumes']
+        resources = sqlalchemy_api._get_reservation_resources(
+            sqlalchemy_api.get_session(), self.ctxt, reservations)
+        self.assertEqual(expected, sorted(resources))
+
     def test_reservation_commit(self):
         reservations = _quota_reserve(self.ctxt, 'project1')
         expected = {'project_id': 'project1',
@@ -2426,6 +2433,24 @@ class DBAPIQuotaTestCase(BaseTest):
                           'volumes': {'reserved': 1, 'in_use': 0}},
                          quota_usage)
 
+    def test__get_quota_usages(self):
+        _quota_reserve(self.ctxt, 'project1')
+        session = sqlalchemy_api.get_session()
+        quota_usage = sqlalchemy_api._get_quota_usages(
+            self.ctxt, session, 'project1')
+
+        self.assertEqual(['gigabytes', 'volumes'],
+                         sorted(quota_usage.keys()))
+
+    def test__get_quota_usages_with_resources(self):
+        _quota_reserve(self.ctxt, 'project1')
+        session = sqlalchemy_api.get_session()
+
+        quota_usage = sqlalchemy_api._get_quota_usages(
+            self.ctxt, session, 'project1', resources=['volumes'])
+
+        self.assertEqual(['volumes'], list(quota_usage.keys()))
+
     @mock.patch('oslo_utils.timeutils.utcnow', return_value=UTC_NOW)
     def test_quota_destroy(self, utcnow_mock):
         db.quota_create(self.ctxt, 'project1', 'resource1', 41)

cinder/tests/unit/test_migrations.py

@@ -1285,6 +1285,10 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin):
         f_keys = self.get_foreign_key_columns(engine, 'backup_metadata')
         self.assertEqual({'backup_id'}, f_keys)
 
+    def _check_111(self, engine, data):
+        self.assertTrue(db_utils.index_exists_on_columns(
+            engine, 'quota_usages', ['project_id', 'resource']))
+
     def test_walk_versions(self):
         self.walk_versions(False, False)
         self.assert_each_foreign_key_is_part_of_an_index()

cinder/tests/unit/test_quota.py

@@ -1712,7 +1712,8 @@ class QuotaReserveSqlAlchemyTestCase(test.TestCase):
         def fake_get_session():
             return FakeSession()
 
-        def fake_get_quota_usages(context, session, project_id):
+        def fake_get_quota_usages(context, session, project_id,
+                                  resources=None):
             return self.usages.copy()
 
         def fake_quota_usage_create(context, project_id, resource, in_use,