Bug #14116
openc-d-s reniceAll does not wait for complete information on Arvados jobs
Description
crunch-dispatch-slurm's reniceAll function begins changing slurm job nice values based on incomplete state - it has the full SLURM squeue output but takes action based on only partial Arvados job information (i.e. it does not wait for the dispatcher's API server updates to complete and therefore operates over a partial list of Arvados jobs, setting priority as if the rest of the jobs were not Arvados jobs).
Updated by Joshua Randall over 6 years ago
It would probably be safe to run something like reniceAll on a partial list of Arvados containers that Dispatcher.Run() is aware of, because they are retrieved from the API server in priority order, but as it is it does not seem to be safe to run it on a random set of containers whose trackers have happened to call SetPriority in time to make it into the batch.
If it weren't for the dispatcher code being in the SDK, reniceAll could be refactored so that it has its own goroutine triggered by the dispatcher and given an awareness of the list of jobs that the dispatcher knows about from the API server (which could be updated at the end of each time through the Dispatcher.Run() loop). It could then wait to make priority decisions until it gets an SqueueChecker update in the same way that the runContainer goroutines do.
Unfortunately the asynchronous nature of the interface between the dispatch code in the SDK and c-d-s itself makes this harder to accomplish - how is c-d-s supposed to know when it has a complete set of (at least the N highest priority) containers?
Can we add additional interfaces to dispatch? Perhaps a channel or a callback that gives the list of containers once that is known?
Alternatively, it seems like the dispatch SDK interface would be more straightforward and flexible if it used either a syncrhonous callback or channels to communicate with c-d-s rather than the RunContainer DispatchFunc callback function that it awkwardly arranges to run in a goroutine. It could basically just pass a `runTracker` out on a channel of trackers to start, and then c-d-s can have a goroutine reading from that channel and taking appropriate action (including launching a runContainer goroutine as well as this new functionality for triggering the setting of priority). The only complication there is that the dispatch SDK would also need a channel on which runContainer can notify it that it is done, so that the dispatcher can remove the tracker from the trackers map (as that is currently done with cleanup code in the same goroutine that calls RunContainer), but that seems like a minor thing.