-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
logging: prune logs on startup #9012
Conversation
testFileName = "Consul.log" | ||
testDuration = 50 * time.Millisecond | ||
testBytes = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this, because test constants like this can actually be harmful instead of helpful.
- it encourages tests which use the same values, instead of different values, which could lead to hidden bugs because only a single value is tested.
- it makes tests less obvious because the input values are hidden away instead of explicit in the test case.
// Prune if there are more files stored than the configured max | ||
stale = len(matches) - l.MaxFiles | ||
|
||
switch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: A comment above this statement would be awesome, not necessary since it isn't that complicated, but would be a nice touch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, even tested locally and got smiley faces. Thanks for this 🤙
31be934
to
653a5d7
Compare
To ensure that files are pruned before a new one is created. Also clean up the logic in pruneFiles
653a5d7
to
bc86896
Compare
Standardize naming Use stricter assertions and reduce boilerplate to make the intent of the tests more obvious. Also explicitly sort the filenames so that the correct files are pruned, and so that the tests can not flake.
bc86896
to
752e3ec
Compare
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/287125. |
🍒✅ Cherry pick of commit 5e69cab onto |
logging: prune logs on startup
The issue was reported here: https://discuss.hashicorp.com/t/log-rotate-max-files-not-working/14834
When Consul starts it creates a new log file, but it only prunes log files when
rotate
is called. This could result in more thanMaxFiles
number of log files.By calling
pruneFiles
on startup we not only ensure the number of files is enforced, we also get the error frompruneFiles
immediately. Normally if a log file fails to write, rotate, or prune we would not see the error anywhere, because where would it go? It can't go to logs because that would likely fail. So seeing any failures to prune on startup should help debug issues with file permissions.Also does a bunch of cleanup in both
pruneFiles
and the tests forLogFile
.