GCS-Based actor management implementation #6763

wumuzi520 · 2020-01-10T06:44:38Z

Why are these changes needed?

Pls see the <Design Document> first.

This PR implements the creation and reconstruction of actors based on gcs server.

Changes on gcs server side

Several important classes are added: GcsActor, GcsActorManager, GcsActorScheduler.

GcsActor: An abstraction of actor at GcsServer side, which wrapper the ActorTableData and provides some simple interface to access the field inside ActorTableData.
GcsActorManager: It is responsible for managing the lifecycle of all registered actors.
GcsActorScheduler: It is responsible for scheduling actors registered to GcsActorManager, it also contains a inner class called GcsLeasedWorker which is an abstraction of remote leased worker in raylet.

In addition, this PR has also made some changes to GcsNodeManager, it is responsible for monitoring and manage nodes.

Changes on raylet side

In the old actor management scheme, raylet will be responsible for updating ActorTableData, while in the new GCS-Based actor management scheme, we expect that GCS will be responsible for updating all ActorTableData. So, you will see that all logic about updating ActorTableData will be get ride off.
Besides, the raylet should cache the relationship of actor and leased worker because that the raylet should fast reply gcs server without lease anything when gcs server rebuild actors after restart. Pls see the <Design Document>.

Chages on worker side

invoke the gcs_rpc_client.CreateActor on the callback of ResolveDependencies.
Fast reply the gcs server without create anyting if it is already bound with an actor when gcs server rebuild actors rebuild actors after restart.

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://ray.readthedocs.io/en/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.

AmplabJenkins · 2020-01-10T06:46:28Z

Can one of the admins verify this patch?

AmplabJenkins · 2020-01-10T12:52:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/20564/
Test FAILed.

AmplabJenkins · 2020-01-12T10:41:18Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/20583/
Test FAILed.

AmplabJenkins · 2020-03-05T09:07:46Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22763/
Test FAILed.

AmplabJenkins · 2020-03-05T09:44:29Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22764/
Test FAILed.

AmplabJenkins · 2020-03-05T13:55:46Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22771/
Test FAILed.

AmplabJenkins · 2020-03-06T04:18:07Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22797/
Test PASSed.

AmplabJenkins · 2020-03-06T06:19:30Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22800/
Test PASSed.

AmplabJenkins · 2020-03-06T06:48:01Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22801/
Test PASSed.

AmplabJenkins · 2020-03-06T09:35:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22806/
Test PASSed.

AmplabJenkins · 2020-03-06T11:24:31Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22810/
Test FAILed.

AmplabJenkins · 2020-03-06T11:43:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22811/
Test FAILed.

AmplabJenkins · 2020-03-06T11:55:42Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22812/
Test PASSed.

AmplabJenkins · 2020-03-06T13:13:29Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22808/
Test FAILed.

AmplabJenkins · 2020-03-07T05:22:13Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22837/
Test FAILed.

ericl · 2020-03-07T07:31:49Z

(please feel free to re-request once not WIP)

stephanie-wang · 2020-04-09T19:31:13Z

Do not depend on io_context_ in the unit tests. This is important because then we will have fine-grained control over the order of execution and can test complicated concurrency scenarios. Instead, collect any callbacks in the test fixture, choose the order that you flush them in, and add assertions where necessary to check that the right callbacks are getting scheduled. I know that the code uses the io_context_ right now to retry requests. Instead, you can pass in a retry callback that abstracts away the io_context_. The callback can take in the method to retry as an argument and the period to wait before retrying. This will let us have control over when retries are executed and also check that we are retrying under the correct scenarios.

@stephanie-wang I agree with you that it's helpful to make tests more reliable and deterministic. However, I'm concerned there are too many callbacks, which will hurt code readability. Also, adding interfaces like WorkerLeaseInterface only for testing makes the code over abstracted.
Instead of using callbacks, I think we can just add a virtual function (e.g. RequestWorkerLease) in GcsActorScheduler, and then override this function in MockedGcsActorScheduler. This should have the same effect, without over abstracting non-test code.

@stephanie-wang
I also think there are too many input callback parameters, and code readability may be affected. Because only the logic of retry in the bottom layer uses io_context, I extract the retry function separately and define it as a virtual function. When writing unit tests, it only need to mock the virtual function out, and then there will be no logic about io_context, and only the input parameters of the constructor still contains io_context, but it has no impact on the unit test.

Sure, I'm not attached to the testing method that I suggested, and thanks for correcting me on the "bugs" I thought were there :) The main thing I care about is making sure that the tests are reliable and deterministic, as you say. So I think your suggestions about having the retry function be virtual would work as well. Could you add this to the current PR and remove the dependency on the io_context? I think this is important enough that we should have it in the initial commit for actor management. Thanks!

stephanie-wang · 2020-04-09T19:40:59Z

Okay, after some discussion offline, let's get this PR merged without the changes to the unit tests, but the next focus should be on removing the io_context dependency from the unit tests.

By the way, the Travis failures on OSX and RLlib seem like they're related to this PR. Let me know if I can help with debugging.

AmplabJenkins · 2020-04-10T15:16:46Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24517/
Test PASSed.

AmplabJenkins · 2020-04-10T16:33:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24522/
Test PASSed.

stephanie-wang · 2020-04-10T19:21:14Z

src/ray/gcs/gcs_server/test/gcs_actor_scheduler_test.cc

-  OneNodeTest() : GcsActorSchedulerTest(1) {}
-};
+TEST_F(GcsActorSchedulerTest, TestScheduleFailedWithZeroNode) {
+  ASSERT_EQ(0, gcs_node_manager_->GetAllAliveNodes().size());


Usually the actual value goes first and the expected value goes second. Not a big deal, but it makes the output nicer if the values do not match.

stephanie-wang

Thanks, this looks great! I just left two last comments, about notifying the TaskManager when the creation task finishes and about whether we need to flush the actor's leased worker address.

AmplabJenkins · 2020-04-11T07:46:24Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24550/
Test PASSed.

AmplabJenkins · 2020-04-11T08:01:58Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24549/
Test PASSed.

AmplabJenkins · 2020-04-11T08:42:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24553/
Test PASSed.

AmplabJenkins · 2020-04-11T14:39:48Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24555/
Test PASSed.

AmplabJenkins · 2020-04-12T06:10:24Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24581/
Test PASSed.

AmplabJenkins · 2020-04-12T06:28:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24583/
Test PASSed.

AmplabJenkins · 2020-04-12T07:01:54Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24584/
Test PASSed.

AmplabJenkins · 2020-04-13T03:03:34Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24608/
Test PASSed.

AmplabJenkins · 2020-04-13T05:38:04Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24615/
Test PASSed.

ericl self-assigned this Jan 12, 2020

zhijunfu self-requested a review January 15, 2020 11:40

wumuzi520 force-pushed the gcs_actor_manager branch 2 times, most recently from 9e82612 to 979cec7 Compare March 5, 2020 08:50

wumuzi520 force-pushed the gcs_actor_manager branch from 979cec7 to bf7e2d8 Compare March 5, 2020 13:15

wumuzi520 force-pushed the gcs_actor_manager branch from bf7e2d8 to 6debd96 Compare March 6, 2020 03:02

wumuzi520 force-pushed the gcs_actor_manager branch 2 times, most recently from 1ccf692 to 4642605 Compare March 6, 2020 05:36

wumuzi520 force-pushed the gcs_actor_manager branch from 4642605 to 2b8101e Compare March 6, 2020 08:27

wumuzi520 force-pushed the gcs_actor_manager branch 3 times, most recently from c3b1100 to 289feb7 Compare March 6, 2020 10:53

wumuzi520 force-pushed the gcs_actor_manager branch from 289feb7 to f428d4d Compare March 7, 2020 04:29

ericl removed their assignment Mar 7, 2020

fix comment

98184b7

wumuzi520 force-pushed the gcs_actor_manager branch from 33f4625 to 98184b7 Compare April 10, 2020 15:20

stephanie-wang reviewed Apr 10, 2020

View reviewed changes

stephanie-wang approved these changes Apr 10, 2020

View reviewed changes

wumuzi520 force-pushed the gcs_actor_manager branch from 58cc70c to 57b91a8 Compare April 11, 2020 06:23

fix comment

adaea18

wumuzi520 force-pushed the gcs_actor_manager branch from 57b91a8 to adaea18 Compare April 11, 2020 07:22

Merge branch 'master' into gcs_actor_manager

88b61c1

Merge branch 'master' into gcs_actor_manager

373ba07

wumuzi520 closed this Apr 12, 2020

wumuzi520 reopened this Apr 12, 2020

fix compile error

a639ffa

wumuzi520 force-pushed the gcs_actor_manager branch from b9c0278 to a639ffa Compare April 12, 2020 05:37

Merge branch 'master' into gcs_actor_manager

13376ca

fix merge error

786c97e

stephanie-wang merged commit 4a81793 into ray-project:master Apr 13, 2020

ericl mentioned this pull request Apr 18, 2020

GCS actor management introduces reference counting bug #8076

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCS-Based actor management implementation #6763

GCS-Based actor management implementation #6763

wumuzi520 commented Jan 10, 2020 •

edited

AmplabJenkins commented Jan 10, 2020

AmplabJenkins commented Jan 10, 2020

AmplabJenkins commented Jan 12, 2020

AmplabJenkins commented Mar 5, 2020

AmplabJenkins commented Mar 5, 2020

AmplabJenkins commented Mar 5, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 7, 2020

ericl commented Mar 7, 2020

stephanie-wang commented Apr 9, 2020

stephanie-wang commented Apr 9, 2020

AmplabJenkins commented Apr 10, 2020

AmplabJenkins commented Apr 10, 2020

stephanie-wang Apr 10, 2020

stephanie-wang left a comment

AmplabJenkins commented Apr 11, 2020

AmplabJenkins commented Apr 11, 2020

AmplabJenkins commented Apr 11, 2020

AmplabJenkins commented Apr 11, 2020

AmplabJenkins commented Apr 12, 2020

AmplabJenkins commented Apr 12, 2020

AmplabJenkins commented Apr 12, 2020

AmplabJenkins commented Apr 13, 2020

AmplabJenkins commented Apr 13, 2020

GCS-Based actor management implementation #6763

GCS-Based actor management implementation #6763

Conversation

wumuzi520 commented Jan 10, 2020 • edited

Why are these changes needed?

Related issue number

Checks

AmplabJenkins commented Jan 10, 2020

AmplabJenkins commented Jan 10, 2020

AmplabJenkins commented Jan 12, 2020

AmplabJenkins commented Mar 5, 2020

AmplabJenkins commented Mar 5, 2020

AmplabJenkins commented Mar 5, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 6, 2020

AmplabJenkins commented Mar 7, 2020

ericl commented Mar 7, 2020

stephanie-wang commented Apr 9, 2020

stephanie-wang commented Apr 9, 2020

AmplabJenkins commented Apr 10, 2020

AmplabJenkins commented Apr 10, 2020

stephanie-wang Apr 10, 2020

Choose a reason for hiding this comment

stephanie-wang left a comment

Choose a reason for hiding this comment

AmplabJenkins commented Apr 11, 2020

AmplabJenkins commented Apr 11, 2020

AmplabJenkins commented Apr 11, 2020

AmplabJenkins commented Apr 11, 2020

AmplabJenkins commented Apr 12, 2020

AmplabJenkins commented Apr 12, 2020

AmplabJenkins commented Apr 12, 2020

AmplabJenkins commented Apr 13, 2020

AmplabJenkins commented Apr 13, 2020

wumuzi520 commented Jan 10, 2020 •

edited