You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
There were two issues with the log rotation script:
Logs could be lost since copying and then truncating the file might not finish before logs arrive.
The otel collector application will keeps the file handle and offset cached. After truncating, it will write starting at the last offset leaving the unallocated garbage in the beginning of the file. This garbage uses space.
This commit moves the file instead of copying. That allows the collector to continue writing to the rolled file until a SIGHUP is sent. This causes a config refresh, which also opens a new log file. After, the rolled file and the new log file have correct sizes.
-- ADDITIONAL NOTES:
Claude's code review is not accurate here.
The alpine image is based on busybox and fuser is a command implemented by busybox. This can be verified by just running the collector and watching the log rotate behavior.
The mv command updates the name of the file in the file system but doesn't change the inode number. A process only uses the file path the first time the file is open to resolve it into a inode number. Moving the file changes the name but doesn't change the inode number so the process will continue to write to that file.
There were two issues with the log rotation script:
1. Logs could be lost since copying and then truncating the file might
not finish before logs arrive.
2. The otel collector application will keeps the file handle and offset
cached. After truncating, it will write starting at the last offset
leaving the unallocated garbage in the beginning of the file. This
garbage uses space.
This commit moves the file instead of copying. That allows the collector
to continue writing to the rolled file until a SIGHUP is sent. This causes
a config refresh, which also opens a new log file. After, the rolled file
and the new log file have correct sizes.
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
❌ Race condition in SIGHUP signal (log-rotator.sh:42) → fuser -k -HUP sends signal to processes using .1 file, but collector may have already created new log file and closed old handle. Move SIGHUP before mv or target the collector PID directly
⚠️Error handling missing → Add checks: (1) verify mv succeeded before sending SIGHUP, (2) verify fuser command exists and succeeds
⚠️Potential log loss window → Between mv and SIGHUP completion, new log writes go to old file handle (moved file). Document this behavior or add verification that collector reopened file
Additional Concerns
🤔 fuser dependency → While busybox provides fuser, the alpine base image installation is implicit. Consider explicit verification or alternative signal mechanism (e.g., pkill -HUP targeting process name)
📝 Incomplete rotation cleanup → Line 46 removes . after rotation, but if rotation fails mid-process, could accumulate extra files. Consider cleanup in error paths
Recommendation
Test the SIGHUP timing thoroughly - the current implementation may work if collector handles SIGHUP quickly, but the race condition between file move and signal delivery could cause issues under load.
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.
dhable
force-pushed
the
dan/hdx-2747-otel-collector-docker-image-logrotatorsh-does-not-truncate
branch
from
There were two issues with the log rotation script:
Logs could be lost since copying and then truncating the file might not finish before logs arrive.
The otel collector application will keeps the file handle and offset cached. After truncating, it will write starting at the last offset leaving the unallocated garbage in the beginning of the file. This garbage uses space.
This commit moves the file instead of copying. That allows the collector to continue writing to the rolled file until a SIGHUP is sent. This causes a config refresh, which also opens a new log file. After, the rolled file and the new log file have correct sizes.
--
ADDITIONAL NOTES:
Claude's code review is not accurate here.