[experimental] plugn: add new bank_info class
#401
Closed
+545
−103
Conversation
This file contains 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
Problem: the bank_info struct does not contain the name of the bank in the struct, which can make it more tedious to find the name of the bank because you have to look at the key of the map in order to find it. It would be more convenient if the name was also in the struct. Add the bank name to the struct attached with each user/bank combination.
Problem: flux-accounting currently has no support for a user to update the bank of their job. Add a new callback to the plugin which looks for the "job.update.attributes.system.bank" topic string and validates the new bank that the user is trying to update their job to. If they have access to the bank they want to update their job to but it is currently at its max active jobs OR max running jobs limits, reject the update. Add a check for this update in the job.update callback which will look for an updated bank. If the updated bank has been validated, change the bank_info struct assigned to that job to reflect the attributes of the new bank.
Problem: flux-accounting has no tests for updating the bank of a pending job. Add some tests.
Problem: flux-accounting has no tests for updating both the queue and the bank of a job at the same time. Add a new test file which tests updating both the queue and the bank at the same time as well as some combinations where at least one attribute update is expected to fail (i.e an invalid bank or an invalid queue). In both of the cases where at least one attribute fails, the entire request will fail and the job will remain under its original attributes.
Problem: The plugin uses its own bank_info struct to hold user/bank information and has a number of methods for access and modification of these structs. As the plugin's feature set has grown, so have the requirements for the bank_info struct, resulting in a very large and hard-to-parse piece code. Begin to clean up this plugin. Start by creating a new user_bank_info class and place it in a separate file that gets compiled with the plugin. Replace all instances of "struct bank_info" with the new "user_bank_info" class type.
cmoussa1
added
new feature
Problem: There is some functionality in the priority plugin (mf_priority.cpp) that is specific to accessing user_bank_info objects in a map data structure. These methods can be abstracted out to the class files to clean up some of the plugin code. Add two new functions to the bank_info.cpp class file that handle looking up a user/bank combo and fetching a user_bank_info object from the users map.
Problem: The plugin now has access to helper functions for accessing user/bank information in bank_info.cpp, but it does not make use of them. In validate_cb (), use the new helper functions to help with the validation of a user/bank job.
Problem: The arguments to the get_queue_info () function include an iterator to the bank_info object, which is used in the function to access the user/bank's list of permissible queues for validation. This can be confusing to read and would be easier if just the list of permissible queues was passed to the function. Change the argument to the get_queue_info () function from an iterator to the user/bank object to just the user/bank's list of permissible queues. Change the calls to this function to account for the argument change.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #401 +/- ##
==========================================
+ Coverage 81.00% 81.26% +0.26%
==========================================
Files 20 22 +2
Lines 1437 1484 +47
==========================================
+ Hits 1164 1206 +42
- Misses 273 278 +5
|
|
Think I'm gonna close this PR, as I've just opened #403 that I think does a better job of what I tried to do here. 😆 |
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.
Background
The plugin uses its own
bank_infostruct to hold user/bank information and has a number of methods for access and modification of these objects. As the plugin's feature set has grown, so have the requirements for thebank_infostruct, resulting in a very large and hard-to-parse piece of code.This is more of an experimental PR based on some of the feedback in #400 that suggested considering creating a class for the
bank_infoobject. For now, I've just built this on top of #400 with one additional commit that does the following:A new
.hppand.cppfile are added to theplugins/directory called bank_info.cpp and bank_info.hpp. This set of files stores a new class calleduser_bank_info, which is made up of the same attributes as the originalbank_infostruct. They are#included to the plugin file and the definition of the struct is thus removed from the plugin and instances of that struct are replaced with the use of the new class.For now, this class file just has the definition of the
user_bank_infoobject, but, if this is the right idea, then I can begin to slowly move some of the related functionality that is currently defined in the plugin over to this new class file.