[core] Only get single node info rather then all when needed #49727

dayshah · 2025-01-08T21:44:44Z

Why are these changes needed?

GetAllNodeInfo is a heavily hit request on large clusters that starts to take a long time as clusters get large since it has to copy and send back info for all nodes. However, on some of these requests we're only looking for a single node with its id or ip address, and GetAllNodeInfo already has the option for a filter, so we should use that to only get info for the node we need it for. Here I'm utilizing the node_id filter and adding a node_ip_address filter, and using it in the global_state_accessor.

GetNodeToConnectForDriver Old Logic:

Get all the alive nodes
Get the gcs_server_address
If we find the node which matches the ip_address we wanted, return that node_info.

Else

If we find a node with an ip_address that matches the gcs_server_address, return that node_info.
If we wanted an ip_address that matched the gcs_server_address and we find an ip_address that matches the 127.0.0.1

GcsNodeToConnectForDriver New Logic

Try to get the info for the address we requested
If we get it return that info

Else

Get the gcs_server_address
Try to get the node info for the gcs_server_address.
If the ip_address we wanted matches the gcs_server_addresss, try to get the node info for 127.0.0.1

The 127.0.0.1 is the last prioritized requst because the assumption is that the cluster is running locally and rpc performance shouldn't really matter.

Related issue number

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
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 added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
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 :(

Signed-off-by: dayshah <dhyey2019@gmail.com>

dayshah · 2025-01-13T07:39:04Z

src/ray/gcs/gcs_client/global_state_accessor.cc

-        // TODO(kfstorm): Do we need to replace `node_ip_address` with
-        // `get_node_ip_address()`?
-        if ((ip_address == "127.0.0.1" && gcs_address.first == node_ip_address) ||
-            ip_address == gcs_address.first) {


i feel like the error messages don't totally align with the error messages, here we'll just return a node_info as long it matches the gcs_address we fetched

python/ray/includes/global_state_accessor.pxd

src/ray/gcs/gcs_client/accessor.cc

jjyao · 2025-01-13T16:24:21Z

src/ray/gcs/gcs_client/accessor.h

+  /// Get information of all nodes from an RPC to GCS synchronously with filter.
+  ///
+  /// \return All nodes that match the given filter from the gcs without the cache.
+  virtual StatusOr<std::vector<rpc::GcsNodeInfo>> GetAllNoCacheWithFilter(


src/ray/gcs/gcs_client/accessor.h

src/ray/gcs/gcs_client/global_state_accessor.cc

jjyao · 2025-01-13T16:31:41Z

src/ray/gcs/gcs_client/global_state_accessor.cc

+    filter.set_state(rpc::GcsNodeInfo_GcsNodeState::GcsNodeInfo_GcsNodeState_ALIVE);
+    filter.set_node_id(node_id.Binary());
+    {
+      absl::ReaderMutexLock lock(&mutex_);


gcs_client_ is protected by mutex, this is how it's used throughout

jjyao · 2025-01-13T16:34:32Z

src/ray/gcs/gcs_client/global_state_accessor.cc

+      RAY_LOG(INFO) << "This node has an IP address of " << node_ip_address
+                    << ", but we cannot find a local Raylet with the same address. "
+                    << "This can happen when you connect to the Ray cluster "
+                    << "with a different IP address or when connecting to a container.";


previously we were still logging this if we don't get any node info that matches with the node_ip_address id and have to resort to using the gcs_address we fetched instead of the node_ip_address passed into the function

Signed-off-by: dayshah <dhyey2019@gmail.com>

src/ray/gcs/gcs_client/global_state_accessor.cc

rynewang · 2025-01-13T19:12:03Z

[jokes] Nowadays we are reinventing sql server, step 2: query by index. lol

dentiny · 2025-01-13T19:38:28Z

src/ray/gcs/gcs_client/global_state_accessor.cc

+      absl::ReaderMutexLock lock(&mutex_);
+      auto timeout_ms =
+          std::max(end_time_point - current_time_ms(), static_cast<int64_t>(0));
+      node_infos_status =


Would RAY_ASSIGN_OR_RETURN work here?

ya this was super useful ty!

Signed-off-by: dayshah <dhyey2019@gmail.com>

jjyao

LG

src/ray/gcs/gcs_client/accessor.h

jjyao · 2025-01-14T00:37:54Z

src/ray/gcs/gcs_client/global_state_accessor.cc

+    if (node_infos.empty() && node_ip_address == gcs_address) {
+      filters.set_node_ip_address("127.0.0.1");


If you know, can you comment on what this case is?

Related code (listen_to_localhost_only sets GrpcServer to listen to 127.0.0.1) with and pr that introduced this #16810. Don't see a concrete reason to have this last case anywhere, ideally the above case of looking for gcs_address should cover it, but maybe some case of container or something?

ok, lets keep it for now.

src/ray/gcs/gcs_server/gcs_node_manager.cc

Signed-off-by: dayshah <dhyey2019@gmail.com>

dayshah force-pushed the get-with-node-id branch from 5048ac9 to 84a2666 Compare January 13, 2025 07:27

dayshah changed the title ~~[core] Get node info with filter by id~~ [core] Only get single node info rather then all when needed Jan 13, 2025

dayshah marked this pull request as ready for review January 13, 2025 07:28

dayshah requested review from a team, pcmoritz and raulchen as code owners January 13, 2025 07:28

Only get single node info when needed

1bce3af

Signed-off-by: dayshah <dhyey2019@gmail.com>

dayshah force-pushed the get-with-node-id branch from 84a2666 to 1bce3af Compare January 13, 2025 07:37

dayshah commented Jan 13, 2025

View reviewed changes

jjyao reviewed Jan 13, 2025

View reviewed changes

fix max compile error

55a5d5b

Signed-off-by: dayshah <dhyey2019@gmail.com>

dentiny reviewed Jan 13, 2025

View reviewed changes

src/ray/gcs/gcs_client/global_state_accessor.cc Outdated Show resolved Hide resolved

dentiny reviewed Jan 13, 2025

View reviewed changes

address comments

d67620d

Signed-off-by: dayshah <dhyey2019@gmail.com>

jjyao added the go add ONLY when ready to merge, run all tests label Jan 13, 2025

jjyao reviewed Jan 14, 2025

View reviewed changes

address comments

e5434bc

Signed-off-by: dayshah <dhyey2019@gmail.com>

jjyao approved these changes Jan 15, 2025

View reviewed changes

jjyao merged commit 6c0fa3d into ray-project:master Jan 16, 2025
5 checks passed

srinathk10 pushed a commit that referenced this pull request Feb 2, 2025

[core] Only get single node info rather then all when needed (#49727)

56c0601

Signed-off-by: dayshah <dhyey2019@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Only get single node info rather then all when needed #49727

[core] Only get single node info rather then all when needed #49727

dayshah commented Jan 8, 2025 •

edited

Loading

dayshah Jan 13, 2025

jjyao Jan 13, 2025

jjyao Jan 13, 2025

dayshah Jan 13, 2025

jjyao Jan 13, 2025

dayshah Jan 13, 2025

rynewang commented Jan 13, 2025

dentiny Jan 13, 2025

dayshah Jan 13, 2025

jjyao left a comment

jjyao Jan 14, 2025

dayshah Jan 15, 2025 •

edited

Loading

jjyao Jan 15, 2025

		if (node_infos.empty() && node_ip_address == gcs_address) {
		filters.set_node_ip_address("127.0.0.1");

[core] Only get single node info rather then all when needed #49727

[core] Only get single node info rather then all when needed #49727

Conversation

dayshah commented Jan 8, 2025 • edited Loading

Why are these changes needed?

GetNodeToConnectForDriver Old Logic:

GcsNodeToConnectForDriver New Logic

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynewang commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dayshah Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dayshah commented Jan 8, 2025 •

edited

Loading

dayshah Jan 15, 2025 •

edited

Loading