Skip to content

[core] Back off failed placement group recovery retries - #65970

Open
hikaru-212 wants to merge 1 commit into
ray-project:masterfrom
hikaru-212:fix/65147-placement-group-recovery-backoff
Open

hikaru-212 wants to merge 1 commit into
ray-project:masterfrom
hikaru-212:fix/65147-placement-group-recovery-backoff

Conversation

@hikaru-212

Copy link
Copy Markdown

Description

This PR addresses a scheduler-liveness issue observed while investigating #65147.

Fresh placement-group recovery triggered by node loss is intentionally scheduled at the highest priority. However, when a feasible RESCHEDULING attempt fails, the current path requeues the placement group at rank 0 again with fresh retry state.

Repeated recovery failures can therefore continue taking the highest-priority scheduling turn and prevent unrelated, already-due pending placement groups from reaching the scheduler.

This change preserves rank 0 for the initial recovery attempt, but reuses the existing ExponentialBackoff after a feasible RESCHEDULING attempt fails.

As a result:

  • fresh recovery still receives highest priority;
  • a failed recovery retry is delayed using the existing backoff;
  • unrelated due placement groups can receive scheduling turns during that cooldown;
  • recovery retries again when its delay expires;
  • the backoff state is preserved across subsequent failures rather than being reset.

The focused regression exercises the full manager-level transition from a successfully created placement group through node loss and RESCHEDULING. It verifies that the initial recovery attempt wins over a normal pending placement group, but after recovery fails, the pending placement group receives a scheduling attempt while recovery is in backoff.

The same regression also verifies that recovery becomes eligible again when its retry time is reached and that a subsequent failure receives a larger retry delay.

This does not resolve the broader complementary partial-placement-group resource cycle described in #65147. It does not introduce victim selection, disruption, cycle detection, or a rescheduler policy.

Related issues

Related to #65147

Additional information

The focused regression was validated as a RED → GREEN test:

  • before the production change, the regression failed because the failed RESCHEDULING attempt still had highest_retry_delay_ms == 0;
  • after the change, the same regression passed with the expected initial retry delay and subsequent exponential backoff.

Local validation:

  • GcsPlacementGroupManagerMockTest.PendingQueuePriorityReschedule: 10/10 runs passed.
  • Relevant GCS placement-group manager and scheduler C++ test targets passed.
  • Source-built placement-group failover validation passed:
    • partial placement-group recovery;
    • GCS restart during placement-group recovery;
    • complete test_placement_group_failover target: 6 tests passed.

For the source-built validation, the Ray Python package and native artifacts were generated from the corresponding checkout. The tests used the checkout-specific GCS server, raylet, _raylet, and Redis artifacts.

@hikaru-212
hikaru-212 marked this pull request as ready for review September 10, 2026 16:34
@hikaru-212
hikaru-212 requested a review from a team as a code owner September 10, 2026 16:34

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the placement group rescheduling logic in GcsPlacementGroupManager to apply a backoff delay to failed recovery retries instead of always scheduling them with the highest priority (rank 0). This prevents failed retries from starving other pending placement groups. Additionally, the test suite is updated to thoroughly verify this backoff behavior and ensure that normal placement groups can be scheduled during a recovery cooldown. I have no feedback to provide.

@ray-gardener ray-gardener Bot added core Issues that should be addressed in Ray Core community-contribution Contributed by the community labels Sep 10, 2026
@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@github-actions github-actions Bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Sep 26, 2026
Signed-off-by: Yen-Hua Chen <226400984+hikaru-212@users.noreply.github.com>
@hikaru-212
hikaru-212 force-pushed the fix/65147-placement-group-recovery-backoff branch from 676def3 to cd65fc9 Compare September 26, 2026 13:36
@github-actions github-actions Bot added unstale A PR that has been marked unstale. It will not get marked stale again if this label is on it. and removed stale The issue is stale. It will be closed within 7 days unless there are further conversation labels Sep 27, 2026
@dancingactor

Copy link
Copy Markdown
Contributor

Thanks for the contribution! But I think the original code was designed this way on purpose: always letting the rescheduled PG be scheduled with top priority

Your change does let other PGs get scheduled, but it also makes the rescheduled PG less likely to get scheduled, so it's actually a tradeoff

@hikaru-212

Copy link
Copy Markdown
Author

Thanks, I agree this is a tradeoff.
My intent is to preserve top priority for the fresh recovery attempt, while applying backoff only after a feasible RESCHEDULING attempt has already failed.
The question I’m trying to clarify is whether Ray intends strict top priority to continue across every failed retry, even when another due PG with independently available resources could otherwise make progress.
If that is the intended policy, then this PR is changing that policy rather than fixing unintended starvation.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Contributed by the community core Issues that should be addressed in Ray Core unstale A PR that has been marked unstale. It will not get marked stale again if this label is on it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants