[Dashboard] Migrate Logs page to use state api. #41474

alanwguo · 2023-11-29T02:42:40Z

Why are these changes needed?

Migrates to use state-api, to unify behavior with CLI and UI
Delete log_proxy API, it's legacy and has some security issues.

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

Delete log_proxy API Signed-off-by: Alan Guo <aguo@anyscale.com>

Signed-off-by: Alan Guo <aguo@anyscale.com>

rkooo567

It seems good to me in my first look! I think for the frontend code, it'd be great other experience team members take a look at it. (I will wait for their approval)

rkooo567 · 2023-11-29T03:31:46Z

dashboard/modules/log/log_agent.py

@@ -296,7 +296,7 @@ async def ListLogs(self, request, context):
            )
        log_files = []
        for p in path.glob(request.glob_filter):
-            log_files.append(str(p.relative_to(path)))
+            log_files.append(str(p.relative_to(path)) + ("/" if p.is_dir() else ""))


does it work with nested directory inside log dir btw?

(it doesn't have to work imo, but i'd like to just know)

yup, I tested this

rkooo567 · 2023-11-29T03:36:12Z

dashboard/client/src/pages/log/Logs.tsx

+              </Button>
+              <SearchInput
+                defaultValue={fileName}
+                label="File Name"


Btw, this doesn't seem to work when I used it

oh shoot, I didn't test this. let me test thi

Signed-off-by: Alan Guo <aguo@anyscale.com>

brycehuang30

Frontend code LGTM! Though I have limited context and I didn't test-run the code.

brycehuang30 · 2023-11-29T05:15:00Z

dashboard/client/src/pages/log/Logs.tsx

+            const fileNameWithoutParent = fileName.startsWith(parentFolder)
+              ? fileName.substring(parentFolder.length)
+              : fileName;


To confirm, the files listed by listStateApiLogs could be either filepath or filename, so that's why we need this branching, right?

the files listed by listStateApiLogs contains the full path of the logs relative to the log root. This logic just hides the parent directories from the file name when browsing through folders.

Signed-off-by: Alan Guo <aguo@anyscale.com>

pcmoritz · 2023-11-29T06:04:56Z

@alanwguo Thanks a lot for doing this -- there is still a failing test, can you fix it? https://buildkite.com/ray-project/premerge/builds/13040#018c197d-5f5b-4abd-999b-08be7f0af010

pcmoritz

I tried it locally and it is working :)

alanwguo · 2023-11-29T07:14:12Z

verified that multi-node log viewing continues to work. will fix the faiiling test

Signed-off-by: Alan Guo <aguo@anyscale.com>

alanwguo · 2023-11-29T07:49:04Z

verified again with latest changes.
This should be good to merge as soon as tests pass

pcmoritz · 2023-11-29T08:14:56Z

@alanwguo There seem to be a few more Javascript test assertions failing :)

Signed-off-by: Alan Guo <aguo@anyscale.com>

alanwguo · 2023-11-29T10:06:18Z

ugh. at least the test case is properly checking things. Okay. I've updated the test cases and it should be passing now...

Migrates to use state-api, to unify behavior with CLI and UI Delete log_proxy API, it's legacy and has some security issues. --------- Signed-off-by: Alan Guo <aguo@anyscale.com>

Migrates to use state-api, to unify behavior with CLI and UI Delete log_proxy API, it's legacy and has some security issues. --------- Signed-off-by: Alan Guo <aguo@anyscale.com> Co-authored-by: Alan Guo <aguo@anyscale.com>

* [Dashboard] Prevent Directory Traversal (#39018) Ensure we can only go to subdirectories of logs and static resources. * [core][state][log] State API should not allow reading files outside of the ray log directory on all ray nodes. (#41467) State API log retrieval has a security bug where one could pass: relative paths like "../../../xxx" to get file outside of ray's log dir absolute path that's refers to other files to get file somewhere else. This PR fixes both issues such that one could only read logs under the ray logs directory. --------- Signed-off-by: rickyyx <rickyx@anyscale.com> * [Dashboard] Migrate Logs page to use state api. (#41474) Migrates to use state-api, to unify behavior with CLI and UI Delete log_proxy API, it's legacy and has some security issues. --------- Signed-off-by: Alan Guo <aguo@anyscale.com> * [core][state][log] Enable following symlinks that point outside of the `root_log_dir` when resolving paths (#41502) Follow-up to: #41467. The change incidentally broke log retrieval on mac os because /tmp is a symlink to /private/tmp. This PR avoids resolving the symlink until after we do the subdir check. This solves the mac os problem and generically enables file paths that contain symlinks outside of the root_log_dir. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> --------- Signed-off-by: rickyyx <rickyx@anyscale.com> Signed-off-by: Alan Guo <aguo@anyscale.com> Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Ian Rodney <ian.rodney@gmail.com> Co-authored-by: Ricky Xu <xuchen727@hotmail.com> Co-authored-by: Alan Guo <aguo@anyscale.com> Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>

[Dashboard] Migrate Logs page to use state api.

d047cbf

Delete log_proxy API Signed-off-by: Alan Guo <aguo@anyscale.com>

alanwguo requested review from rickyyx and rkooo567 November 29, 2023 02:42

alanwguo added 2 commits November 28, 2023 18:43

fixup

9fb8c4e

Signed-off-by: Alan Guo <aguo@anyscale.com>

add test

f93cf59

Signed-off-by: Alan Guo <aguo@anyscale.com>

rkooo567 approved these changes Nov 29, 2023

View reviewed changes

alanwguo added 3 commits November 28, 2023 20:26

fix not using glob for filename search

6d89262

Signed-off-by: Alan Guo <aguo@anyscale.com>

fix log links from other parts of the UI

d97dcf2

Signed-off-by: Alan Guo <aguo@anyscale.com>

fix lint

20433a9

Signed-off-by: Alan Guo <aguo@anyscale.com>

brycehuang30 approved these changes Nov 29, 2023

View reviewed changes

only show logs for live nodes

be562f9

Signed-off-by: Alan Guo <aguo@anyscale.com>

pcmoritz approved these changes Nov 29, 2023

View reviewed changes

alanwguo added 2 commits November 28, 2023 23:15

delete outdated test

009ffff

Signed-off-by: Alan Guo <aguo@anyscale.com>

filter for ALIVE nodes correctly

7043cd3

Signed-off-by: Alan Guo <aguo@anyscale.com>

pcmoritz approved these changes Nov 29, 2023

View reviewed changes

fix test and add test case for dead nodes

079c8d8

Signed-off-by: Alan Guo <aguo@anyscale.com>

pcmoritz merged commit 2a6c6ee into ray-project:master Nov 29, 2023
8 of 15 checks passed

pcmoritz mentioned this pull request Nov 30, 2023

[Release] Cherry pick #41474 into 2.8.1 #41522

Merged

8 tasks

pcmoritz mentioned this pull request Nov 30, 2023

[Release] Cherry pick #39018, #41474, #41467, #41502 into 2.7.2 #41523

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] Migrate Logs page to use state api. #41474

[Dashboard] Migrate Logs page to use state api. #41474

alanwguo commented Nov 29, 2023 •

edited

Loading

rkooo567 left a comment

rkooo567 Nov 29, 2023

rkooo567 Nov 29, 2023

alanwguo Nov 29, 2023

rkooo567 Nov 29, 2023

alanwguo Nov 29, 2023

alanwguo Nov 29, 2023

brycehuang30 left a comment

brycehuang30 Nov 29, 2023

alanwguo Nov 29, 2023

pcmoritz commented Nov 29, 2023

pcmoritz left a comment •

edited

Loading

alanwguo commented Nov 29, 2023

alanwguo commented Nov 29, 2023

pcmoritz commented Nov 29, 2023

alanwguo commented Nov 29, 2023 •

edited

Loading

[Dashboard] Migrate Logs page to use state api. #41474

[Dashboard] Migrate Logs page to use state api. #41474

Conversation

alanwguo commented Nov 29, 2023 • 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

brycehuang30 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoritz commented Nov 29, 2023

pcmoritz left a comment • edited Loading

Choose a reason for hiding this comment

alanwguo commented Nov 29, 2023

alanwguo commented Nov 29, 2023

pcmoritz commented Nov 29, 2023

alanwguo commented Nov 29, 2023 • edited Loading

alanwguo commented Nov 29, 2023 •

edited

Loading

pcmoritz left a comment •

edited

Loading

alanwguo commented Nov 29, 2023 •

edited

Loading