plugin: improve check of internal user/bank map in job.validate
#386
Conversation
src/plugins/mf_priority.cpp
Outdated
| static bool check_map_for_dne_only () | ||
| { | ||
| // check if the map only contains DNE entries | ||
| bool only_dne_data = true; |
There was a problem hiding this comment.
This variable appears to be unused
There was a problem hiding this comment.
Whoops, thanks for catching this. I must have missed that when I was moving this block of code to a helper function. Will fix this
| // Scan the users map and look at each user's default bank to see if any one | ||
| // of them have a valid bank (i.e one that is not "DNE"; if any of the users do | ||
| // do have a valid bank, it will return false) | ||
| static bool check_map_for_dne_only () |
There was a problem hiding this comment.
A really quick thought here before a full review.
If this function instead returned the default bank for a user or DNE if the user has no default bank, would it potentially be useful elsewhere? Also, it might be more clear if the users map was passed as a parameter to the function...
There was a problem hiding this comment.
I think there are a couple of other places where the default bank for a user is queried, but I had actually posted an experimental PR (#371) where the bank lookup was moved to its own helper function. It looks like this:
static bank_info_result get_bank_info (
std::map<int, std::map<std::string, struct bank_info>>::iterator it,
int userid,
char *bank)
{
std::map<std::string, struct bank_info>::iterator bank_it;
// make sure user belongs to bank they specified; if no bank was passed in,
// look up their default bank
if (bank != NULL) {
bank_it = it->second.find (std::string (bank));
if (bank_it == it->second.end ())
return {INVALID_BANK, bank_it};
} else {
bank = const_cast<char*> (users_def_bank[userid].c_str ());
bank_it = it->second.find (std::string (bank));
if (bank_it == it->second.end ())
return {NO_DEFAULT_BANK, bank_it};
}
return {BANK_SUCCESS, bank_it};
}I had originally thought that it would be useful to have functionality like the one proposed in this PR (just scanning the users map for the first non-DNE bank it sees) kept separate, but perhaps it would be better to combine the two? Off the top of my head, maybe in the check where it looks for a default bank, it could add something like:
else {
bank = const_cast<char*> (users_def_bank[userid].c_str ());
bank_it = it->second.find (std::string (bank));
+ if (it->second == "DNE")
+ return {DNE_DEFAULT_BANK, bank_it};
else if (bank_it == it->second.end ())
return {NO_DEFAULT_BANK, bank_it};
}Also, it might be more clear if the users map was passed as a parameter to the function...
The users map is a global variable in the plugin. Would it be sufficient to add a comment in this function to mention this?
There was a problem hiding this comment.
Yeah, a comment would definitely help!
There was a problem hiding this comment.
just force-pushed up a change to add a comment about both the users and users_def_bank maps since they are both global variables.
cmoussa1
force-pushed
the
issue#385
branch
2 times, most recently
from
d27c4c6 to
236a728
Compare
|
Thanks, will set MWP on this shortly! |
Problem: In validate_cb (), the multi-factor priority plugin has a bug
where even if it only has the special does-not-exist ("DNE") entries for
at least one user/bank (indicating that the plugin has not received any
flux-accounting data yet), subsequently submitted jobs by other users
get rejected with the "no bank found for user" error message, which is
the rejection message typically output when the plugin has valid
user/bank data.
Add a helper function to the priority plugin that scans the users map
and checks the default bank for each user. As soon as it finds a
user/bank with a default bank that is not the special "DNE" entry,
return false. Otherwise, return true.
Use this helper function in the callback for job.validate to ensure that
the plugin's internal user/bank map contains valid user/bank data before
rejecting a user's job because they don't have a valid entry in the
flux-accounting database.
Problem: the flux-accounting testsuite has no tests that check that multiple users can submit jobs to the multi-factor priority plugin while it has no data from the flux-accounting DB and have them be held in state PRIORITY while the plugin waits for data. Add some tests that exercise this functionality.
Codecov Report
@@ Coverage Diff @@
## master #386 +/- ##
==========================================
+ Coverage 81.32% 81.43% +0.10%
==========================================
Files 20 20
Lines 1360 1368 +8
==========================================
+ Hits 1106 1114 +8
Misses 254 254
|
Problem
As noted in #385, in
validate_cb (), the multi-factor priority plugin has a bug where even if it only has the special does-not-exist ("DNE") entries for at least one user/bank (indicating that the plugin has not received any flux-accounting data yet), subsequently submitted jobs by other users get rejected with the"no bank found for user"error message, which is the rejection message typically output when the plugin has valid user/bank data.This PR adds a helper function to the priority plugin that scans the users map and checks the default bank for each user. As soon as it finds a user/bank with a default bank that is not the special
"DNE"entry, it returnsfalse. This helper function is called in thejob.validatecallback to ensure that the plugin's internal user/bank map contains valid user/bank data beforerejecting a user's job because they don't have a valid entry in the flux-accounting database.
It also adds a small set of tests that exercise this functionality of submitting multiple jobs from multiple users and checking that they are held in state
PRIORITYbefore any data is loaded to the plugin; then, valid data is sent to the plugin and the jobs are checked to confirm that they transition toRUN. It also adds a test to make sure that a user that does not have a valid entry in the flux-accounting DB cannot submit jobs to the plugin.Fixes #385