cache R in the job manager #5472
Conversation
|
Oops one place where I made some sloppy errors caught by CI and will force push with fixes in a sec as soon as tests pass locally. |
|
Great!
A quick thought here - should the
All I was thinking was to add |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -177,8 +182,10 @@ | |||
| } | |||
| } | |||
|
|
|||
| static void start_response_cb (flux_t *h, flux_msg_handler_t *mh, | |||
| const flux_msg_t *msg, void *arg) | |||
| static void start_response_cb (flux_t *h, | |||
Check warning
Code scanning / CodeQL
Poorly documented large function Warning
I didn't check the code here yet, does the scheduler still commit R to the KVS and send it back in the alloc response in parallel, or does the job-manager now commit R? The alloc-bypass plugin has to commit the provided R to the KVS itself, but if the job-manager already does this on behalf of the scheduler, perhaps we just need a way to allow a plugin to assign R and then we can drop this step and the job manager will have the copy of R in this case. (Oh, wait, won't this be necessary anyway if R is sent to the job-exec module with the start request?) |
That was true before, but after this PR, the I'm not sure if fluxion relies on this. Maybe (R is passed to |
Yes. That seemed the safest since I think we've advertised the invariant that once the
It does not already do it so Edit: well actually that invariant could be maintained if we committed R from the job-manager, so that's not necessarily a reason to do it this way if we need to reconsider. |
All very true! However, it saves a KVS lookup at restart only, with a tradeoff of keeping JGF for every job (including inactive jobs) and we currently don't even support restart with running jobs (though I guess we do support reloading the scheduler). If the scheduler continued to lookup R in the KVS, but instead used the new job-info lookup which incorporates
True. Though it would presumably be easy to delay the alloc event in the job manager until the commit of R is complete. It actually seems fine to me do it this way for now though (probably faster too)
Yes, that is what I was proposing, a way for a plugin to assign R. Not sure what that would look like. Sounds like if we send R in the start request this will be necessary to keep alloc-bypass functionality though? |
True!
Good point, let's do it that way! Regarding the interface to R for jobtap plugins, I have a commit ready to push that adds an "R" key to the jobtap input args. I was going to try also adding "R" to the output args just for |
|
Re-pushed with the following changes
|
That's not available yet, correct? |
PR posted in #5467 - unfortunately I just realized that job-info lookup RPC still returns the original R because the application of the resource-update events is done on the client side in Edit: In a way it would be nice if |
apologies, there's actually two PRs so it may be confusing #5467 - an update-watch service that will get the currently up to date "R" (w/ resource-update events applied) and then stream any new #5464 -
There is no roadblock that I can see. The entirety of why it was done client side in |
|
This PR would be useful in development of duration update for running jobs. The job-manager would not have to fetch R from the job-info service to apply a duration update. A cursory examination of this PR indicates it is in pretty good shape. @garlick, what remains to do here to remove the WIP? (Did we ever decide if the cached R should be freed for inactive jobs? Given that we have users wanting run 1M jobs, perhaps that would be a good idea...) |
|
This has been sitting for a while. A brief review didn't turn up any big gaps, that I caught anyway. Just force pushed with the following changes:
|
|
Per offline discussion with @grondo, repushed with the R scheduling key redacted upon receipt. It's still in the KVS of course, and schedutil still looks it up there when the scheduler is reloaded. |
Problem: libschedutil code sometimes breaks long function parameter lists in blocks instead of one per line, which is inconsistent with the modern flux code base. Break function parameters one per line when the list cannot fit on one line (except json pack/unpack key-value pairs).
Problem: alloc responses use a common function that is not suited to adding one more optional parameter. Refactor alloc.c to use a more general "pack" style utility function.
Problem: schedutil_alloc_respond_success_pack() writes R to the KVS then responds to the job-manager without R as required by RFC 27, but in emerging cases, it may be handy to give jobtap plugins access to R. Include R in the alloc response.
Problem: resource_set_create uses a calloc() buffer without checking for NULL. Add check.
Problem: when R is passed to job-exec as a json object, it will be convenient to have some rset interfaces to handle R as a json object. Add resource_set_create_fromjson (). Add resource_set_get_json ().
Problem: there is no place to store R in the job manager job object. Add R to struct and call json_decref() from job destructor.
Problem: job_create_from_eventlog() does not accept R, but this will be required once R is tracked by the job-manager. Add R argument to job_create_from_eventlog(). This initializes job->R before the eventlog is replayed, for the eventual support of resource-update events. Update unit test.
Problem: the scheduler now returns R in the sched.alloc response, but the job manager ignores it. Accept R and make it part of the job object.
Problem: lookup_job() repeats a block of code for each KVS key it looks up, and adding one more raises the annoyance level. Create local job data lookup helpers to reduce code duplication.
Problem: R is expected to be part of the job-manager job object after allocation, but this is not the case for jobs loaded from the KVS after a job manager restart. Load R from the KVS, if available, and make it part of the job object.
Problem: there is not a simple way to access the job manager's in-memory copy of R for testing. Allow R to be fetched with the job-manager.getattr RPC.
Problem: there are no tests that show the job manager holds a copy of R. Add a sharness test. In the future it can cover resource-update changes to R.
Problem: job-manager/start.c sometimes breaks long function parameter lists in blocks instead of one per line, which is inconsistent with the modern flux code base. Break function parameters one per line when the list cannot fit on one line (except json pack/unpack key-value pairs).
Problem: jobtap_call() defines 'rc' both at function scope and within a local block. Rename the variable in the local block so they don't confuse anyone.
Problem: jobtap plugins may need access to R. Add an "R" key to the input args if R has been allocated.
Problem: alloc-bypass needs to need set job->R. Add an "R" key to the output args. If present in output args and preconditions are met, namely: - callback is job.state.sched - not already set in the job struct then set job->R_redacted to the argument value.
Problem: alloc-bypass represents R internally as a string for storage to the KVS, but soon we will need it in object form for adding to the plugin output args. Represent it internally as an object and use KVS interfaces that accept an object.
Problem: alloc-bypass now needs to set R in the output args so that the job manager's in-memory copy of R is set. Set "R" in the output args of the job.state.sched callback.
Problem: job-exec fetches R from the KVS but we have it now in the job manager so it could be included in the exec.start request. Add R to the exec.start request.
Problem: R is now present in the exec.start so job-exec no longer needs to look it up. Initialize job->R and critical ranks in the start request handler rather than later in a continuation.
Problem: a job exec test that fakes an invalid R by placing it in the KVS no longer works because R comes directly from the job manager. Remove the test for now.
Codecov Report
@@ Coverage Diff @@
## master #5472 +/- ##
==========================================
- Coverage 83.47% 83.42% -0.06%
==========================================
Files 487 487
Lines 81918 81990 +72
==========================================
+ Hits 68381 68398 +17
- Misses 13537 13592 +55
|
Problem: jobtap plugins might need convenient access to R per #5471
Per discussion, this prototypes keeping a copy of R in the job manager so that it could potentially be offered to jobtap plugins. Some notes/caveats:
sched.allocandjob-manager.sched-helloresponses.resource-updateevents on top of the original R loaded from the KVSjob-exec.startto save a lookup (would require an update to RFC 32). (Edit: oops I included a couple of commits that started this - maybe I'll just finish it and tack it on)