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

tracing-appender: log rotation doesn't work with musl target #2999

Open
zerolfx opened this issue Jun 6, 2024 · 5 comments · May be fixed by #3000
Open

tracing-appender: log rotation doesn't work with musl target #2999

zerolfx opened this issue Jun 6, 2024 · 5 comments · May be fixed by #3000

Comments

@zerolfx
Copy link

zerolfx commented Jun 6, 2024

When compiled with --target=x86_64-unknown-linux-musl, log rotation doesn't work. That is caused by:

  1. The file creation time cannot be retrieved when compiled with musl. https://users.rust-lang.org/t/musl-and-file-creation-time/111559
  2. When the creation time of a file cannot be obtained, the file is considered nonexistent.
    let created = metadata.created().ok()?;

Suggestion: Fallback to the modification time when the creation time cannot be retrieved. (Modification time is part of POSIX standard but creation time is not.)

@kaffarell
Copy link
Contributor

Well, we could simply write it like this:

let created = metadata.created().ok().or(metadata.modified().ok())?;

or we could wait until rust-lang/rust#125692 gets merged. (The build in the PR worked fine and it seems like it's going to get merged soon.) @mladedav or @davidbarsky what do you think?

@mladedav
Copy link
Contributor

mladedav commented Jun 6, 2024

I honestly don't know much about the appender.

Falling back to the modified timestamp seems better than just not deleting the files at all. But I wonder if we couldn't instead fall back to parsing the name since it should contain the creation time.

And whether there shouldn't be an infallible fallback at the end with UNIX_EPOCH or something, just so we can be sure that whatever happens and even if the system/filesystem doesn't support even the modified timestamp, we delete some files instead of letting the logs grow without bounds. I'm not sure what's more important though, keeping the number of generated files from growing or keeping latest log files for at least as long as configured.

@kaffarell
Copy link
Contributor

Falling back to the modified timestamp seems better than just not deleting the files at all. But I wonder if we couldn't instead fall back to parsing the name since it should contain the creation time.

That's actually a good idea. I think it's even better than using the modified time! With the modified time it could happen that some user manually checks the log files, accidentally modifies it and then it gets deleted on the next iteration. I'll submit a PR with this change!

And whether there shouldn't be an infallible fallback at the end with UNIX_EPOCH or something, just so we can be sure that whatever happens and even if the system/filesystem doesn't support even the modified timestamp, we delete some files instead of letting the logs grow without bounds. I'm not sure what's more important though, keeping the number of generated files from growing or keeping latest log files for at least as long as configured.

My personal view is that it's better to keep more files than promised than less :)

kaffarell added a commit to kaffarell/tracing that referenced this issue Jun 7, 2024
When using the linux-musl target for rust, the file creation time
cannot be retrieved, as the current version does not support it yet (
This will be fixed with [0]). In the meantime, we parse the datetime
from the filename and use that as a fallback.

Fixes: tokio-rs#2999

[0]: rust-lang/rust#125692
@kaffarell kaffarell linked a pull request Jun 7, 2024 that will close this issue
kaffarell added a commit to kaffarell/tracing that referenced this issue Jun 11, 2024
When using the linux-musl target for rust, the file creation time
cannot be retrieved, as the current version does not support it yet (
This will be fixed with [0]). In the meantime, we parse the datetime
from the filename and use that as a fallback.

Fixes: tokio-rs#2999

[0]: rust-lang/rust#125692
kaffarell added a commit to kaffarell/tracing that referenced this issue Jun 12, 2024
When using the linux-musl target for rust, the file creation time
cannot be retrieved, as the current version does not support it yet (
This will be fixed with [0]). In the meantime, we parse the datetime
from the filename and use that as a fallback.

Fixes: tokio-rs#2999

[0]: rust-lang/rust#125692
kaffarell added a commit to kaffarell/tracing that referenced this issue Jun 13, 2024
When using the linux-musl target for rust, the file creation time
cannot be retrieved, as the current version does not support it yet (
This will be fixed with [0]). In the meantime, we parse the datetime
from the filename and use that as a fallback.

Fixes: tokio-rs#2999

[0]: rust-lang/rust#125692
kaffarell added a commit to kaffarell/tracing that referenced this issue Jun 17, 2024
When using the linux-musl target for rust, the file creation time
cannot be retrieved, as the current version does not support it yet (
This will be fixed with [0]). In the meantime, we parse the datetime
from the filename and use that as a fallback.

Fixes: tokio-rs#2999

[0]: rust-lang/rust#125692
@tpoliaw
Copy link

tpoliaw commented Dec 3, 2024

In case anyone else comes across it, this is also the case when logging to an NFS mount with a non-musl build. Looks like #3000 should still fix it though.

@kaffarell
Copy link
Contributor

kaffarell commented Dec 6, 2024

Yep, creation time (birth time) was introduced in NFSv4 and kernel 5.18. But nevertheless not all filesystems support birth date so the PR would be useful anyway.

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 a pull request may close this issue.

4 participants