-
Notifications
You must be signed in to change notification settings - Fork 13
Allow dynamic quota creation and removal #287
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
base: main
Are you sure you want to change the base?
Conversation
| defaults={"value": json.dumps(new_quota_dict)}, | ||
| ) | ||
|
|
||
| # TODO (Quan): Dict update allows migration of existing quotas. This is fine? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knikolla @jtriley This is a pre-existing feature, so I assume the answer is yes. Just to make sure.
| "OpenStack Storage", | ||
| openstack_nese_storage_rate, | ||
| ) | ||
| # TODO (Quan): An illustration of how billing could be simplified. Shuold I follow with this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knikolla I couldn't do the same refactoring for the Openshift allocations because different storages have their own rates. I could have refactored the code further to circumvent that issue, but I didn't want the PR to be too long.
| }, | ||
| ) | ||
|
|
||
| # TODO (Quan): What happens when a quota is removed? Should the attribute be removed from Coldfront? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knikolla @jtriley @joachimweyl This also has implications for billing storage. This test case is failing here since I would like people's consensus on desired behavior.
Closes nerc-project/operations#1391. This is how I would suggest to review this PR.
Two CLI commands have been added,
add_quota_to_resource.pyandremove_quota_from_resource.py. I would suggest understanding those two commands first. These commands allow us to dynamically add/remove quotas instead of having them hard-coded as they are currently done. These commands don't impact the quota objects in the clusters, nor the quota attributes in allocations. Their full impact is illustrated when used within the typical user workflow, or in tandem withvalidate_allocations.py. I would now suggest checking the changes tofunctional/openshift/test_allocations.pyto see the full implications of this PR. The other functional test cases only contain minor changes.Afterwards,
tasks.py,validate_allocations.py, and the allocator base and subclasses should be reviewed. They are the main consumers of quota information. All other changes relatively minor.This is a draft for now since I have some questions, and the tests are failing. I just wanted people to know my general direction with this feature.
I will wait for people's feedback before continuing work on this PR, since I assume substantial feedback will be given.