plugin: disable queue validation on an unknown queue, no configured "default" queue #281
Conversation
| @@ -30,9 +30,9 @@ extern "C" { | |||
| #include <sstream> | |||
|
|
|||
| #define BANK_INFO_MISSING -9 | |||
| #define NO_SUCH_QUEUE -5 | |||
| #define UNKNOWN_QUEUE 0 | |||
| #define NO_QUEUE_SPECIFIED 0 | |||
There was a problem hiding this comment.
Commit message nit: The commit message says:
Change the preprocessor directive INVALID_QUEUE to UNKNOWN_QUEUE
But it appears that here INVALID_QUEUE is still defined.
Also:
Change the preprocessor directive NO_DEFAULT_QUEUE to UNSPECIFIED_QUEUE,
I don't see a UNSPECIFIED_QUEUE, but there is a NO_QUEUE_SPECIFIED...
There was a problem hiding this comment.
Whoops, I totally messed that up, didn't I? I meant to update this commit message before posting the PR but completely forgot. I'll update the commit message to use both UNKNOWN_QUEUE (instead of INVALID_QUEUE) and NO_QUEUE_SPECIFIED (instead of UNSPECIFIED_QUEUE). Sorry for the confusion!
There was a problem hiding this comment.
While you are in there, it might be good to add comments on each of these #defines to say more of what each means. Not critical, but if you are modifying the commit anyway...
There was a problem hiding this comment.
Yes, good suggestion. I will do that!
| @@ -44,7 +44,7 @@ test_expect_success 'submit a job successfully under default bank' ' | |||
| jobid1=$(flux mini submit -n1 hostname) && | |||
| flux job wait-event -f json $jobid1 priority | jq '.context.priority' > job1.test && | |||
| cat <<-EOF >job1.expected && | |||
| 10050000 | |||
| 50000 | |||
There was a problem hiding this comment.
It might be useful to state in the commit message why the priority changes are a result of the changes in validating queues since, at least to me, that is not an obvious effect. Though please correct me if this should be clear.
There was a problem hiding this comment.
Yeah, looking back I realize this isn't as obvious as I initially thought, sorry about that.
The reason the priority changes for these jobs is because the queue factor is UNSPECIFIED_QUEUE, which is 0. This changes the resulting integer priority of the job from 1005000 to just 50000, as before, the multi-factor priority plugin had a "default" queue entry (and a factor of 1000) when an association submitted jobs under a default queue (like in the test cases above). I should make note of this in the commit message, and will push up a fix.
The plugin currently rejects jobs which submit a queue that the plugin does not know about. The changes proposed in flux-framework/flux-core#4627 enable the job-manager to be more aware of queues and instill queue validation. As a result of this, the plugin should relax some of its current enforcements on queue validation. Change the preprocessor directive NO_SUCH_QUEUE to UNKNOWN_QUEUE, an integer to act as a default factor of 0 when a queue is specified that flux-accounting does not know about. Change the preprocessor directive NO_DEFAULT_QUEUE to NO_QUEUE_SPECIFIED, an integer to act as a default factor of 0 when no queue is specified when an association submits a job. Add comments to each preprocessor directive to explain more clearly what each directive represents and is used for.
Remove the job rejection on a queue that flux-accounting does not know about or if there is no "default" queue. If an association submits a job to a queue that 1) flux-accounting knows about and 2) the user is not allowed to submit jobs to, the job can still be rejected.
Now that the plugin no longer rejects job when no "default" queue is configured, remove the "default" entry from the queues map when the plugin is loaded.
Add a comment to the queue_info struct to explain that the min_nodes_per_job, max_nodes_per_job, and max_time_per_job fields are not currently enforced in the plugin.
Change the tests in t1013-mf-priority-queues.t to account for the changes in validating queues in the multi-factor priority plugin.
Problem: The removal of the "default" queue entry in the priority plugin will change the priorities of jobs submitted under a default queue in the sharness tests, since the "default" entry was using a queue_factor of 1000 and the new UNSPECIFIED_QUEUE preprocessor directive uses a queue_factor of 0. Change the tests in t1018-mf-priority-disable-entry.t to account for the changes in validating queues and generating job priorities in the multi-factor priority plugin.
Codecov Report
@@ Coverage Diff @@
## master #281 +/- ##
==========================================
- Coverage 83.77% 83.75% -0.02%
==========================================
Files 23 23
Lines 1245 1225 -20
==========================================
- Hits 1043 1026 -17
+ Misses 202 199 -3
|
|
Thanks for reviewing and approving @grondo! I've force-pushed a couple of fixes based on your suggestions to both fix the commit messages and add some comments in the plugin to describe the preprocessor directives. I'll go ahead and set MWP shortly! |
Background
The multi-factor priority plugin currently rejects jobs which submit a queue that the plugin does
not know about and when the plugin does not have a configured "default" queue. The changes proposed in flux-framework/flux-core#4627
enable the flux-core job-manager to be more aware of queues and instill queue validation on its own.
As a result of this, the plugin should relax some of its current enforcements
on queue validation.
This PR relaxes some of its queue validation in 2 cases when an association submits a job:
In both of these cases, the plugin assign's a default factor of 0 to the overall job priority calculation. It changes a couple of the preprocessor directives the plugin was using to account for these validation changes. However, it is worth noting that the plugin still has the ability to reject jobs when an association submits a job to a queue they do not belong to.
For example, let's say that flux-accounting knows of 3 queues:
bronze,silver, andgoldand an association has access to both thebronzeandsilverqueues. If the association submits a job under thegoldqueue, the job will be rejected. If the same association submits a job under any other named queue that flux-accounting does not know about (e.g aspecialqueue), the plugin will assign a queue factor of 0, not positively or negatively affecting the priority of a job.It also removes the "default" queue entry in the
queuesmap of the plugin, since it no longer needs a "default" queue for jobs to be submitted under.As a result of the changes made to the plugin, a couple of changes also needed to be made to the sharness tests that deal with job rejection based on queue validation.
Fixes #280