Honor stderrthreshold when logtostderr is enabled#3611
Honor stderrthreshold when logtostderr is enabled#3611pierluigilenoci wants to merge 1 commit intokubernetes-sigs:masterfrom
Conversation
Opt into the corrected klog behavior by setting -legacy_stderr_threshold_behavior=false so that -stderrthreshold is respected even when -logtostderr=true. Set -stderrthreshold=INFO as the new default to preserve backward-compatible behavior (all log levels written to stderr). Users can now override -stderrthreshold to WARNING or ERROR to reduce stderr noise. Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com> Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pierluigilenoci The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
There was a problem hiding this comment.
Pull request overview
This PR updates the azuredisk CSI driver’s startup flag configuration to opt into klog’s corrected behavior so that stderrthreshold is honored even when logtostderr=true, while keeping default behavior (“all logs to stderr”) backward-compatible.
Changes:
- Sets
-legacy_stderr_threshold_behavior=falseto make klog honorstderrthresholdwithlogtostderr=true. - Sets
-stderrthreshold=INFOto preserve the previous default of sending all log levels to stderr unless the user overrides it.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flag.Parse() | ||
| if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | ||
| klog.Errorf("Failed to set legacy_stderr_threshold_behavior: %v", err) | ||
| } | ||
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | ||
| klog.Errorf("Failed to set stderrthreshold: %v", err) | ||
| } |
There was a problem hiding this comment.
flag.Set is being called after flag.Parse(), which will override any user-provided --legacy_stderr_threshold_behavior / --stderrthreshold values from the command line. That contradicts the PR goal of letting users tune --stderrthreshold (e.g., to WARNING/ERROR). Consider setting these values before flag.Parse() so CLI args can still override them, or only applying the stderrthreshold=INFO fallback when that flag wasn't explicitly provided (and update the flag's default/usage value if you want --help / usage text to reflect the new default).
| flag.Parse() | |
| if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | |
| klog.Errorf("Failed to set legacy_stderr_threshold_behavior: %v", err) | |
| } | |
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.Errorf("Failed to set stderrthreshold: %v", err) | |
| } | |
| if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | |
| klog.Errorf("Failed to set legacy_stderr_threshold_behavior: %v", err) | |
| } | |
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.Errorf("Failed to set stderrthreshold: %v", err) | |
| } | |
| flag.Parse() |
| func main() { | ||
| flag.Parse() | ||
| if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | ||
| klog.Errorf("Failed to set legacy_stderr_threshold_behavior: %v", err) | ||
| } | ||
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | ||
| klog.Errorf("Failed to set stderrthreshold: %v", err) |
There was a problem hiding this comment.
There are existing tests for this package, but none currently validate the new CLI behavior around honoring --stderrthreshold when logtostderr is enabled. Adding a unit test that sets os.Args with an explicit --stderrthreshold value and asserts it is not overridden would help prevent regressions in this flag-handling logic.
| func main() { | |
| flag.Parse() | |
| if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | |
| klog.Errorf("Failed to set legacy_stderr_threshold_behavior: %v", err) | |
| } | |
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.Errorf("Failed to set stderrthreshold: %v", err) | |
| func hasExplicitFlag(args []string, name string) bool { | |
| prefix := "--" + name | |
| for i := 0; i < len(args); i++ { | |
| arg := args[i] | |
| if arg == prefix { | |
| return true | |
| } | |
| if strings.HasPrefix(arg, prefix+"=") { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| func main() { | |
| flag.Parse() | |
| if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | |
| klog.Errorf("Failed to set legacy_stderr_threshold_behavior: %v", err) | |
| } | |
| if !hasExplicitFlag(os.Args[1:], "stderrthreshold") { | |
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.Errorf("Failed to set stderrthreshold: %v", err) | |
| } |
What type of PR is this?
/kind bug
What this PR does / why we need it
When
logtostderris set totrue(klog default), thestderrthresholdflag is silently ignored due to a legacy default behavior in klog v2. This means users cannot reduce stderr noise by setting-stderrthreshold=WARNINGor-stderrthreshold=ERROR.This PR opts into the corrected klog behavior (introduced in klog PR #432) by setting
-legacy_stderr_threshold_behavior=false, and explicitly sets-stderrthreshold=INFOto preserve backward-compatible behavior (all log levels still go to stderr by default).After this change, users can override
-stderrthresholdtoWARNINGorERRORto filter log output on stderr.Which issue(s) this PR fixes
Related: kubernetes/klog#212
Special notes for your reviewer
The driver already uses klog v2.140.0 which includes the
legacy_stderr_threshold_behaviorflag. This PR simply opts into the corrected behavior. No functional change for existing deployments — the default remains "all logs to stderr".The same fix was merged in: