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.

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.

src/coldfront_plugin_cloud/management/commands/validate_allocations.py

src/coldfront_plugin_cloud/openstack.py

src/coldfront_plugin_cloud/base.py

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

src/coldfront_plugin_cloud/base.py

src/coldfront_plugin_cloud/openstack.py

src/coldfront_plugin_cloud/base.py

Copilot

Pull request overview

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

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

Copilot · 2026-04-08T14:48:43Z

src/coldfront_plugin_cloud/base.py

+                utils.set_attribute_on_allocation(self.allocation, attr, current_quota)
+                expected_quota = (
+                    current_quota  # To pass `current_quota != expected_quota` check
+                )


In check_and_apply_quota_attr, the branch where expected_quota is None but current_quota is present sets the allocation attribute (good) but still returns failed_validation=True. That forces set_quota_config() to call set_quota() even though the cluster is already the source of truth in this case, which can cause unnecessary (and potentially disruptive) quota reapplication—especially for OpenShift where set_quota() deletes and recreates ResourceQuotas. Consider distinguishing “allocation state fixed” from “cluster state needs updating” (e.g., return False after successfully setting the allocation attribute when no cluster change is needed, or return a structured result with separate flags).

Suggested change

)

)

failed_validation = False

I agree, let's treat failed_validation as a flag for when we need to perform changes to the cluster. In this case we only need to set the allocation attribute, so we can leave failed_validation as False.

src/coldfront_plugin_cloud/openstack.py

knikolla

This looks good to me, the only outstanding things is the failed_validation comment by Copilot.

With regards to the Swift comments, if you want to do a bit more investigating and see if they are important to fix.

knikolla · 2026-04-08T17:55:09Z

@naved001 would appreciate a pass from you.

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`

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

src/coldfront_plugin_cloud/base.py Outdated Show resolved Hide resolved

knikolla reviewed Apr 2, 2026

View reviewed changes

src/coldfront_plugin_cloud/base.py Outdated Show resolved Hide resolved

knikolla requested changes Apr 2, 2026

View reviewed changes

QuanMPhm force-pushed the 239/agnostic_validate branch from a22bf80 to 6ebd705 Compare April 6, 2026 16:04

QuanMPhm requested a review from knikolla April 6, 2026 17:48

knikolla requested a review from Copilot April 8, 2026 14:43

Copilot started reviewing on behalf of knikolla April 8, 2026 14:43 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

knikolla requested changes Apr 8, 2026

View reviewed changes

QuanMPhm force-pushed the 239/agnostic_validate branch from 6ebd705 to 30236f0 Compare April 8, 2026 18:24

QuanMPhm requested a review from knikolla April 8, 2026 18:25

knikolla approved these changes Apr 8, 2026

View reviewed changes

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

knikolla Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

knikolla left a comment

Choose a reason for hiding this comment

Uh oh!

knikolla commented Apr 8, 2026

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