Execute the quota reservation removal in an isolated DB txn

The goal of [1] is to, in case of failing when removing the quota
reservation, continue the operation. Any expired reservation will
be removed automatically in any driver.

If the DB transaction fails, it should affect only to the reservation
trying to be deleted. This is why this patch isolates the
"remove_reservation" method and guarantees it is called outside an
active DB session. That guarantees, in case of failure, no other DB
operation will be affected.

This patch also partially reverts [2] but still checks the security
group rule quota when a new security group is created. Instead of
creating and releasing a quota reservation for the security group
rules created, now only the available quota limit is checked before
creating them. That won't prevent another operation to create security
group rules in parallel, exceeding the available quota. However, this
is not even guaranteed with the current quota driver.

[1]https://review.opendev.org/c/openstack/neutron/+/805031
[2]https://review.opendev.org/c/openstack/neutron/+/701565

Closes-Bug: #1943714

Change-Id: Id73368576a948f78a043d7cf0be16661a65626a9
This commit is contained in:
Rodolfo Alonso Hernandez 2021-09-20 09:27:39 +00:00
parent d3fa1ac3bf
commit 603abeb977
15 changed files with 278 additions and 109 deletions

View File

@ -498,7 +498,6 @@ class Controller(object):
def notify(create_result):
# Ensure usage trackers for all resources affected by this API
# operation are marked as dirty
with db_api.CONTEXT_WRITER.using(request.context):
# Commit the reservation(s)
for reservation in reservations:
quota.QUOTAS.commit_reservation(

View File

@ -36,6 +36,7 @@ from eventlet.green import subprocess
import netaddr
from neutron_lib.api.definitions import availability_zone as az_def
from neutron_lib import constants as n_const
from neutron_lib import context as n_context
from neutron_lib.db import api as db_api
from neutron_lib.services.trunk import constants as trunk_constants
from neutron_lib.utils import helpers
@ -675,15 +676,22 @@ def transaction_guard(f):
If you receive this error, you must alter your code to handle the fact that
the thing you are calling can have side effects so using transactions to
undo on failures is not possible.
This method can be called from a class method or a static method. "inner"
should consider if "self" is passed or not. The next mandatory parameter is
the context ``neutron_lib.context.Context``.
"""
@functools.wraps(f)
def inner(self, context, *args, **kwargs):
def inner(*args, **kwargs):
context = (args[0] if issubclass(type(args[0]),
n_context.ContextBaseWithSession) else
args[1])
# FIXME(kevinbenton): get rid of all uses of this flag
if (context.session.is_active and
getattr(context, 'GUARD_TRANSACTION', True)):
raise RuntimeError(_("Method %s cannot be called within a "
"transaction.") % f)
return f(self, context, *args, **kwargs)
return f(*args, **kwargs)
return inner

View File

@ -19,10 +19,12 @@ import datetime
from neutron_lib.db import api as db_api
from oslo_db import exception as db_exc
from neutron.common import utils
from neutron.objects import quota as quota_obj
RESERVATION_EXPIRATION_TIMEOUT = 20 # seconds
UNLIMITED_QUOTA = -1
# Wrapper for utcnow - needed for mocking it in unit tests
@ -201,11 +203,11 @@ def get_reservation(context, reservation_id):
for delta in reserv_obj.resource_deltas))
@utils.transaction_guard
@utils.skip_exceptions(db_exc.DBError)
@db_api.CONTEXT_WRITER
def remove_reservation(context, reservation_id, set_dirty=False):
try:
reservation = quota_obj.Reservation.get_object(context,
id=reservation_id)
reservation = quota_obj.Reservation.get_object(context, id=reservation_id)
if not reservation:
# TODO(salv-orlando): Raise here and then handle the exception?
return
@ -217,8 +219,6 @@ def remove_reservation(context, reservation_id, set_dirty=False):
# be marked as dirty
set_resources_quota_usage_dirty(context, resources, tenant_id)
return 1
except db_exc.DBError:
context.session.rollback()
@db_api.retry_if_session_inactive()
@ -374,6 +374,37 @@ class QuotaDriverAPI(object, metaclass=abc.ABCMeta):
quota.
"""
@staticmethod
@abc.abstractmethod
def get_resource_usage(context, project_id, resources, resource_name):
"""Return the resource current usage
:param context: The request context, for access checks.
:param project_id: The ID of the project to make the reservations for.
:param resources: A dictionary of the registered resources.
:param resource_name: The name of the resource to retrieve the usage.
:return: The current resource usage.
"""
@staticmethod
@abc.abstractmethod
def quota_limit_check(context, project_id, resources, deltas):
"""Check the current resource usage against a set of deltas.
This method will check if the provided resource deltas could be
assigned depending on the current resource usage and the quota limits.
If the resource deltas plus the resource usage fit under the quota
limit, the method will pass. If not, a ``OverQuota`` will be raised.
:param context: The request context, for access checks.
:param project_id: The ID of the project to make the reservations for.
:param resources: A dictionary of the registered resource.
:param deltas: A dictionary of the values to check against the
quota limits.
:return: None if passed; ``OverQuota`` if quota limits are exceeded,
``InvalidQuotaValue`` if delta values are invalid.
"""
class NullQuotaDriver(QuotaDriverAPI):
@ -416,3 +447,11 @@ class NullQuotaDriver(QuotaDriverAPI):
@staticmethod
def limit_check(context, project_id, resources, values):
pass
@staticmethod
def get_resource_usage(context, project_id, resources, resource_name):
pass
@staticmethod
def quota_limit_check(context, project_id, resources, deltas):
pass

View File

@ -310,3 +310,37 @@ class DbQuotaDriver(quota_api.QuotaDriverAPI):
overs = [key for key, val in values.items() if 0 <= quotas[key] < val]
if overs:
raise exceptions.OverQuota(overs=sorted(overs))
@staticmethod
def get_resource_usage(context, project_id, resources, resource_name):
tracked_resource = resources.get(resource_name)
if not tracked_resource:
return
return tracked_resource.count(context, None, project_id,
resync_usage=False)
def quota_limit_check(self, context, project_id, resources, deltas):
# Ensure no value is less than zero
unders = [key for key, val in deltas.items() if val < 0]
if unders:
raise exceptions.InvalidQuotaValue(unders=sorted(unders))
current_limits = self.get_project_quotas(context, resources,
project_id)
overs = set()
for resource_name, delta in deltas.items():
resource_limit = current_limits.get(resource_name)
if resource_limit in (None, quota_api.UNLIMITED_QUOTA):
continue
resource_usage = self.get_resource_usage(context, project_id,
resources, resource_name)
if resource_usage is None:
continue
if resource_usage + delta > resource_limit:
overs.add(resource_name)
if overs:
raise exceptions.OverQuota(overs=sorted(overs))

View File

@ -60,12 +60,8 @@ class DbQuotaNoLockDriver(quota_driver.DbQuotaDriver):
# 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:
tracked_resource = resources.get(resource_name)
if not tracked_resource:
continue
used_and_reserved = tracked_resource.count(
context, None, project_id, count_db_registers=True)
used_and_reserved = self.get_resource_usage(
context, project_id, resources, resource_name)
resource_num = deltas[resource_name]
if limits[resource_name] < (used_and_reserved + resource_num):
resources_over_limit.append(resource_name)
@ -77,3 +73,11 @@ class DbQuotaNoLockDriver(quota_driver.DbQuotaDriver):
def cancel_reservation(self, context, reservation_id):
quota_api.remove_reservation(context, reservation_id, set_dirty=False)
@staticmethod
def get_resource_usage(context, project_id, resources, resource_name):
tracked_resource = resources.get(resource_name)
if not tracked_resource:
return
return tracked_resource.count(context, None, project_id,
count_db_registers=True)

View File

@ -106,9 +106,8 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase,
with db_api.CONTEXT_WRITER.using(context):
delta = len(ext_sg.sg_supported_ethertypes)
delta = delta * 2 if default_sg else delta
reservation = quota.QUOTAS.make_reservation(
context, tenant_id, {'security_group_rule': delta},
self)
quota.QUOTAS.quota_limit_check(context, tenant_id,
security_group_rule=delta)
sg = sg_obj.SecurityGroup(
context, id=s.get('id') or uuidutils.generate_uuid(),
@ -135,10 +134,6 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase,
sg.rules.append(egress_rule)
sg.obj_reset_changes(['rules'])
if reservation:
quota.QUOTAS.commit_reservation(context,
reservation.reservation_id)
# fetch sg from db to load the sg rules with sg model.
# NOTE(slaweq): With new system/project scopes it may happen that
# project admin will try to list security groups for different

View File

@ -77,7 +77,7 @@ class QuotaEnforcementHook(hooks.PecanHook):
reservations = state.request.context.get('reservations') or []
if not reservations and state.request.method != 'DELETE':
return
with db_api.CONTEXT_WRITER.using(neutron_context):
# Commit the reservation(s)
for reservation in reservations:
quota.QUOTAS.commit_reservation(

View File

@ -145,5 +145,9 @@ class QuotaEngine(object):
return self.get_driver().limit_check(
context, tenant_id, resource_registry.get_all_resources(), values)
def quota_limit_check(self, context, project_id, **deltas):
return self.get_driver().quota_limit_check(
context, project_id, resource_registry.get_all_resources(), deltas)
QUOTAS = QuotaEngine.get_instance()

View File

@ -98,7 +98,7 @@ class BaseResource(object, metaclass=abc.ABCMeta):
value = getattr(cfg.CONF.QUOTAS,
self.flag,
cfg.CONF.QUOTAS.default_quota)
return max(value, -1)
return max(value, quota_api.UNLIMITED_QUOTA)
@property
@abc.abstractmethod

View File

@ -45,10 +45,8 @@ class NetworkRBACTestCase(testlib_api.SqlTestCase):
self.subnet_1_id = uuidutils.generate_uuid()
self.subnet_2_id = uuidutils.generate_uuid()
self.port_id = uuidutils.generate_uuid()
make_res = mock.patch.object(quota.QuotaEngine, 'make_reservation')
self.mock_quota_make_res = make_res.start()
commit_res = mock.patch.object(quota.QuotaEngine, 'commit_reservation')
self.mock_quota_commit_res = commit_res.start()
quota_check = mock.patch.object(quota.QuotaEngine, 'quota_limit_check')
self.mock_quota_check = quota_check.start()
def _create_network(self, tenant_id, network_id, shared, external=False):
network = {'tenant_id': tenant_id,

View File

@ -92,6 +92,9 @@ class TestDbQuotaDriver(testlib_api.SqlTestCase,
self.plugin = FakePlugin()
self.context = context.get_admin_context()
self.setup_coreplugin(core_plugin=DB_PLUGIN_KLASS)
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'
def test_create_quota_limit(self):
defaults = {RESOURCE: TestResource(RESOURCE, 4)}
@ -142,16 +145,13 @@ class TestDbQuotaDriver(testlib_api.SqlTestCase,
self.assertFalse(self.plugin.get_project_quotas(user_ctx, {}, PROJECT))
def test_get_all_quotas(self):
project_1 = 'prj_test_1'
project_2 = 'prj_test_2'
resource_1 = 'res_test_1'
resource_2 = 'res_test_2'
resources = {self.resource_1: TestResource(self.resource_1, 3),
self.resource_2: TestResource(self.resource_2, 5)}
resources = {resource_1: TestResource(resource_1, 3),
resource_2: TestResource(resource_2, 5)}
self.plugin.update_quota_limit(self.context, project_1, resource_1, 7)
self.plugin.update_quota_limit(self.context, project_2, resource_2, 9)
self.plugin.update_quota_limit(self.context, self.project_1,
self.resource_1, 7)
self.plugin.update_quota_limit(self.context, self.project_2,
self.resource_2, 9)
quotas = self.plugin.get_all_quotas(self.context, resources)
# Expect two projects' quotas
@ -162,15 +162,15 @@ class TestDbQuotaDriver(testlib_api.SqlTestCase,
# Check the expected limits. The quotas can be in any order.
for quota in quotas:
project = quota['project_id']
self.assertIn(project, (project_1, project_2))
if project == project_1:
self.assertIn(project, (self.project_1, self.project_2))
if project == self.project_1:
expected_limit_r1 = 7
expected_limit_r2 = 5
if project == project_2:
if project == self.project_2:
expected_limit_r1 = 3
expected_limit_r2 = 9
self.assertEqual(expected_limit_r1, quota[resource_1])
self.assertEqual(expected_limit_r2, quota[resource_2])
self.assertEqual(expected_limit_r1, quota[self.resource_1])
self.assertEqual(expected_limit_r2, quota[self.resource_2])
def test_limit_check(self):
resources = {RESOURCE: TestResource(RESOURCE, 2)}
@ -222,23 +222,20 @@ class TestDbQuotaDriver(testlib_api.SqlTestCase,
reservation.project_id)
def test_make_reservation_single_resource(self):
quota_driver = driver.DbQuotaDriver()
self._test_make_reservation_success(
quota_driver, RESOURCE, {RESOURCE: 1})
self.quota_driver, RESOURCE, {RESOURCE: 1})
def test_make_reservation_fill_quota(self):
quota_driver = driver.DbQuotaDriver()
self._test_make_reservation_success(
quota_driver, RESOURCE, {RESOURCE: 2})
self.quota_driver, RESOURCE, {RESOURCE: 2})
def test_make_reservation_multiple_resources(self):
quota_driver = driver.DbQuotaDriver()
resources = {RESOURCE: TestResource(RESOURCE, 2),
ALT_RESOURCE: TestResource(ALT_RESOURCE, 2)}
deltas = {RESOURCE: 1, ALT_RESOURCE: 2}
self.plugin.update_quota_limit(self.context, PROJECT, RESOURCE, 2)
self.plugin.update_quota_limit(self.context, PROJECT, ALT_RESOURCE, 2)
reservation = quota_driver.make_reservation(
reservation = self.quota_driver.make_reservation(
self.context,
self.context.project_id,
resources,
@ -252,13 +249,12 @@ class TestDbQuotaDriver(testlib_api.SqlTestCase,
reservation.project_id)
def test_make_reservation_over_quota_fails(self):
quota_driver = driver.DbQuotaDriver()
resources = {RESOURCE: TestResource(RESOURCE, 2,
fake_count=2)}
deltas = {RESOURCE: 1}
self.plugin.update_quota_limit(self.context, PROJECT, RESOURCE, 2)
self.assertRaises(exceptions.OverQuota,
quota_driver.make_reservation,
self.quota_driver.make_reservation,
self.context,
self.context.project_id,
resources,
@ -269,8 +265,7 @@ class TestDbQuotaDriver(testlib_api.SqlTestCase,
res = {RESOURCE: TestTrackedResource(RESOURCE, test_quota.MehModel)}
self.plugin.update_quota_limit(self.context, PROJECT, RESOURCE, 6)
quota_driver = driver.DbQuotaDriver()
quota_driver.make_reservation(self.context, PROJECT, res,
self.quota_driver.make_reservation(self.context, PROJECT, res,
{RESOURCE: 1}, self.plugin)
quota_api.set_quota_usage(self.context, RESOURCE, PROJECT, 2)
detailed_quota = self.plugin.get_detailed_project_quotas(
@ -279,32 +274,99 @@ class TestDbQuotaDriver(testlib_api.SqlTestCase,
self.assertEqual(2, detailed_quota[RESOURCE]['used'])
self.assertEqual(1, detailed_quota[RESOURCE]['reserved'])
def _create_resources(self):
return {
self.resource_1:
TestTrackedResource(self.resource_1, test_quota.MehModel),
self.resource_2:
TestCountableResource(self.resource_2, _count_resource)}
def test_get_detailed_project_quotas_multiple_resource(self):
project_1 = 'prj_test_1'
resource_1 = 'res_test_1'
resource_2 = 'res_test_2'
resources = {resource_1:
TestTrackedResource(resource_1, test_quota.MehModel),
resource_2:
TestCountableResource(resource_2, _count_resource)}
resources = self._create_resources()
self.plugin.update_quota_limit(self.context, self.project_1,
self.resource_1, 6)
self.plugin.update_quota_limit(self.context, self.project_1,
self.resource_2, 9)
self.quota_driver.make_reservation(
self.context, self.project_1, resources,
{self.resource_1: 1, self.resource_2: 7}, self.plugin)
self.plugin.update_quota_limit(self.context, project_1, resource_1, 6)
self.plugin.update_quota_limit(self.context, project_1, resource_2, 9)
quota_driver = driver.DbQuotaDriver()
quota_driver.make_reservation(self.context, project_1,
resources,
{resource_1: 1, resource_2: 7},
self.plugin)
quota_api.set_quota_usage(self.context, resource_1, project_1, 2)
quota_api.set_quota_usage(self.context, resource_2, project_1, 3)
quota_api.set_quota_usage(self.context, self.resource_1,
self.project_1, 2)
quota_api.set_quota_usage(self.context, self.resource_2,
self.project_1, 3)
detailed_quota = self.plugin.get_detailed_project_quotas(
self.context, resources, project_1)
self.context, resources, self.project_1)
self.assertEqual(6, detailed_quota[resource_1]['limit'])
self.assertEqual(1, detailed_quota[resource_1]['reserved'])
self.assertEqual(2, detailed_quota[resource_1]['used'])
self.assertEqual(6, detailed_quota[self.resource_1]['limit'])
self.assertEqual(1, detailed_quota[self.resource_1]['reserved'])
self.assertEqual(2, detailed_quota[self.resource_1]['used'])
self.assertEqual(9, detailed_quota[resource_2]['limit'])
self.assertEqual(7, detailed_quota[resource_2]['reserved'])
self.assertEqual(3, detailed_quota[resource_2]['used'])
self.assertEqual(9, detailed_quota[self.resource_2]['limit'])
self.assertEqual(7, detailed_quota[self.resource_2]['reserved'])
self.assertEqual(3, detailed_quota[self.resource_2]['used'])
def test_quota_limit_check(self):
resources = self._create_resources()
self.plugin.update_quota_limit(self.context, self.project_1,
self.resource_1, 10)
self.plugin.update_quota_limit(self.context, self.project_1,
self.resource_2, 10)
reservations = {self.resource_1: 8}
self.quota_driver.make_reservation(
self.context, self.project_1, resources, reservations, self.plugin)
resources[self.resource_2]._count_func = lambda x, y, z: 8
self.assertIsNone(self.quota_driver.quota_limit_check(
self.context, self.project_1, resources, {self.resource_1: 2}))
self.assertIsNone(self.quota_driver.quota_limit_check(
self.context, self.project_1, resources, {self.resource_2: 2}))
self.assertRaises(
exceptions.OverQuota, self.quota_driver.quota_limit_check,
self.context, self.project_1, resources, {self.resource_1: 3})
self.assertRaises(
exceptions.OverQuota, self.quota_driver.quota_limit_check,
self.context, self.project_1, resources, {self.resource_2: 3})
self.assertRaises(
exceptions.OverQuota, self.quota_driver.quota_limit_check,
self.context, self.project_1, resources,
{self.resource_1: 3, self.resource_2: 3})
def test_quota_limit_check_unlimited(self):
resources = self._create_resources()
self.plugin.update_quota_limit(self.context, self.project_1,
self.resource_1, -1)
self.plugin.update_quota_limit(self.context, self.project_1,
self.resource_2, -1)
reservations = {self.resource_1: 8}
self.quota_driver.make_reservation(
self.context, self.project_1, resources, reservations, self.plugin)
resources[self.resource_2]._count_func = lambda x, y, z: 8
self.assertIsNone(self.quota_driver.quota_limit_check(
self.context, self.project_1, resources, {self.resource_1: 2}))
self.assertIsNone(self.quota_driver.quota_limit_check(
self.context, self.project_1, resources, {self.resource_2: 2}))
self.assertIsNone(self.quota_driver.quota_limit_check(
self.context, self.project_1, resources,
{self.resource_1: 10 ** 9, self.resource_2: 10 ** 9}))
def test_quota_limit_check_untracked_resource(self):
resources = self._create_resources()
self.plugin.update_quota_limit(self.context, self.project_1,
self.resource_1, -1)
self.plugin.update_quota_limit(self.context, self.project_1,
self.resource_2, -1)
reservations = {self.resource_1: 8}
self.quota_driver.make_reservation(
self.context, self.project_1, resources, reservations, self.plugin)
resources[self.resource_2]._count_func = lambda x, y, z: 8
self.assertIsNone(self.quota_driver.quota_limit_check(
self.context, self.project_1, resources,
{self.resource_1: 10 ** 9}))
self.assertIsNone(self.quota_driver.quota_limit_check(
self.context, self.project_1, resources,
{self.resource_2: 10 ** 9}))
self.assertIsNone(self.quota_driver.quota_limit_check(
self.context, self.project_1, resources, {'untracked': 10 ** 9}))

View File

@ -0,0 +1,24 @@
# Copyright (c) 2021 Red Hat, Inc.
#
# 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 neutron.db.quota import driver_nolock
from neutron.tests.unit.db.quota import test_driver
class TestDbQuotaDriverNoLock(test_driver.TestDbQuotaDriver):
def setUp(self):
super(TestDbQuotaDriverNoLock, self).setUp()
self.quota_driver = driver_nolock.DbQuotaNoLockDriver()

View File

@ -74,10 +74,8 @@ class SecurityGroupDbMixinTestCase(testlib_api.SqlTestCase):
self.setup_coreplugin(core_plugin=DB_PLUGIN_KLASS)
self.ctx = context.get_admin_context()
self.mixin = SecurityGroupDbMixinImpl()
make_res = mock.patch.object(quota.QuotaEngine, 'make_reservation')
self.mock_quota_make_res = make_res.start()
commit_res = mock.patch.object(quota.QuotaEngine, 'commit_reservation')
self.mock_quota_commit_res = commit_res.start()
quota_check = mock.patch.object(quota.QuotaEngine, 'quota_limit_check')
self.mock_quota_check = quota_check.start()
is_ext_supported = mock.patch(
'neutron_lib.api.extensions.is_extension_supported')
self.is_ext_supported = is_ext_supported.start()

View File

@ -2246,10 +2246,8 @@ class TestMl2PortBinding(Ml2PluginV2TestCase,
cfg.CONF.set_override(
'enable_security_group', self.ENABLE_SG,
group='SECURITYGROUP')
make_res = mock.patch.object(quota.QuotaEngine, 'make_reservation')
self.mock_quota_make_res = make_res.start()
commit_res = mock.patch.object(quota.QuotaEngine, 'commit_reservation')
self.mock_quota_commit_res = commit_res.start()
quota_check = mock.patch.object(quota.QuotaEngine, 'quota_limit_check')
self.mock_quota_check = quota_check.start()
super(TestMl2PortBinding, self).setUp()
def _check_port_binding_profile(self, port, profile=None):
@ -2970,10 +2968,8 @@ class TestMl2PortSecurity(Ml2PluginV2TestCase):
cfg.CONF.set_override('enable_security_group',
False,
group='SECURITYGROUP')
make_res = mock.patch.object(quota.QuotaEngine, 'make_reservation')
self.mock_quota_make_res = make_res.start()
commit_res = mock.patch.object(quota.QuotaEngine, 'commit_reservation')
self.mock_quota_commit_res = commit_res.start()
quota_check = mock.patch.object(quota.QuotaEngine, 'quota_limit_check')
self.mock_quota_check = quota_check.start()
super(TestMl2PortSecurity, self).setUp()
def test_port_update_without_security_groups(self):

View File

@ -0,0 +1,8 @@
---
features:
- |
Added two new API methods to ``QuotaDriverAPI`` class.
``get_resource_usage`` returns the current resource usage.
``quota_limit_check`` checks the current resource usage of several
resources against a set of deltas (a dictionary of resource names
and resource counters).