Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

Change logrotate config #4792

Merged
merged 7 commits into from
Aug 9, 2020
Merged

Change logrotate config #4792

merged 7 commits into from
Aug 9, 2020

Conversation

Binyang2014
Copy link
Contributor

@Binyang2014 Binyang2014 commented Aug 7, 2020

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

@coveralls
Copy link

coveralls commented Aug 7, 2020

Coverage Status

Coverage remained the same at 34.34% when pulling 622814c on binyli/logrotate into 41afdc7 on master.

@Binyang2014 Binyang2014 marked this pull request as ready for review August 7, 2020 03:45
@Binyang2014
Copy link
Contributor Author

fix #4777

@Binyang2014 Binyang2014 linked an issue Aug 7, 2020 that may be closed by this pull request
missingok
notifempty
copytruncate
maxsize 300M
size 100M
Copy link
Member

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

Copy link
Contributor Author

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]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about [256, 512]?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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) {
Copy link
Member

@yqwang-ms yqwang-ms Aug 7, 2020

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@Binyang2014 Binyang2014 merged commit fbba438 into master Aug 9, 2020
@Binyang2014 Binyang2014 deleted the binyli/logrotate branch August 17, 2020 05:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot find the stdout/stderr log of a long run job
3 participants