From 7da1724d446b6804c6be7a602532fbae58d9f008 Mon Sep 17 00:00:00 2001 From: Salvatore Orlando Date: Tue, 25 Aug 2015 02:21:06 -0700 Subject: [PATCH] Improve DB operations for quota reservation This patch deals with the lock wait timeout and the deadlock errors observed under high concurrency (api_workers >= 4) with the pymysql driver. It includes the following changes: - Stop setting dirty status for resource usage when creating reservation, as usage of reserved resources is not tracked anymore; - Add a variable, increasing delay when retrying make_reservation upon a DBDeadlock error in order to reduce the chances of further collisions; - Enable transaction retry upon DBDeadlock errors for set_quota_usage; - Do not resync quota usage while making reservation. This puts a lot of stress on the database and is also wasteful since resource usage is very likely to change again once the transaction is committed; - Use autonested_transaction to simplify logic around when the nested flag should be used. Change-Id: I7a335f9ebea3c0d6fee6e6b757554e045a66075c Closes-Bug: #1486134 Related-Blueprint: better-quotas --- neutron/db/quota/api.py | 16 ++++++-------- neutron/db/quota/driver.py | 6 +++-- neutron/quota/resource.py | 22 +++++++++---------- neutron/quota/resource_registry.py | 2 +- neutron/tests/unit/db/quota/test_api.py | 16 -------------- .../unit/quota/test_resource_registry.py | 2 +- 6 files changed, 24 insertions(+), 40 deletions(-) diff --git a/neutron/db/quota/api.py b/neutron/db/quota/api.py index 6e1a0e47a5e..8b109a1a369 100644 --- a/neutron/db/quota/api.py +++ b/neutron/db/quota/api.py @@ -19,6 +19,7 @@ import sqlalchemy as sa from sqlalchemy.orm import exc as orm_exc from sqlalchemy import sql +from neutron.db import api as db_api from neutron.db import common_db_mixin as common_db_api from neutron.db.quota import models as quota_models @@ -96,10 +97,11 @@ def set_quota_usage(context, resource, tenant_id, :param delta: Specifies whether in_use is an absolute number or a delta (default to False) """ - query = common_db_api.model_query(context, quota_models.QuotaUsage) - query = query.filter_by(resource=resource).filter_by(tenant_id=tenant_id) - usage_data = query.first() - with context.session.begin(subtransactions=True): + with db_api.autonested_transaction(context.session): + query = common_db_api.model_query(context, quota_models.QuotaUsage) + query = query.filter_by(resource=resource).filter_by( + tenant_id=tenant_id) + usage_data = query.first() if not usage_data: # Must create entry usage_data = quota_models.QuotaUsage( @@ -174,10 +176,6 @@ def create_reservation(context, tenant_id, deltas, expiration=None): quota_models.ResourceDelta(resource=resource, amount=delta, reservation=resv)) - # quota_usage for all resources involved in this reservation must - # be marked as dirty - set_resources_quota_usage_dirty( - context, deltas.keys(), tenant_id) return ReservationInfo(resv['id'], resv['tenant_id'], resv['expiration'], @@ -249,7 +247,7 @@ def get_reservations_for_resources(context, tenant_id, resources, quota_models.ResourceDelta.resource, quota_models.Reservation.expiration) return dict((resource, total_reserved) - for (resource, exp, total_reserved) in resv_query) + for (resource, exp, total_reserved) in resv_query) def remove_expired_reservations(context, tenant_id=None): diff --git a/neutron/db/quota/driver.py b/neutron/db/quota/driver.py index 1645b75bfe7..5358960be9f 100644 --- a/neutron/db/quota/driver.py +++ b/neutron/db/quota/driver.py @@ -134,6 +134,8 @@ class DbQuotaDriver(object): context, tenant_id=tenant_id) @oslo_db_api.wrap_db_retry(max_retries=db_api.MAX_RETRIES, + retry_interval=0.1, + inc_retry_interval=True, retry_on_request=True, retry_on_deadlock=True) def make_reservation(self, context, tenant_id, resources, deltas, plugin): @@ -150,7 +152,7 @@ class DbQuotaDriver(object): # locks should be ok to use when support for sending "hotspot" writes # to a single node will be avaialable. requested_resources = deltas.keys() - with context.session.begin(): + with db_api.autonested_transaction(context.session): # Gather current usage information # TODO(salv-orlando): calling count() for every resource triggers # multiple queries on quota usage. This should be improved, however @@ -160,7 +162,7 @@ class DbQuotaDriver(object): # instances current_usages = dict( (resource, resources[resource].count( - context, plugin, tenant_id)) for + context, plugin, tenant_id, resync_usage=False)) for resource in requested_resources) # get_tenant_quotes needs in inout a dictionary mapping resource # name to BaseResosurce instances so that the default quota can be diff --git a/neutron/quota/resource.py b/neutron/quota/resource.py index 3861afb61eb..aa580d9c554 100644 --- a/neutron/quota/resource.py +++ b/neutron/quota/resource.py @@ -131,7 +131,7 @@ class CountableResource(BaseResource): name, flag=flag, plural_name=plural_name) self._count_func = count - def count(self, context, plugin, tenant_id): + def count(self, context, plugin, tenant_id, **kwargs): return self._count_func(context, plugin, self.plural_name, tenant_id) @@ -176,10 +176,10 @@ class TrackedResource(BaseResource): def dirty(self): return self._dirty_tenants - def mark_dirty(self, context, nested=False): + def mark_dirty(self, context): if not self._dirty_tenants: return - with context.session.begin(nested=nested, subtransactions=True): + with db_api.autonested_transaction(context.session): # It is not necessary to protect this operation with a lock. # Indeed when this method is called the request has been processed # and therefore all resources created or deleted. @@ -211,6 +211,7 @@ class TrackedResource(BaseResource): # ensure that an UPDATE statement is emitted rather than an INSERT one @oslo_db_api.wrap_db_retry( max_retries=db_api.MAX_RETRIES, + retry_on_deadlock=True, exception_checker=lambda exc: isinstance(exc, oslo_db_exception.DBDuplicateEntry)) def _set_quota_usage(self, context, tenant_id, in_use): @@ -239,7 +240,7 @@ class TrackedResource(BaseResource): # Update quota usage return self._resync(context, tenant_id, in_use) - def count(self, context, _plugin, tenant_id, resync_usage=False): + def count(self, context, _plugin, tenant_id, resync_usage=True): """Return the current usage count for the resource. This method will fetch aggregate information for resource usage @@ -279,15 +280,14 @@ class TrackedResource(BaseResource): # Update quota usage, if requested (by default do not do that, as # typically one counts before adding a record, and that would mark # the usage counter as dirty again) - if resync_usage or not usage_info: + if resync_usage: usage_info = self._resync(context, tenant_id, in_use) else: - # NOTE(salv-orlando): Passing 0 for reserved amount as - # reservations are currently not supported - usage_info = quota_api.QuotaUsageInfo(usage_info.resource, - usage_info.tenant_id, - in_use, - usage_info.dirty) + resource = usage_info.resource if usage_info else self.name + tenant_id = usage_info.tenant_id if usage_info else tenant_id + dirty = usage_info.dirty if usage_info else True + usage_info = quota_api.QuotaUsageInfo( + resource, tenant_id, in_use, dirty) LOG.debug(("Quota usage for %(resource)s was recalculated. " "Used quota:%(used)d."), diff --git a/neutron/quota/resource_registry.py b/neutron/quota/resource_registry.py index 8d462e5e843..b6a41fa4d47 100644 --- a/neutron/quota/resource_registry.py +++ b/neutron/quota/resource_registry.py @@ -67,7 +67,7 @@ def set_resources_dirty(context): for res in get_all_resources().values(): with context.session.begin(subtransactions=True): if is_tracked(res.name) and res.dirty: - res.mark_dirty(context, nested=True) + res.mark_dirty(context) def resync_resource(context, resource_name, tenant_id): diff --git a/neutron/tests/unit/db/quota/test_api.py b/neutron/tests/unit/db/quota/test_api.py index e78f7943df6..ea8b2aeeaa9 100644 --- a/neutron/tests/unit/db/quota/test_api.py +++ b/neutron/tests/unit/db/quota/test_api.py @@ -263,22 +263,6 @@ class TestQuotaDbApi(testlib_api.SqlTestCaseLight): self.assertIsNone(quota_api.get_reservations_for_resources( self.context, self.tenant_id, [])) - def _test_remove_reservation(self, set_dirty): - resources = {'goals': 2, 'assists': 1} - resv = self._create_reservation(resources) - self.assertEqual(1, quota_api.remove_reservation( - self.context, resv.reservation_id, set_dirty=set_dirty)) - - def test_remove_reservation(self): - self._test_remove_reservation(False) - - def test_remove_reservation_and_set_dirty(self): - routine = 'neutron.db.quota.api.set_resources_quota_usage_dirty' - with mock.patch(routine) as mock_routine: - self._test_remove_reservation(False) - mock_routine.assert_called_once_with( - self.context, mock.ANY, self.tenant_id) - def test_remove_expired_reservations(self): with mock.patch('neutron.db.quota.api.utcnow') as mock_utcnow: mock_utcnow.return_value = datetime.datetime( diff --git a/neutron/tests/unit/quota/test_resource_registry.py b/neutron/tests/unit/quota/test_resource_registry.py index 6d1d272060f..0fa06d5bfab 100644 --- a/neutron/tests/unit/quota/test_resource_registry.py +++ b/neutron/tests/unit/quota/test_resource_registry.py @@ -156,4 +156,4 @@ class TestAuxiliaryFunctions(base.DietTestCase): # This ensures dirty is true res._dirty_tenants.add('tenant_id') resource_registry.set_resources_dirty(ctx) - mock_mark_dirty.assert_called_once_with(ctx, nested=True) + mock_mark_dirty.assert_called_once_with(ctx)