Add heartbeat test + Fix monitor.py #5191

richardliaw · 2019-07-13T23:56:14Z

What do these changes do?

Related issue number

Linter

I've run scripts/format.sh to lint the changes in this PR.

AmplabJenkins · 2019-07-14T02:38:25Z

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

AmplabJenkins · 2019-07-14T02:54:59Z

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

Initial implementation for heartbeat test

richardliaw · 2019-07-14T08:29:46Z

@zhijunfu @raulchen @pcmoritz we're debugging the autoscaler, and it looks like the heartbeats are completely missing resource data. After a bisection, it looks like it stems from #5024; do you know what to do to fix this?

Thanks!

AmplabJenkins · 2019-07-14T08:47: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/15370/
Test PASSed.

AmplabJenkins · 2019-07-14T09:35:15Z

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

AmplabJenkins · 2019-07-14T22:21:21Z

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

AmplabJenkins · 2019-07-14T22:42:30Z

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

AmplabJenkins · 2019-07-15T01:29:23Z

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

raulchen · 2019-07-15T08:50:26Z

@richardliaw sorry for introducing this bug. It should be fixed by the following patch. Do you want to include the patch in this PR? Or I can submit another fix as well.

diff --git a/src/ray/raylet/node_manager.cc b/src/ray/raylet/node_manager.cc
index 2b26579ae..b8ded2e6b 100644
--- a/src/ray/raylet/node_manager.cc
+++ b/src/ray/raylet/node_manager.cc
@@ -547,8 +547,8 @@ void NodeManager::HeartbeatAdded(const ClientID &client_id,
   SchedulingResources &remote_resources = it->second;
 
   ResourceSet remote_available(
-      VectorFromProtobuf(heartbeat_data.resources_total_label()),
-      VectorFromProtobuf(heartbeat_data.resources_total_capacity()));
+      VectorFromProtobuf(heartbeat_data.resources_available_label()),
+      VectorFromProtobuf(heartbeat_data.resources_available_capacity()));
   ResourceSet remote_load(VectorFromProtobuf(heartbeat_data.resource_load_label()),
                           VectorFromProtobuf(heartbeat_data.resource_load_capacity()));
   // TODO(atumanov): assert that the load is a non-empty ResourceSet.

AmplabJenkins · 2019-07-15T13:29:58Z

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

richardliaw · 2019-07-15T15:32:19Z

Hi Hao, Does this patch fix the test? Right now I think we are missing all resource information when we receive a heartbeat message. Feel free to push to this branch too.

…

On Mon, Jul 15, 2019 at 6:30 AM UCB AMPLab ***@***.***> wrote: Test FAILed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1674/ Test FAILed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5191?email_source=notifications&email_token=ABCRZZKHVXLKSAGQRIKBLTLP7R3WNA5CNFSM4IDEOOPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ5WD7A#issuecomment-511402492>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABCRZZPHIIANRUJXP7KOQO3P7R3WNANCNFSM4IDEOOPA> .

This reverts commit 60aebdd.

hartikainen · 2019-07-15T16:31:04Z

Hey @raulchen, thanks for the patch! I tried those changes, but they don't seem to solve the underlying issue. As Richard said, it seems like all the resource information is missing currently.

src/ray/raylet/node_manager.cc

AmplabJenkins · 2019-07-15T18:57:18Z

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

AmplabJenkins · 2019-07-15T19:17: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/15389/
Test PASSed.

AmplabJenkins · 2019-07-15T20:48:38Z

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

AmplabJenkins · 2019-07-15T21:45:15Z

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

AmplabJenkins · 2019-07-16T02:11:20Z

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

AmplabJenkins · 2019-07-16T02:17: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/15396/
Test PASSed.

hartikainen

Looks good to me!

hartikainen · 2019-07-16T02:37:01Z

python/ray/monitor.py

            # Update the load metrics for this raylet.
            client_id = ray.utils.binary_to_hex(heartbeat_message.client_id)
            ip = self.raylet_id_to_ip_map.get(client_id)
+            load_metrics_id = ip + "-" + client_id


Is this the best way to do this?

I think this needs a corresponding change to somewhere else. I currently see the following in my autoscaler logs:

2019-07-16 17:22:38,247 INFO autoscaler.py:198 -- LoadMetrics: Removed 1 stale ip mappings: {'10.138.0.90-e7a33a624c198a4a3280ed3ee9fbe3fdd13d287e'} not in {'10.138.0.94', '10.138.0.95', '10.138.0.91', '10.138.0.92', '10.138.0.90', '10.138.0.93'}

Hm I think this might be wrong (or requires a larger change to get right)...

python/ray/monitor.py

raulchen · 2019-07-16T03:12:51Z

Hi, @richardliaw, it seems that the test in this PR has already covered my bug. Should I add another one?

hartikainen · 2019-07-16T03:50:20Z

@raulchen, I think it would be useful to have a test for it on the backend side. What do you think?

AmplabJenkins · 2019-07-16T04: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/15404/
Test PASSed.

AmplabJenkins · 2019-07-16T04:28:21Z

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

AmplabJenkins · 2019-07-16T04:35: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/15405/
Test PASSed.

AmplabJenkins · 2019-07-16T05:26:09Z

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

AmplabJenkins · 2019-07-16T06:04:33Z

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

raulchen · 2019-07-16T06:16:17Z

@raulchen, I think it would be useful to have a test for it on the backend side. What do you think?

yeah, I agree that having such a test in C++ would be useful. However, it's not easy to do that. Because we don't have C++ workers. Currently, there are only some unit tests in C++ that test particular components of the backend. And integration tests are written in Python and Java.

AmplabJenkins · 2019-07-16T06:49:20Z

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

Co-Authored-By: Kristian Hartikainen <kristian.hartikainen@gmail.com>

AmplabJenkins · 2019-07-16T19:23:43Z

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

AmplabJenkins · 2019-07-16T19:55:57Z

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

AmplabJenkins · 2019-07-16T22:46: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/15420/
Test PASSed.

AmplabJenkins · 2019-07-16T23:54:56Z

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

AmplabJenkins · 2019-07-17T01:40: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/15428/
Test PASSed.

AmplabJenkins · 2019-07-17T04:24:52Z

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

AmplabJenkins · 2019-07-17T06:31:55Z

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

AmplabJenkins · 2019-07-17T12:06:59Z

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

richardliaw and others added 4 commits July 13, 2019 16:55

fix heartbeats

ec1c6ee

lint

4c46617

Fix heartbeats test

66661be

Initial implementation for heartbeat test

1d0ad83

hartikainen and others added 5 commits July 13, 2019 20:11

Rename XRAY_DRIVER_CHANNEL -> XRAY_JOB_CHANNEL

e6a0946

Use string keys instead of binary in heartbeat test

ebb49cc

Merge pull request #16 from hartikainen/testheart2

83dafc9

Initial implementation for heartbeat test

tests

8f595e6

lint

39e4a00

try debug

60aebdd

Revert "try debug"

86508c3

This reverts commit 60aebdd.

Fix heartbeat data available resources in node manager.

8dc289d

hartikainen reviewed Jul 15, 2019

View reviewed changes

src/ray/raylet/node_manager.cc Show resolved Hide resolved

Merge branch 'master' into testheart

fb01987

richardliaw changed the title ~~Add heartbeat test.~~ Add heartbeat test + Fix monitor.py Jul 16, 2019

hartikainen approved these changes Jul 16, 2019

View reviewed changes

hartikainen and others added 2 commits July 16, 2019 09:54

Merge branch 'master' into testheart

be011a8

Apply suggestions from code review

16c27d0

Co-Authored-By: Kristian Hartikainen <kristian.hartikainen@gmail.com>

monitor

3671e0f

fixtest

2f7b771

richardliaw added 2 commits July 16, 2019 18:48

fixtest

af6d7ec

fix

a6afae0

richardliaw mentioned this pull request Jul 17, 2019

Cannot load-balance actors with resource requirements #5209

Closed

richardliaw merged commit 3e0ad11 into ray-project:master Jul 17, 2019

richardliaw deleted the testheart branch July 17, 2019 04:59

Add heartbeat test + Fix monitor.py #5191

Add heartbeat test + Fix monitor.py #5191

Uh oh!

Conversation

richardliaw commented Jul 13, 2019

What do these changes do?

Related issue number

Linter

Uh oh!

AmplabJenkins commented Jul 14, 2019

Uh oh!

AmplabJenkins commented Jul 14, 2019

Uh oh!

richardliaw commented Jul 14, 2019

Uh oh!

AmplabJenkins commented Jul 14, 2019

Uh oh!

AmplabJenkins commented Jul 14, 2019

Uh oh!

AmplabJenkins commented Jul 14, 2019

Uh oh!

AmplabJenkins commented Jul 14, 2019

Uh oh!

AmplabJenkins commented Jul 15, 2019

Uh oh!

raulchen commented Jul 15, 2019

Uh oh!

AmplabJenkins commented Jul 15, 2019

Uh oh!

richardliaw commented Jul 15, 2019 via email

Uh oh!

hartikainen commented Jul 15, 2019

Uh oh!

Uh oh!

AmplabJenkins commented Jul 15, 2019

Uh oh!

AmplabJenkins commented Jul 15, 2019

Uh oh!

AmplabJenkins commented Jul 15, 2019

Uh oh!

AmplabJenkins commented Jul 15, 2019

Uh oh!

AmplabJenkins commented Jul 16, 2019

Uh oh!

AmplabJenkins commented Jul 16, 2019

Uh oh!

hartikainen left a comment

Choose a reason for hiding this comment

Uh oh!

hartikainen Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

hartikainen Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

richardliaw Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raulchen commented Jul 16, 2019

Uh oh!

hartikainen commented Jul 16, 2019

Uh oh!

AmplabJenkins commented Jul 16, 2019

Uh oh!

AmplabJenkins commented Jul 16, 2019

Uh oh!

AmplabJenkins commented Jul 16, 2019

Uh oh!

AmplabJenkins commented Jul 16, 2019

Uh oh!

AmplabJenkins commented Jul 16, 2019

Uh oh!

raulchen commented Jul 16, 2019

Uh oh!

AmplabJenkins commented Jul 16, 2019

Uh oh!