-
Notifications
You must be signed in to change notification settings - Fork 227
Always write to log file if configured #424
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
base: main
Are you sure you want to change the base?
Always write to log file if configured #424
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lxuan94-pp The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This issue is currently awaiting triage. If klog contributors determine this is a relevant issue, they will accept it by applying the The 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. |
|
Welcome @lxuan94-pp! |
|
Why did this stop working? You justify this change by saying that it fixes autoscaler, but this repo isn't autoscaler - it's klog. This needs to be explained as either a bug (used to work, no longer does) or as an improvement. Either way this could be a problematic change because other users of klog might depend on the current behavior. I haven't looked into this because I expect the description of the PR to explain this too me and other potentially affected users of klog. |
|
@pohly Thanks for the comment. This introduced a regression, even when --log-file is specified, no file is actually created. From a user’s perspective, if a log file is explicitly specified, shouldn’t it always be written to? My understanding here may be limited, so I’d appreciate your insight. |
|
As you pointed out, that change was in component-base, not in klog. component-base does not have the same strict "no changes" rule as klog, so users of it (like apparently autoscaler) occasionally have to adapt. Instead of modifying klog, it sounds to me like it is the autoscaler which needs to be updated. |
|
@pohly Thanks for the insights, I will dig further and get back. |
What this PR does / why we need it:
This PR fixes an issue introduced in kubernetes/autoscaler#7842
where the Cluster Autoscaler --log-file argument stopped working. After the referenced commit, if the logger is not nil, klog.go bypasses the log file creation logic, so no log file is created. This PR modifies klog.go to ensure that the log file is always written when configured, regardless of the logger state.
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 kubernetes/autoscaler#7842
Special notes for your reviewer:


This has been tested and used in production of OCI provider clusters
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: