plugin: add instance owner info to plugin #477
Conversation
Problem: The instance owner cannot run jobs when the priority plugin is loaded because they are not a part of the flux-accounting database. flux-accounting should be able to handle this automatically so that administrators do not have to manually add instance owner information. Send instance owner information to the priority plugin in the bulk update script as another valid user to run jobs under. Give it very high jobs and nodes limits.
cmoussa1
force-pushed
the
add.instance.owner.to.db
branch
from
8b901aa to
1374786
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #477 +/- ##
==========================================
- Coverage 83.30% 82.34% -0.96%
==========================================
Files 23 20 -3
Lines 1557 1456 -101
==========================================
- Hits 1297 1199 -98
+ Misses 260 257 -3 |
There was a problem hiding this comment.
LGTM! I did wonder if there was some way the instance owner data could be sent only once, but I'm not sure how that would work.
I guess an alternative would be to have the mf_priority plugin manually set up the instance owner associations internally at startup, but the benefit of this approach is that it is simple...
| flux account add-user --username=user1001 --userid=1001 --bank=account1 --max-running-jobs=2 && | ||
| flux account add-user --username=user1001 --userid=1001 --bank=account2 | ||
| flux account add-user --username=user5001 --userid=5001 --bank=account1 --max-running-jobs=2 && | ||
| flux account add-user --username=user5001 --userid=5001 --bank=account2 |
There was a problem hiding this comment.
Is there something in the testsuite that ensures the user running it is not 5001 (or for that matter is 1001). Or is the testsuite only expected to be run in certain environments?
One idea would be to add a check for the current uid in sharness.d/flux-accounting.sh and set up some environment variables with userids instead of explicitly using 1001 and 5001 (in case those conflict with system uids)
This could be done in a future cleanup PR if this is working for now.
There was a problem hiding this comment.
I guess there isn't anything explicit that is checking it (I just checked the logs from the tests that failed via GitHub actions), but that's a good idea to set up some environment variables so that I don't have to define them every single time I create a new test. I'll open an issue on this so I can get this cleaned up in the future.
Problem: Now that instance owner information is sent to the priority plugin, a few of the tests that look for certain information via the plugin.query callback also need to be updated since the instance owner information is now included. In the tests where users are created with a userid of 1001, change these to 5001 to avoid potential conflict with the uid of the instance owner. Change the jq checks to search with select() functions to search via userid instead of indexing the JSON array by index. Remove the test_cmp calls with the expected files in t/expected/plugin_state/ and instead just use jq calls to check for expected fields.
Problem: There exists no tests in flux-accounting for submitting jobs as the instance owner with the priority plugin loaded. Add some tests.
cmoussa1
force-pushed
the
add.instance.owner.to.db
branch
from
1374786 to
83bff16
Compare
|
Thanks for reviewing @grondo - I'll set MWP here so I can get this landed in time for some testing |
Problem
The instance owner cannot run jobs when the priority plugin is loaded because they are not a part of the flux-accounting database. flux-accounting should be able to handle this automatically so that administrators do not have to manually add instance owner information.
This PR sends instance owner information to the priority plugin along with the rest of the flux-accounting DB information during an accounting update. It leverages Flux's
handle.attr_get("security.owner")to get the uid and corresponding username of the instance owner. Once retrieved, it is sent and stored by the plugin, which will allow the instance owner to submit jobs with the plugin loaded. I set the limits values very high for the instance owner, but just gave it a default fair-share value of 0.5. I am definitely open to making this value higher, however.I had to adjust a few of the existing sharness tests that created users with conflicting uid's as the instance owner in the testsuite, so the second commit in this PR is just some cleanup and adjusting of some of those tests to use a different uid. I also refined a couple of the tests that use
jqto search for key-value pairs by first callingselect()with a uid instead of manually indexing the JSON object.Finally, I added a short set of tests that loads the plugin and and submits a job as the instance owner and makes sure it receives a priority.
Fixes #476