[PlacementGroup]Fix destroy placement group bug by ffbin · Pull Request #13320 · ray-project/ray

ffbin · 2021-01-10T10:32:33Z

Why are these changes needed?

At present, if the placement group creator actor restarts, the placement group will be destroyed. We should destroy placement group only when the placement group creator actor is dead.

Related issue number

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

ffbin · 2021-01-10T10:35:00Z

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

    const PlacementGroupID &placement_group_id,
    StatusCallback on_placement_group_removed) {
  RAY_CHECK(on_placement_group_removed);
+  RAY_LOG(INFO) << "Removing placement group " << placement_group_id;


The printing frequency of these logs is very low. In order to facilitate the problem location, it is set to INFO.

Hmm I still feel like this log is unnecessary. Isn't it better adding a log when the placement group is actually deleted instead?

rkooo567 · 2021-01-12T04:17:32Z

Hmm I am not sure if this semantic is a good idea. Think about this scenario.

Create an actor
In the actor constructor we create a new placement group.

In this case, isn't it creating placement groups whenever we restart the actor? So if we restart 5 times, there are 4 placement groups that are not removed and retrieveable, but we don't need them because the actor created a new one.

rkooo567

Request change until the discussion is resolved.

ffbin · 2021-01-12T06:40:24Z

In this case, isn't it creating placement groups whenever we restart the actor? So if we restart 5 times, there are 4 placement groups that are not removed and retrieveable, but we don't need them because the actor created a new one.

Hi @rkooo567 , I actually encountered this problem in the production environment, two detached actors (A and B). Actor A creates PG, and then uses PG to create actor B. Actor A is killed, causing PG to be deleted and actor B to exit without restarting. So I think this is definitely a problem that needs to be solved, but the scenario you mentioned really can't be solved very well. I generally suggest users not to create PG in the constructor, because at present, the actor constructor fails and users can't perceive it.

ffbin · 2021-01-14T14:17:17Z

Hi @rkooo567 Pls help take a look, thx :)

rkooo567

Can you update a doc to specify this behavior? (saying if the actor is restarted, the placement group created from it is not removed).

It'll be also great if you add a warning not to create a placement group in a constructor.

cc @richardliaw Will this behavior change cause any issue?

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

rkooo567 · 2021-01-14T19:01:03Z

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

    const PlacementGroupID &placement_group_id,
    StatusCallback on_placement_group_removed) {
  RAY_CHECK(on_placement_group_removed);
+  RAY_LOG(INFO) << "Removing placement group " << placement_group_id;


Hmm I still feel like this log is unnecessary. Isn't it better adding a log when the placement group is actually deleted instead?

rkooo567 · 2021-01-15T23:24:16Z

Hey @ffbin I thought more about it, and I think we shouldn't push this change. I think the scenario you mentioned is not a bug because we have the exact same behavior for actor handles. (imagine two detached actors. actor A creates a actor handle C and pass it to actor B. Current pg behavior is consistent with this).

I think we cannot "encourage" users not to create pgs in the constructor. This is a really bad UX since many users don't see docs, and this can lead them to unexpected placement group leaks which are nearly impossible to debug in their end.

I think the right solution is to keep the current behavior, but we implement this (#10508), so that users can clearly know why the actor B was killed, or we just don't fate-share actors with the placement group.

But since we have diverged opinions, I'd like to hear what @ericl thinks about this.

ericl · 2021-01-15T23:46:45Z

It sounds like what you actually want here is to "detach" the placement group.

Should we just support lifetime="detached" for placement group explicitly?

rkooo567 · 2021-01-15T23:48:46Z

That's a good idea. Serve also asked about the need for detached placement groups (for better fault tolerance). And I think that's the best solution here I believe.

ffbin · 2021-01-17T04:08:38Z

That's a good idea. Serve also asked about the need for detached placement groups (for better fault tolerance). And I think that's the best solution here I believe.

Support lifetime="detached" for placement group is a good idea, i will open a new PR for it.

rkooo567 · 2021-01-20T20:16:15Z

Can we close PR and instead merging the detached pg PR?

ffbin · 2021-01-21T02:26:19Z

Close this PR and instead merging the detached pg PR.

[PlacementGroup]Fix destroy owned placement_group bug

53ff4d4

ffbin commented Jan 10, 2021

View reviewed changes

ffbin requested review from clay4megtr, raulchen and rkooo567 January 10, 2021 10:40

ffbin assigned rkooo567 Jan 10, 2021

raulchen approved these changes Jan 10, 2021

View reviewed changes

rkooo567 requested changes Jan 12, 2021

View reviewed changes

ffbin mentioned this pull request Jan 12, 2021

[PlacementGroup]Fix the bug that actor does not restart #13321

Closed

6 tasks

ffbin requested a review from rkooo567 January 14, 2021 09:55

rkooo567 reviewed Jan 14, 2021

View reviewed changes

灵洵 added 3 commits January 15, 2021 20:40

fix review comment

e4b33df

fix review comment

7689e4f

fix review comment

1902b97

ffbin requested a review from rkooo567 January 15, 2021 13:04

ericl added the core-api-change-approval-required label Jan 15, 2021

clay4megtr mentioned this pull request Jan 19, 2021

Add detached for Placement Group Creation #13541

Closed

ffbin closed this Jan 21, 2021

Conversation

ffbin commented Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

ffbin Jan 10, 2021

Choose a reason for hiding this comment

Uh oh!

rkooo567 Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

ffbin Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

rkooo567 commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkooo567 left a comment

Choose a reason for hiding this comment

Uh oh!

ffbin commented Jan 12, 2021

Uh oh!

ffbin commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkooo567 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rkooo567 Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

rkooo567 commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericl commented Jan 15, 2021

Uh oh!

rkooo567 commented Jan 15, 2021

Uh oh!

ffbin commented Jan 17, 2021

Uh oh!

rkooo567 commented Jan 20, 2021

Uh oh!

ffbin commented Jan 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ffbin commented Jan 10, 2021 •

edited

Loading

rkooo567 commented Jan 12, 2021 •

edited

Loading

ffbin commented Jan 14, 2021 •

edited

Loading

rkooo567 commented Jan 15, 2021 •

edited

Loading