Allow dynamic quota creation and removal by QuanMPhm · Pull Request #287 · nerc-project/coldfront-plugin-cloud

QuanMPhm · 2026-01-21T16:13:06Z

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.py and remove_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 with validate_allocations.py. I would now suggest checking the changes to functional/openshift/test_allocations.py to 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.

QuanMPhm · 2026-01-21T16:13:50Z

src/coldfront_plugin_cloud/management/commands/add_quota_to_resource.py

+            defaults={"value": json.dumps(new_quota_dict)},
+        )
+
+        # TODO (Quan): Dict update allows migration of existing quotas. This is fine?


@knikolla @jtriley This is a pre-existing feature, so I assume the answer is yes. Just to make sure.

I don't think I fully understand this comment. Can you elaborate?

We currently allow migrating the quota's cluster label (i.e "limits.cpu" for Openshift CPUs) by changing the hardcoded values in the QUOTA_KEY_MAPPING of the appropriate allocator. This migration feature is demonstrated in the functional test that I linked.

Below my TODO comment:

if not created: available_quotas_dict = json.loads(available_quotas_attr.value) available_quotas_dict.update(new_quota_dict) QuotaSpecs.model_validate(available_quotas_dict) # Validate uniqueness available_quotas_attr.value = json.dumps(available_quotas_dict) available_quotas_attr.save()

I wanted to show that this migration feature will still be available, because if you decide to add the same quota to the same resource, available_quotas_dict.update(new_quota_dict) means you can update/migrate everything about the quota, including its cluster label (with the exception of the display name, which you've mentioned and I responded here).

Understood.

QuanMPhm · 2026-01-21T16:15:56Z

src/coldfront_plugin_cloud/management/commands/calculate_storage_gb_hours.py

-                    "OpenStack Storage",
-                    openstack_nese_storage_rate,
-                )
+                # TODO (Quan): An illustration of how billing could be simplified. Shuold I follow with this?


@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.

QuanMPhm · 2026-01-21T16:19:52Z

src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py

+            },
+        )
+
+        # TODO (Quan): What happens when a quota is removed? Should the attribute be removed from Coldfront?


@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.

My hunch is no, but I want to wait for @knikolla input

For now just have the quota be removed from the Resource Attribute but untouched in the allocations.

While not entirely clear, it seems the recent Pandas relase (3.0.0) changed how casting to decimal types works, causing some invoicing code to throw errors, specifically calls to read_csv() Seperating loading of the CSV and casting seems to fix this

QuanMPhm · 2026-01-29T17:28:11Z

@knikolla I addressed all your suggestions on Slack except one:

To migrate the display name of an attribute
Before, since the attributes were stored in code, the migrations were also stored in code
Now, since the adding of new quota is a command, migrating the display name of an attribute should also be a command.

May I ask that I implement this feature in a subsequent PR, to prevent this PR from bloating even more? If not, I will implement this after I receive answers for my questions above.

joachimweyl · 2026-01-29T17:31:50Z

What is the impact of this omission?

QuanMPhm · 2026-01-29T21:47:44Z

@joachimweyl The impact will be that to change the display names of attributes (the names that users will see in the Coldfront UI, i.e OpenShift Limit on CPU Quota) will be a bit inconvenient. An admin will have to do some manual renaming in Coldfront. Still doable, but not in a way that's quick and programmatic. We ideally want a CLI command that makes renaming easier, but I didn't want this PR to take too long to review because of the February maintenance.

joachimweyl · 2026-01-30T16:01:19Z

Makes sense to me.

knikolla · 2026-02-05T14:42:23Z

src/coldfront_plugin_cloud/models/quota_models.py

+        """
+        return self.static_quota + self.multiplier * int(quantity)
+
+    def formatted_quota(self, quota_value: int) -> int | str:


I really dislike that this can return two different types.

knikolla · 2026-02-05T14:46:47Z

src/coldfront_plugin_cloud/models/quota_models.py

+    """
+    Fields:
+    - quota_label: human readable label for the quota (must be unique across the dict)
+    - default_quota: default quota value (int, >= 0)


I don't understand what the purpose of this is? The default is equal to (quantity * multiplier) + static_quota, no? It seems unused anywhere else besides the command line.

I now see I have been hallucinating and though I needed it :(

src/coldfront_plugin_cloud/models/quota_models.py

knikolla · 2026-02-05T14:52:30Z

src/coldfront_plugin_cloud/openstack.py

    def _get_network_quota(self, quotas, project_id):
        network_quota = self.network.show_quota(project_id)["quota"]
-        for k in self.QUOTA_KEY_MAPPING["network"]["keys"].values():
+        for cf_k in self.SERVICE_QUOTA_MAPPING["network"]:


You could have used the resource_type field of the QuotaSpec here. This will result in an error if not all quotaspecs are defined for OpenStack resources.

knikolla · 2026-02-05T14:55:28Z

src/coldfront_plugin_cloud/management/commands/add_quota_to_resource.py

+            dest="quota_label",
+            type=str,
+            required=True,
+            help="Human-readable quota_label for this quota (must be unique).",


I don't think this description is right. This maps to the key on the cluster sides.

knikolla · 2026-02-05T14:57:54Z

src/coldfront_plugin_cloud/management/commands/add_quota_to_resource.py

+            defaults={"value": json.dumps(new_quota_dict)},
+        )
+
+        # TODO (Quan): Dict update allows migration of existing quotas. This is fine?


I don't think I fully understand this comment. Can you elaborate?

knikolla · 2026-02-05T14:59:06Z

src/coldfront_plugin_cloud/models/quota_models.py

+        return self
+
+    @cached_property
+    def storage_quotas(self) -> dict[str, QuotaSpec]:


A more generic get_quotas_by_type would be more flexible and allow you to query other kinds of resource types like compute, network, etc.

knikolla · 2026-02-05T15:06:43Z

src/coldfront_plugin_cloud/management/commands/add_openshift_resource.py

add_openstack_resource and add_openshift_resource now don't provide ALL the required QuotaSpecs with the EXACT same multiplier and static values as they are now, otherwise you are changing the behavior.

YOU NEED TO provide a separate python command or shell file that registers ALL the values as they are now.

Otherwise, as it is you haven't provided a smooth transition from the current system to the new Dynamic Quota system and an admin would need to type out a lot of error prone commands manually to make this upgrade work.

naved001

thanks @QuanMPhm! Some basic questions in there as I try to refresh my memory of coldfront. Will do another pass.

naved001 · 2026-02-05T15:00:21Z

src/coldfront_plugin_cloud/management/commands/add_quota_to_resource.py

+class Command(BaseCommand):
+    def add_arguments(self, parser):
+        parser.add_argument(
+            "--display_name",


you have an underscore instead of a dash.

--display_name -> --display-name

naved001 · 2026-02-05T15:03:51Z

src/coldfront_plugin_cloud/management/commands/add_quota_to_resource.py

+            help="The default quota value for the storage attribute. In GB",
+        )
+        parser.add_argument(
+            "--resource_name",


--resource_name -> --resource-name

naved001 · 2026-02-05T15:09:50Z

src/coldfront_plugin_cloud/management/commands/add_quota_to_resource.py

+            type=str,
+            default="",
+            help="Name of quota as it appears on invoice. Required if --is-storage-type is set.",
+        )


how come you didn't specify dest= for some of these arguments?

I normally wouldn't include dest=, and didn't review closely enough what Copilot generated this code for me. I've removed the dest=. Apologies

naved001 · 2026-02-05T16:01:14Z

src/coldfront_plugin_cloud/management/commands/add_quota_to_resource.py

+    def handle(self, *args, **options):
+        if options["resource_type"] == "storage" and not options["invoice_name"]:
+            logger.error(
+                "--invoice-name must be provided when storage type is  `storage`."


"when resource type is storage."

My idea is any quota that is relevant for storage billing should have the resource type storage, such as:

QUOTA_REQUESTS_IBM_STORAGE = "OpenShift Request on IBM Storage Quota (GiB)" QUOTA_REQUESTS_NESE_STORAGE = "OpenShift Request on NESE Storage Quota (GiB)"

naved001 · 2026-02-05T16:15:31Z

src/coldfront_plugin_cloud/management/commands/add_quota_to_resource.py

+            "--invoice-name",
+            type=str,
+            default="",
+            help="Name of quota as it appears on invoice. Required if --is-storage-type is set.",


where's --is-storage-type? Did you mean --resource-type is set to storage?

Ah yes. My bad

naved001 · 2026-02-05T16:38:47Z

src/coldfront_plugin_cloud/management/commands/add_openshift_resource.py

            else options["name"],
        )
+
+        # Add common Openshift resources (cpu, memory, etc)


remind how were these resources created before this?

Currently, the information for these quotas are spread in multiple places in the repo. The display names are in attributes.py, the multiplier and static quantities are in tasks.py, other info in other places. The allocation attributes for these quotas were loaded by register_cloud_attributes.py, which consumes the attributes defined in attributes.py.

A by-product of this PR is that now all that info is created and stored in one place.

QuanMPhm requested review from Milstein, jtriley, knikolla and naved001 January 21, 2026 16:13

QuanMPhm commented Jan 21, 2026

View reviewed changes

QuanMPhm marked this pull request as draft January 21, 2026 18:05

QuanMPhm force-pushed the ops_1391/final branch 6 times, most recently from b3c58d8 to 35273aa Compare January 29, 2026 17:08

QuanMPhm marked this pull request as ready for review February 4, 2026 18:24

knikolla reviewed Feb 5, 2026

View reviewed changes

src/coldfront_plugin_cloud/models/quota_models.py Show resolved Hide resolved

knikolla requested changes Feb 5, 2026

View reviewed changes

naved001 reviewed Feb 5, 2026

View reviewed changes

Allow dynamic quota creation and removal

01021dd

QuanMPhm force-pushed the ops_1391/final branch from 35273aa to 01021dd Compare February 5, 2026 20:10

Conversation

QuanMPhm commented Jan 21, 2026

Uh oh!

QuanMPhm Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joachimweyl Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuanMPhm commented Jan 29, 2026

Uh oh!

joachimweyl commented Jan 29, 2026

Uh oh!

QuanMPhm commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joachimweyl commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naved001 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuanMPhm Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

QuanMPhm Jan 21, 2026 •

edited

Loading

joachimweyl Feb 4, 2026 •

edited

Loading

QuanMPhm commented Jan 29, 2026 •

edited

Loading

joachimweyl commented Jan 30, 2026 •

edited

Loading

QuanMPhm Feb 5, 2026 •

edited

Loading