[core] Add PREPARED state for placement groups. #46858

rynewang · 2024-07-29T23:48:42Z

Ray Placement Groups use 2pc protocol to reserve resources. However we don't track PREPARED state; we only had PENDING (before any prepare) and CREATED (after all committed). This means when the GCS restarts, it does not know any Commits ever happened. Indeed, it instructs all raylets to return all bundles prepared or committed.

If the GCS restart happens when a PG has finished all Prepare and is doing Commit, the already-Committed bundles may have scheduled actors/tasks, and those actors/tasks may be killed by the GCS restart, making the GCS restart user-visible.

This PR introduces a PREPARED state between PENDING and CREATED. Rules:

(New!) On a PG received all Prepare replies successfully: state change PENDING -> PREPARED, and persist to Redis. This also persists all bundle locations (node IDs).
After the persist: send all Commit calls.
On a PG received all Commit replies successfully: state change PREPARED -> CREATED, and persist to Redis.
On GCS Restart, if a PG is PENDING: return any bundles, reschedule from zero.
(New!) On GCS Restart, if a PG is PREPARED, continue to send all Commit calls.

In order to do this cleanly, did a bunch of refactoring. The idea is to keep all interface changes to the GcsPlacementGroupScheduler but not to GcsPlacementGroupManager. Specifically:

GcsPlacementGroupScheduler::ScheduleUnplacedBundles now accepts an aggregated arg struct SchedulePgRequest.
GcsPlacementGroupScheduler::Initialize accepts one more vector of arg struct SchedulePreparedPgRequest, which is the regular SchedulePgRequest plus the prepared bundle information.
GcsPlacementGroupManager::Initialize now distinguishes PREPARED PGs by creating requests and send them to the Scheduler. They are also kept in used_bundles to restrain from being removed (<- actual fix).
Unit test for the original issue. Failing on master and passing on this PR.
Quality of life changes: RAY_LOG().WithField(PlacementGroupID).

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

src/ray/gcs/gcs_server/gcs_placement_group_scheduler.cc

src/ray/gcs/gcs_server/gcs_placement_group_scheduler.h

jjyao · 2024-07-30T17:28:32Z

Do we have tests that during recommitting the prepared bundles, some recommits failed and the PG goes to RESCHEDULING state?

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-07-30T22:18:26Z

Do we have tests that during recommitting the prepared bundles, some recommits failed and the PG goes to RESCHEDULING state?

updated python test.

jjyao

lgtm.

In the future, we can refactor the code to be more reconciliation style.

Please wait for @rkooo567's approval before merging.

jjyao · 2024-07-31T15:51:56Z

python/ray/tests/test_placement_group_failover.py

+        # 5.2. Kill the slow committing node.
+        cluster.remove_node(bad_node)
+
+    time.sleep(1)


can we get rid of time.sleep using wait_for_condition etc?

rynewang · 2024-08-20T21:22:42Z

@rkooo567 can you take a look on this one? if you are ok with it pls merge

rkooo567 · 2024-08-20T21:35:14Z

let me try this by today. If it is urgent, please merge it without my approval (if I don't review tonight)

rkooo567

WIP review. I think the motivation makes a lot of sense. One thing I'd like to be careful is to handle edge cases in removing pg (I will need time to review this part of code one more time..). In the meantime, can you run a release test?

stress_test_placement_group
pg_autoscaling_regression_test
placement_group_performance_test

I think if stress_test_placement_group passes, I don't worry much about handling removal of pg after introducing a new state

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

rkooo567

Looks pretty good! Just a couple comments on unit tests; AFter that, let's merge

Can you make sure we handle removing placement group is handled properly (add unit tests) if it is removed after redis.Put is called (and before it is returned)? I barely remembered we don't remove pg that's in progress to simplify the protocol, but I am not 100% sure if it is still the case.
CreatePrepared
cpp tests in gcs placement group manager (if exists)?

rkooo567 · 2024-08-21T20:55:56Z

python/ray/tests/test_placement_group_failover.py

+        # 5.2. Kill the slow committing node.
+        cluster.remove_node(bad_node)
+
+    time.sleep(1)


src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

rkooo567 · 2024-08-22T00:13:32Z

src/ray/gcs/gcs_server/gcs_placement_group_scheduler.cc

+      placement_group->GetPlacementGroupTableData(),
+      [this, lease_status_tracker, schedule_failure_handler, schedule_success_handler](
+          Status status) {
+        RAY_CHECK_OK(status);


I think it may be possible the pg is removed before this Put is replied (since it is async). Can you add an unit test here? I.e., Update redis -> remove pg -> make sure we don't send commit bundle requests

src/ray/gcs/gcs_server/gcs_placement_group_scheduler.h

rkooo567 · 2024-08-22T00:24:38Z

src/ray/gcs/gcs_server/gcs_placement_group_scheduler.h

@@ -346,10 +354,12 @@ class GcsPlacementGroupScheduler : public GcsPlacementGroupSchedulerInterface {
  /// This should be called when GCS server restarts after a failure.
  ///
  /// \param node_to_bundles Bundles used by each node.


can you update this docstring? seems to be incorrect

This is still correct

rynewang · 2024-08-22T18:21:06Z

So the 3 release tests passed: https://buildkite.com/ray-project/release/builds/20250

Co-authored-by: SangBin Cho <rkooo567@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

rkooo567

let's merge it after adding remove unit tests to cpp (remove happens after Put(PREPARED) is sent and before it is returned). I think there's no race condition now given stress tests pass but just in case

rynewang · 2024-08-22T21:29:09Z

Just to be safe I plan to add a "is cancelled?" check after the put (in the put callback).

rkooo567 · 2024-08-22T21:32:51Z

that sounds good. or some kind of assert that it is not removed if we don't remove pg while it is scheduled (I think this was the case last time)

rynewang · 2024-08-22T22:14:02Z

For the Put-then-cancel case it should already work: the pg is committed, and after all-committed the scheduler finds it marked cancelled and cancel it. But sure I added a TOCTOU check before doing any committing, and destroy it. this should prevent any tasks/actors from starting.

rynewang · 2024-08-22T22:14:37Z

wydm "we don't remove pg while it is scheduled"? I think user can always cancel it at any time?

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-08-22T22:49:59Z

@rkooo567 cpp test added, "is it cancelled?" check before starting any commit added

rkooo567 · 2024-08-22T23:37:32Z

let's merge!

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

can-anyscale · 2024-08-23T23:11:11Z

this is broken window builds https://buildkite.com/ray-project/postmerge/builds/5995#_, i'm reverting to unblock @rynewang

This reverts commit 2897f46.

Reverts #46858 To unblock window builds https://buildkite.com/ray-project/postmerge/builds/5995#_

rynewang added 2 commits July 29, 2024 16:34

[core] Add PREPARED state for placement groups.

2939e9a

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

reorder enum

ab6f72a

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang requested review from SongGuyang, raulchen, kfstorm, a team, ericl and pcmoritz as code owners July 29, 2024 23:48

rynewang assigned jjyao Jul 29, 2024

fix field order

b25d7c0

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang added the go add ONLY when ready to merge, run all tests label Jul 30, 2024

jjyao reviewed Jul 30, 2024

View reviewed changes

src/ray/gcs/gcs_server/gcs_placement_group_scheduler.cc Show resolved Hide resolved

src/ray/gcs/gcs_server/gcs_placement_group_scheduler.h Outdated Show resolved Hide resolved

src/ray/gcs/gcs_server/gcs_placement_group_scheduler.h Outdated Show resolved Hide resolved

rkooo567 self-assigned this Jul 30, 2024

remove useless struct; fix cpp tests; add -> RESCHEDULING test

b10c150

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

jjyao approved these changes Jul 31, 2024

View reviewed changes

rkooo567 reviewed Aug 21, 2024

View reviewed changes

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc Outdated Show resolved Hide resolved

rkooo567 reviewed Aug 22, 2024

View reviewed changes

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 22, 2024

rynewang and others added 4 commits August 22, 2024 11:45

Update src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

fa9dab3

Co-authored-by: SangBin Cho <rkooo567@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

Update src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

0fafa1c

Co-authored-by: SangBin Cho <rkooo567@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

Update src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

e7a34ee

Co-authored-by: SangBin Cho <rkooo567@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

Update src/ray/gcs/gcs_server/gcs_placement_group_scheduler.h

d348ca5

Co-authored-by: SangBin Cho <rkooo567@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

rkooo567 approved these changes Aug 22, 2024

View reviewed changes

fixes, add cpp test

01e6125

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 22, 2024

rkooo567 enabled auto-merge (squash) August 22, 2024 23:37

fix cpp

15c430a

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

github-actions bot disabled auto-merge August 23, 2024 17:34

Merge branch 'master' into prepared-pg

cec311f

rynewang enabled auto-merge (squash) August 23, 2024 17:34

rynewang merged commit 2897f46 into ray-project:master Aug 23, 2024
6 checks passed

rynewang deleted the prepared-pg branch August 23, 2024 20:12

can-anyscale added a commit that referenced this pull request Aug 23, 2024

Revert "[core] Add PREPARED state for placement groups. (#46858)"

3a30641

This reverts commit 2897f46.

can-anyscale mentioned this pull request Aug 23, 2024

Revert "[core] Add PREPARED state for placement groups." #47307

Merged

can-anyscale added a commit that referenced this pull request Aug 24, 2024

Revert "[core] Add PREPARED state for placement groups." (#47307)

36e9d03

Reverts #46858 To unblock window builds https://buildkite.com/ray-project/postmerge/builds/5995#_

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Add PREPARED state for placement groups. #46858

[core] Add PREPARED state for placement groups. #46858

rynewang commented Jul 29, 2024

jjyao commented Jul 30, 2024

rynewang commented Jul 30, 2024

jjyao left a comment

jjyao Jul 31, 2024

rkooo567 Aug 21, 2024

rynewang commented Aug 20, 2024

rkooo567 commented Aug 20, 2024 •

edited

Loading

rkooo567 left a comment

rkooo567 left a comment

rkooo567 Aug 21, 2024

rkooo567 Aug 22, 2024

rkooo567 Aug 22, 2024

rynewang Aug 22, 2024

rynewang commented Aug 22, 2024

rkooo567 left a comment

rynewang commented Aug 22, 2024

rkooo567 commented Aug 22, 2024

rynewang commented Aug 22, 2024

rynewang commented Aug 22, 2024

rynewang commented Aug 22, 2024

rkooo567 commented Aug 22, 2024

can-anyscale commented Aug 23, 2024

[core] Add PREPARED state for placement groups. #46858

[core] Add PREPARED state for placement groups. #46858

Conversation

rynewang commented Jul 29, 2024

jjyao commented Jul 30, 2024

rynewang commented Jul 30, 2024

jjyao left a comment

Choose a reason for hiding this comment

jjyao Jul 31, 2024

Choose a reason for hiding this comment

rkooo567 Aug 21, 2024

Choose a reason for hiding this comment

rynewang commented Aug 20, 2024

rkooo567 commented Aug 20, 2024 • edited Loading

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 Aug 21, 2024

Choose a reason for hiding this comment

rkooo567 Aug 22, 2024

Choose a reason for hiding this comment

rkooo567 Aug 22, 2024

Choose a reason for hiding this comment

rynewang Aug 22, 2024

Choose a reason for hiding this comment

rynewang commented Aug 22, 2024

rkooo567 left a comment

Choose a reason for hiding this comment

rynewang commented Aug 22, 2024

rkooo567 commented Aug 22, 2024

rynewang commented Aug 22, 2024

rynewang commented Aug 22, 2024

rynewang commented Aug 22, 2024

rkooo567 commented Aug 22, 2024

can-anyscale commented Aug 23, 2024

rkooo567 commented Aug 20, 2024 •

edited

Loading