Repository navigation
[core] Stop writing PG formatted resources to the GCS cluster resource view - #66061
Conversation
…uster resource view Signed-off-by: dancingactor <s990346@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request removes the CommitBundleResources method and its helper ToNodeBundleResourcesMap from GcsPlacementGroupScheduler. The review feedback suggests further cleaning up the codebase by removing the now-unused commited_bundle_locations map and its population logic to avoid unnecessary overhead, as well as removing the IsPlacementGroupWildcardResource helper method and its corresponding unit tests since they have become dead code.
I am having trouble creating individual review comments. Click here to see my feedback.
src/ray/gcs/gcs_placement_group_scheduler.cc (386-391)
With the removal of CommitBundleResources, the commited_bundle_locations map is now populated but never used. We should clean up this unused variable and the loop that populates it to avoid unnecessary memory allocation and overhead.
Here is how the lambda can be simplified:
auto commit_resources_callback = [this,
lease_status_tracker,
bundles_per_node,
node_id,
schedule_failure_handler,
schedule_success_handler](const Status &status) {
for (const auto &bundle : bundles_per_node) {
lease_status_tracker->MarkCommitRequestReturned(node_id, bundle, status);
}
if (lease_status_tracker->AllCommitRequestReturned()) {
OnAllBundleCommitRequestReturned(
lease_status_tracker, schedule_failure_handler, schedule_success_handler);
}
};src/ray/gcs/gcs_placement_group_scheduler.cc (761-762)
Since CommitBundleResources has been removed, the helper method IsPlacementGroupWildcardResource is now dead code and no longer used anywhere in the class (except for its unit test TestCheckingWildcardResource).
Consider removing IsPlacementGroupWildcardResource from both the source and header files, along with its corresponding unit test, to keep the codebase clean and maintainable.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit ee855a0. Configure here.
Yicheng-Lu-llll
left a comment
There was a problem hiding this comment.
LGTM! just one nit: please also drop the unused commited_bundle_locations IsPlacementGroupWildcardResource etc.
Signed-off-by: dancingactor <s990346@gmail.com>
|
@Yicheng-Lu-llll Thanks for review! I've already removed that unused code as well |
Signed-off-by: dancingactor <s990346@gmail.com>
Signed-off-by: dancingactor <s990346@gmail.com>
Signed-off-by: dancingactor <s990346@gmail.com>
…e view (ray-project#66061) ## Description After ray-project#63723 removed GCS-side PG resource restoration on scheduling failures and switched to relying on Raylet resource view broadcasts, GCS no longer needs to store PG-formatted resources in its cluster resource view. Originally, these entries served two purposes: * **PG destruction cleanup:** Determining which resources to restore when a PG is destroyed. This was already removed in ray-project#63723. * **Commit phase partial failure:** As noted in the https://github.com/ray-project/ray/blob/f02bd2216a05c1c60f4d3776dac2ba53beebff9a/src/ray/gcs/gcs_placement_group_scheduler.cc#L501-L502 this now also relies on Raylet sync to reconcile the GCS resource view. Since neither path uses these entries anymore, this PR removes PG-formatted resources from the GCS cluster resource view. ## Related PR Related to ray-project#63723 --------- Signed-off-by: dancingactor <s990346@gmail.com> Co-authored-by: Yicheng-Lu-llll <51814063+Yicheng-Lu-llll@users.noreply.github.com>
## Why are these changes needed? #43405 fixed a GCS crash (#43371) when a placement group's commit phase raced with a node dying. The crash came from the GCS-side `GcsPlacementGroupScheduler::CommitBundleResources`, which looked up the dead node in the cluster resource view. #66061 has since removed that function, so the crash can't happen anymore, but the underlying race is still there. When a raylet receives SIGTERM (for example, a spot or preemptible node being reclaimed), its signal handler ([`src/ray/raylet/main.cc`](<permalink>)) sends `UnregisterNode` to the GCS. The raylet only stops its io_context once that RPC replies, so until then it keeps serving RPCs. That includes replying OK to a `CommitBundleResources` request. On the GCS, the commit reply and the `UnregisterNode` request arrive on different threads, and nothing orders them. If `UnregisterNode` is handled first: 1. The node is removed and `GcsPlacementGroupManager::OnNodeDead` runs. It only reschedules bundles in `committed_bundle_location_index_`. Bundles from an in-progress commit aren't added there until every commit reply has returned, so it skips them. 2. The OK commit reply then arrives, and the placement group is marked CREATED with a bundle on the dead node. `pg.ready()` resolves. Because `OnNodeDead` already ran, the bundle is never rescheduled. This isn't limited to the SIGTERM race. With multiple nodes, a node can reply OK to its commit and then die for any reason while another node's commit is still in flight. Its bundle isn't indexed until the last reply returns, so OnNodeDead misses it in the same way. ## What does this PR do? In `OnAllBundleCommitRequestReturned`, any prepared bundle whose node is no longer alive (checked with `GcsNodeManager::GetAliveNode`) is marked uncommitted and left out of both bundle location indexes. The existing failure path then clears the bundle's `node_id` and reschedules it. If `OnNodeDead` runs after this check, there's no gap. `GcsNodeManager` removes the node from its alive map before posting the node-removed listeners. So if the check sees the node as alive, `OnNodeDead` hasn't run yet, and it will find the bundle in the index and reschedule it. `GetAliveNode` takes the node manager's reader lock. It's called once per prepared bundle when a scheduling attempt's last commit reply returns. ## Alternatives considered I considered having `OnNodeDead` add the dead node's in-progress bundles to `uncommitted_bundle_locations_`, instead of querying the node manager. That runs into an existing indexing bug: - When a commit fails, `OnAllBundleCommitRequestReturned` still adds the bundle to `committed_bundle_location_index_` under its old node, and `AddBundleLocations` doesn't overwrite existing entries. After the bundle is rescheduled to a new node, the index still points at the old one. Removing the placement group then never releases the bundle on the new node. - If dead-node bundles are only marked uncommitted, they get indexed under the dead node after its cleanup has already run, and they hit the same leak. - Leaving all uncommitted bundles out of the index avoids that. But the uncommitted set also holds commit failures on live nodes, such as a transient RPC error. Those raylets still hold the prepared reservation, and the index is the only thing that later releases it, so the reservation would leak there instead. Fixing the index for live-node commit failures also requires releasing a single bundle on a live node. Today the raylet's `RemovePlacementGroupBundles` kills every worker of the placement group on that node. I'm leaving that for a follow-up, so this PR doesn't change behavior for live-node commit failures. --------- Signed-off-by: Joshua Lee <joshlee@anyscale.com>

Description
After #63723 removed GCS-side PG resource restoration on scheduling failures and switched to relying on Raylet resource view broadcasts, GCS no longer needs to store PG-formatted resources in its cluster resource view.
Originally, these entries served two purposes:
ray/src/ray/gcs/gcs_placement_group_scheduler.cc
Lines 501 to 502 in f02bd22
Since neither path uses these entries anymore, this PR removes PG-formatted resources from the GCS cluster resource view.
Related PR
Related to #63723