-
Notifications
You must be signed in to change notification settings - Fork 549
Conversation
fix #4777 |
missingok | ||
notifempty | ||
copytruncate | ||
maxsize 300M | ||
size 100M |
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.
So user log can only keep [100M, 200M]? Which is too small.
Let's keep 300M here
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.
You mean [300M, 600M]? or [150M, 300M]. I think keep 600MB logs it too large, I'm OK to change to it [150M,300M]
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.
How about [256, 512]?
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.
256M seems fine. I will raise my concern here
In current config, if log file not exceed size
. The log file will be keep forever. So if we set size to 256M
, and many jobs scheduled to this node write much log. The max log size will be 256M * number_of_jobs_scheduled _to_this_node (even this job is deleted by FC). That's the reason I don't want to keep too much user log here.
After we make a final log collection solution, this will be solved. But for now, still has such issue.
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.
If so why previous 300MB is fine but current 256MB is not?
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.
Before, we use weekly
rotate. Logs exists more than one week will be rotated. And rotated logs exists more than one month will be dropped.
And we compress the rotated logs before.
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.
BTW, does the log forever kept? Why not delete the log after its job is deleted in K8s or DB (our next version)? Because after that, user cannot access the job anyway.
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.
So, change to rotate by size, we also need to support a way to delete log in this PR
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.
...It's not a easy fix and need more efforts. I think this PR is for quick fix and I want to redesign the log collection mechanism in next release.
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.
fixed
@@ -222,6 +222,14 @@ export async function getContainerLog(logUrl) { | |||
throw new Error(`Log not available`); | |||
} | |||
} else if (config.logType === 'log-manager') { | |||
if (text.length === 0) { |
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.
Did you concat these 2 files to expose to user? This may be better to help user, as anyway backend store 2 files.
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.
Front-end only return 16MB logs to end-user. And we already provide a link to log folder, user can find the full logs when he/she need more information.
Concat the two logs will cause fron-end read too much data and slow down the website.
Here we only look the rotated log when origin log file is empty.
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.
If so, you should first concat (may not need real concat) then extract first latest 15M.
Otherwise, if the first file is 1B, and 2nd is 100MB, you can only show the 1B to user
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.
log-rotate doesn't promise not loss log. Concat the two logs may cause strange log string. We'd better give user some hint when the log is rotated.
copytruncate
Truncate the original log file in place after creating a copy, instead of moving the old log file and optionally creating a new one. It can be used when some program cannot be told to close its logfile and thus might continue writing (appending) to the previous log file forever. Note that there is a very small time slice between copying the file and truncating it, so some logging data might be lost. When this option is used, the create option will have no effect, as the old log file stays in place.
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.
Maybe insert one line below is better than just suddenly can only see 1B 1st file:
1st file content
--------log is rotated, may be lost during this--------
2nd file content
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.
OK, will change to this
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.
fixed
We only rotate the log if log size is more than 100MB. Max keep 2 logs, if user still write log, the old log will be dropped.
log-rotate will run every 10 mins