Skip to content
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

Add a log-file-max-size flag to fix large logfile (>1.8GB) truncation. #56

Merged
merged 1 commit into from
Apr 11, 2019

Conversation

yuwenma
Copy link

@yuwenma yuwenma commented Apr 9, 2019

What this PR does / why we need it:
Allows klog users to disable the max-size limitation (cleanup log-file when exceeds 1.8GB) when flag log-file is applied.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #55

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 9, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 9, 2019
@yuwenma yuwenma force-pushed the issue-55 branch 2 times, most recently from e2e05a0 to e025e58 Compare April 9, 2019 22:55
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2019
@yuwenma
Copy link
Author

yuwenma commented Apr 9, 2019

/assign @dims
/cc @mborsz @tallclair @wojtek-t

@yuwenma yuwenma force-pushed the issue-55 branch 4 times, most recently from a77a608 to 651f8e7 Compare April 9, 2019 23:14
@dims
Copy link
Member

dims commented Apr 9, 2019

/assign @DirectXMan12

@dims
Copy link
Member

dims commented Apr 9, 2019

Solly, currently when someone uses log-file, we wrap around the file when it reaches 1.8GB. I don't think we should do that as we basically lose the log of what happened until then unless someone logrotate (with copy truncation) out. By default if someone uses log-file we should honor that and let them use other tools to manage the logfile (like logrotate). If they do want to specify a max size, we can use a new toggle log-file-max-size ... WDYT? please see the referenced issues for the discussion

klog.go Outdated Show resolved Hide resolved
klog.go Outdated Show resolved Hide resolved
klog.go Outdated Show resolved Hide resolved
klog.go Outdated Show resolved Hide resolved
klog.go Outdated Show resolved Hide resolved
klog.go Show resolved Hide resolved
@yuwenma
Copy link
Author

yuwenma commented Apr 10, 2019

@tallclair Refactored the code to precompute the max number. PTAL

klog.go Outdated Show resolved Hide resolved
klog.go Outdated Show resolved Hide resolved
klog.go Outdated Show resolved Hide resolved
@dims
Copy link
Member

dims commented Apr 11, 2019

@yuwenma can you please look at the lint error in the CI?

klog.go Outdated Show resolved Hide resolved
klog.go Outdated Show resolved Hide resolved
klog.go Outdated Show resolved Hide resolved
@DirectXMan12
Copy link

DirectXMan12 commented Apr 11, 2019

If they do want to specify a max size, we can use a new toggle log-file-max-size ... WDYT? please see the referenced issues for the discussion

When did --log-file go in? That seems reasonable to me, although to avoid breaking people by out-of-disking them, we should default log-file-max-size to 1.8 GiB and then warn if it's not set for a couple of releases, to give people some time to fix their clusters.

@DirectXMan12
Copy link

(FWIW, I agree that having no limit is probably the expected behavior, but in case anyone is accidentally depending on this behavior, it's probably good to give them at least a release's worth of heads-up)

@yuwenma
Copy link
Author

yuwenma commented Apr 11, 2019

@DirectXMan12 Updated the default to be 1800MB. 😃

@dims
Copy link
Member

dims commented Apr 11, 2019

/approve

leaving /lgtm for @tallclair or @DirectXMan12

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2019
@tallclair
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2019
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, tallclair, yuwenma

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid log truncation due to log-rotation delays (when logfiles exceeds 1.8GB faster than log rotation)
5 participants