[Enable gcs actor scheduler 1/n] Raylet and GCS schedulers share cluster_task_manager #23829
Conversation
rkooo567
added
the
@author-action-required
|
@iycheng @scv119 Do you have any comments? |
|
@Chong-Li thanks, the PR looks very good. I'll take a detailed review soon. Meanwhile the test failure looks related to this PR. |
| @@ -115,7 +107,7 @@ scheduling::NodeID ClusterResourceScheduler::GetBestSchedulableNode( | |||
| bool *is_infeasible) { | |||
| // The zero cpu actor is a special case that must be handled the same way by all | |||
| // scheduling policies. | |||
| if (actor_creation && resource_request.IsEmpty()) { | |||
There was a problem hiding this comment.
I feel we can decouple this change from this PR, seems unrelated?
Sure, I'm going to handle this together with the issue mentioned above (fixing exclude_local_node parameter).
| false); | ||
| cluster_resource_manager.AddNodeAvailableResources( | ||
| scheduling::NodeID(actor->GetNodeID().Binary()), acquired_resources); | ||
| cluster_task_manager_->ScheduleAndDispatchTasks(); |
There was a problem hiding this comment.
should we call normal_task_resources_changed_callback_ here? also should we create a helper function if this block is same as HandleWorkerLeaseRejectedReply ?
There was a problem hiding this comment.
should we call
normal_task_resources_changed_callback_here?
The actor's resources have nothing to do with normal_task_resources, so we don't need to call the callback.
also should we create a helper function if this block is same as
HandleWorkerLeaseRejectedReply?
Sure, just did.
|
let's wait for @jjyao's review as well! |
| @@ -115,7 +107,7 @@ scheduling::NodeID ClusterResourceScheduler::GetBestSchedulableNode( | |||
| bool *is_infeasible) { | |||
| // The zero cpu actor is a special case that must be handled the same way by all | |||
| // scheduling policies. | |||
| if (actor_creation && resource_request.IsEmpty()) { | |||
There was a problem hiding this comment.
I feel we can decouple this change from this PR, seems unrelated?
Sure, I'm going to handle this together with the issue mentioned above (fixing exclude_local_node parameter).
grant_or_reject for raylet based actor scheduling is implemented as part of #23829, so spread scheduling now works for actors just like tasks.
Why are these changes needed?
As discussed in #23460, the long-term plan to enable gcs scheduler is as follows:
Remove RayletBasedActorScheduler and GcsBasedActorScheduler. Instead, handling different actor requests (willing to be scheduled at raylet or gcs) in GcsActorScheduler.
GcsActorScheduler calls ClusterTaskManager, and then ClusterTaskManager calls ClusterResourceScheduler to select node. Besides, because ClusterTaskManager maintains the infeasible and ready queues, the pending queue in GcsActorManager should be removed. After this change, the GcsActorScheduler only manages the lifecycle of leasing (send/cancel lease and push creation task), while the queueing and scheduling (this PR is actually doing the same thing in a quicker way) are handled by ClusterTaskManager. If the leftover responsibility of GcsActorScheduler is simple enough, we may even remove it and reimplement the functions in GcsActorManager.
At raylets, the lease requests (that have already been handled by gcs' ClusterTaskManager) can skip the ClusterTaskManager and directly go to the LocalTaskManager.
Turn on the gcs scheduling feature flag and resolve any leftover failures.
This PR unifies
RayletBasedActorSchedulerandGcsBasedActorSchedulerintoGcsActorScheduler, which relies onClusterTaskManagerfor queueing and scheduling.The key function is
GcsActorScheduler::Schedule(), in which we decide whether scheduling at GCS, or forwarding the request to a remote Raylet (owner node in most cases) for scheduling.Related issue number
#23460
Checks
scripts/format.shto lint the changes in this PR.