fix(supervisor): reopen the log only when it is actually rotated - #1358
Merged
Merged
Conversation
The logrotate watcher matched `is_remove() || is_modify()`, and on Linux
`recommended_watcher` is inotify, where `is_modify()` also matches
`IN_MODIFY` -- which is what the supervisor's own `write_all` generates.
So a chatty child paid a synchronous `fsync` plus a reopen per read
buffer, on its own output, with nothing rotating anything.
Measured with `strace -e trace=fsync,openat` against the real binary,
8 MiB of child output through one supervised process:
before: 1145 fsync, 1146 opens of a log nothing had rotated
after: 0 fsync, 1 open
Match the events that mean the path stopped naming the file we hold: a
remove, or a rename. inotify reports `IN_MOVED_FROM` as `Modify(Name(_))`
rather than as a remove, so `logrotate(8)`'s default mode is still
followed. A truncate-in-place rotation -- what `dstack-vmm`'s own
`logrotate` module does, and `logrotate(8)`'s `copytruncate` -- needs no
reopen: the file is opened `O_APPEND`, so the next write lands at its new
end. That is the requirement the vmm module documents, and the "reopens
them when they change" half of its comment was never what made it hold.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The log redirect watcher reopened on
is_remove() || is_modify(). On Linux, inotify reports the supervisor's ownwrite_allasModify(Data), so every read buffer cost anfsyncand a reopen with nothing rotating. Measured with strace, 8 MiB through one child: 1145 fsyncs and 1146 opens before, 0 and 1 after.Fix
Reopen only on
Remove(_)orModify(Name(_)). A rename (logrotate's default) isModify(Name). Truncate-in-place (copytruncate, dstack-vmm's own rotation) needs no reopen because the fd isO_APPEND. Also fixed the stale comment invmm/src/app.rs.Verification
cargo test -p supervisor, fmt and clippy clean.Split out of #1263.