[Dashboard] Start the new dashboard #9860

fyrestone · 2020-08-01T04:42:32Z

This PR contains the following features:

set the RAY_USE_NEW_DASHBOARD environment var before ray.init(include_dashboard=True) will stat the new dashboard.
raylet fork new dashboard agent if RAY_USE_NEW_DASHBOARD environment var exists.
New dashboard head starts a grpc server (all head modules can extend the grpc server).
New dashboard agent starts a http server (all agent modules can extend the http server).
Add basic tests for new dashboard.
ClassMethodRouteTable for head and agent http routes.
Port features from reporter.py to reporter module.
Some other improvements.

Why are these changes needed?

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://docs.ray.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/.
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested (please justify below)

…w dashboard startup

AmplabJenkins · 2020-08-01T04:51:01Z

Can one of the admins verify this patch?

AmplabJenkins · 2020-08-01T14:40: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/29276/
Test FAILed.

AmplabJenkins · 2020-08-01T14:57:30Z

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

AmplabJenkins · 2020-08-01T15:50:04Z

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

AmplabJenkins · 2020-08-01T15:58:34Z

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

AmplabJenkins · 2020-08-05T06:06:09Z

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

rkooo567

LGTM in general. I have several questions though.

It is really lacking tests. We need A LOT of tests to make sure it works properly. What's you guys test plan?
What's the reason why each agents run http servers? It seems to be unncessary.
Also, what's the reason why the head dashboard has grpc endpoints?

dashboard/agent.py

rkooo567 · 2020-08-05T06:03:13Z

dashboard/agent.py

+ app.add_routes(routes=routes.bound_routes())
+
+ # Enable CORS on all routes.
+ cors = aiohttp_cors.setup(


Previously, we disabled cors when it was dev mode because frontend usually wants to start a react server. We aren't doing that anymore?

Currently, the CORS is for the new log module. Each agent starts a http server for log file access. Allow new dashboard frontend to access log file as static resource directly.

Hmm, so we allow access to all methods on all routes of the app. Isn't this a security risk?

dashboard has the same access level with agent. We can access all routes of dashboard, why disallow access to all routes of agent?

The loghttp server on each agent can reduce the load of the dashboard.

rkooo567 · 2020-08-05T06:07:55Z

dashboard/agent.py

+ logger.info("Registered %s routes.", len(dump_routes))
+
+ # Write the dashboard agent port to redis.
+ await self.aioredis_client.set(


We should remove this from Redis when agents are shutdown. One of the most common workflow of Ray is using autoscaling clusters (and we will invest lots of resources on this), and this means agents will be killed occasionally from the cluster.

The agent will be restarted after killed, and the new value will be set to Redis. So, can we leave this from Redis when agents are shutdown?

If a node is removed from the cluster, will the redis entry for the agent from that node be cleaned up?

No, If a node is removed from the cluster, the entry for the agent will not be cleaned up. Dashboard use the node ip to access agent entry, so the dead agent entry is not a problem. If we really need to cleanup the dead agent entry, we can:

cleanup the agent entry by GCS. (node can't ensure the cleanup)

set TTL for the agent entry, agent will update the TTL periodically.

I think this is not a big deal for now if the only concern is having unused entries to GCS.

dashboard/head.py

python/setup.py

src/ray/gcs/gcs_server/gcs_actor_scheduler.cc

src/ray/raylet/main.cc

rkooo567 · 2020-08-05T06:30:57Z

src/ray/raylet/agent_manager.cc

+
+ RAY_LOG(WARNING) << "Agent process with pid " << child.GetId()
+ << " exit, return value " << exit_code;
+ RAY_UNUSED(delay_executor_([this] { StartAgent(); },


We should kill this thread in this case?

The monitor_thread has been detached. So, the thread will be exit after this lambda returns.

Ah, that makes sense. I was confused.

Co-authored-by: SangBin Cho <[email protected]>

AmplabJenkins · 2020-08-05T08:03:07Z

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

Co-authored-by: SangBin Cho <[email protected]>

AmplabJenkins · 2020-08-05T11:13: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/29444/
Test FAILed.

AmplabJenkins · 2020-08-06T12:07:11Z

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

AmplabJenkins · 2020-08-07T03:31:56Z

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

qicosmos · 2020-08-07T06:11:46Z

src/ray/util/process.cc

@@ -395,7 +395,7 @@ int Process::Wait() const {
 return status;
 }

-void Process::Kill() {
+void Process::Kill() const {


Maybe there are some risks of the Process, i don't see any unit test for the Process.
To be honest i don't think it is necessary to write such a Process, because boost.process has already done all the things, i recommend that we use boost.process instead of current Process.
@mehrdadn @rkooo567 @mfitton

Actually Boost.Process is what we tried before this. It turned out that Boost.Process was quite unstable and had bugs (and slowed down compilation dramatically too), and after struggling with it for over 1 month, we had to scrap it and roll our own solution, which works fine. Please don't revert back to it; it is very likely to cause problems again.

@fyrestone Regarding the const, this shouldn't be const. What was the reason for adding const here?

Actually Boost.Process is what we tried before this. It turned out that Boost.Process was quite unstable and had bugs (and slowed down compilation dramatically too), and after struggling with it for over 1 month, we had to scrap it and roll our own solution, which works fine. Please don't revert back to it; it is very likely to cause problems again.

@fyrestone Regarding the const, this shouldn't be const. What was the reason for adding const here?

Got it. It's necessary to add some useful interface for Process: WaitFor, WaitUntil.
Another suggestion, Process should forbidden copy construct and copy assign function, just move only will be better.
Hope you could add unit tests for Process, especially the exception tests.

These are not trivial changes to make correctly. I don't think we should alter this specific class without compelling reasons.

The entire design of the API here was honestly quite deliberate. For example, I specifically allowed copying because the initial code used pid_t everywhere and this ensured semantic equivalence and minimized the changes of bugs being introduced. Requiring unique ownership would require a significant refactor of the rest of the codebase to accommodate it. If someone feels compelled to do it and can do it without introducing bugs, I suppose they can go ahead, but it is a significant effort and in the end there seems to be very little to gain.

Note that the primary intention for this class is to provide an easy-to-use interface that works correctly across all operating systems. While there are certain features missing that could be added and potentially come in handy, the limited interface here was primarily designed to satisfy our actual needs without adding unnecessary maintenance burden. Process management is possibly the biggest source of pain when porting code across operating systems, and every change would have to be shown to work correctly on all 3 platforms with comparable semantics.

Overall, this is one of the few classes classes where it's not a good idea to change it just for the sake of elegance. If there is a functionality we actually need, then perhaps that warrants adding it. But as long as it is working correctly and other solutions can be reasonably implemented on top, it should probably be left alone.

Actually Boost.Process is what we tried before this. It turned out that Boost.Process was quite unstable and had bugs (and slowed down compilation dramatically too), and after struggling with it for over 1 month, we had to scrap it and roll our own solution, which works fine. Please don't revert back to it; it is very likely to cause problems again.

@fyrestone Regarding the const, this shouldn't be const. What was the reason for adding const here?

I have removed const from Process::Kill().

src/ray/util/asio_util.h

src/ray/raylet/agent_manager.cc

AmplabJenkins · 2020-08-07T08:37:04Z

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

AmplabJenkins · 2020-08-08T03:39:50Z

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

AmplabJenkins · 2020-08-10T03:54:02Z

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

rkooo567 · 2020-08-10T16:24:32Z

Please make sure to pass all CIs. Also, please don't merge the PR until @mfitton approves!

AmplabJenkins · 2020-08-11T03:43:50Z

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

mfitton · 2020-08-11T21:37:04Z

Approved, but as Sang said, let's be careful not to break CI when merging.

This reverts commit 739933e.

刘宝 added 8 commits July 27, 2020 19:45

Use new dashboard if environment var RAY_USE_NEW_DASHBOARD exists; ne…

f54d8eb

…w dashboard startup

Make fake client/build/static directory for dashboard

c89e64a

Add test_dashboard.py for new dashboard

39a3143

Travis CI enable new dashboard test

96042fd

Update new dashboard

ae042ec

Agent manager service

fd8bbb2

Merge remote-tracking branch 'origin_ray/master' into dashboard_switch

7abc734

Add agent manager

841c745

刘宝 added 3 commits August 1, 2020 13:32

Merge remote-tracking branch 'origin_ray/master' into dashboard_switch

bdfdd56

Register agent to agent manager

848d2fd

Add a new line to the end of agent_manager.cc

b7c3d97

fyrestone changed the title ~~[WIP][Dashboard] Start new dashboard~~ [Dashboard] Start new dashboard Aug 1, 2020

fyrestone changed the title ~~[Dashboard] Start new dashboard~~ [Dashboard] Start the new dashboard Aug 1, 2020

fyrestone requested review from rkooo567 and raulchen August 3, 2020 09:26

rkooo567 self-assigned this Aug 3, 2020

刘宝 added 2 commits August 5, 2020 13:00

Merge remote-tracking branch 'origin_ray/master' into dashboard_switch

d4c1076

Fix merge; Fix lint

8b900d9

rkooo567 reviewed Aug 5, 2020

View reviewed changes

Update dashboard/agent.py

1fc7207

Co-authored-by: SangBin Cho <[email protected]>

Update dashboard/head.py

bca3fb3

Co-authored-by: SangBin Cho <[email protected]>

Fix bug

8c9544f

Fix

a2b3393

Merge remote-tracking branch 'origin_ray/master' into dashboard_switch

7e63177

qicosmos reviewed Aug 7, 2020

View reviewed changes

Remove const from Process::Kill() & Fix bugs

4969f56

Revert error check of execute_after

7082824

刘宝 added 2 commits August 10, 2020 10:30

Raise exception from DashboardAgent.run

71c220d

Add more tests.

8084c8e

rkooo567 approved these changes Aug 10, 2020

View reviewed changes

Fix compile on Linux

b98543f

mfitton approved these changes Aug 11, 2020

View reviewed changes

刘宝 added 4 commits August 12, 2020 11:12

Merge remote-tracking branch 'origin_ray/master' into dashboard_switch

08bbd48

Use dict comprehension instead of dict(generator)

3b212ff

Fix lint

f2df79b

Merge remote-tracking branch 'origin_ray/master' into dashboard_switch

c2e3f08

raulchen merged commit 739933e into ray-project:master Aug 13, 2020

raulchen deleted the dashboard_switch branch August 13, 2020 03:01

robertnishihara added a commit to robertnishihara/ray that referenced this pull request Aug 14, 2020

Revert "[Dashboard] Start the new dashboard (ray-project#9860)"

b7c413f

This reverts commit 739933e.

robertnishihara mentioned this pull request Aug 14, 2020

Revert "[Dashboard] Start the new dashboard (#9860)" #10116

Merged

rkooo567 pushed a commit that referenced this pull request Aug 14, 2020

Revert "[Dashboard] Start the new dashboard (#9860)" (#10116)

36e626e

This reverts commit 739933e.

fyrestone restored the dashboard_switch branch August 15, 2020 09:10

fyrestone mentioned this pull request Aug 15, 2020

[Dashboard] Start the new dashboard #10131

Merged

6 tasks

fyrestone mentioned this pull request Jan 7, 2021

Fix passing env on windows #13253

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] Start the new dashboard #9860

[Dashboard] Start the new dashboard #9860

fyrestone commented Aug 1, 2020 •

edited

Loading

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 5, 2020

rkooo567 left a comment

rkooo567 Aug 5, 2020

fyrestone Aug 5, 2020

mfitton Aug 5, 2020

fyrestone Aug 6, 2020

rkooo567 Aug 5, 2020

fyrestone Aug 5, 2020

mfitton Aug 5, 2020

fyrestone Aug 6, 2020 •

edited

Loading

rkooo567 Aug 10, 2020

rkooo567 Aug 5, 2020

fyrestone Aug 5, 2020

rkooo567 Aug 6, 2020

AmplabJenkins commented Aug 5, 2020

AmplabJenkins commented Aug 5, 2020

AmplabJenkins commented Aug 6, 2020

AmplabJenkins commented Aug 7, 2020

qicosmos Aug 7, 2020

mehrdadn Aug 7, 2020

qicosmos Aug 7, 2020 •

edited

Loading

mehrdadn Aug 7, 2020 •

edited

Loading

fyrestone Aug 7, 2020

AmplabJenkins commented Aug 7, 2020

AmplabJenkins commented Aug 8, 2020

AmplabJenkins commented Aug 10, 2020

rkooo567 commented Aug 10, 2020

AmplabJenkins commented Aug 11, 2020

mfitton commented Aug 11, 2020

[Dashboard] Start the new dashboard #9860

[Dashboard] Start the new dashboard #9860

Conversation

fyrestone commented Aug 1, 2020 • edited Loading

Why are these changes needed?

Related issue number

Checks

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 1, 2020

AmplabJenkins commented Aug 5, 2020

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

fyrestone Aug 6, 2020 • 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

AmplabJenkins commented Aug 5, 2020

AmplabJenkins commented Aug 5, 2020

AmplabJenkins commented Aug 6, 2020

AmplabJenkins commented Aug 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qicosmos Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

mehrdadn Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Aug 7, 2020

AmplabJenkins commented Aug 8, 2020

AmplabJenkins commented Aug 10, 2020

rkooo567 commented Aug 10, 2020

AmplabJenkins commented Aug 11, 2020

mfitton commented Aug 11, 2020

fyrestone commented Aug 1, 2020 •

edited

Loading

fyrestone Aug 6, 2020 •

edited

Loading

qicosmos Aug 7, 2020 •

edited

Loading

mehrdadn Aug 7, 2020 •

edited

Loading