[UI] [Nodes] Disable actions for dead nodes #34387

chaowanggg · 2023-04-14T00:15:03Z

Why are these changes needed?

We are going to remove logs for dead nodes and remove Stack Trace and CPU Flame Graph for dead workers.
Otherwise, users will click the link and jump to a page without information they need.

Alive Nodes	Dead Nodes

Summary of Changes

hide logs for dead nodes in NodeRow.tsx

Discussion and Confusion

As part of the PR, we have also come to an agreement, which I will list here for future reference.

There are three entities on the Cluster page: node, raylet, and worker.
We have a state field for node and raylet, but we do not have the state of the worker returned from the backend API.
In Ray, a node is equal to a raylet.

Related issue number

fixes #32593

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: chaowang <[email protected]>

chaowanggg · 2023-04-14T00:17:41Z

needs more discussion with @scottsun94 about how to deal with dead workers logs

alanwguo

For all UI changes, please add a screenshot to the PR description.

dashboard/client/src/pages/node/NodeRow.tsx

alanwguo · 2023-04-14T18:09:56Z

dashboard/client/src/pages/node/NodeRow.tsx

- CPU&nbsp;Flame&nbsp;Graph
- </a>
- <br />
+ {/* )} */}


dashboard/client/src/pages/node/NodeRow.component.test.tsx

chaowanggg · 2023-04-18T00:34:43Z

@alanwguo I am curious why do we hardcode the status of a worker as "ALIVE"?

For this task, I need to hide CPU profiling & stack trace for DEAD workers and I think I could judge whether it is dead by using "is_alive" prop from the API

ray/dashboard/client/src/pages/node/NodeRow.tsx

Line 190 in 50aebb8

alanwguo · 2023-04-18T00:43:20Z

@alanwguo I am curious why do we hardcode the status of a worker as "ALIVE"?

For this task, I need to hide CPU profiling & stack trace for DEAD workers and I think I could judge whether it is dead by using "is_alive" prop from the API

ray/dashboard/client/src/pages/node/NodeRow.tsx

Line 190 in 50aebb8

<StatusChip type="worker" status="ALIVE" />

Hm. I'm not sure. I think at one point we only showed alive Nodes in the list view but that is no longer the case. Please remove the hard-coding.

chaowanggg · 2023-04-18T00:49:35Z

@alanwguo I am curious why do we hardcode the status of a worker as "ALIVE"?
For this task, I need to hide CPU profiling & stack trace for DEAD workers and I think I could judge whether it is dead by using "is_alive" prop from the API

ray/dashboard/client/src/pages/node/NodeRow.tsx

Line 190 in 50aebb8

<StatusChip type="worker" status="ALIVE" />

Hm. I'm not sure. I think at one point we only showed alive Nodes in the list view but that is no longer the case. Please remove the hard-coding.

Thanks Alan. I just wanted to double-confirm that the backend logic has filtered the workers and is only returning "ALIVE" workers to the front end. This is because I need to display different UI for workers that are "ALIVE" versus those that are "DEAD".

Could you please let me know whom I should contact to confirm the backend logic, or provide me with a code pointer that I can refer to?

alanwguo · 2023-04-18T00:51:41Z

Please check get_all_nodes in node_head.py

chaowanggg · 2023-04-18T23:37:33Z

@alanwguo In this PR, I need to demonstrate distinct UIs for dead workers and alive workers. However, I'm unable to determine the state of a worker(not node) using the available API, as it doesn't include a "state" or "is_alive" field

chaowanggg · 2023-04-18T23:38:24Z

Please check get_all_nodes in node_head.py

Double confirmed, no filtering in get_all_nodes function

alanwguo · 2023-04-19T00:31:33Z

@alanwguo In this PR, I need to demonstrate distinct UIs for dead workers and alive workers. However, I'm unable to determine the state of a worker(not node) using the available API, as it doesn't include a "state" or "is_alive" field

I think all workers are guaranteed to be alive. We don't show information about dead workers.

Can we apply this change only to nodes for this PR?

chaowanggg · 2023-04-19T00:40:50Z

@scottsun94 All workers are guaranteed to be alive and we don't show information about dead workers.

According to this figma. Do you think we could skip the UI changes for workers?

scottsun94 · 2023-04-19T04:39:39Z

@scottsun94 All workers are guaranteed to be alive and we don't show information about dead workers.

According to this figma. Do you think we could skip the UI changes for workers?

SG. Let's skip it for this PR.

However, is there any reason why we don't expose/have the info about the dead workers? @rkooo567
Let's say if a worker died because of OOMs, and usually the error message asks people to look at the worker logs. If a dead worker is gone, they have to go to the logs UI to find the logs. With the new worker log change, it's really hard for people to find it any more...

chaowanggg · 2023-04-19T16:58:18Z

dashboard/client/src/pages/node/NodeRow.tsx

@@ -94,7 +95,9 @@ export const NodeRow = ({
 </Box>
 </TableCell>
 <TableCell>
- <Link to={`/logs/${encodeURIComponent(logUrl)}`}>Log</Link>


I tried to get the state of a node to decide whether to show logs or hide logs. But I noticed that in line 78, we are using raylet.state.

<TableCell> <StatusChip type="node" status={raylet.state} /> </TableCell>

Thus, we have two state here, node's state and raylet's state.

Why do we need to show raylet's state here? Do I need to hide or show the logs based on raylet's state?
@alanwguo

@rkooo567 can you explain the difference between these states?

node == raylet in Ray

Besides, we can refactor these API responses from the dashboard at some point..

rkooo567 · 2023-04-19T23:24:12Z

dashboard/client/src/pages/node/NodeRow.tsx

@@ -94,7 +95,9 @@ export const NodeRow = ({
 </Box>
 </TableCell>
 <TableCell>
- <Link to={`/logs/${encodeURIComponent(logUrl)}`}>Log</Link>


Besides, we can refactor these API responses from the dashboard at some point..

rkooo567 · 2023-04-19T23:24:33Z

oops sorry ignore my aproval ^

Signed-off-by: chaowang <[email protected]>

We are going to remove logs for dead nodes and remove Stack Trace and CPU Flame Graph for dead workers. Otherwise, users will click the link and jump to a page without information they need. Signed-off-by: Jack He <[email protected]>

We are going to remove logs for dead nodes and remove Stack Trace and CPU Flame Graph for dead workers. Otherwise, users will click the link and jump to a page without information they need.

feat

50f97b7

Signed-off-by: chaowang <[email protected]>

chaowanggg force-pushed the cw/disable_dead_nodes_actions branch from 25f2abc to 50aebb8 Compare April 14, 2023 00:15

chaowanggg marked this pull request as ready for review April 14, 2023 00:17

alanwguo reviewed Apr 14, 2023

View reviewed changes

chaowanggg changed the title ~~[UI] [Nodes] Disable actions for dead nodes & dead workers~~ [UI] [Nodes] Disable actions for dead nodes Apr 19, 2023

chaowanggg marked this pull request as draft April 19, 2023 16:33

chaowanggg commented Apr 19, 2023

View reviewed changes

chaowanggg marked this pull request as ready for review April 19, 2023 20:07

rkooo567 approved these changes Apr 19, 2023

View reviewed changes

chaowanggg added 7 commits April 19, 2023 23:52

code review

afe01b4

Signed-off-by: chaowang <[email protected]>

merge conflict of ActorTable

0466b72

Signed-off-by: chaowang <[email protected]>

feat: unit test

62ee1bc

Signed-off-by: chaowang <[email protected]>

dead or alive worker

84bd9fc

Signed-off-by: chaowang <[email protected]>

revert worker changes

6d9c7c9

Signed-off-by: chaowang <[email protected]>

add new line

a0f6eba

Signed-off-by: chaowang <[email protected]>

dead nodes

b261517

Signed-off-by: chaowang <[email protected]>

chaowanggg force-pushed the cw/disable_dead_nodes_actions branch from 453d9b3 to b261517 Compare April 19, 2023 23:52

chaowanggg requested a review from alanwguo April 19, 2023 23:53

nit

087bf69

Signed-off-by: chaowang <[email protected]>

alanwguo approved these changes Apr 25, 2023

View reviewed changes

chaowanggg assigned rkooo567 Apr 25, 2023

rkooo567 merged commit 1ce7303 into ray-project:master Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UI] [Nodes] Disable actions for dead nodes #34387

[UI] [Nodes] Disable actions for dead nodes #34387

chaowanggg commented Apr 14, 2023 •

edited

Loading

chaowanggg commented Apr 14, 2023

alanwguo left a comment

alanwguo Apr 14, 2023

chaowanggg commented Apr 18, 2023 •

edited

Loading

alanwguo commented Apr 18, 2023

chaowanggg commented Apr 18, 2023

alanwguo commented Apr 18, 2023

chaowanggg commented Apr 18, 2023

chaowanggg commented Apr 18, 2023

alanwguo commented Apr 19, 2023

chaowanggg commented Apr 19, 2023

scottsun94 commented Apr 19, 2023

chaowanggg Apr 19, 2023 •

edited

Loading

alanwguo Apr 19, 2023

rkooo567 Apr 19, 2023

rkooo567 Apr 19, 2023

chaowanggg Apr 19, 2023

rkooo567 Apr 19, 2023

rkooo567 commented Apr 19, 2023

[UI] [Nodes] Disable actions for dead nodes #34387

[UI] [Nodes] Disable actions for dead nodes #34387

Conversation

chaowanggg commented Apr 14, 2023 • edited Loading

Why are these changes needed?

Summary of Changes

Discussion and Confusion

Related issue number

Checks

chaowanggg commented Apr 14, 2023

alanwguo left a comment

Choose a reason for hiding this comment

alanwguo Apr 14, 2023

Choose a reason for hiding this comment

chaowanggg commented Apr 18, 2023 • edited Loading

alanwguo commented Apr 18, 2023

chaowanggg commented Apr 18, 2023

alanwguo commented Apr 18, 2023

chaowanggg commented Apr 18, 2023

chaowanggg commented Apr 18, 2023

alanwguo commented Apr 19, 2023

chaowanggg commented Apr 19, 2023

scottsun94 commented Apr 19, 2023

chaowanggg Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

alanwguo Apr 19, 2023

Choose a reason for hiding this comment

rkooo567 Apr 19, 2023

Choose a reason for hiding this comment

rkooo567 Apr 19, 2023

Choose a reason for hiding this comment

chaowanggg Apr 19, 2023

Choose a reason for hiding this comment

rkooo567 Apr 19, 2023

Choose a reason for hiding this comment

rkooo567 commented Apr 19, 2023

chaowanggg commented Apr 14, 2023 •

edited

Loading

chaowanggg commented Apr 18, 2023 •

edited

Loading

chaowanggg Apr 19, 2023 •

edited

Loading