[ML] Avoid ModelAssignment deadlock #109684
Conversation
The model loading scheduled thread iterates through the model queue and deploys each model. Rather than block and wait on each deployment, the thread will attach a listener that will either iterate to the next model (if one is in the queue) or reschedule the thread. This change should not impact: 1. the iterative nature of the model deployment process - each model is still deployed one at a time, and no additional threads are consumed per model. 2. the 1s delay between model deployment tries - if a deployment fails but can be retried, the retry is added to the next batch of models that are consumed after the 1s scheduled delay. Relate elastic#109134
|
@elasticmachine update branch |
|
Pinging @elastic/ml-core (Team:ML) |
|
Hi @prwhelan, I've created a changelog YAML for you. |
| // if someone calls stop halfway through, abandon this entire chain | ||
| var loadingToRetry = new ConcurrentLinkedDeque<TrainedModelDeploymentTask>(); | ||
| var deploymentChain = SubscribableListener.<Void>newSucceeded(null); | ||
| while (loadingModels.isEmpty() == false) { |
There was a problem hiding this comment.
Would it simplify the code to remove the while() loop in favour of just taking the head of the queue and loading the next model on the next scheduled invocation?
var loadingTask = loadingModels.poll();
if (loadingTask == null) {
onFinish.run();
return;
}
This function loadQueuedModels() will be called again periodically anyway so it's an option to let next invocation handle the next model. Would an error loading 1 model fail any other models in this chain?
In practice there are only a small number of models per ml node due to the memory and CPU demands. In an autoscaling situation when an ml node joins the cluster it may have to load 1 or 2 models, certainly not 10s of models.
There was a problem hiding this comment.
Yes I would very much like to simplify this, if we're okay waiting for ~1s between iterations, I'd be happy to remove the while loop.
Now that I look at it, we could even just schedule immediately if the queue is not empty?
Would an error loading 1 model fail any other models in this chain?
No, because it squelches the error when it calls listener.onResponse so the next iteration will run.
| private static <T> ActionListener<T> thenRun(Runnable runnable) { | ||
| return ActionListener.runAfter(ActionListener.noop(), runnable); |
There was a problem hiding this comment.
Why not ActionListener.running()?
There was a problem hiding this comment.
We should swap to it - initially I had this as runBefore so the runnable could safely throw an exception, then I thought failing silently might be confusing and swapped to runAfter without thinking that is effectively ActionListener.running()
| // don't bother calling the listener, the lifecycle will not resume the instance of this class | ||
| return; |
There was a problem hiding this comment.
I'd sorta recommend just completing the chain of listeners in this case unless there's a strong argument (e.g. performance) not to do so, and then putting a stopped check in the final listener too. Leaking listeners is a source of super-painful bugs, even at shutdown.
.../java/org/elasticsearch/xpack/ml/inference/assignment/TrainedModelAssignmentNodeService.java
Outdated
Show resolved
Hide resolved
…rence/assignment/TrainedModelAssignmentNodeService.java Co-authored-by: David Kyle <[email protected]>
|
@elasticmachine update branch |
… because some users report 1k+ models in a single digit cluster size so startup would take ~3m
The model loading scheduled thread iterates through the model queue and deploys each model. Rather than block and wait on each deployment, the thread will attach a listener that will either iterate to the next model (if one is in the queue) or reschedule the thread.
This change should not impact:
Relate #109134