fix: honor -stderrthreshold when -logtostderr is set#2421
fix: honor -stderrthreshold when -logtostderr is set#2421pierluigilenoci wants to merge 2 commits intoopenkruise:masterfrom
Conversation
klog v2 defaults -logtostderr to true, which silently causes -stderrthreshold to be ignored. klog v2.140.0 introduced the legacy_stderr_threshold_behavior flag that restores the correct interaction between the two flags (see kubernetes/klog#432). This commit: - bumps k8s.io/klog/v2 from v2.130.1 to v2.140.0 - sets legacy_stderr_threshold_behavior=false after klog.InitFlags in both main.go and cmd/daemon/main.go - sets stderrthreshold=INFO so the default behavior is preserved but users can now override it on the command line Ref: kubernetes/klog#212 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: 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 |
|
Welcome @pierluigilenoci! It looks like this is your first PR to openkruise/kruise 🎉 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2421 +/- ##
==========================================
+ Coverage 48.15% 48.85% +0.70%
==========================================
Files 325 325
Lines 34824 27950 -6874
==========================================
- Hits 16769 13656 -3113
+ Misses 16523 12763 -3760
+ Partials 1532 1531 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates klog to a version that supports honoring -stderrthreshold even when -logtostderr=true, and configures the binaries to use the new behavior while preserving current defaults.
Changes:
- Bump
k8s.io/klog/v2fromv2.130.1tov2.140.0. - Set
legacy_stderr_threshold_behavior=falseand defaultstderrthreshold=INFOafterklog.InitFlagsin both entrypoints.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| main.go | Opt into new klog stderr-threshold behavior and set default threshold to INFO |
| cmd/daemon/main.go | Same klog flag defaults as the main binary |
| go.mod | Bump klog dependency to v2.140.0 |
| go.sum | Update checksums for klog v2.140.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flag.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck | ||
| flag.Set("stderrthreshold", "INFO") //nolint:errcheck |
There was a problem hiding this comment.
flag.Set(...) can fail (e.g., if the flag name changes, isn’t registered due to init order changes, or klog behavior differs between builds). Silencing these errors risks silently reverting to the problematic behavior in production. Consider handling the returned errors and failing fast (or at least emitting a clear warning) so misconfiguration is immediately visible.
| flag.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck | |
| flag.Set("stderrthreshold", "INFO") //nolint:errcheck | |
| if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | |
| klog.Fatalf("failed to set flag %q: %v", "legacy_stderr_threshold_behavior", err) | |
| } | |
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.Fatalf("failed to set flag %q: %v", "stderrthreshold", err) | |
| } |
There was a problem hiding this comment.
Thanks — good catch. Updated both flag.Set calls in main.go to check the returned error and klog.Fatalf on failure, so misconfiguration is immediately visible instead of silently reverting to the old behavior.
| flag.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck | ||
| flag.Set("stderrthreshold", "INFO") //nolint:errcheck |
There was a problem hiding this comment.
Same concern as in main.go: ignoring flag.Set errors can lead to silently incorrect logging behavior. Please handle the errors (fail fast or log a warning) to avoid a hard-to-diagnose operational regression if the flags aren’t present/registered as expected.
| flag.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck | |
| flag.Set("stderrthreshold", "INFO") //nolint:errcheck | |
| if err := flag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | |
| klog.Fatalf("Failed to set legacy_stderr_threshold_behavior: %v", err) | |
| } | |
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.Fatalf("Failed to set stderrthreshold: %v", err) | |
| } |
There was a problem hiding this comment.
Fixed — same treatment applied here: both flag.Set calls now check for errors and klog.Fatalf on failure.
| func main() { | ||
| utilfeature.DefaultMutableFeatureGate.AddFlag(pflag.CommandLine) | ||
| klog.InitFlags(nil) | ||
| // Opt into the new klog behavior so that -stderrthreshold is honored even | ||
| // when -logtostderr=true (the default). | ||
| // Ref: kubernetes/klog#212, kubernetes/klog#432 | ||
| flag.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck | ||
| flag.Set("stderrthreshold", "INFO") //nolint:errcheck |
There was a problem hiding this comment.
The same multi-line comment and flag-defaulting logic is duplicated in two entrypoints. To avoid drift (e.g., one binary missing a future tweak), consider extracting a small helper (e.g., initKlogFlags() in a shared package) called by both mains.
| func main() { | |
| utilfeature.DefaultMutableFeatureGate.AddFlag(pflag.CommandLine) | |
| klog.InitFlags(nil) | |
| // Opt into the new klog behavior so that -stderrthreshold is honored even | |
| // when -logtostderr=true (the default). | |
| // Ref: kubernetes/klog#212, kubernetes/klog#432 | |
| flag.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck | |
| flag.Set("stderrthreshold", "INFO") //nolint:errcheck | |
| func initKlogFlags() { | |
| klog.InitFlags(nil) | |
| // Opt into the new klog behavior so that -stderrthreshold is honored even | |
| // when -logtostderr=true (the default). | |
| // Ref: kubernetes/klog#212, kubernetes/klog#432 | |
| flag.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck | |
| flag.Set("stderrthreshold", "INFO") //nolint:errcheck | |
| } | |
| func main() { | |
| utilfeature.DefaultMutableFeatureGate.AddFlag(pflag.CommandLine) | |
| initKlogFlags() |
There was a problem hiding this comment.
Good suggestion. Since these are two separate main packages (main.go and cmd/daemon/main.go), extracting a shared helper would require introducing a new internal package — which feels like scope creep for this PR. The duplication is minimal (4 lines) and the pattern is now consistent between both entrypoints. Happy to revisit if the maintainers prefer a shared helper.
Replace //nolint:errcheck suppressions with proper error handling. Both flag.Set calls now fail fast via klog.Fatalf if the flag cannot be set, making misconfiguration immediately visible. Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
What this PR does
klog v2 defaults
-logtostderrtotrue, which silently causes-stderrthresholdto be ignored — all log messages of every severity are unconditionally written to stderr. There is no way for users to filter which severity levels reach stderr.This has been an open issue since 2020: kubernetes/klog#212.
klog v2.140.0 (kubernetes/klog#432) introduces a
legacy_stderr_threshold_behaviorflag that fixes this. This PR:k8s.io/klog/v2from v2.130.1 to v2.140.0legacy_stderr_threshold_behavior=falseafterklog.InitFlagsin bothmain.goandcmd/daemon/main.gostderrthreshold=INFOso the default behavior is preserved but users can now override it on the command line (e.g.-stderrthreshold=WARNING)Why it matters
In production clusters, log aggregation systems (Fluentd, Fluent Bit, Loki, Datadog, etc.) typically ingest logs from container stderr. When every log level goes to stderr indiscriminately:
The
-stderrthresholdflag exists precisely to solve this, but it has never worked under the default klog v2 configuration — until now.Changed files
main.golegacy_stderr_threshold_behavior=false+stderrthreshold=INFOafterklog.InitFlagscmd/daemon/main.gogo.mod/go.sum