Fix quota updating when admin deletes common user's instance

Add 'project_id=None' optional argument in QUOTAS related methods.

There are two problems when admin deletes instances of common user
before fixing this bug:
Firstly, It will get tenant quotas using context.project_id which
results in getting admin's quotas not common user's.
Secondly, It will also get wrong quota usages using context.project_id
when updating common user's quota_usages.

So we could pass into QUOTAS related methods an optional argument to
specify the tenant ID to be affected. And if it's not set, then we
default to drawing the tenant ID from the context.

Fixes: bug 1078668
Change-Id: I59bc240d6547122d803a3ed3e1eeef26fc78f4f0

6 files changed, 144 insertions, 61 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 22d0fc015..d0a039644 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -885,6 +885,12 @@ class API(base.Base):
         bdms = self.db.block_device_mapping_get_all_by_instance(
                     context, instance['uuid'])
         reservations = None
+
+        if context.is_admin and context.project_id != instance['project_id']:
+            project_id = instance['project_id']
+        else:
+            project_id = context.project_id
+
         try:
             # NOTE(maoy): no expected_task_state needs to be set
             attrs = {'progress': 0}
@@ -899,6 +905,7 @@ class API(base.Base):
                 old['task_state'] not in (task_states.DELETING,
                                           task_states.SOFT_DELETING)):
                 reservations = QUOTAS.reserve(context,
+                                              project_id=project_id,
                                               instances=-1,
                                               cores=-instance['vcpus'],
                                               ram=-instance['memory_mb'])
@@ -910,7 +917,9 @@ class API(base.Base):
                     self.db.instance_destroy(context, instance['uuid'],
                                              constraint)
                     if reservations:
-                        QUOTAS.commit(context, reservations)
+                        QUOTAS.commit(context,
+                                      reservations,
+                                      project_id=project_id)
                     return
                 except exception.ConstraintNotMet:
                     # Refresh to get new host information
@@ -962,15 +971,21 @@ class API(base.Base):
                 # If compute node isn't up, just delete from DB
                 self._local_delete(context, instance, bdms)
             if reservations:
-                QUOTAS.commit(context, reservations)
+                QUOTAS.commit(context,
+                              reservations,
+                              project_id=project_id)
         except exception.InstanceNotFound:
             # NOTE(comstud): Race condition. Instance already gone.
             if reservations:
-                QUOTAS.rollback(context, reservations)
+                QUOTAS.rollback(context,
+                                reservations,
+                                project_id=project_id)
         except Exception:
             with excutils.save_and_reraise_exception():
                 if reservations:
-                    QUOTAS.rollback(context, reservations)
+                    QUOTAS.rollback(context,
+                                    reservations,
+                                    project_id=project_id)
 
     def _local_delete(self, context, instance, bdms):
         LOG.warning(_("instance's host %s is down, deleting from "
diff --git a/nova/db/api.py b/nova/db/api.py
index 3a57e71af..b1552b480 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -1016,20 +1016,22 @@ def reservation_destroy(context, uuid):
 
 
 def quota_reserve(context, resources, quotas, deltas, expire,
-                  until_refresh, max_age):
+                  until_refresh, max_age, project_id=None):
     """Check quotas and create appropriate reservations."""
     return IMPL.quota_reserve(context, resources, quotas, deltas, expire,
-                              until_refresh, max_age)
+                              until_refresh, max_age, project_id=project_id)
 
 
-def reservation_commit(context, reservations):
+def reservation_commit(context, reservations, project_id=None):
     """Commit quota reservations."""
-    return IMPL.reservation_commit(context, reservations)
+    return IMPL.reservation_commit(context, reservations,
+                                   project_id=project_id)
 
 
-def reservation_rollback(context, reservations):
+def reservation_rollback(context, reservations, project_id=None):
     """Roll back quota reservations."""
-    return IMPL.reservation_rollback(context, reservations)
+    return IMPL.reservation_rollback(context, reservations,
+                                     project_id=project_id)
 
 
 def quota_destroy_all_by_project(context, project_id):
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 698f79317..8930f6ccc 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -2642,12 +2642,12 @@ def reservation_destroy(context, uuid):
 # code always acquires the lock on quota_usages before acquiring the lock
 # on reservations.
 
-def _get_quota_usages(context, session):
+def _get_quota_usages(context, session, project_id):
     # Broken out for testability
     rows = model_query(context, models.QuotaUsage,
                        read_deleted="no",
                        session=session).\
-                   filter_by(project_id=context.project_id).\
+                   filter_by(project_id=project_id).\
                    with_lockmode('update').\
                    all()
     return dict((row.resource, row) for row in rows)
@@ -2655,12 +2655,16 @@ def _get_quota_usages(context, session):
 
 @require_context
 def quota_reserve(context, resources, quotas, deltas, expire,
-                  until_refresh, max_age):
+                  until_refresh, max_age, project_id=None):
     elevated = context.elevated()
     session = get_session()
     with session.begin():
+
+        if project_id is None:
+            project_id = context.project_id
+
         # Get the current usages
-        usages = _get_quota_usages(context, session)
+        usages = _get_quota_usages(context, session, project_id)
 
         # Handle usage refresh
         work = set(deltas.keys())
@@ -2671,7 +2675,7 @@ def quota_reserve(context, resources, quotas, deltas, expire,
             refresh = False
             if resource not in usages:
                 usages[resource] = _quota_usage_create(elevated,
-                                                      context.project_id,
+                                                      project_id,
                                                       resource,
                                                       0, 0,
                                                       until_refresh or None,
@@ -2694,12 +2698,12 @@ def quota_reserve(context, resources, quotas, deltas, expire,
                 # Grab the sync routine
                 sync = resources[resource].sync
 
-                updates = sync(elevated, context.project_id, session)
+                updates = sync(elevated, project_id, session)
                 for res, in_use in updates.items():
                     # Make sure we have a destination for the usage!
                     if res not in usages:
                         usages[res] = _quota_usage_create(elevated,
-                                                         context.project_id,
+                                                         project_id,
                                                          res,
                                                          0, 0,
                                                          until_refresh or None,
@@ -2749,7 +2753,7 @@ def quota_reserve(context, resources, quotas, deltas, expire,
                 reservation = reservation_create(elevated,
                                                  str(uuid.uuid4()),
                                                  usages[resource],
-                                                 context.project_id,
+                                                 project_id,
                                                  resource, delta, expire,
                                                  session=session)
                 reservations.append(reservation.uuid)
@@ -2797,10 +2801,10 @@ def _quota_reservations_query(session, context, reservations):
 
 
 @require_context
-def reservation_commit(context, reservations):
+def reservation_commit(context, reservations, project_id=None):
     session = get_session()
     with session.begin():
-        usages = _get_quota_usages(context, session)
+        usages = _get_quota_usages(context, session, project_id)
         reservation_query = _quota_reservations_query(session, context,
                                                       reservations)
         for reservation in reservation_query.all():
@@ -2812,10 +2816,10 @@ def reservation_commit(context, reservations):
 
 
 @require_context
-def reservation_rollback(context, reservations):
+def reservation_rollback(context, reservations, project_id=None):
     session = get_session()
     with session.begin():
-        usages = _get_quota_usages(context, session)
+        usages = _get_quota_usages(context, session, project_id)
         reservation_query = _quota_reservations_query(session, context,
                                                       reservations)
         for reservation in reservation_query.all():
diff --git a/nova/quota.py b/nova/quota.py
index c2e34cca5..96e612503 100644
--- a/nova/quota.py
+++ b/nova/quota.py
@@ -198,7 +198,7 @@ class DbQuotaDriver(object):
 
         return quotas
 
-    def _get_quotas(self, context, resources, keys, has_sync):
+    def _get_quotas(self, context, resources, keys, has_sync, project_id=None):
         """
         A helper method which retrieves the quotas for the specific
         resources identified by keys, and which apply to the current
@@ -211,6 +211,9 @@ class DbQuotaDriver(object):
                          have a sync attribute; if False, indicates
                          that the resource must NOT have a sync
                          attribute.
+        :param project_id: Specify the project_id if current context
+                           is admin and admin wants to impact on
+                           common user's tenant.
         """
 
         # Filter resources
@@ -229,12 +232,12 @@ class DbQuotaDriver(object):
 
         # Grab and return the quotas (without usages)
         quotas = self.get_project_quotas(context, sub_resources,
-                                         context.project_id,
+                                         project_id,
                                          context.quota_class, usages=False)
 
         return dict((k, v['limit']) for k, v in quotas.items())
 
-    def limit_check(self, context, resources, values):
+    def limit_check(self, context, resources, values, project_id=None):
         """Check simple quota limits.
 
         For limits--those quotas for which there is no usage
@@ -254,6 +257,9 @@ class DbQuotaDriver(object):
         :param resources: A dictionary of the registered resources.
         :param values: A dictionary of the values to check against the
                        quota.
+        :param project_id: Specify the project_id if current context
+                           is admin and admin wants to impact on
+                           common user's tenant.
         """
 
         # Ensure no value is less than zero
@@ -261,9 +267,13 @@ class DbQuotaDriver(object):
         if unders:
             raise exception.InvalidQuotaValue(unders=sorted(unders))
 
+        # If project_id is None, then we use the project_id in context
+        if project_id is None:
+            project_id = context.project_id
+
         # Get the applicable quotas
         quotas = self._get_quotas(context, resources, values.keys(),
-                                  has_sync=False)
+                                  has_sync=False, project_id=project_id)
 
         # Check the quotas and construct a list of the resources that
         # would be put over limit by the desired values
@@ -273,7 +283,8 @@ class DbQuotaDriver(object):
             raise exception.OverQuota(overs=sorted(overs), quotas=quotas,
                                       usages={})
 
-    def reserve(self, context, resources, deltas, expire=None):
+    def reserve(self, context, resources, deltas, expire=None,
+                project_id=None):
         """Check quotas and reserve resources.
 
         For counting quotas--those quotas for which there is a usage
@@ -303,6 +314,9 @@ class DbQuotaDriver(object):
                        default expiration time set by
                        --default-reservation-expire will be used (this
                        value will be treated as a number of seconds).
+        :param project_id: Specify the project_id if current context
+                           is admin and admin wants to impact on
+                           common user's tenant.
         """
 
         # Set up the reservation expiration
@@ -315,12 +329,16 @@ class DbQuotaDriver(object):
         if not isinstance(expire, datetime.datetime):
             raise exception.InvalidReservationExpiration(expire=expire)
 
+        # If project_id is None, then we use the project_id in context
+        if project_id is None:
+            project_id = context.project_id
+
         # Get the applicable quotas.
         # NOTE(Vek): We're not worried about races at this point.
         #            Yes, the admin may be in the process of reducing
         #            quotas, but that's a pretty rare thing.
         quotas = self._get_quotas(context, resources, deltas.keys(),
-                                  has_sync=True)
+                                  has_sync=True, project_id=project_id)
 
         # NOTE(Vek): Most of the work here has to be done in the DB
         #            API, because we have to do it in a transaction,
@@ -328,27 +346,40 @@ class DbQuotaDriver(object):
         #            session isn't available outside the DBAPI, we
         #            have to do the work there.
         return db.quota_reserve(context, resources, quotas, deltas, expire,
-                                CONF.until_refresh, CONF.max_age)
+                                CONF.until_refresh, CONF.max_age,
+                                project_id=project_id)
 
-    def commit(self, context, reservations):
+    def commit(self, context, reservations, project_id=None):
         """Commit reservations.
 
         :param context: The request context, for access checks.
         :param reservations: A list of the reservation UUIDs, as
                              returned by the reserve() method.
+        :param project_id: Specify the project_id if current context
+                           is admin and admin wants to impact on
+                           common user's tenant.
         """
+        # If project_id is None, then we use the project_id in context
+        if project_id is None:
+            project_id = context.project_id
 
-        db.reservation_commit(context, reservations)
+        db.reservation_commit(context, reservations, project_id=project_id)
 
-    def rollback(self, context, reservations):
+    def rollback(self, context, reservations, project_id=None):
         """Roll back reservations.
 
         :param context: The request context, for access checks.
         :param reservations: A list of the reservation UUIDs, as
                              returned by the reserve() method.
+        :param project_id: Specify the project_id if current context
+                           is admin and admin wants to impact on
+                           common user's tenant.
         """
+        # If project_id is None, then we use the project_id in context
+        if project_id is None:
+            project_id = context.project_id
 
-        db.reservation_rollback(context, reservations)
+        db.reservation_rollback(context, reservations, project_id=project_id)
 
     def usage_reset(self, context, resources):
         """
@@ -843,7 +874,7 @@ class QuotaEngine(object):
 
         return res.count(context, *args, **kwargs)
 
-    def limit_check(self, context, **values):
+    def limit_check(self, context, project_id=None, **values):
         """Check simple quota limits.
 
         For limits--those quotas for which there is no usage
@@ -863,11 +894,15 @@ class QuotaEngine(object):
         nothing.
 
         :param context: The request context, for access checks.
+        :param project_id: Specify the project_id if current context
+                           is admin and admin wants to impact on
+                           common user's tenant.
         """
 
-        return self._driver.limit_check(context, self._resources, values)
+        return self._driver.limit_check(context, self._resources, values,
+                                        project_id=project_id)
 
-    def reserve(self, context, expire=None, **deltas):
+    def reserve(self, context, expire=None, project_id=None, **deltas):
         """Check quotas and reserve resources.
 
         For counting quotas--those quotas for which there is a usage
@@ -897,25 +932,32 @@ class QuotaEngine(object):
                        default expiration time set by
                        --default-reservation-expire will be used (this
                        value will be treated as a number of seconds).
+        :param project_id: Specify the project_id if current context
+                           is admin and admin wants to impact on
+                           common user's tenant.
         """
 
         reservations = self._driver.reserve(context, self._resources, deltas,
-                                            expire=expire)
+                                            expire=expire,
+                                            project_id=project_id)
 
         LOG.debug(_("Created reservations %(reservations)s") % locals())
 
         return reservations
 
-    def commit(self, context, reservations):
+    def commit(self, context, reservations, project_id=None):
         """Commit reservations.
 
         :param context: The request context, for access checks.
         :param reservations: A list of the reservation UUIDs, as
                              returned by the reserve() method.
+        :param project_id: Specify the project_id if current context
+                           is admin and admin wants to impact on
+                           common user's tenant.
         """
 
         try:
-            self._driver.commit(context, reservations)
+            self._driver.commit(context, reservations, project_id=project_id)
         except Exception:
             # NOTE(Vek): Ignoring exceptions here is safe, because the
             # usage resynchronization and the reservation expiration
@@ -924,16 +966,19 @@ class QuotaEngine(object):
             LOG.exception(_("Failed to commit reservations "
                             "%(reservations)s") % locals())
 
-    def rollback(self, context, reservations):
+    def rollback(self, context, reservations, project_id=None):
         """Roll back reservations.
 
         :param context: The request context, for access checks.
         :param reservations: A list of the reservation UUIDs, as
                              returned by the reserve() method.
+        :param project_id: Specify the project_id if current context
+                           is admin and admin wants to impact on
+                           common user's tenant.
         """
 
         try:
-            self._driver.rollback(context, reservations)
+            self._driver.rollback(context, reservations, project_id=project_id)
         except Exception:
             # NOTE(Vek): Ignoring exceptions here is safe, because the
             # usage resynchronization and the reservation expiration
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index bf619bbec..fc0feea53 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -3934,12 +3934,12 @@ class ComputeAPITestCase(BaseTestCase):
     def test_repeated_delete_quota(self):
         in_use = {'instances': 1}
 
-        def fake_reserve(context, **deltas):
+        def fake_reserve(context, expire=None, project_id=None, **deltas):
             return dict(deltas.iteritems())
 
         self.stubs.Set(QUOTAS, 'reserve', fake_reserve)
 
-        def fake_commit(context, deltas):
+        def fake_commit(context, deltas, project_id=None):
             for k, v in deltas.iteritems():
                 in_use[k] = in_use.get(k, 0) + v
 
@@ -3993,7 +3993,8 @@ class ComputeAPITestCase(BaseTestCase):
                 'host': CONF.host})
 
         self.mox.StubOutWithMock(nova.quota.QUOTAS, 'commit')
-        nova.quota.QUOTAS.commit(mox.IgnoreArg(), mox.IgnoreArg())
+        nova.quota.QUOTAS.commit(mox.IgnoreArg(), mox.IgnoreArg(),
+                                 project_id=mox.IgnoreArg())
         self.mox.ReplayAll()
 
         self.compute_api.soft_delete(self.context, instance)
@@ -4021,7 +4022,8 @@ class ComputeAPITestCase(BaseTestCase):
                 'host': CONF.host})
 
         self.mox.StubOutWithMock(nova.quota.QUOTAS, 'rollback')
-        nova.quota.QUOTAS.rollback(mox.IgnoreArg(), mox.IgnoreArg())
+        nova.quota.QUOTAS.rollback(mox.IgnoreArg(), mox.IgnoreArg(),
+                                   project_id=mox.IgnoreArg())
         self.mox.ReplayAll()
 
         def fail(*args, **kwargs):
diff --git a/nova/tests/test_quota.py b/nova/tests/test_quota.py
index b6759de54..08b33e201 100644
--- a/nova/tests/test_quota.py
+++ b/nova/tests/test_quota.py
@@ -281,18 +281,21 @@ class FakeDriver(object):
                             project_id, quota_class, defaults, usages))
         return resources
 
-    def limit_check(self, context, resources, values):
-        self.called.append(('limit_check', context, resources, values))
-
-    def reserve(self, context, resources, deltas, expire=None):
-        self.called.append(('reserve', context, resources, deltas, expire))
+    def limit_check(self, context, resources, values, project_id=None):
+        self.called.append(('limit_check', context, resources,
+                            values, project_id))
+
+    def reserve(self, context, resources, deltas, expire=None,
+                project_id=None):
+        self.called.append(('reserve', context, resources, deltas,
+                            expire, project_id))
         return self.reservations
 
-    def commit(self, context, reservations):
-        self.called.append(('commit', context, reservations))
+    def commit(self, context, reservations, project_id=None):
+        self.called.append(('commit', context, reservations, project_id))
 
-    def rollback(self, context, reservations):
-        self.called.append(('rollback', context, reservations))
+    def rollback(self, context, reservations, project_id=None):
+        self.called.append(('rollback', context, reservations, project_id))
 
     def usage_reset(self, context, resources):
         self.called.append(('usage_reset', context, resources))
@@ -600,7 +603,7 @@ class QuotaEngineTestCase(test.TestCase):
                         test_resource2=3,
                         test_resource3=2,
                         test_resource4=1,
-                        )),
+                        ), None),
                 ])
 
     def test_reserve(self):
@@ -615,6 +618,9 @@ class QuotaEngineTestCase(test.TestCase):
         result2 = quota_obj.reserve(context, expire=3600,
                                     test_resource1=1, test_resource2=2,
                                     test_resource3=3, test_resource4=4)
+        result3 = quota_obj.reserve(context, project_id='fake_project',
+                                    test_resource1=1, test_resource2=2,
+                                    test_resource3=3, test_resource4=4)
 
         self.assertEqual(driver.called, [
                 ('reserve', context, quota_obj._resources, dict(
@@ -622,13 +628,19 @@ class QuotaEngineTestCase(test.TestCase):
                         test_resource2=3,
                         test_resource3=2,
                         test_resource4=1,
-                        ), None),
+                        ), None, None),
+                ('reserve', context, quota_obj._resources, dict(
+                        test_resource1=1,
+                        test_resource2=2,
+                        test_resource3=3,
+                        test_resource4=4,
+                        ), 3600, None),
                 ('reserve', context, quota_obj._resources, dict(
                         test_resource1=1,
                         test_resource2=2,
                         test_resource3=3,
                         test_resource4=4,
-                        ), 3600),
+                        ), None, 'fake_project'),
                 ])
         self.assertEqual(result1, [
                 'resv-01', 'resv-02', 'resv-03', 'resv-04',
@@ -636,6 +648,9 @@ class QuotaEngineTestCase(test.TestCase):
         self.assertEqual(result2, [
                 'resv-01', 'resv-02', 'resv-03', 'resv-04',
                 ])
+        self.assertEqual(result3, [
+                'resv-01', 'resv-02', 'resv-03', 'resv-04',
+                ])
 
     def test_commit(self):
         context = FakeContext(None, None)
@@ -644,7 +659,7 @@ class QuotaEngineTestCase(test.TestCase):
         quota_obj.commit(context, ['resv-01', 'resv-02', 'resv-03'])
 
         self.assertEqual(driver.called, [
-                ('commit', context, ['resv-01', 'resv-02', 'resv-03']),
+                ('commit', context, ['resv-01', 'resv-02', 'resv-03'], None),
                 ])
 
     def test_rollback(self):
@@ -654,7 +669,7 @@ class QuotaEngineTestCase(test.TestCase):
         quota_obj.rollback(context, ['resv-01', 'resv-02', 'resv-03'])
 
         self.assertEqual(driver.called, [
-                ('rollback', context, ['resv-01', 'resv-02', 'resv-03']),
+                ('rollback', context, ['resv-01', 'resv-02', 'resv-03'], None),
                 ])
 
     def test_usage_reset(self):
@@ -1205,7 +1220,7 @@ class DbQuotaDriverTestCase(test.TestCase):
 
     def _stub_quota_reserve(self):
         def fake_quota_reserve(context, resources, quotas, deltas, expire,
-                               until_refresh, max_age):
+                               until_refresh, max_age, project_id=None):
             self.calls.append(('quota_reserve', expire, until_refresh,
                                max_age))
             return ['resv-1', 'resv-2', 'resv-3']
@@ -1389,7 +1404,7 @@ class QuotaReserveSqlAlchemyTestCase(test.TestCase):
         def fake_get_session():
             return FakeSession()
 
-        def fake_get_quota_usages(context, session):
+        def fake_get_quota_usages(context, session, project_id):
             return self.usages.copy()
 
         def fake_quota_usage_create(context, project_id, resource, in_use,