[Job Submission][refactor 1/N] Add AgentInfo to GCSNodeInfo #26302

Catch-Bull · 2022-07-05T17:11:58Z

Why are these changes needed?

This is the first PR of #25963 :

Moved the agent information from internal KV to GCSNodeInfo`,
raylet registers itself after the agent process finished register.

Motivation:
Storing agent information in internal KV and registering nodes in GCS (write node information to GCSNodeInfo) are two asynchronous operations, which will bring some complex timing problems, especially after raylet failover

Related issue number

#25963

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 :(

Catch-Bull · 2022-07-06T11:14:17Z

python/ray/tests/BUILD

@@ -61,7 +61,8 @@ py_test_module_list(
    "test_healthcheck.py",
    "test_kill_raylet_signal_log.py",
    "test_memstat.py",
-    "test_protobuf_compatibility.py"
+    "test_protobuf_compatibility.py",
+    "test_scheduling_performance.py"


after this PR, the node needs to wait for the agent to register before registering itself to GCS, cluster.add_node will be nearly one second slower, resulting in an increase of 16 * 2 = 32 seconds in total time, so set the test size to medium.

fishbone · 2022-07-06T22:39:04Z

src/ray/raylet/raylet.cc

+                    /*set_agent_info_and_register_node*/
+                    [this](const AgentInfo &agent_info) {
+                      self_node_info_.mutable_agent_info()->CopyFrom(agent_info);
+                      RAY_CHECK_OK(RegisterGcs());
+                    }),


It looks bad. Why do it this way? We should try to decouple node_manager_ with the Raylet.

To be clear, I'm not comfortable that NodeManager <-> Raylet has circular dependence here.

How about move the logic of RegisterGcs from Raylet to NodeManager? Maybe registering node should be a part of NodeManager?

fishbone · 2022-07-06T22:46:38Z

What if the agent failed, will the address be updated in case of that? (I hope we won't fail raylet in this case)

fishbone

request changes

Catch-Bull · 2022-07-07T11:10:33Z

What if the agent failed, will the address be updated in case of that? (I hope we won't fail raylet in this case)

for now, raylet is fate-sharing with the agent process.
here are the details: https://github.com/ray-project/ray/blob/master/src/ray/raylet/agent_manager.cc#L117

Based on this, I think merging agent and node register into one RPC request is make sense, it can avoid dealing with consistency problems after raylet failover.

src/ray/raylet/agent_manager.cc

src/ray/raylet/node_manager.h

fishbone · 2022-07-14T18:56:28Z

src/ray/raylet/raylet.cc

@@ -96,7 +96,11 @@ Raylet::Raylet(instrumented_io_context &main_service,
 Raylet::~Raylet() {}

 void Raylet::Start() {
-  RAY_CHECK_OK(RegisterGcs());
+  register_thread_ = std::thread([this]() mutable {


Why do we need a new thread for this?

because Raylet::Start and AgentManager::HandleRegisterAgent will run in the same thread, and Raylet::Start will block the thread to wait until AgentManager::HandleRegisterAgent finished, it's a dead lock

Should a post from io context just work? I feel we don't need a thread here.

raylet::Start already run in main_service_, so it's no use to post this wait function into main_service_
detail:

ray/src/ray/raylet/main.cc

Line 283 in 1949f35

raylet->Start();

How about this:

std::function<void()> register; register = [&]() { if ready: set agent info; RegisterGCS; else: sleep 1s; io_context.post(register); }; register();

The reason I don't like thread here is that it increase complexity especially when we don't have a good threading model. For example it's easy to run into race conditions (self_node_info_ might be accessed in different thread, RegisterGcs now is running in a different thread. ).

@iycheng Here I modified it again, please take a look again

fishbone · 2022-07-15T05:14:05Z

src/ray/raylet/raylet.cc

-  RAY_CHECK_OK(RegisterGcs());
+  register_thread_ = std::thread([this]() mutable {
+    SetThreadName("raylet.register_itself");
+    self_node_info_.mutable_agent_info()->CopyFrom(node_manager_.SyncGetAgentInfo());


Here, please add time out for sync call. If time out, we should just fail the raylet (otherwise, it'll hang) and also print a detailed log about what happened for observability.

the agent register timeout is implemented here:

ray/src/ray/raylet/agent_manager.cc

Line 95 in 1949f35

auto timer = delay_executor_(

maybe I should replace time_ with agent_info_promise_？

I think future.get with a timeout should be good here.

rkooo567 · 2022-07-27T14:01:20Z

Maybe it is related? I've not seen any failure from this test https://flakey-tests.ray.io/?owner=core

SongGuyang · 2022-07-27T14:40:07Z

A lot of tests timed out. But the previous pipeline is green. Do we need to retry the tests?

rkooo567 · 2022-07-27T14:43:52Z

Some of tests are unstable in the master. I think test_ray_shutdown is the only one that I am really concerned about

SongGuyang · 2022-07-27T14:57:49Z

@Catch-Bull Take a look?

Do we need to merge this before the cut of 2.0?

Catch-Bull · 2022-07-27T15:41:56Z

@rkooo567 @SongGuyang this case seems never timeout, I will check it.

Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

…dress_to_node_table

Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

…ay-project#26302)" This reverts commit 14dee5f.

…26302)" (#27242) This reverts commit 14dee5f.

…deInfo (#26302)" (#27242)" This reverts commit ec69fec. Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

…ect#26302) This is the first PR of ray-project#25963 : 1. Moved the agent information from `internal KV to `GCSNodeInfo`, 2. raylet registers itself after the agent process finished register. Motivation: Storing agent information in `internal KV` and registering nodes in GCS (write node information to `GCSNodeInfo`) are two asynchronous operations, which will bring some complex timing problems, especially after `raylet` failover Signed-off-by: Stefan van der Kleij <s.vanderkleij@viroteq.com>

…ay-project#26302)" (ray-project#27242) This reverts commit 14dee5f. Signed-off-by: Stefan van der Kleij <s.vanderkleij@viroteq.com>

Catch-Bull requested review from wuisawesome, ericl, AmeerHajAli, robertnishihara, pcmoritz, raulchen, fishbone, scv119 and mwtian as code owners July 5, 2022 17:11

Catch-Bull marked this pull request as draft July 5, 2022 17:12

Catch-Bull changed the title ~~Add AgentInfo to GCSNodeInfo~~ [Job Submission][refactor 1/N] Add AgentInfo to GCSNodeInfo Jul 6, 2022

Catch-Bull commented Jul 6, 2022

View reviewed changes

Catch-Bull marked this pull request as ready for review July 6, 2022 11:28

Catch-Bull requested a review from architkulkarni July 6, 2022 11:46

fishbone reviewed Jul 6, 2022

View reviewed changes

fishbone requested changes Jul 6, 2022

View reviewed changes

fishbone self-assigned this Jul 6, 2022

Catch-Bull force-pushed the add_agent_address_to_node_table branch 2 times, most recently from 2db6db7 to 1034cca Compare July 12, 2022 09:39

xwjiang2010 mentioned this pull request Jul 12, 2022

[CI] run air related doc/example tests as part of pre-submit CI. #26466

Merged

6 tasks

fishbone reviewed Jul 14, 2022

View reviewed changes

src/ray/raylet/agent_manager.cc Outdated Show resolved Hide resolved

fishbone reviewed Jul 14, 2022

View reviewed changes

src/ray/raylet/node_manager.h Outdated Show resolved Hide resolved

fishbone reviewed Jul 14, 2022

View reviewed changes

fishbone reviewed Jul 15, 2022

View reviewed changes

Catch-Bull force-pushed the add_agent_address_to_node_table branch 2 times, most recently from 6f79f43 to 08e7bb0 Compare July 18, 2022 16:46

Catch-Bull requested review from krfricke and rkooo567 as code owners July 18, 2022 16:46

Catch-Bull added 5 commits July 28, 2022 00:33

fix test_ray_shutdown

158a744

Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

fix lint

9f6bbc9

Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

Merge branch 'master' of github.com:ray-project/ray into add_agent_ad…

1fe9eed

…dress_to_node_table

fix lint

5cd1d6e

Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

fix test_multiprocessing

99e930a

Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

SongGuyang merged commit 14dee5f into ray-project:master Jul 28, 2022

kfstorm deleted the add_agent_address_to_node_table branch July 28, 2022 18:46

rkooo567 added a commit to rkooo567/ray that referenced this pull request Jul 29, 2022

Revert "[Job Submission][refactor 1/N] Add AgentInfo to GCSNodeInfo (r…

04f01c8

…ay-project#26302)" This reverts commit 14dee5f.

rkooo567 mentioned this pull request Jul 29, 2022

Revert "[Job Submission][refactor 1/N] Add AgentInfo to GCSNodeInfo (… #27242

Merged

7 tasks

rkooo567 added a commit that referenced this pull request Jul 30, 2022

Revert "[Job Submission][refactor 1/N] Add AgentInfo to GCSNodeInfo (#…

ec69fec

…26302)" (#27242) This reverts commit 14dee5f.

Catch-Bull added a commit that referenced this pull request Jul 30, 2022

Revert "Revert "[Job Submission][refactor 1/N] Add AgentInfo to GCSNo…

f1d139a

…deInfo (#26302)" (#27242)" This reverts commit ec69fec. Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

Catch-Bull added a commit that referenced this pull request Aug 2, 2022

Revert "Revert "[Job Submission][refactor 1/N] Add AgentInfo to GCSNo…

68c44fe

…deInfo (#26302)" (#27242)" This reverts commit ec69fec. Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

Catch-Bull added a commit that referenced this pull request Aug 4, 2022

Revert "Revert "[Job Submission][refactor 1/N] Add AgentInfo to GCSNo…

4d5f481

…deInfo (#26302)" (#27242)" This reverts commit ec69fec. Signed-off-by: Catch-Bull <burglarralgrub@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Job Submission][refactor 1/N] Add AgentInfo to GCSNodeInfo #26302

[Job Submission][refactor 1/N] Add AgentInfo to GCSNodeInfo #26302

Catch-Bull commented Jul 5, 2022 •

edited

Loading

Catch-Bull Jul 6, 2022

fishbone Jul 6, 2022

SongGuyang Jul 7, 2022

fishbone commented Jul 6, 2022

fishbone left a comment

Catch-Bull commented Jul 7, 2022 •

edited

Loading

fishbone Jul 14, 2022

Catch-Bull Jul 15, 2022 •

edited

Loading

fishbone Jul 15, 2022

Catch-Bull Jul 15, 2022

fishbone Jul 15, 2022

Catch-Bull Jul 18, 2022

fishbone Jul 15, 2022

Catch-Bull Jul 15, 2022 •

edited

Loading

fishbone Jul 15, 2022

rkooo567 commented Jul 27, 2022

SongGuyang commented Jul 27, 2022

rkooo567 commented Jul 27, 2022

SongGuyang commented Jul 27, 2022

Catch-Bull commented Jul 27, 2022 •

edited

Loading

[Job Submission][refactor 1/N] Add AgentInfo to GCSNodeInfo #26302

[Job Submission][refactor 1/N] Add AgentInfo to GCSNodeInfo #26302

Conversation

Catch-Bull commented Jul 5, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishbone commented Jul 6, 2022

fishbone left a comment

Choose a reason for hiding this comment

Catch-Bull commented Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Catch-Bull Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Catch-Bull Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkooo567 commented Jul 27, 2022

SongGuyang commented Jul 27, 2022

rkooo567 commented Jul 27, 2022

SongGuyang commented Jul 27, 2022

Catch-Bull commented Jul 27, 2022 • edited Loading

Catch-Bull commented Jul 5, 2022 •

edited

Loading

Catch-Bull commented Jul 7, 2022 •

edited

Loading

Catch-Bull Jul 15, 2022 •

edited

Loading

Catch-Bull Jul 15, 2022 •

edited

Loading

Catch-Bull commented Jul 27, 2022 •

edited

Loading