Cinder nested quotas does not handle unlimited quotas (-1) well

[Ryan McNair]

Substituting id's with name and showing only relevant info for clarity

Issue #1:
========================================================

The project heirarchy is as follows:
                    A
                 / \
               B C

~(keystone_admin)]$ cinder quota-show A

+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | 10 |
+------------------------+-------+

~(keystone_admin)]$ cinder quota-update C --volumes 5

+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | 5 |
+------------------------+-------+

~(keystone_admin)]$ cinder quota-update B --volumes -1

+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | -1 |
+------------------------+-------+

~(keystone_admin)]$ cinder quota-update B --volumes 11
ERROR: Free quota available is 6. (HTTP 400) (Request-ID: req-e904d5df-7a1d-4516-8ccc-29605923d510)

***** Should be 5 is free ********

~(keystone_admin)]$ cinder quota-update C --volumes -1

+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | -1 |
+------------------------+-------+

~(keystone_admin)]$ cinder quota-update B --volumes 12
ERROR: Free quota available is 12. (HTTP 400) (Request-ID: req-07cbb4eb-e33e-4601-a8cb-d28e69071750)

***** Should be 10 is free ********

~(keystone_admin)]$ cinder quota-update eae947cadd93421f994dff8947622ab1 --volumes 11

+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | 11 |
+------------------------+-------+

Issue #2:
========================================================

The project heirarchy is as follows:
                    A
                       \
                        B
                          \
                           C

~(keystone_admin)]$ cinder quota-show A

+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | 10 |
+------------------------+-------+

~(keystone_admin)]$ cinder quota-update B --volumes -1

+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | -1 |
+------------------------+-------+

~(keystone_admin)]$ cinder quota-show C
+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | 0 |
+------------------------+-------+

************* Coming from default quota ***********
~(keystone_admin)]$ cinder quota-set C --volumes 0

ERROR: Free quota available is -1. (HTTP 400) (Request-ID: req-322d165c-e4f8-4fdc-a62b-998ef597f4a6)

Issue #3:
========================================================

The project heirarchy is as follows:
                    A
                       \
                        B
                          \
                           C

~(keystone_admin)]$ cinder quota-show A

+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | 1 |
+------------------------+-------+

~(keystone_admin)]$ cinder quota-update B --volumes -1

+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | -1 |
+------------------------+-------+

~(keystone_admin)]$ cinder quota-update C --volumes -1
+------------------------+-------+
| Property | Value |
+------------------------+-------+
| volumes | -1 |
+------------------------+-------+

< Authenticate to use project C>

~(keystone_admin)]$ cinder create 1
   ... success ....
~(keystone_admin)]$ cinder create 1
  ... success ...

**** Shouldn't work because max limit should be the grandparent, which is 1! The validation is not going recursively up the heirarchy in the case of unlimited / shared quotas. *****

[Ryan McNair (rdmcnair)]

Here are test-cases which will expose the three issues on master:

    # Issue 1
    def test_neg_child_limit_affecting_free(self):
        self.controller._get_project = mock.Mock()
        self.controller._get_project.side_effect = self._get_project
        self.req.environ['cinder.context'].project_id = self.A.id

        cur_quota_a = self.controller.show(self.req, self.A.id)
        self.assertEqual(10, cur_quota_a['quota_set']['volumes'])

        quota = {'volumes': -1}
        body = {'quota_set': quota}
        self.controller.update(self.req, self.C.id, body)
        self.controller.update(self.req, self.B.id, body)

        # Shouldn't be able to set greater than parent
        quota['volumes'] = 11
        self.assertRaises(
            webob.exc.HTTPBadRequest, self.controller.update, self.req,
            self.B.id, body)

    # Issue 2
    def test_child_neg_limit_set_grandkid_zero_limit(self):
        self.controller._get_project = mock.Mock()
        self.controller._get_project.side_effect = self._get_project
        self.req.environ['cinder.context'].project_id = self.A.id

        cur_quota_a = self.controller.show(self.req, self.A.id)
        self.assertEqual(10, cur_quota_a['quota_set']['volumes'])

        quota = {'volumes': -1}
        body = {'quota_set': quota}
        self.controller.update(self.req, self.B.id, body)

        cur_quota_d = self.controller.show(self.req, self.D.id)
        # Default child value is 0
        self.assertEqual(0, cur_quota_d['quota_set']['volumes'])
        # Should be able to set D explicitly to 0 since that's already the val
        quota['volumes'] = 0
        self.controller.update(self.req, self.D.id, body)

    # Issue 3
    def test_grandkid_negative_one_limit_enforced(self):
        self.controller._get_project = mock.Mock()
        self.controller._get_project.side_effect = self._get_project
        self.req.environ['cinder.context'].project_id = self.A.id

        quota = {'volumes': 1}
        body = {'quota_set': quota}
        self.controller.update(self.req, self.A.id, body)

        quota['volumes'] = -1
        self.controller.update(self.req, self.B.id, body)
        self.controller.update(self.req, self.C.id, body)

        quotas.QUOTAS._driver.reserve(
            self.req.environ['cinder.context'], quotas.QUOTAS.resources,
            {'volumes': 1, 'gigabytes': 1}, project_id=self.C.id)

        self.assertRaises(
            Exception, quotas.QUOTAS._driver.reserve,
            self.req.environ['cinder.context'], quotas.QUOTAS.resources,
            {'volumes': 1, 'gigabytes': 1}, project_id=self.C.id)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/274825
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=7ebd4904b977d29c97447b53fbd718bccfa39969
Submitter: Jenkins
Branch: master

commit 7ebd4904b977d29c97447b53fbd718bccfa39969
Author: Ryan McNair <email address hidden>
Date: Sat Jan 30 16:24:32 2016 +0000

    Split out NestedQuotas into a separate driver

    Fixes the following issues with NestedQuotas:
      * Requires conf setting change to use nested quota driver
      * Enforces default child quota value with volume creation
      * Disables the use of -1 to be set for child quotas
      * Adds an admin only API command which can be used to validate
        the current setup for nested quotas, and can update existing
        allocated quotas in the DB which have been incorrectly set
        by previous use of child limits with -1

    There will be follow-up patches with the following improvements:
      * make -1 limits functional for child projects
      * cache the Keystone project heirarchies to improve efficiency

    Note: ideally validation of nested quotas would occur in the setup
    of the nested quota driver, but doing the validation requires a
    view of ALL projects present in Keystone, so unless we require Keystone
    change to allow "cinder" service user to be able to list/get projects,
    we need the admin-only API for validation that should be called by
    cloud-admin.

    DocImpact

    Change-Id: Ibbd6f47c370d8f10c08cba358574b55e3059dcd1
    Closes-Bug: #1531502
    Partial-Bug: #1537189
    Related-Bug: #1535878

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/282068
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=c02336e4dd889b7b2a474488c4e964d08d558901
Submitter: Jenkins
Branch: master

commit c02336e4dd889b7b2a474488c4e964d08d558901
Author: Ryan McNair <email address hidden>
Date: Tue Feb 16 17:12:53 2016 +0000

    Re-enable -1 child limits for nested quotas

    Add back support for -1 limits of child projects. The way that we
    support the -1 child limits requires the following changes:
      * Continue quota validation up the hierarchy if the current limit is
        -1 until we hit a hard limit or no more parents, and update the
        any relevant parents' allocated value along the way
      * When updating limits, special care needs to be taken when updating
        child limit to be -1, or when changing from a -1 limit
      * Enable support for creating reservations for "allocated" values
        to support the scenario that:
          - a volume is created on a project with a limit of -1
          - the parent's allocated value has been updated appropriately
          - the volume create fails and the child's in_use quota rolls back
          - now we must also rollback the parent's allocated value

    NOTE: There is a race condition between validation the NestedQuotas
    and when the driver may be switched into use, and if -1 quotas are used
    the validation could be out of date. Will look into better support for
    switching on of NestedQuotas on live deployment with -1 limits, which
    would likely leverage the "allocated" reservation system.

    Closes-Bug: #1548645
    Closes-Bug: #1544774
    Closes-Bug: #1537189
    Change-Id: I2d1dba87baf3595cc8f48574e0281ac17509fe7d

[Doug Hellmann (doug-hellmann)]

This issue was fixed in the openstack/cinder 8.0.0.0b3 development milestone.