Refactor to allow resource-agnostic validation by QuanMPhm · Pull Request #248 · nerc-project/coldfront-plugin-cloud

QuanMPhm · 2025-09-15T17:47:02Z

Closes #239. This PR consists of the last commit. Built on top of #245. More details in the commit message.

@knikolla @jtriley I have a small bug in how error handling is done when the object storage quota is missing. Just want to confirm that my change is desirable.

knikolla

This is a good start, but not going as far as I would like in terms of making it "resource-agnostic", as you still have a lot of if resource_name in OPENSHIFT|OPENSTACK.

Try thinking about a way to push those resource-specific functions inside the respective allocators and creating a new function in the base Allocator that serves as the abstraction.

See my comment about validate project exist.

src/coldfront_plugin_cloud/management/commands/validate_allocations.py

QuanMPhm · 2025-11-11T19:25:23Z

Note-to-self: The branch name for the original solution (before time of this comment) is 239/agnostic_validate_first

QuanMPhm · 2025-11-11T20:10:24Z

@knikolla I've cleaned house on validate_allocations.py. Hopefully this is closer to your vision. I haven't included tests for idempotency yet. For the sake of not making this PR too complicated, I believe that should be done in a subsequent PR?

knikolla · 2026-03-18T13:15:47Z

@QuanMPhm can you resolve conflicts (and update to new quota system if necessary). I would like to have this be part of next month's update.

QuanMPhm · 2026-03-20T14:37:05Z

@knikolla I have resolved all conflicts. This is ready to review

knikolla · 2026-03-23T19:09:38Z

src/coldfront_plugin_cloud/openshift.py

+            current_value = quota.get(quota_key, None)
+            current_value = parse_quota_value(current_value, attr)
+
+            self.check_and_apply_quota_attr(


Is this going to trigger a delete and set quota for each quota attribute that is out of date?

Yes. The check_and_apply_quota_attr() (defined in base.py) will have the same quota validation logic as the current validate_allocation command. Out-of-date quotas will trigger a call to set_quota, which deletes and sets the quotas

Yes, but in this implementation the call to set quota is made for each allocation attribute, rather than setting a flag for failed_validation which calls set_quota once at the end and setting all the quotas and avoiding unnecessary deletions and recreations.

Ah yes. I noticed that inefficiency too with the validation code. I'll refactor that

@knikolla the quota validation function has been refactored

knikolla · 2026-03-23T19:11:34Z

I did a quick first pass, I like this approach much more. Will do a second pass early tomorrow morning.

Quota validation will now behave the same for both resource types. OpenStack's particular use of default quotas is reflected in a new test in `openstack/test_allocation.py` OpenStack integration code is slightly changed to better handle missing object storage quotas Much of the validation logic has been pushed into `base.py`, `openshift.py`, and `openstack.py`

Copilot

Pull request overview

Refactors validate_allocations to be more resource-agnostic by delegating validation/sync behavior to each resource allocator (OpenStack/OpenShift/ESI), aligning behavior across resource types as requested in #239 (built on #245).

Changes:

Move common validation/sync logic into ResourceAllocator (user sync + quota comparison/apply helper) and implement per-resource set_project_configuration() flows.
Update OpenStack/OpenShift allocators to own quota validation/application (and OpenShift labels/limitrange validation).
Update functional/unit tests to use the new public get_project() API and to cover “new quota attribute added after allocations exist”.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/coldfront_plugin_cloud/tests/unit/openshift/test_project.py	Switch to `get_project()` public API.
src/coldfront_plugin_cloud/tests/functional/openstack/test_allocation.py	Update logging assertion target; add functional test for new quota attribute behavior.
src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py	Switch to `get_project()` public API in functional checks.
src/coldfront_plugin_cloud/openstack.py	Implement `set_project_configuration()` + quota validation/apply; adjust Swift quota handling.
src/coldfront_plugin_cloud/openshift.py	Implement `set_project_configuration()` + limitrange/label/quota validation/apply; expose `get_project()`.
src/coldfront_plugin_cloud/management/commands/validate_allocations.py	Simplify command to iterate resource types and call allocator-provided configuration sync.
src/coldfront_plugin_cloud/base.py	Add shared user sync + quota comparison/apply helpers and `allocation_str`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T14:01:15Z

src/coldfront_plugin_cloud/management/commands/validate_allocations.py

                try:
-                    allocator.set_quota(
-                        allocation.get_attribute(attributes.ALLOCATION_PROJECT_ID)
-                    )
-                except Exception as e:
+                    allocator.get_project(project_id)
+                except http.NotFound:


get_project() for OpenShift raises kubernetes.dynamic.exceptions.NotFoundError (not keystoneauth1.exceptions.http.NotFound). Catching only http.NotFound here means missing OpenShift projects will bubble up and abort the command. Consider normalizing allocator get_project() to raise a common exception type, or catch both NotFound exception types here.

Copilot · 2026-04-02T14:01:16Z

src/coldfront_plugin_cloud/openstack.py

+
+            expected_value = self.allocation.get_attribute(attr)
+            current_value = quota.get(key, None)
+            if key == OPENSTACK_OBJ_KEY and expected_value <= 0:


expected_value can be None for pre-existing allocations when a quota attribute is newly added to the resource. The expected_value <= 0 check will raise TypeError in that case (this matches the PR description about missing object storage quota). Guard against None before the comparison (e.g., handle the expected_value is None case first).

Suggested change

if key == OPENSTACK_OBJ_KEY and expected_value <= 0:

if (

key == OPENSTACK_OBJ_KEY

and expected_value is not None

and expected_value <= 0

):

Copilot · 2026-04-02T14:01:16Z

src/coldfront_plugin_cloud/openstack.py

+            try:
+                swift = self.object(project_id).head_account()
+            except ksa_exceptions.catalog.EndpointNotFound:
+                logger.debug("No swift available, skipping its quota.")


In the Swift quota lookup, EndpointNotFound is logged but execution continues into the second head_account() call, which is not wrapped in an EndpointNotFound handler. This will re-raise and fail get_quota() for deployments without an object-store endpoint. After catching EndpointNotFound, return/skip the object quota block (or include EndpointNotFound in the second try/except).

Copilot · 2026-04-02T14:01:16Z

src/coldfront_plugin_cloud/base.py

+    def check_and_apply_quota_attr(
+        self,
+        attr: str,
+        expected_quota: int,
+        current_quota: int,
+        apply: bool,
+    ) -> bool:


Type hints here say expected_quota/current_quota are int, but the method explicitly handles None values. Updating the annotations to Optional[int] (and the return type/docstring if any) would better reflect actual usage and avoid misleading type checking.

Copilot · 2026-04-02T14:01:16Z

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

+        self.assertEqual(actual_nova_quota.cores, 2)
+        self.assertTrue(default_ram_quota > 0)
+
+        # Add a new attribute for Openshift


Comment says "Add a new attribute for Openshift" but this test is exercising OpenStack quota defaults. Renaming the comment to OpenStack avoids confusion when debugging failures.

Suggested change

# Add a new attribute for Openshift

# Add a new attribute for OpenStack

Copilot · 2026-04-02T14:01:17Z

src/coldfront_plugin_cloud/base.py

 from coldfront.core.allocation import models as allocation_models
 from coldfront.core.resource import models as resource_models

-from coldfront_plugin_cloud import attributes
+from coldfront_plugin_cloud import attributes, tasks, utils
 from coldfront_plugin_cloud.models.quota_models import QuotaSpecs


base.py now imports tasks at module import time, but tasks.py also imports base (tasks -> base -> tasks). While this may work depending on import order, it introduces a circular dependency that is easy to break later. Consider importing tasks lazily inside set_users() (or moving user-sync orchestration out of base).

@QuanMPhm base.py should not import tasks.py. Please call the methods within the allocator.

Copilot · 2026-04-02T14:01:17Z

src/coldfront_plugin_cloud/openstack.py

+                current_value = int(
+                    self.object(project_id).head_account().get(OPENSTACK_OBJ_KEY)
+                )


head_account().get(OPENSTACK_OBJ_KEY) can return None when the quota header is missing; int(None) will raise TypeError. If the goal is to treat a missing object quota as unset, handle the None case explicitly (e.g., default to 0/None or trigger initialization) before converting to int.

Suggested change

current_value = int(

self.object(project_id).head_account().get(OPENSTACK_OBJ_KEY)

)

quota_header_value = self.object(project_id).head_account().get(

OPENSTACK_OBJ_KEY

)

current_value = (

int(quota_header_value) if quota_header_value is not None else None

)

knikolla · 2026-04-02T14:10:50Z

src/coldfront_plugin_cloud/base.py

+                    f"{coldfront_user.user.username} is not part of {project_id}"
+                )
+                if apply:
+                    tasks.add_user_to_allocation(coldfront_user.pk)


Call the allocator's method rather than tasks.py

knikolla · 2026-04-02T14:11:22Z

src/coldfront_plugin_cloud/base.py

+
+        return failed_validation
+
+        # if apply:


Should this be deleted?

QuanMPhm requested review from jtriley, knikolla and naved001 September 15, 2025 17:47

QuanMPhm changed the title ~~239/agnostic validate~~ Refactor to allow resource-agnostic validation Sep 15, 2025

knikolla mentioned this pull request Oct 1, 2025

Kristi does Code Reviews (Week of 2025-09-29) CCI-MOC/ops-issues#1621

Closed

9 tasks

knikolla mentioned this pull request Oct 14, 2025

Kristi does Code Reviews (Week of 2025-10-06 and 13) CCI-MOC/ops-issues#1624

Closed

4 tasks

knikolla requested changes Oct 23, 2025

View reviewed changes

src/coldfront_plugin_cloud/management/commands/validate_allocations.py Outdated Show resolved Hide resolved

joachimweyl mentioned this pull request Nov 6, 2025

Kristi reviews automatic approval of allocation change requests CCI-MOC/ops-issues#1637

Open

3 tasks

QuanMPhm force-pushed the 239/agnostic_validate branch from 9153d7b to a86da68 Compare November 11, 2025 19:33

QuanMPhm requested a review from knikolla November 11, 2025 19:33

QuanMPhm force-pushed the 239/agnostic_validate branch 2 times, most recently from 40883e2 to 129234b Compare November 11, 2025 20:08

QuanMPhm force-pushed the 239/agnostic_validate branch from 129234b to a0ba80d Compare March 20, 2026 13:57

knikolla reviewed Mar 23, 2026

View reviewed changes

QuanMPhm force-pushed the 239/agnostic_validate branch from a0ba80d to a22bf80 Compare March 31, 2026 19:18

QuanMPhm requested a review from knikolla March 31, 2026 19:50

knikolla requested a review from Copilot April 2, 2026 13:55

Copilot started reviewing on behalf of knikolla April 2, 2026 13:55 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

knikolla reviewed Apr 2, 2026

View reviewed changes

knikolla requested changes Apr 2, 2026

View reviewed changes

	# Add a new attribute for Openshift
	# Add a new attribute for OpenStack

Conversation

QuanMPhm commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knikolla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

QuanMPhm commented Nov 11, 2025

Uh oh!

QuanMPhm commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knikolla commented Mar 18, 2026

Uh oh!

QuanMPhm commented Mar 20, 2026

Uh oh!

knikolla Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

QuanMPhm Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

knikolla Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

QuanMPhm Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

QuanMPhm Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

knikolla commented Mar 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

knikolla Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

knikolla Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

knikolla Apr 2, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

QuanMPhm commented Sep 15, 2025 •

edited

Loading

QuanMPhm commented Nov 11, 2025 •

edited

Loading

knikolla Apr 2, 2026 •

edited

Loading