[core] Fix the spilling back failure in case of node missing #19564

fishbone · 2021-10-20T21:04:47Z

Why are these changes needed?

When ray spill back, it'll check whether the node exists or not through gcs, so there is a race condition and sometimes raylet crashes due to this.

This PR filter out the node that's not available when select the node.

Related issue number

#19438

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

rkooo567

LGTM in general. Just a couple nits. How do you plan to verify this works btw? Will you just run the nightly test?

rkooo567 · 2021-10-21T06:52:59Z

src/ray/gcs/accessor.h

@@ -462,8 +462,8 @@ class NodeInfoAccessor {
  /// \param filter_dead_nodes Whether or not if this method will filter dead nodes.
  /// \return The item returned by GCS. If the item to read doesn't exist or the node is
  /// dead, this optional object is empty.
-  virtual absl::optional<rpc::GcsNodeInfo> Get(const NodeID &node_id,
-                                               bool filter_dead_nodes = true) const = 0;
+  virtual const rpc::GcsNodeInfo *Get(const NodeID &node_id,


Any reason why we move back to the raw pointer? I feel like raw pointers are really easy to make a mistake because it is common to forget not to handle the return nullptr in the caller.

I think for optional it's the same. For example, optional can also be nullopt right? So in either case you need to handle nullptr (or nullopt). I feel if we'd like to model return by value data, we should use optional, otherwise, pointer should also work since there is no difference.

The reason I make this ptr is that I'd like to avoid the copy of GcsNodeInfo. I check every node during scheduler, and seeing the copy here makes me feel a little bit bad. So I updated this one. optionalrcp::GcsNodeInfo& is not allowed.

I think one of the cons is that if we use the nullptr, the program will crash, but not for nullopt? (am I correct?)

But if the performance is the concern is makes sense. I remember the syntax to pass references to the optional was too complex and ugly.

https://en.cppreference.com/w/cpp/utility/optional/value From the doc it says, it'll throw an exception and we are not catching it, so it'll crash as well.

I change this not purely because of the performance, but just feel

it's the same (both will crash).

it might potentially have a performance issue (not tested).

It looks like only gain, so I just update this.

Ah, makes sense. So technically having optional doesn't really have any meaning within Ray... good to know

This makes me think if we should have an exception handler in the outermost file (main.cc) (and at least print it although we don't handle them). I assume there are some exceptions that happen but not printed because we don't catch them.

rkooo567 · 2021-10-21T06:53:20Z

src/ray/raylet/placement_group_resource_manager_test.cc

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

+// clang-format off


Is it needed?

Yes, because I need mock/ray/gcs/gcs_client.h to be the last one.

Why does include order matter here, is it because mock/ray/gcs/gcs_client.h has incomplete include?

Yes. It's mock type, so it assumes the context has all the necessary structure.

src/ray/raylet/scheduling/cluster_resource_scheduler.cc

mwtian · 2021-10-21T20:27:06Z

src/ray/raylet/placement_group_resource_manager_test.cc

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

+// clang-format off


Why does include order matter here, is it because mock/ray/gcs/gcs_client.h has incomplete include?

src/ray/raylet/scheduling/cluster_resource_scheduler.h

src/ray/raylet/placement_group_resource_manager_test.cc

src/ray/raylet/scheduling/cluster_resource_scheduler.cc

mwtian · 2021-10-22T03:04:22Z

src/ray/raylet/scheduling/cluster_resource_scheduler.cc

+          best_node = iter->first;
+          break;
+        }
+        ++iter;


Just to be sure, dead nodes are pretty rare, right? Otherwise with always searching forward when seeing a dead node, the distribution will no longer be uniform which may or may not matter.

It's rare. Otherwise, it should be a bug I think.

Ah, this is a really good point. I think we will start doing chaos testing, so we should handle this better. If distribution is bad, it can actually cause memory issues easily I think. If it is not an easy fix, can we add a TODO?

I'm thinking about how to fix this and I can't come out with an easy, elegant algorithm for this :( any ideas? otherwise, I'll just add TODO

Let me first add todo for now.

Not sure about a way besides scanning all nodes once. Leaving a TODO seems fine.

src/ray/raylet/scheduling/cluster_task_manager_test.cc

rkooo567

I might still prefer optional return, but if it is due to the perf reason, I have no strong preference (maybe we can follow up in the thread we are talking about it).

fishbone added 21 commits October 19, 2021 04:32

fix metric issues

e1c4500

format

d9b1b55

formatg

c293ad0

Merge remote-tracking branch 'upstream/master' into fix-metrics

1505f5c

minor fix

813e783

fix one

1f5d120

add filter

3feffcb

format

ec874db

fix

cda36df

format

2ac675a

up

076ccb0

up

a11c81c

up

336558c

format

c235ad3

up

5e54abe

format

b51fc31

some fix

0500695

up

99de7a2

fix

476d1a5

fix

6e5dbc8

format

5c50043

fishbone changed the title ~~[wip] Spill back node fix~~ [core] Fix the spilling back failure in case of node missing Oct 21, 2021

fishbone assigned wuisawesome and rkooo567 Oct 21, 2021

fishbone marked this pull request as ready for review October 21, 2021 06:44

Merge remote-tracking branch 'upstream/master' into spill-back-node-fix

12f2b86

rkooo567 reviewed Oct 21, 2021

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 Oct 21, 2021

fishbone assigned mwtian Oct 21, 2021

mwtian reviewed Oct 21, 2021

View reviewed changes

fix some comment

6a4758c

mwtian approved these changes Oct 22, 2021

View reviewed changes

rkooo567 approved these changes Oct 22, 2021

View reviewed changes

fishbone added 3 commits October 22, 2021 05:09

up

ed29f4e

Merge remote-tracking branch 'upstream/master' into spill-back-node-fix

b912ba2

fix

8be55ae

fishbone merged commit 48fb86a into ray-project:master Oct 22, 2021

fishbone mentioned this pull request Oct 30, 2021

[core] Fix wrong local resource view in raylet #19911

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Fix the spilling back failure in case of node missing #19564

[core] Fix the spilling back failure in case of node missing #19564

fishbone commented Oct 20, 2021 •

edited

Loading

rkooo567 left a comment

rkooo567 Oct 21, 2021

fishbone Oct 21, 2021

rkooo567 Oct 22, 2021

fishbone Oct 22, 2021

rkooo567 Oct 22, 2021

rkooo567 Oct 22, 2021

rkooo567 Oct 21, 2021

fishbone Oct 21, 2021

mwtian Oct 21, 2021

fishbone Oct 22, 2021

mwtian Oct 21, 2021

mwtian Oct 22, 2021

fishbone Oct 22, 2021

rkooo567 Oct 22, 2021

fishbone Oct 22, 2021

fishbone Oct 22, 2021

mwtian Oct 22, 2021

rkooo567 left a comment

[core] Fix the spilling back failure in case of node missing #19564

[core] Fix the spilling back failure in case of node missing #19564

Conversation

fishbone commented Oct 20, 2021 • edited Loading

Why are these changes needed?

Related issue number

Checks

rkooo567 left a 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

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

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

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

fishbone commented Oct 20, 2021 •

edited

Loading