Enforce quota usage from placement when unshelving

When [quota]count_usage_from_placement = true or
[quota]driver = nova.quota.UnifiedLimitsDriver, cores and ram quota
usage are counted from placement. When an instance is SHELVED_OFFLOADED,
it will not have allocations in placement, so its cores and ram should
not count against quota during that time.

This means however that when an instance is unshelved, there is a
possibility of going over quota if the cores and ram it needs were
allocated by some other instance(s) while it was SHELVED_OFFLOADED.

This fixes a bug where quota was not being properly enforced during
unshelve of a SHELVED_OFFLOADED instance when quota usage is counted
from placement. Test coverage is also added for the "recheck" quota
cases.

Closes-Bug: #2003991

Change-Id: I4ab97626c10052c7af9934a80ff8db9ddab82738
(cherry picked from commit 6f79d63)

Author: melwitt

nova/api/openstack/compute/shelve.py

@@ -81,7 +81,7 @@ def _shelve_offload(self, req, id, body):
             raise exc.HTTPBadRequest(explanation=e.format_message())
 
     @wsgi.response(202)
-    @wsgi.expected_errors((400, 404, 409))
+    @wsgi.expected_errors((400, 403, 404, 409))
     @wsgi.action('unshelve')
     # In microversion 2.77 we support specifying 'availability_zone' to
     # unshelve a server. But before 2.77 there is no request body
@@ -142,3 +142,5 @@ def _unshelve(self, req, id, body):
             exception.ComputeHostNotFound,
         ) as e:
             raise exc.HTTPBadRequest(explanation=e.format_message())
+        except exception.OverQuota as e:
+            raise exc.HTTPForbidden(explanation=e.format_message())

nova/compute/api.py

@@ -60,6 +60,7 @@
 from nova.image import glance
 from nova.limit import local as local_limit
 from nova.limit import placement as placement_limits
+from nova.limit import utils as limit_utils
 from nova.network import constants
 from nova.network import model as network_model
 from nova.network import neutron
@@ -4522,6 +4523,42 @@ def _validate_unshelve_az(self, context, instance, availability_zone):
                             "vol_zone": volume['availability_zone']}
                         raise exception.MismatchVolumeAZException(reason=msg)
 
+    @staticmethod
+    def _check_quota_unshelve_offloaded(
+        context: nova_context.RequestContext,
+        instance: 'objects.Instance',
+        request_spec: 'objects.RequestSpec'
+    ):
+        if not (CONF.quota.count_usage_from_placement or
+                limit_utils.use_unified_limits()):
+            return
+        # TODO(melwitt): This is ugly but we have to do it this way because
+        # instances quota is currently counted from the API database but cores
+        # and ram are counted from placement. That means while an instance is
+        # SHELVED_OFFLOADED, it will still consume instances quota but it will
+        # not consume cores and ram. So we need an instances delta of
+        # 0 but cores and ram deltas from the flavor.
+        # Once instances usage is also being counted from placement, we can
+        # replace this method with a normal check_num_instances_quota() call.
+        vcpus = instance.flavor.vcpus
+        memory_mb = instance.flavor.memory_mb
+        # We are not looking to create a new server, we are unshelving an
+        # existing one.
+        deltas = {'instances': 0, 'cores': vcpus, 'ram': memory_mb}
+
+        objects.Quotas.check_deltas(
+            context,
+            deltas,
+            context.project_id,
+            user_id=context.user_id,
+            check_project_id=instance.project_id,
+            check_user_id=instance.user_id,
+        )
+        # Do the same for unified limits.
+        placement_limits.enforce_num_instances_and_flavor(
+            context, context.project_id, instance.flavor, request_spec.is_bfv,
+            0, 0, delta_updates={'servers': 0})
+
     @block_extended_resource_request
     @check_instance_lock
     @check_instance_state(
@@ -4550,6 +4587,15 @@ def unshelve(
         request_spec = objects.RequestSpec.get_by_instance_uuid(
             context, instance.uuid)
 
+        # Check quota before we save any changes to the database, but only if
+        # we are counting quota usage from placement. When an instance is
+        # SHELVED_OFFLOADED, it will not consume cores or ram resources in
+        # placement. This means it is possible that an unshelve would cause the
+        # project/user to go over quota.
+        if instance.vm_state == vm_states.SHELVED_OFFLOADED:
+            self._check_quota_unshelve_offloaded(
+                context, instance, request_spec)
+
         # We need to check a list of preconditions and validate inputs first
 
         # Ensure instance is shelve offloaded

nova/conductor/manager.py

@@ -20,6 +20,7 @@
 import eventlet
 import functools
 import sys
+import typing as ty
 
 from keystoneauth1 import exceptions as ks_exc
 from oslo_config import cfg
@@ -48,6 +49,7 @@
 from nova.i18n import _
 from nova.image import glance
 from nova.limit import placement as placement_limits
+from nova.limit import utils as limit_utils
 from nova import manager
 from nova.network import neutron
 from nova import notifications
@@ -970,6 +972,33 @@ def _restrict_request_spec_to_cell(context, instance, request_spec):
                 objects.Destination(
                     cell=instance_mapping.cell_mapping))
 
+    def _recheck_quota(
+        self,
+        context: nova_context.RequestContext,
+        flavor: 'objects.Flavor',
+        request_spec: 'objects.RequestSpec',
+        orig_num_req: int,
+        project_id: ty.Optional[str] = None,
+        user_id: ty.Optional[str] = None
+    ) -> None:
+        # A quota "recheck" is a quota check that is performed *after* quota
+        # limited resources are consumed. It is meant to address race
+        # conditions where a request that was not over quota at the beginning
+        # of the request before resources are allocated becomes over quota
+        # after resources (like database rows or placement allocations) are
+        # created. An example of this would be a large number of requests for
+        # the same resource for the same project sent simultaneously.
+        if CONF.quota.recheck_quota:
+            # The orig_num_req is the number of instances requested, which is
+            # the delta that was quota checked before resources were allocated.
+            # This is only used for the exception message is the recheck fails
+            # for lack of enough quota.
+            compute_utils.check_num_instances_quota(
+                context, flavor, 0, 0, project_id=project_id,
+                user_id=user_id, orig_num_req=orig_num_req)
+            placement_limits.enforce_num_instances_and_flavor(
+                context, project_id, flavor, request_spec.is_bfv, 0, 0)
+
     # TODO(mriedem): Make request_spec required in ComputeTaskAPI RPC v2.0.
     @targets_cell
     def unshelve_instance(self, context, instance, request_spec=None):
@@ -1055,6 +1084,30 @@ def safe_image_show(ctx, image_id):
                     host_lists = self._schedule_instances(context,
                             request_spec, [instance.uuid],
                             return_alternates=False)
+
+                    # NOTE(melwitt): We recheck the quota after allocating the
+                    # resources in placement, 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.
+                    try:
+                        # Quota should only be checked for unshelve only if
+                        # resources are being counted in placement. Legacy
+                        # quotas continue to consume resources while
+                        # SHELVED_OFFLOADED and will not allocate any new
+                        # resources during unshelve.
+                        if (CONF.quota.count_usage_from_placement or
+                                limit_utils.use_unified_limits()):
+                            self._recheck_quota(
+                                context, instance.flavor, request_spec, 0,
+                                project_id=instance.project_id,
+                                user_id=instance.user_id)
+                    except (exception.TooManyInstances,
+                            limit_exceptions.ProjectOverLimit):
+                        with excutils.save_and_reraise_exception():
+                            self.report_client.delete_allocation_for_instance(
+                                context, instance.uuid, force=True)
+
                     host_list = host_lists[0]
                     selection = host_list[0]
                     scheduler_utils.populate_filter_properties(
@@ -1677,27 +1730,22 @@ def schedule_and_build_instances(self, context, build_requests,
                     instances.append(instance)
                     cell_mapping_cache[instance.uuid] = cell
 
-        # NOTE(melwitt): We recheck the quota after creating the
-        # objects to prevent users from allocating more resources
+        # NOTE(melwitt): We recheck the quota after allocating the
+        # resources 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:
-            try:
-                compute_utils.check_num_instances_quota(
-                    context, instance.flavor, 0, 0,
-                    orig_num_req=len(build_requests))
-                placement_limits.enforce_num_instances_and_flavor(
-                    context, context.project_id, instance.flavor,
-                    request_specs[0].is_bfv, 0, 0)
-            except (exception.TooManyInstances,
-                    limit_exceptions.ProjectOverLimit) as exc:
-                with excutils.save_and_reraise_exception():
-                    self._cleanup_build_artifacts(context, exc, instances,
-                                                  build_requests,
-                                                  request_specs,
-                                                  block_device_mapping, tags,
-                                                  cell_mapping_cache)
+        try:
+            self._recheck_quota(context, instance.flavor, request_specs[0],
+                len(build_requests), project_id=instance.project_id,
+                user_id=instance.user_id
+            )
+        except (exception.TooManyInstances,
+                limit_exceptions.ProjectOverLimit) as exc:
+            with excutils.save_and_reraise_exception():
+                self._cleanup_build_artifacts(
+                    context, exc, instances, build_requests, request_specs,
+                    block_device_mapping, tags, cell_mapping_cache)
 
         zipped = zip(build_requests, request_specs, host_lists, instances)
         for (build_request, request_spec, host_list, instance) in zipped:

nova/limit/placement.py

@@ -156,7 +156,8 @@ def enforce_num_instances_and_flavor(
     is_bfvm: bool,
     min_count: int,
     max_count: int,
-    enforcer: ty.Optional[limit.Enforcer] = None
+    enforcer: ty.Optional[limit.Enforcer] = None,
+    delta_updates: ty.Optional[ty.Dict[str, int]] = None,
 ) -> int:
     """Return max instances possible, else raise TooManyInstances exception."""
     if not limit_utils.use_unified_limits():
@@ -169,7 +170,10 @@ def enforce_num_instances_and_flavor(
         raise ValueError("invalid max_count")
 
     deltas = _get_deltas_by_flavor(flavor, is_bfvm, max_count)
-    enforcer = _get_enforcer(context, project_id)
+    if delta_updates:
+        deltas.update(delta_updates)
+
+    enforcer = enforcer or _get_enforcer(context, project_id)
     try:
         enforcer.enforce(project_id, deltas)
     except limit_exceptions.ProjectOverLimit as e:

nova/tests/functional/test_servers.py

@@ -414,15 +414,12 @@ def _test_unshelve_offloaded_overquota_placement(self):
         # quota.
         self._create_server(flavor_id=1)
 
-        # FIXME(melwitt): This is bug #2003991, the unshelve is supposed to
-        # fail if we would be over quota after unshelving.
         # Now try to unshelve the earlier instance. It should fail because it
         # would put us over quota to have 4 running instances.
-        # ex = self.assertRaises(client.OpenStackApiException,
-        #                        self._unshelve_server,
-        #                        server)
-        # self.assertEqual(403, ex.response.status_code)
-        self._unshelve_server(server)
+        ex = self.assertRaises(client.OpenStackApiException,
+                               self._unshelve_server,
+                               server)
+        self.assertEqual(403, ex.response.status_code)
 
     def test_unshelve_offloaded_overquota_placement(self):
         # Count quota usage from placement.