From a48e3d02ca8a0c25c3aba562ecdcac992fb7eec7 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Thu, 30 Mar 2017 18:13:47 +0000 Subject: [PATCH] Count server group members to check quota This changes server group members from a ReservableResource to a CountableResource and replaces quota reserve/commit/rollback with check_deltas accordingly. Part of blueprint cells-count-resources-to-check-quota-in-api Change-Id: I19d3dab5c849a664f2241abbeafd03efbbaa1764 --- nova/compute/api.py | 27 ++++++--- nova/quota.py | 24 +++++++- nova/tests/functional/db/test_quota.py | 77 +++++++++++++++++++++++++ nova/tests/unit/compute/test_compute.py | 68 ++++++++++++++++++++++ 4 files changed, 187 insertions(+), 9 deletions(-) create mode 100644 nova/tests/functional/db/test_quota.py diff --git a/nova/compute/api.py b/nova/compute/api.py index e6c8fca74a77..30bb8aa1ce4b 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1054,14 +1054,10 @@ class API(base.Base): if instance_group: if check_server_group_quota: - count = objects.Quotas.count_as_dict(context, - 'server_group_members', - instance_group, - context.user_id) - count_value = count['user']['server_group_members'] try: - objects.Quotas.limit_check( - context, server_group_members=count_value + 1) + objects.Quotas.check_deltas( + context, {'server_group_members': 1}, + instance_group, context.user_id) except exception.OverQuota: msg = _("Quota exceeded, too many servers in " "group") @@ -1069,6 +1065,23 @@ class API(base.Base): members = objects.InstanceGroup.add_members( context, instance_group.uuid, [instance.uuid]) + + # NOTE(melwitt): We recheck the quota after creating the + # object to prevent users from allocating more resources + # than their allowed quota in the event of a race. This is + # configurable because it can be expensive if strict quota + # limits are not required in a deployment. + if CONF.quota.recheck_quota and check_server_group_quota: + try: + objects.Quotas.check_deltas( + context, {'server_group_members': 0}, + instance_group, context.user_id) + except exception.OverQuota: + objects.InstanceGroup._remove_members_in_db( + context, instance_group.id, [instance.uuid]) + msg = _("Quota exceeded, too many servers in " + "group") + raise exception.QuotaError(msg) # list of members added to servers group in this iteration # is needed to check quota of server group during add next # instance diff --git a/nova/quota.py b/nova/quota.py index 843ba36aaccd..022ebe7fe446 100644 --- a/nova/quota.py +++ b/nova/quota.py @@ -25,10 +25,12 @@ from oslo_utils import timeutils import six import nova.conf +from nova import context as nova_context from nova import db from nova import exception from nova.i18n import _LE from nova import objects +from nova import utils LOG = logging.getLogger(__name__) @@ -1853,8 +1855,26 @@ def _keypair_get_count_by_user(context, user_id): def _server_group_count_members_by_user(context, group, user_id): - count = group.count_members_by_user(user_id) - return {'user': {'server_group_members': count}} + # NOTE(melwitt): This is mostly duplicated from + # InstanceGroup.count_members_by_user() to query across multiple cells. + # We need to be able to pass the correct cell context to + # InstanceList.get_by_filters(). + # TODO(melwitt): Counting across cells for instances means we will miss + # counting resources if a cell is down. In the future, we should query + # placement for cores/ram and InstanceMappings for instances (once we are + # deleting InstanceMappings when we delete instances). + cell_mappings = objects.CellMappingList.get_all(context) + greenthreads = [] + filters = {'deleted': False, 'user_id': user_id, 'uuid': group.members} + for cell_mapping in cell_mappings: + with nova_context.target_cell(context, cell_mapping) as cctxt: + greenthreads.append(utils.spawn( + objects.InstanceList.get_by_filters, cctxt, filters)) + instances = objects.InstanceList(objects=[]) + for greenthread in greenthreads: + found = greenthread.wait() + instances = instances + found + return {'user': {'server_group_members': len(instances)}} def _server_group_count(context, project_id, user_id=None): diff --git a/nova/tests/functional/db/test_quota.py b/nova/tests/functional/db/test_quota.py new file mode 100644 index 000000000000..9101cab8a84f --- /dev/null +++ b/nova/tests/functional/db/test_quota.py @@ -0,0 +1,77 @@ +# 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_utils import uuidutils + +from nova import context +from nova import objects +from nova import quota +from nova import test +from nova.tests import fixtures as nova_fixtures + + +class QuotaTestCase(test.NoDBTestCase): + USES_DB_SELF = True + + def setUp(self): + super(QuotaTestCase, self).setUp() + self.useFixture(nova_fixtures.SpawnIsSynchronousFixture()) + self.useFixture(nova_fixtures.Database(database='api')) + fix = nova_fixtures.CellDatabases() + fix.add_cell_database('cell1') + fix.add_cell_database('cell2') + self.useFixture(fix) + + def test_server_group_members_count_by_user(self): + ctxt = context.RequestContext('fake-user', 'fake-project') + mapping1 = objects.CellMapping(context=ctxt, + uuid=uuidutils.generate_uuid(), + database_connection='cell1', + transport_url='none:///') + mapping2 = objects.CellMapping(context=ctxt, + uuid=uuidutils.generate_uuid(), + database_connection='cell2', + transport_url='none:///') + mapping1.create() + mapping2.create() + + # Create a server group the instances will use. + group = objects.InstanceGroup(context=ctxt) + group.create() + instance_uuids = [] + + # Create an instance in cell1 + with context.target_cell(ctxt, mapping1) as cctxt: + instance = objects.Instance(context=cctxt, + project_id='fake-project', + user_id='fake-user') + instance.create() + instance_uuids.append(instance.uuid) + + # Create an instance in cell2 + with context.target_cell(ctxt, mapping2) as cctxt: + instance = objects.Instance(context=cctxt, + project_id='fake-project', + user_id='fake-user') + instance.create() + instance_uuids.append(instance.uuid) + + # Add the uuids to the group + objects.InstanceGroup.add_members(ctxt, group.uuid, instance_uuids) + # add_members() doesn't add the members to the object field + group.members.extend(instance_uuids) + + # Count server group members across cells + count = quota._server_group_count_members_by_user(ctxt, group, + 'fake-user') + + self.assertEqual(2, count['user']['server_group_members']) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 57aaec45795b..aaaacc01d7a6 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -8429,6 +8429,74 @@ class ComputeAPITestCase(BaseTestCase): group = objects.InstanceGroup.get_by_uuid(self.context, group.uuid) self.assertIn(refs[0]['uuid'], group.members) + @mock.patch('nova.objects.quotas.Quotas.check_deltas') + @mock.patch('nova.compute.api.API._get_requested_instance_group') + def test_create_instance_group_members_over_quota_during_recheck( + self, get_group_mock, check_deltas_mock): + self.stub_out('nova.tests.unit.image.fake._FakeImageService.show', + self.fake_show) + + # Simulate a race where the first check passes and the recheck fails. + check_deltas_mock.side_effect = [ + None, exception.OverQuota(overs='server_group_members')] + + group = objects.InstanceGroup(self.context) + group.uuid = uuids.fake + group.project_id = self.context.project_id + group.user_id = self.context.user_id + group.create() + get_group_mock.return_value = group + + inst_type = flavors.get_default_flavor() + self.assertRaises(exception.QuotaError, self.compute_api.create, + self.context, inst_type, self.fake_image['id'], + scheduler_hints={'group': group.uuid}, + check_server_group_quota=True) + + self.assertEqual(2, check_deltas_mock.call_count) + call1 = mock.call(self.context, {'server_group_members': 1}, group, + self.context.user_id) + call2 = mock.call(self.context, {'server_group_members': 0}, group, + self.context.user_id) + check_deltas_mock.assert_has_calls([call1, call2]) + + # Verify we removed the group members that were added after the first + # quota check passed. + group = objects.InstanceGroup.get_by_uuid(self.context, group.uuid) + self.assertEqual(0, len(group.members)) + + @mock.patch('nova.objects.quotas.Quotas.check_deltas') + @mock.patch('nova.compute.api.API._get_requested_instance_group') + def test_create_instance_group_members_no_quota_recheck(self, + get_group_mock, + check_deltas_mock): + self.stub_out('nova.tests.unit.image.fake._FakeImageService.show', + self.fake_show) + # Disable recheck_quota. + self.flags(recheck_quota=False, group='quota') + + group = objects.InstanceGroup(self.context) + group.uuid = uuids.fake + group.project_id = self.context.project_id + group.user_id = self.context.user_id + group.create() + get_group_mock.return_value = group + + inst_type = flavors.get_default_flavor() + (refs, resv_id) = self.compute_api.create( + self.context, inst_type, self.fake_image['id'], + scheduler_hints={'group': group.uuid}, + check_server_group_quota=True) + self.assertEqual(len(refs), len(group.members)) + + # check_deltas should have been called only once. + check_deltas_mock.assert_called_once_with( + self.context, {'server_group_members': 1}, group, + self.context.user_id) + + group = objects.InstanceGroup.get_by_uuid(self.context, group.uuid) + self.assertIn(refs[0]['uuid'], group.members) + def test_instance_create_with_group_uuid_fails_group_not_exist(self): self.stub_out('nova.tests.unit.image.fake._FakeImageService.show', self.fake_show)