Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Sync logs to local file #632

Merged
merged 6 commits into from
Nov 1, 2022

Conversation

Basasuya
Copy link
Contributor

@Basasuya Basasuya commented Oct 13, 2022

Why are these changes needed?

support sync log to file in pod.
this feature is controllered by log_file parameter in command line, default only output log to std

Related issue number

#593

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

Usage

use -logFilePath=output.txt when executing apiserver
use -log-file-path=output.txt when executing ray-operator
you can add these parameter in ENTRYPOINT of Dockerfile
then swap the based image in Dockerfile which can exec into

@Basasuya Basasuya changed the title [WIP] [logger] sync to file [logger] sync to file Oct 13, 2022
@Basasuya
Copy link
Contributor Author

manual testing through replace the based image in dockerfile.
the previous image can not exec into it.

ray-operator/main.go Outdated Show resolved Hide resolved
@Jeffwan
Copy link
Collaborator

Jeffwan commented Oct 18, 2022

Overall the change looks good to me beside the parameter pattern

@Basasuya Basasuya force-pushed the basasuya_master_write_to_file branch from e722268 to ee0f729 Compare October 19, 2022 06:07
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @Basasuya for your contribution!

apiserver/cmd/main.go Show resolved Hide resolved
apiserver/cmd/main.go Show resolved Hide resolved
apiserver/pkg/client/cluster.go Show resolved Hide resolved
@Jeffwan Jeffwan changed the title [logger] sync to file [Feature] Sync logs to local file Oct 20, 2022
@Jeffwan
Copy link
Collaborator

Jeffwan commented Oct 20, 2022

@Basasuya can you help address @kevin85421's comment?

@Basasuya Basasuya force-pushed the basasuya_master_write_to_file branch from ee0f729 to 97aac7b Compare October 28, 2022 03:29
@Basasuya
Copy link
Contributor Author

@kevin85421 sorry for the slow reply, I have rebase the master again.

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add more details about how to use this feature in your PR description? This PR will be very helpful for both users and developers to debug. Thank you! Overall, the code looks good to me, and I just added some nit comments on it.

apiserver/cmd/main.go Outdated Show resolved Hide resolved
ray-operator/main.go Outdated Show resolved Hide resolved
@Basasuya
Copy link
Contributor Author

Can you add more details about how to use this feature in your PR description? This PR will be very helpful for both users and developers to debug. Thank you! Overall, the code looks good to me, and I just added some nit comments on it.

I add the detail in PR description.

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Wait for CI to pass.

@kevin85421
Copy link
Member

@Jeffwan is this PR ready to merge? The merge is blocked by your change requests. Thank you!

@Jeffwan Jeffwan merged commit 56b2f61 into ray-project:master Nov 1, 2022
@Jeffwan
Copy link
Collaborator

Jeffwan commented Nov 1, 2022

@kevin85421 Yeah, it looks good to me.

lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
* [logger] sync to file

* fix for UT

* default not output to file

* fix for UT

* fix for review

* fix for review

Co-authored-by: huyuanzhe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants