plugin: record bank name to jobspec when user submits under default bank #301
Conversation
cmoussa1
force-pushed
the
issue#272
branch
2 times, most recently
from
67198fd to
4955c73
Compare
cmoussa1
changed the title
src/plugins/mf_priority.cpp
Outdated
| // post jobspec-update event | ||
| if (flux_jobtap_event_post_pack (p, FLUX_JOBTAP_CURRENT_JOB, | ||
| "jobspec-update", "O", | ||
| bank_name) < 0) { | ||
| json_decref (bank_name); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
I think you can use the new flux_jobtap_jobspec_update_pack() now instead.
t/t1028-mf-priority-bank.t
Outdated
| test_expect_success 'submit a job before plugin has any flux-accounting information' ' | ||
| job4=$(flux python ${SUBMIT_AS} 1002 hostname) && | ||
| test $(flux jobs -no {state} ${job4}) = PRIORITY |
There was a problem hiding this comment.
did you re-order the tests? Just noticed job4 variable used before job1,2,3 :-)
There was a problem hiding this comment.
d'oh 🤦 you're right; i re-ordered these and didn't re-number them. Thanks for pointing this out! I just went ahead and removed the numbers altogether from the jobid variable.
Problem: If an association submits a job under a default bank, there is no record of what the default bank is in the jobspec or eventlog of the job. Add the bank name via jobspec-update under attributes.system.bank during job.new in the case where a user is submitting a job under their default bank. In the case where a user submits a job before any flux-accounting info is sent to the plugin, their job entry will be associated with the special "DNE" bank until their information is updated. In this case, once the information is updated and the submitted job is once again going through job.state.priority, add the jobspec-update call inside the if-else branch where the bank_info struct for the job is being updated.
|
|
||
| test_expect_success 'submit a job before plugin has any flux-accounting information' ' | ||
| jobid=$(flux python ${SUBMIT_AS} 1002 hostname) && | ||
| flux job info $jobid eventlog > eventlog.out && |
There was a problem hiding this comment.
I think there is a race condition here because the job may not have reached the DEPEND state by the time flux job info is run. Maybe flux job wait-event -vt 60 $jobid depend would be better here? (Sorry if I originally had suggested flux job info!)
I'm not actually sure how to definitively test that a job stays in DEPEND state though.. so maybe right after the wait-event you can dump the eventlog and check again like you have here now?
There was a problem hiding this comment.
Thanks for this suggestion! Sorry that I was using the wrong method to check for the job state here. I just re-ran this set of tests (both under and not under valgrind) and using flux job wait-event seemed to work. I can then check the eventlog afterwards and look for the "depend" event. I've just pushed up this change. Let me know if it looks better!
On a side note, I should probably step through the test suite tomorrow (like you mentioned in #294) and change the tests that test for job states to use the suggestion above. I'll open a separate issue on that.
Problem: There are no tests for checking successful updates of a user's default bank being added to jobspec when submitting jobs under a default bank. Add some tests.
Codecov Report
@@ Coverage Diff @@
## master #301 +/- ##
==========================================
- Coverage 81.40% 81.23% -0.18%
==========================================
Files 20 20
Lines 1366 1380 +14
==========================================
+ Hits 1112 1121 +9
- Misses 254 259 +5
|
|
Thanks! I believe I've fixed the test as suggested in the review above. I'll set MWP on this 👍 |
cmoussa1
changed the title
Problem
Described in #272, currently, when an association submits a job under their default bank, the bank name is not recorded anywhere in the jobspec. Only when an association specifies the bank they want to submit jobs under (i.e
flux mini submit --setattr=system.bank=my_bank) does the bank name get recorded. It was proposed in that issue to record the bank name in the PRIORITY event of a job even when a user does not specify a bank when they submit a job (i.e they use their default bank).This PR records the bank name in the jobspec during the PRIORITY event when an association submits a job under their default bank. It makes use of the
jobspec-updateevent to post thebankkey-value pair. This event takes place just before the integer priority for the job is calculated. This event should take place if the association was not known to the flux-accounting DB/priority plugin at the time of job submission; in the latter case, when all pending jobs are reprioritized after a plugin update, the default bank is looked up and then added to the jobspec.In the case where the default bank is known at the time of job submission, it adds the bank name via
jobspec-updateunderattributes.system.bankduringjob.new.Some sharness tests are also added to a new test file,
t1029-mf-priority-bank.t, that test this functionality, making sure the bank name can be found in the eventlog of a job when jobs are submitted under a default bank as well as when an association does not yet exist in the flux-accounting DB.