Merge "Default user_id when not specified in check_num_instances_quota"

Author: Zuul

nova/compute/utils.py

@@ -1105,9 +1105,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

nova/tests/functional/regressions/test_bug_1893284.py

@@ -84,10 +84,8 @@ def test_create_server_with_per_user_quota(self):
         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(

nova/tests/unit/compute/test_api.py

@@ -231,6 +231,8 @@ def test_create_with_networks_max_count_none(self, provision_instances,
                                     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')
@@ -4170,6 +4172,8 @@ def test_check_injected_file_quota_onset_file_content_limit(self):
             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')
@@ -4194,9 +4198,11 @@ def test_restore_by_admin(self, update_qfd, action_start, instance_save,
         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,
@@ -4205,9 +4211,11 @@ def test_restore_by_admin(self, update_qfd, action_start, instance_save,
             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')
@@ -4231,9 +4239,11 @@ def test_restore_by_instance_owner(self, update_qfd, action_start,
         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,
@@ -4242,7 +4252,7 @@ def test_restore_by_instance_owner(self, update_qfd, action_start,
             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')

nova/tests/unit/compute/test_compute_utils.py

@@ -1396,6 +1396,46 @@ def test_check_instance_quota_exceeds_with_multiple_resources(self,
             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):