Default user_id when not specified in check_num_instances_quota

The Quotas.check_deltas method needs a user_id keyword arg in order to
scope a quota check to a particular user. However, when we call
check_num_instances_quota we don't pass a project_id or user_id because
at the time of the quota check, we have not yet created an instance
record and thus will not use that to determine the appropriate project
and user.

Instead, we should rely on the RequestContext.project_id and
RequestContext.user_id as defaults in this case, but
check_num_instances_quota only defaults project_id and not user_id.

check_num_instances_quota should also default user_id to the
RequestContext.user_id when user_id is not explicitly passed.

check_num_instances_quota should also check whether any per-user quota
limits are defined for instance-related resources before passing along
the user_id to scope resource counting and limit checking. Counting
resources across a user is costly, so we should avoid it if it's not
needed.

Closes-Bug: #1893284

Change-Id: I3cfb1edc30b0bda4671e0d2cc2a8993055dcc9ff
(cherry picked from commit 4c11d5467a30506a82dd5d32dd22b8958a187c0b)

4 files changed, 66 insertions, 9 deletions

diff --git a/nova/compute/utils.py b/nova/compute/utils.py
index 767050c123..13093e5f67 100644
--- a/nova/compute/utils.py
+++ b/nova/compute/utils.py
@@ -1112,9 +1112,18 @@ def check_num_instances_quota(context, instance_type, min_count,
                               max_count, project_id=None, user_id=None,
                               orig_num_req=None):
     """Enforce quota limits on number of instances created."""
-    # project_id is used for the TooManyInstances error message
+    # project_id is also used for the TooManyInstances error message
     if project_id is None:
         project_id = context.project_id
+    if user_id is None:
+        user_id = context.user_id
+    # Check whether we need to count resources per-user and check a per-user
+    # quota limit. If we have no per-user quota limit defined for a
+    # project/user, we can avoid wasteful resource counting.
+    user_quotas = objects.Quotas.get_all_by_project_and_user(
+        context, project_id, user_id)
+    if not any(r in user_quotas for r in ['instances', 'cores', 'ram']):
+        user_id = None
     # Determine requested cores and ram
     req_cores = max_count * instance_type.vcpus
     req_ram = max_count * instance_type.memory_mb
diff --git a/nova/tests/functional/regressions/test_bug_1893284.py b/nova/tests/functional/regressions/test_bug_1893284.py
index 9e9d48ff9a..91774c9834 100644
--- a/nova/tests/functional/regressions/test_bug_1893284.py
+++ b/nova/tests/functional/regressions/test_bug_1893284.py
@@ -84,10 +84,8 @@ class TestServersPerUserQuota(test.TestCase,
         server_req = self._build_server(
             image_uuid=fake_image.AUTO_DISK_CONFIG_ENABLED_IMAGE_UUID,
             networks='none')
-        # FIXME(melwitt): Uncomment this when the bug is fixed. Because of the
-        # the bug, the first request by the non-admin user will fail.
-        # server = self.api.post_server({'server': server_req})
-        # self._wait_for_state_change(server, 'ACTIVE')
+        server = self.api.post_server({'server': server_req})
+        self._wait_for_state_change(server, 'ACTIVE')
         # A request to boot a second instance should fail because the
         # non-admin has already booted 1 allowed instance.
         ex = self.assertRaises(
diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py
index 9b34d2f314..21a3003344 100644
--- a/nova/tests/unit/compute/test_compute_api.py
+++ b/nova/tests/unit/compute/test_compute_api.py
@@ -231,6 +231,8 @@ class _ComputeAPIUnitTestMixIn(object):
                                     requested_networks=requested_networks,
                                     max_count=None)
 
+    @mock.patch('nova.objects.Quotas.get_all_by_project_and_user',
+                new=mock.MagicMock())
     @mock.patch('nova.objects.Quotas.count_as_dict')
     @mock.patch('nova.objects.Quotas.limit_check')
     @mock.patch('nova.objects.Quotas.limit_check_project_and_user')
@@ -4195,6 +4197,8 @@ class _ComputeAPIUnitTestMixIn(object):
             self._test_check_injected_file_quota_onset_file_limit_exceeded,
             side_effect)
 
+    @mock.patch('nova.objects.Quotas.get_all_by_project_and_user',
+                new=mock.MagicMock())
     @mock.patch('nova.objects.Quotas.count_as_dict')
     @mock.patch('nova.objects.Quotas.limit_check_project_and_user')
     @mock.patch('nova.objects.Instance.save')
@@ -4219,9 +4223,11 @@ class _ComputeAPIUnitTestMixIn(object):
         self.assertEqual(instance.task_state, task_states.RESTORING)
         # mock.ANY might be 'instances', 'cores', or 'ram' depending on how the
         # deltas dict is iterated in check_deltas
+        # user_id is expected to be None because no per-user quotas have been
+        # defined
         quota_count.assert_called_once_with(admin_context, mock.ANY,
                                             instance.project_id,
-                                            user_id=instance.user_id)
+                                            user_id=None)
         quota_check.assert_called_once_with(
             admin_context,
             user_values={'instances': 2,
@@ -4230,9 +4236,11 @@ class _ComputeAPIUnitTestMixIn(object):
             project_values={'instances': 2,
                             'cores': 1 + instance.flavor.vcpus,
                             'ram': 512 + instance.flavor.memory_mb},
-            project_id=instance.project_id, user_id=instance.user_id)
+            project_id=instance.project_id)
         update_qfd.assert_called_once_with(admin_context, instance, False)
 
+    @mock.patch('nova.objects.Quotas.get_all_by_project_and_user',
+                new=mock.MagicMock())
     @mock.patch('nova.objects.Quotas.count_as_dict')
     @mock.patch('nova.objects.Quotas.limit_check_project_and_user')
     @mock.patch('nova.objects.Instance.save')
@@ -4256,9 +4264,11 @@ class _ComputeAPIUnitTestMixIn(object):
         self.assertEqual(instance.task_state, task_states.RESTORING)
         # mock.ANY might be 'instances', 'cores', or 'ram' depending on how the
         # deltas dict is iterated in check_deltas
+        # user_id is expected to be None because no per-user quotas have been
+        # defined
         quota_count.assert_called_once_with(self.context, mock.ANY,
                                             instance.project_id,
-                                            user_id=instance.user_id)
+                                            user_id=None)
         quota_check.assert_called_once_with(
             self.context,
             user_values={'instances': 2,
@@ -4267,7 +4277,7 @@ class _ComputeAPIUnitTestMixIn(object):
             project_values={'instances': 2,
                             'cores': 1 + instance.flavor.vcpus,
                             'ram': 512 + instance.flavor.memory_mb},
-            project_id=instance.project_id, user_id=instance.user_id)
+            project_id=instance.project_id)
         update_qfd.assert_called_once_with(self.context, instance, False)
 
     @mock.patch.object(objects.InstanceAction, 'action_start')
diff --git a/nova/tests/unit/compute/test_compute_utils.py b/nova/tests/unit/compute/test_compute_utils.py
index 0f84b3e513..2ea3971360 100644
--- a/nova/tests/unit/compute/test_compute_utils.py
+++ b/nova/tests/unit/compute/test_compute_utils.py
@@ -1398,6 +1398,46 @@ class ComputeUtilsQuotaTestCase(test.TestCase):
             else:
                 self.fail("Exception not raised")
 
+    @mock.patch('nova.objects.Quotas.get_all_by_project_and_user')
+    @mock.patch('nova.objects.Quotas.check_deltas')
+    def test_check_num_instances_omits_user_if_no_user_quota(self, mock_check,
+                                                             mock_get):
+        # Return no per-user quota.
+        mock_get.return_value = {'project_id': self.context.project_id,
+                                 'user_id': self.context.user_id}
+        fake_flavor = objects.Flavor(vcpus=1, memory_mb=512)
+        compute_utils.check_num_instances_quota(
+            self.context, fake_flavor, 1, 1)
+        deltas = {'instances': 1, 'cores': 1, 'ram': 512}
+        # Verify that user_id has not been passed along to scope the resource
+        # counting.
+        mock_check.assert_called_once_with(
+            self.context, deltas, self.context.project_id, user_id=None,
+            check_project_id=self.context.project_id, check_user_id=None)
+
+    @mock.patch('nova.objects.Quotas.get_all_by_project_and_user')
+    @mock.patch('nova.objects.Quotas.check_deltas')
+    def test_check_num_instances_passes_user_if_user_quota(self, mock_check,
+                                                           mock_get):
+        for resource in ['instances', 'cores', 'ram']:
+            # Return some per-user quota for each of the instance-related
+            # resources.
+            mock_get.return_value = {'project_id': self.context.project_id,
+                                     'user_id': self.context.user_id,
+                                     resource: 5}
+            fake_flavor = objects.Flavor(vcpus=1, memory_mb=512)
+            compute_utils.check_num_instances_quota(
+                self.context, fake_flavor, 1, 1)
+            deltas = {'instances': 1, 'cores': 1, 'ram': 512}
+            # Verify that user_id is passed along to scope the resource
+            # counting and limit checking.
+            mock_check.assert_called_once_with(
+                self.context, deltas, self.context.project_id,
+                user_id=self.context.user_id,
+                check_project_id=self.context.project_id,
+                check_user_id=self.context.user_id)
+            mock_check.reset_mock()
+
 
 class IsVolumeBackedInstanceTestCase(test.TestCase):
     def setUp(self):