Allow validation of Openshift allocations when changing quotas #208
Conversation
In combination with changes to `coldfront-plugin-cloud` [1], a Coldfront patch is added to allow validation of Openshift allocation change requests (arc). Currently only one scenario is tested: If the acr requests for quotas below the current usage of the project, the user will be sent back to the change request form with an informative error message. This feature is possible by leveraging Django formsets and their built-in custom validation feature [2] [1] nerc-project/coldfront-plugin-cloud#208 [2] https://docs.djangoproject.com/en/5.1/topics/forms/formsets/#custom-formset-validation
In combination with changes to `coldfront-plugin-cloud` [1], a Coldfront patch is added to allow validation of Openshift allocation change requests (arc). Currently only one scenario is tested: If the acr requests for quotas below the current usage of the project, the user will be sent back to the change request form with an informative error message. This feature is possible by leveraging Django formsets and their built-in custom validation feature [2] [1] nerc-project/coldfront-plugin-cloud#208 [2] https://docs.djangoproject.com/en/5.1/topics/forms/formsets/#custom-formset-validation
|
Can you please resolve the merge conflicts? I plan on reviewing this week. |
QuanMPhm
force-pushed
the
138/validate_cr
branch
2 times, most recently
from
67c73af to
4613114
Compare
|
@knikolla Done |
|
@QuanMPhm CI seems unhappy. |
QuanMPhm
force-pushed
the
138/validate_cr
branch
from
4613114 to
2d80a0e
Compare
|
@knikolla My bad. Some breaking changes not caught by the merge conflicts. |
src/coldfront_plugin_cloud/tasks.py
Outdated
| allocation = Allocation.objects.get(pk=allocation_pk) | ||
| if allocator := find_allocator(allocation): | ||
| if project_id := allocation.get_attribute(attributes.ALLOCATION_PROJECT_ID): | ||
| return allocator.get_quota_used(project_id) |
There was a problem hiding this comment.
This has not been defined in the base class, therefore you can't assume all Allocators have it. (OpenStack's allocator doesn't). And therefore I expected this to fail for OpenStack allocations.
You can implement the function in the base class and raise a NotImplemented error and catch the error here to gracefully handle that case.
And if I can offer a suggestion for the naming, get_usage makes more sense than get_quota_used.
src/coldfront_plugin_cloud/utils.py
Outdated
| return math.ceil(total_interval_duration) | ||
|
|
||
|
|
||
| def parse_openshift_quota_value(attr_name, quota_value): |
There was a problem hiding this comment.
This should go in the openshift module rather than here.
src/coldfront_plugin_cloud/utils.py
Outdated
| return False | ||
|
|
||
|
|
||
| def coldfront_to_openshift_quota_name(coldfront_quota_name: str): |
There was a problem hiding this comment.
This should go in the openshift module.
| """ | ||
| Obtains the current quota usage for the allocation. | ||
| For example, the output for an Openshift quota would be: |
There was a problem hiding this comment.
Would it make more sense for the allocator to return usage with the attribute names rather than the internal names?
There was a problem hiding this comment.
I have no preference. Either way some translation will have to be done so that the requested quota dict and the usage dict use the same keys for quota attributes. Do you think one option is better than the other?
There was a problem hiding this comment.
I think it makes more sense for the allocator to abstract away internal implementation details. In this way the interface between ColdFront and the Allocator is the attribute name. However, I do see that get_quota doesn't abide by this interface and instead exposes the internal names, so that would have to be changed too at some point.
QuanMPhm
force-pushed
the
138/validate_cr
branch
2 times, most recently
from
fb2e771 to
a4070a3
Compare
QuanMPhm
force-pushed
the
138/validate_cr
branch
from
a4070a3 to
1775cb4
Compare
|
This seems to assume there is only one cluster, but that is no longer the case. I believe from our design discussion with MOC folks that quotas will apply to all clusters, not just prod. Is that still the intention? |
This does work with multiple clusters. It will infer which cluster an allocation is part of when validating whether the quotas are valid. |
As part of allowing the validation of allocation change requests (acr) [1], various functions have been added to the plugin that are mostly called from Coldfront to validate an acr. Among these changes are: - Several new functions to `utils.py` to perform basic tasks - Functions in the openshift allocator to obtain the usage of a Openshift project - Refactoring of how the Openshift quota value is parsed - New unit tests [1] nerc-project/coldfront-nerc#138
QuanMPhm
force-pushed
the
138/validate_cr
branch
from
1775cb4 to
ecf3a5c
Compare
Partly closes nerc-project/coldfront-nerc#138, dependant on #205, this PR consists of the last commit. More details in the commit message.