quota: Add SAPQuotaEngine and SAPCustomResource #478
Conversation
This commit partly reverts Ib32b03cfa45ec05785b9fa67a9502922baa36ce4 by introducing `SAPQuotaEngine`, which holds the update functionality in `_resources` by making it a property. Therefore, the changes to use `self.resources` instead of `self._resources` everywhere can be reverted. Change-Id: I4b30967b5f49ffc23cbb31c55d1d7fab6bd88a38
|
Verified in QA that all the |
|
Compare #469 for the discussion. |
| @@ -892,6 +893,18 @@ def __init__(self, name, count_as_dict, flag=None, default=None): | |||
| self.count_as_dict = count_as_dict | |||
|
|
|||
|
|
|||
| class SAPCustomResource(CountableResource): | |||
There was a problem hiding this comment.
Bikeshed: I'm starting to dislike the trend of the SAP* things. It's not descriptive.
SAPQuotaEngine is kind of okay, because one could say it basically says "our setup". But personally something like DynamicQuotaEngine (not really very good either... 🤔) would suit it better.
SAPCustomResource then is too generic a name for what it does for my tastes. It would better be DedupedCountableResource or CheckedCountableResource or something similar.
This wouldn't block merging for me, though. So unless we come up with a good alternative consensus, then it's fine for me to merge as-is.
There was a problem hiding this comment.
The SAP prefix between both is common, because they handle the same thing ;)
There was a problem hiding this comment.
For anything we could upstream, I agree. This doesn't look upstreamable to me, so I'm fine with making clear that this is non-upstream functionality and make it easy to detect custom patches. I don't feel too strongly about that, though.
There was a problem hiding this comment.
My problem is not with upstream, but with us looking at this in 10 months and not knowing what the class is about, because just because it's "SAP custom stuff" doesn't say anything about its purpose. You certainly have better recall than I do, but I'd have to re-read the code.
There was a problem hiding this comment.
fair. calling it "SAP" is the easy way to find a name, yes, but doesn't contain much. I would struggle to find a good name here, though. I'll merge as is, but will consider more rounds of name-finding for future things.
When we add our own resources for quota, e.g. the "instances_$FLAVOR_NAME" resources for counting instances of a specific flavor, these resources usually don't come up in the usage-count unless a project uses them. Since we use the same function for counting the instances_* resources and the instances/cores/ram, and the latter always gets set, we can check for them to see if we have to run our resource-counting function (again). Previously, this check was hard-coded to `instances`, but since we're going to add more resources in `SAPQuotaEngine`, we introduce `SAPCustomResource` to hold a property to check for - defaulting to `instances` again. Change-Id: I8b05c08c977e56d7466b4942fdf6c927c841af1b
joker-at-work
force-pushed
the
quota-sapquotaengine-sapcustomresource
branch
from
f0d1117 to
ec77f4d
Compare
Please see the individual commits for details.