add default quota to AW #396
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
z103cb
reviewed
May 29, 2023
|
What is not clear from this PR how are "default" quota contexts going to be handled? Where do you intend to add the default values and what are those default values? As this PR stands now, it seems incomplete. |
z103cb
suggested changes
May 29, 2023
Closed
|
After a conversation with @tardieu, I understand what will need to happen to have the "default" quota tree nodes. The only outstanding objection is symplication of the logic |
dmatch01
reviewed
Jun 1, 2023
… is a label associated with every quota-sub-tree for the AppWrapper; if there is none, then it adds it with 'default' value. This way, AppWrappers submitted without labels can be dispatched using the quota management library. Added tests for this, and fixed 'quota_service' sub tree to have a 'default' node. This code assumes the quota administrator created a leaf node name 'default' that can be used to schedule AppWrappers without quota labels.
metalcycling
reviewed
Jun 2, 2023
There was a problem hiding this comment.
I made the changes to the logic @z103cb requested as well as implementing the behavior discussed with @tardieu (and some of the suggestions by @dmatch01). @z103cb you are the gatekeeper right now, so we'll wait for your feedback. In the meantime, I'll deploy this version of quota management (with the newest changes) to the DEV cluster while the PR is reviewed.
… The code would think there is no quota (even without quota management) because the new code wasn't inside the proper scope.
z103cb
suggested changes
Jun 6, 2023
There was a problem hiding this comment.
- The changes in
queuejob_controller_ex.gohave a bug, it does not preserve any prior labels. - The new method name in the
quota_manager_interface.goleaks information about how the quota subsystem in implemented. I suggest chaninging is name to something a little bit more abstract, and indicative of its usage.
…nd updates the Kubernetes object correctly. New tests added to check for AppWrappers with no labels at all and with existing labels but no quota labels.
metalcycling
requested changes
Jun 11, 2023
z103cb
approved these changes
Jun 12, 2023
Srihari1192
pushed a commit
to Srihari1192/multi-cluster-app-dispatcher
that referenced
this pull request
Jul 6, 2023
* add default quota to AW * add default quota * resolve merge * Added support to query existing QuotaSubTrees. MCAD checks that there is a label associated with every quota-sub-tree for the AppWrapper; if there is none, then it adds it with 'default' value. This way, AppWrappers submitted without labels can be dispatched using the quota management library. Added tests for this, and fixed 'quota_service' sub tree to have a 'default' node. This code assumes the quota administrator created a leaf node name 'default' that can be used to schedule AppWrappers without quota labels. * Fixed bug that manifested when running MCAD without quota management. The code would think there is no quota (even without quota management) because the new code wasn't inside the proper scope. * Renamed the function 'GetTreeNames' to 'GetValidQuotaLabels' * Default quota labels code fixed. Code preserves the existing labels and updates the Kubernetes object correctly. New tests added to check for AppWrappers with no labels at all and with existing labels but no quota labels. * relax testing scenario --------- Co-authored-by: Pedro D. Bello-Maldonado <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.