Skip to content

Add hardware-versioned quota based on Flavor extra_specs #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: stable/xena-m3
Choose a base branch
from

Conversation

joker-at-work
Copy link

@joker-at-work joker-at-work commented Jan 31, 2024

We want to support adding cores/ram quota for different hardware versions. Reading the quota:hw_version property from all flavors' extra_specs, we build a list of supported hardware versions and create quota resources of the form cores_{hw_version}.

Change-Id: Ic7725f49a788188e3b1f046ed857c582b2646819

Copy link
Member

@fwiesel fwiesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach, but I am not too happy with the function signatures having to change, and that the changes are spread over an upstream class.

It is the result of a decision we have taken before your change, so it is more a suggestion from my side, than a change I expect you to implement:

I am wondering, if it wouldn't make sense to create a derived SAPQuotaEngine like this:

class QuotaEngine(object):
    def __init__(self, resources=None):
      self._resources = resources or {}

    def do_something_with_resources(self):
      print(self._resources)


class SAPQuotaEngine(QuotaEngine):
    def __init__(self, resources=None):
      super().__init__(resources)

    @property
    def _resources(self):
      resources = self._original_resources.copy()
      # Add our custom resources logic here
      resources["Added"] = "MyStuff"
      return resources

    @_resources.setter
    def _resources(self, resources):
      self._original_resources = resources


resources = {'Normal': '1'}

a = QuotaEngine(resources)
a.do_something_with_resources()

b = SAPQuotaEngine(resources)
b.do_something_with_resources()

@joker-at-work
Copy link
Author

I am wondering, if it wouldn't make sense to create a derived SAPQuotaEngine like this:

Yes, that would probably make more sense than changing everyone to use self.resources instead of self._resources. That's the approac Cinder does for implementing the VolumeTypeQuotaEngine. I refrained from that at time, iirc, because initializing the quota-engine into QUOTAS is hard-coded - but that would be a single line change then, which is much easier to port. Great suggestion.

@joker-at-work
Copy link
Author

Updated with SAPQuotaEngine, caching and also introduced SAPCustomResource for another place in the code.

@joker-at-work joker-at-work force-pushed the quota-for-hw-versions branch 2 times, most recently from 1642da5 to 0e2ebac Compare February 5, 2024 08:33
nova/quota.py Outdated
# TODO(jkulik) make expiration time configurable
self._cache = cache.CacheClient(
cache._get_custom_cache_region(expiration_time=600,
backend='oslo_cache.dict'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the proper caching infrastructure, wouldn't it make sense to use the configured default one (memcached in our case)?

The advantage would be that it would be more quickly consistent.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure it's worth the serialization/deserialization of CountableResource objects and references to functions - is that even supported with memcached as backend?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed the part that it stores a function, so I needed to check it first myself.
AFAIK, the caching uses pickling to store the data, and that handles even

functions (built-in and user-defined) accessible from the top level of a module (using def, not lambda);

from nova import quota
import pickle

[pickle.dumps(res) for res in quota.QUOTAS._resources.values()]

So, we might be fine here using a different caching backend.

nova/quota.py Outdated
@_resources.setter
def _resources(self, resources):
self._original_resources = resources
self._cache.delete('resources')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we should decide to use an external cache, then I think it would be better to remove this invalidation. The cache may very well be still valid then.

@joker-at-work joker-at-work force-pushed the quota-for-hw-versions branch 2 times, most recently from 7a02d13 to f61bd58 Compare February 29, 2024 15:48
@joker-at-work joker-at-work force-pushed the quota-for-hw-versions branch 3 times, most recently from 07bb81e to 634c792 Compare May 29, 2024 14:27
@joker-at-work joker-at-work force-pushed the quota-for-hw-versions branch 2 times, most recently from c8963da to a843792 Compare June 14, 2024 10:17
@joker-at-work joker-at-work changed the title WIP: Add quota resources based on flavor extra_specs Add hardware-versioned quota based on Flavor extra_specs Jun 14, 2024
@joker-at-work joker-at-work marked this pull request as ready for review June 14, 2024 10:19
majewsky added a commit to sapcc/limes that referenced this pull request Jun 21, 2024
@majewsky
Copy link

My side of this is in sapcc/limes#488.

As already discussed with Johannes, I would like to leave the door open for having the instances quota also be hardware-versioned. Since instances_$VERSION conflicts with instances_$FLAVOR, I have based my PR on the alternate naming pattern of hw_version_$VERSION_{cores,instances,ram} that we discussed. Would that be acceptable for everyone?

majewsky added a commit to sapcc/limes that referenced this pull request Jun 21, 2024
In addition to instances_* quota resources we now also add
cores_$HW_VERSION and ram_$HW_VERSION quota resources. These resources
are based on the `extra_specs` of our flavors and are added if a any
flavor defines a `quota:hw_version` property.

Change-Id: I93f922c3369ec293b215b4e93db977af13a70bd7
We want to split up quota between hardware versions and thus need the
reporting to report different cores/ram quota based on the
`quota:hw_version` `extra_spec` property of the instances' flavor.

We extract the `hw_version` at the same place we already extract the
`separate` property.

Change-Id: Ic1c79dab1e6a8619e68e1467888a500b5f8dcf71
We need to prevent unexpected values for our newly-introduced
`quota:hw_version` extra specs property.

Change-Id: I66ee04541a0facc7d8430313b73567db1e81c5b0
In the same way as with `quota:separate` we need to add non-yet-used
resources not returned by `count_as_dict()` into the limit check
arguments created in `check_deltas()`.

We need to check explicitly for the `hw_version_` and `instances_`
prefix, because we a) do not return the resources from our
`count_as_dict()` unless they are already in use by the project and b)
because there are other resources (e.g. `key_pairs`) that would
otherwise also be added even though the code originally doesn't expect
them to be - which can lead to failures in limit checking.

Change-Id: I904080b60bafd9608509870b051d284ee49ed08d
Since we now have to check for different cores and ram keys, this
function gets more complicated. Especially since resizing between
different hardware versions needs to be supported.

Change-Id: I56b2c0ef1871175a17a6ae5735e40f3b265be449
We have to use different keys for cores/ram here, too.

Change-Id: I96706e91aa6075bb08acb7e8bcb4378486b6d04b
We restructure the function a little to imho make it more readable for
the different cases supported: quota:separate, quota:instance_only and
quota:hw_version.

For quota:hw_version we need to use different keys and thus put them
into a variable and use this variable everywhere.

Change-Id: Id2f04b570237020b54364a545bc18619858af284
These limits and usage come from flavors specifying a
`quota:hw_version`. It contains the hardware version as key and a dict
as value, which contains the same keys that are also used for reporting
non-hw-versioned usage/limits.

    {
      "limits": {
        "rate": [],
        "absolute": {
          "maxTotalRAMSize": 512,
          "maxTotalInstances": 5,
          "maxTotalCores": 21,
          "maxTotalKeypairs": 10,
          "totalRAMUsed": 256,
          "totalCoresUsed": 10,
          "totalInstancesUsed": 2,
        },
        "absolutePerFlavor": {},
        "absolutePerHwVersion": {
          "v2": {"maxTotalCores": 10, "totalCoresUsed": 2,
                 "maxTotalRAMSize": 512, "totalRAMUsed": 256}
        },
      }
    }

Change-Id: I8fd88f0f256bf9ee59f06ac19f86bc2ff737cca3
@joker-at-work joker-at-work force-pushed the quota-for-hw-versions branch from a843792 to 877f8fe Compare June 24, 2024 11:38
@joker-at-work
Copy link
Author

Updated to use the prefix instead of the postfix for hardware versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants