Fix(log flags): handle log streams correctly #8836
Fix(log flags): handle log streams correctly #8836k8s-ci-robot merged 1 commit intokubernetes:masterfrom
Conversation
|
Hi @lxuan94-pp. Thanks for your PR. I'm waiting for a github.com 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. DetailsInstructions 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. |
1010e06 to
9ad01ac
Compare
9ad01ac to
3de2db7
Compare
3de2db7 to
de1f71e
Compare
| sigs.k8s.io/structured-merge-diff/v6 v6.3.0/go.mod h1:M3W8sfWvn2HhQDIbGWj3S099YozAsymCo/wrT5ohRUE= | ||
| sigs.k8s.io/yaml v1.6.0 h1:G8fkbMSAFqgEFgh4b1wmtzDnioxFCUgTZhlbj5P9QYs= | ||
| sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4= | ||
| sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4= No newline at end of file |
There was a problem hiding this comment.
please remove this newline change, thank you!
There was a problem hiding this comment.
I think it was changed by go mod tidy I ran, which caused different line endings. I have reverted it to the master version.
de1f71e to
7f925c7
Compare
7f925c7 to
72888ba
Compare
| // default: both to stderr | ||
| var infoW, errW io.Writer = os.Stderr, os.Stderr | ||
|
|
||
| if logFilePath != "" && !logToStderr { |
There was a problem hiding this comment.
Can we drop the -alsologtostderr option?
If just --log-file is set but no --logtostderr (default to false), then we configure logging for redirecting both info and err logging to the configured log file.
If --log-file is set and --logtostderr=true we intrepret that as:
- configure logging for redirecting both info and err logging to the configured log file
- also tee log info and err output to stderr
There was a problem hiding this comment.
We can drop -alsologtostderr if we want to follow this model, but just to note: klog’s guidance is that --log-file and --alsologtostderr are ignored when --logtostderr=true
commandLine.StringVar(&logging.logFile, "log_file", "", "If non-empty, use this log file (no effect when -logtostderr=true)")
commandLine.BoolVar(&logging.toStderr, "logtostderr", true, "log to standard error instead of files")
commandLine.BoolVar(&logging.alsoToStderr, "alsologtostderr", false, "log to standard error as well as files (no effect when -logtostderr=true)")
from klog/v2 https://pkg.go.dev/k8s.io/klog/v2#klog.go
I’m fine proceeding with your proposed interpretation, as long as we’re aware it slightly diverges from the standard klog usage pattern. Let me know if you want me to update the flags accordingly.
There was a problem hiding this comment.
@jackfrancis Please advise on how we should proceed, and I can update or keep the change accordingly.
There was a problem hiding this comment.
ACK, probably best not to overthink and follow that guidance
|
/ok-to-test |
|
/test pull-cluster-autoscaler-e2e-azure-master |
|
Hi @jackfrancis, pls check the comment #8836 (comment) and advise on how we should proceed. IMO I want to keep the current behavior to align with klog. |
|
Bumping #8836 (comment) |
| // default: both to stderr | ||
| var infoW, errW io.Writer = os.Stderr, os.Stderr | ||
|
|
||
| if logFilePath != "" && !logToStderr { |
There was a problem hiding this comment.
ACK, probably best not to overthink and follow that guidance
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, lxuan94-pp The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick cluster-autoscaler-release-1.32 |
|
@jackfrancis: new pull request created: #9101 DetailsIn response to this:
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. |
|
/cherry-pick cluster-autoscaler-release-1.33 |
|
@jackfrancis: new pull request created: #9102 DetailsIn response to this:
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. |
|
@jackfrancis: new pull request created: #9103 DetailsIn response to this:
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. |
|
@jackfrancis: new pull request created: #9104 DetailsIn response to this:
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. |
What type of PR is this?
/kind bug
Optionally add one or more of the following kinds if applicable:
/kind regression
What this PR does / why we need it:
After upgrading to Autoscaler v1.30.x, the log file no longer exists and is not created. This PR fixes the issue without modifying component-base or klog in vendor
Which issue(s) this PR fixes:
Fixes #7842
Special notes for your reviewer:
Persist the behavior from klog:
Does this PR introduce a user-facing change?
None
For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Tested in OCI cluster


