Add log file rotation #127667
Add log file rotation #127667
Conversation
|
Hi @zylxjtu. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
/triage accepted |
|
Can someone please help to take a look at this PR? @mengjiao-liu |
There was a problem hiding this comment.
First, this file lacks the corresponding test file. After all, it has quite a lot of changes.
Then,I used the following command for simulation, but the result is not as expected.
So I suggest you add the corresponding test file for testing and run it to try.
dd if=/dev/zero of=/tmp/test.log count=1 bs=1M
ls -l /tmp/test.log
cd <kube-log-runner-command-dir>
./kube-log-runner -log-file=/tmp/test.log -log-file-size=1 -also-stdout echo "hello world"
ls -l /tmp/test.*
-rw-r--r-- 1 lmj wheel 1048588 Oct 15 17:06 test-20241015-170629.log
-rw-r--r-- 1 lmj wheel 0 Oct 15 17:06 test.log # According to expectations, there should be an output of "hello world" here. But now there isn't. It's 0.
| ) | ||
|
|
||
| var ( | ||
| logFilePath = flag.String("log-file", "", "If non-empty, save stdout to this file") | ||
| logFileSize = flag.Uint("log-file-size", 0, "Useful with log-file, if non-zero, rotate log file when it reaches this size in MB") |
There was a problem hiding this comment.
Is it possible for users to set the byte unit by themselves? For example, 10 MB or 10GB, instead of only having MB as a fixed unit? 😃
There was a problem hiding this comment.
for the file size to be zero, this is actually one of the issues this PR would like to tackle with, that the log file was not able to be refreshed/synced in time. 5s was added in this PR to refresh the logs, so there will be delay but it will be synced much quicker than the original one
There was a problem hiding this comment.
I'm trying to keep the parameters of "kube-log-runner" to be as simple as possible. This looks to me important for the case if the process loaded by "kube-log-runner" has a lot of parameters itself, such as "kubelet". Also the purpose of log file rotation is for the convenience of users to check the text log file. GB size log file does not seem easy for use to look through. So after second thoughts, I would prefer to keep the options to be MB only
There was a problem hiding this comment.
Ambiguous (size in B, KB, or MB?) command line flags are not simple because the user has to be very careful about using them correctly. If we do this, then we should do it properly, which means accepting a string for a resource.Quantity and parsing it accordingly.
The same argument applies to age: it should accept a time.Duration. There's even a flag.Duration for that.
There was a problem hiding this comment.
This looks to me important for the case if the process loaded by "kube-log-runner" has a lot of parameters itself,
I don't understand this argument. Why should the number of arguments for the process influence what parameters get accepted by kube-log-runner?
There was a problem hiding this comment.
I guess what I mean is prefer the option of avoiding complicated parameters if we can achieve the similar functionality
There was a problem hiding this comment.
updated the log-file-size and log-file-age format, to make it more explicitly.
| ) | ||
|
|
||
| var ( | ||
| logFilePath = flag.String("log-file", "", "If non-empty, save stdout to this file") | ||
| logFileSize = flag.Uint("log-file-size", 0, "Useful with log-file, if non-zero, rotate log file when it reaches this size in MB") | ||
| logFileAge = flag.Uint("log-file-age", 0, "Useful with log-file-size, if non-zero, remove log files older than this many days") |
There was a problem hiding this comment.
Like the above suggestion, can we let users define whether it's a few hours, days or months(e.g. 8h,8d)? Instead of a fixed unit with days as the unit.
There was a problem hiding this comment.
Same as the above comment, there is a trad off between the flexibility and complexity here. I would prefer to make the parameters to be simple if possible
| w.currentSize += int64(len(p)) | ||
|
|
||
| // if file size over maxsize rotate the log file | ||
| if (w.currentSize / 1000000) >= int64(w.maxSize) { |
There was a problem hiding this comment.
The problem is that the file size check for rotation is incorrect because it divides the size by 1,000,000 instead of using the correct conversion for megabytes (1 MB = 1024*1024 bytes). In addition, using division for byte conversion can result in an integer division error; instead, multiplication can be used here.
|
@mengjiao-liu and others, please help to take a look, I wish it can catch the k8s 1.32 release train if possible, thanks |
|
Please squash into one commit as part of the next push. |
|
/remove-area/apiserver |
|
@zylxjtu: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test pull-kubernetes-e2e-kind |
|
/skip |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 8a98dc301d65f4b0fb7c38b93a0fa3e6f7fd0dd3
|
|
Please add a release note in the description. |
|
/approve
Never mind, I can do that myself: /release-note-edit |
|
/release-note-edit |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, zylxjtu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
This is to enhance the kube-log-runner
/kind bug
What this PR does / why we need it:
Initial log-runner does not have log flushing, so the logs will not be able to be flushed in time. Add the flushing (every 5 seconds)
Added the (optional) log rotation, will rotate the logs when the log size exceed the maximum size (configurable), will also clean old logs which age exceed the time (configurable)
Keep the file open for each write, close/reopen the fd only when needed during log rotation for better performance
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
NONE
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: