[Core] Enable log rotation for Raylet and GCS server

[kfstorm]

Why are these changes needed?

This PR adds log rotation functionality for Raylet and GCS server by setting a non-empty log_dir when invoking ray::StartRayLog. This is critical for production environments to avoid logs taking too much disk space.

After this PR, raylet_{pid}.log/gcs_server_{pid}.log and potentially raylet_{pid}.{number}.log/gcs_server_{pid}.{number}.log are created as well as the existing .out and .err files. See the sample below.

dashboard_agent.log
dashboard.log
debug_state_gcs.txt
debug_state.txt
events
gcs_server_309463.log
gcs_server.err
gcs_server.out
log_monitor.log
monitor.err
monitor.log
monitor.out
old
python-core-driver-01000000ffffffffffffffffffffffffffffffffffffffffffffffff_309360.log
python-core-worker-28ccdc393f85a1b96961f0794c9305256c6e02d2d91d2cce1ea76aa6_309765.log
raylet_309662.log
raylet.err
raylet.out
worker-28ccdc393f85a1b96961f0794c9305256c6e02d2d91d2cce1ea76aa6-01000000-309765.err
worker-28ccdc393f85a1b96961f0794c9305256c6e02d2d91d2cce1ea76aa6-01000000-309765.out

TODO:

• Code changes in dashboard and agent.
• More test updates and fixes.

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/master/.
• 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 :(

[edoakes]

Assigning to @rkooo567 because he looked into this in the past and has the most context.

[rkooo567]

Thanks for the PR! It is indeed an important feature (and I've heard some issues regarding the disk usage!)

A couple high level comments before deep diving;

1. We should update this doc. https://docs.ray.io/en/master/ray-observability/ray-logging.html. Can you include changes here?
2. Why not completely removing gcs_server.out and gcs_server.err? We can't because of log monitor? (unless we remove it, log rotation has no meaning?)
3. Why do we need {pid} for raylet? Is it because new raylet can be started on that node again? (isn't this creating a new session name anyway?)

[kfstorm]

We should update this doc. docs.ray.io/en/master/ray-observability/ray-logging.html. Can you include changes here?

No problem.

Why not completely removing gcs_server.out and gcs_server.err? We can't because of log monitor? (unless we remove it, log rotation has no meaning?)

In some cases, logs are printed to stderr or stdout. Cases I can think of are:

1. An unhandled C++ exception is thrown.
2. GRPC or ABSL assertion failure.
3. Dashboard agent C++ logs. (Dashboard agent invokes C++ code without initializing Ray C++ logging and it shares stdour and stderr of Raylet.)
4. ERROR or FATAL logs are always printed to stderr as well. See [Logging] Core worker ERRORs/check failures not streamed to drivers. #12893. I'm not sure if we need to expose Raylet and GCS server error logs to driver as well. If not, we can disable printing to stderr for Raylet and GCS server.

Although stderr and stdout logs are uncommon, I do think they are helpful when debugging issues.

Why do we need {pid} for raylet? Is it because new raylet can be started on that node again? (isn't this creating a new session name anyway?)

This is the default behavior of Ray C++ logging. In the case of multi-node Python tests, I guess log rotation won't work well because multiple processes share the same log filename pattern. However, I haven't read and tested the internal logic of rotating_file_sink of spdlog yet. I can test removing _{pid} suffix and see if they work well.

[kfstorm]

After reading the implementation of log rotation in spdlog https://github.com/gabime/spdlog/blob/v1.x/include/spdlog/sinks/rotating_file_sink-inl.h, I don't think multiple processes with the same filename pattern will work well.

[kfstorm]

@rkooo567 Any more high-level comments before I update the PR?

[rkooo567]

Yeah I think it generally looks good. A couple comments;

1. When we have RAY_LOG(ERROR) or RAY_LOG(FATAL), can we also write logs to .err file (so we will have 2 sinks, .log and .err)? i.e. keep the below behavior

ERROR or FATAL logs are always printed to stderr as well. See #12893. I'm not sure if we need to expose Raylet and GCS server error logs to driver as well. If not, we can disable printing to stderr for Raylet and GCS server.

2. Right now, if you use cluster_utils.py, it will create raylet.1.out, raylet.2.out, and etc.. Can you just quickly verify how it'd look like after this PR?

I will start the full review soon!

[rkooo567]

Also, @wuisawesome @ckw017 would there be any compatibility issue with Loki integration?

[rkooo567]

Also for the compatibility (since there are lots of people who will still do cat gcs_server.out), can you write a comment on the top of the .out log file to see .log file instead?

[ckw017]

Also, @wuisawesome @ckw017 would there be any compatibility issue with Loki integration?

Promtail/Loki should be fine with log rotations

[rkooo567]

Sorry for the delay. I think the code itself is okay, but it is a pretty big backward incompatible change (e.g., it can easily break anyone who scrapes logs now). Let's actually do the API review here, or is it possible to keep the basic log file as raylet.out & gcs_server.out instead of raylet_[pid].log? Potential alternative file name is

raylet.out -> same as the current raylet_[pid].log
raylet.err -> same as now

and something else that's for redirecting stdout.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

• If you'd like to keep this open, just leave any comment, and the stale label will be removed.

[stale]

Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message.

Please feel free to reopen or open a new issue if you'd still like it to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for opening the issue!

[jovany-wang]

@rkooo567
I don't think it's a big backward incompatible change.

1. raylet.log and gcs_server.log are often used by Ray developers, even most of them are Ray core developers. It's not used widely by our users.

2. We can update the developing guide for how to grep log messages from raylet and gcs server.

[jovany-wang]

Maybe CC @scv119 @iycheng @jjyao

[rkooo567]

raylet.log and gcs_server.log are often used by Ray developers, even most of them are Ray core developers. It's not used widely by our users.

Hmm I actually found we explicitly mentioned the file name's backward compatibility is not maintained. So it is probably okay. We still need a doc change (actually I've seen many users reading this log when they debug, and changing the name can break their log scraping depending on how it is implemented). I think we still need an API approval. cc @pcmoritz.

[jovany-wang]

I think we still need an API approval. cc @pcmoritz.

Sure.

[jovany-wang]

@ray-project/ray-docs for docs owner approval.

[jovany-wang]

@ericl @pcmoritz for the core api approval.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

• If you'd like to keep this open, just leave any comment, and the stale label will be removed.

[stale]

Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message.

Please feel free to reopen or open a new issue if you'd still like it to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for opening the issue!