plugin: add callback specific for validating an updated queue #399
Conversation
cmoussa1
added
improvement
src/plugins/mf_priority.cpp
Outdated
| "{s:i, s:s, s?s}", | ||
| "userid", &userid, | ||
| "value", &queue, | ||
| "attributes.system.bank", &bank) < 0) |
There was a problem hiding this comment.
I do not think there will be a key called attributes.system.bank in the args to the job.update.attributes.system.queue callback.
Do you mean to be unpacking the bank from the jobspec? Then you have to unpack like in line 1092 above, e.g. something like:
flux_plugin_arg_unpack (args,
FLUX_PLUGIN_ARG_IN,
"{s{s{s{s?s, s?s}}}}",
"jobspec", "attributes", "system", "bank", &bank);However, I'm curious why tests are passing with this code, so maybe you saw different behavior here?
There was a problem hiding this comment.
Oops! Sorry about that. Yes, I do mean to be unpacking the bank from the jobspec - I think I thought I could shorthand the traditional way of unpacking it from jobspec, just like you suggested above. Since the tests were passing, I figured this was okay too...
Perhaps the reason it passes with this is because in this set of tests, the user is submitting their job under a default bank, and thus flux_jobtap_jobspec_update_pack () is being called to add the bank to the main eventlog?
Either way, I can fix this to unpack it like how I always have.
There was a problem hiding this comment.
Maybe also look into adding a test that exercises this code when the user sets a non-default bank? Just in case :-)
There was a problem hiding this comment.
sure thing! i can add another set of tests to t1030 that basically repeats the same workflow, but under a non-default bank. i'll push up a commit to this branch if that's okay.
EDIT: default --> non-default
There was a problem hiding this comment.
btw, i was curious, so i went back and tried submitting a job under a non-default bank and using what i had originally proposed in the callback for job.update.attributes.system.queue:
"{s:i, s:s, s?s}",
"userid", &userid,
"value", &queue,
"attributes.system.bank", &bank) < 0)
and the tests failed. So thanks for catching that!
cmoussa1
force-pushed
the
plugin.queue.updates
branch
from
6be59f7 to
7661d5a
Compare
|
Thanks! Setting MWP here |
Problem: The multi-factor priority plugin uses the same callback for validating an updated queue and for actually updating the queue on a user/bank's job. This will be an issue when additional "updateable" attributes are added to the plugin, such as banks. Add a new callback to the plugin, update_queue_cb (), that is responsible for validating an updated queue and rejects the update if it is not valid for the user/bank. Assign the callback for the "job.update.attributes.system.queue" topic string. As a result of adding the new update_queue_cb () callback, change the job_updated () callback to unpack the "updates" JSON object when unpacking arguments and see if there is an update for the queue attribute. Check its contents for NULL before applying its update to the job that requested the queue update. Add a comment at the top of the job_updated () function with a description of what it does.
Problem: The priority plugin prefixes the error message for an invalid queue with "mf_priority:", which is not included in the "grep" check in t1030-mf-priority-update-queue.t. Update the error message to include this prefix.
Problem: t1030-mf-priority-update-queue.t has no tests that test submitting a pending job under a non-default bank and changing its queue. Add some tests that do this.
cmoussa1
force-pushed
the
plugin.queue.updates
branch
from
4f95abc to
a14133a
Compare
Codecov Report
@@ Coverage Diff @@
## master #399 +/- ##
==========================================
- Coverage 81.04% 81.00% -0.05%
==========================================
Files 20 20
Lines 1419 1437 +18
==========================================
+ Hits 1150 1164 +14
- Misses 269 273 +4
|
Problem
The multi-factor priority plugin uses the same callback for both validating an updated queue and for actually updating the queue on a user/bank's job. This will be an issue when additional "updateable" attributes are added to the plugin, such as banks.
This PR adds a new callback to the plugin,
update_queue_cb (), which is responsible for validating an updated queue and will reject the update if it is not valid for the user/bank. It assigns this callback to the"job.update.attributes.system.queue"topic string.As a result of adding the new
update_queue_cb ()callback, thejob_updated ()callback is also changed to unpack the "updates" JSON object when unpacking arguments and see if there is an update for thequeueattribute. It checks its contents and applies its update to the job that requested the queue update.Lastly, I've added comments at the top of both functions with a short description (mostly so I don't forget how this flow of updating job attributes works in the future 😆).