Honor stderrthreshold when logtostderr is enabled#7568
Honor stderrthreshold when logtostderr is enabled#7568pierluigilenoci wants to merge 7 commits intokedacore:mainfrom
Conversation
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
|
| Status | Scan Engine | Total (0) | ||||
|---|---|---|---|---|---|---|
| Open Source Security | 0 | 0 | 0 | 0 | See details |
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.
af81c70 to
919387e
Compare
|
cc @JorTurFer @zroubalik @rickbrouwer — would you be able to review this when you get a chance? All validation checks are green. This opts into the klog fix for kubernetes/klog#212 so that |
|
Gentle ping — could you take a look when you get a chance? Happy to address any feedback. Thank you! |
|
So, first of all, thanks for the PR. One concern, but I have the feeling that you are better at it than I am, but the klog's default for _ = flag.CommandLine.Set("stderrthreshold", "INFO") |
|
Great catch, @rickbrouwer — you're absolutely right. With the legacy behavior disabled, klog respects the actual To maintain the same observable behavior while using the corrected code path, I'll add: if err := flag.CommandLine.Set("stderrthreshold", "INFO"); err != nil {
klog.Fatalf("Failed to set stderrthreshold: %v", err)
}right after the |
|
Great catch @rickbrouwer, and you're absolutely right! Setting That's exactly why the PR already sets both flags together in all three binaries: flag.CommandLine.Set("legacy_stderr_threshold_behavior", "false")
flag.CommandLine.Set("stderrthreshold", "INFO")The combination ensures:
This way, there's no behavior change for existing users. The fix simply enables users to later adjust The fix is already applied correctly in |
There was a problem hiding this comment.
Pull request overview
This PR updates the vendored k8s.io/klog/v2 dependency and attempts to opt the operator, webhooks, and adapter binaries into klog’s fixed behavior so -stderrthreshold is honored even when -logtostderr=true, reducing stderr noise when users raise the threshold.
Changes:
- Bump
k8s.io/klog/v2fromv2.130.1tov2.140.0(and update vendored sources). - Set
legacy_stderr_threshold_behavior=falsein operator/webhooks/adapter startup. - Add a changelog entry documenting the behavior change.
Reviewed changes
Copilot reviewed 5 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Bumps k8s.io/klog/v2 to v2.140.0. |
go.sum |
Updates checksums for the new klog version. |
vendor/modules.txt |
Reflects the updated klog module version and Go version metadata. |
vendor/k8s.io/klog/v2/klog.go |
Vendored upstream changes implementing the new stderr threshold behavior. |
vendor/k8s.io/klog/v2/klogr.go |
Vendored upstream key/value merge behavior changes. |
vendor/k8s.io/klog/v2/klogr_slog.go |
Vendored upstream quoting + KV formatting changes. |
vendor/k8s.io/klog/v2/textlogger/options.go |
Vendored upstream config option changes (WithHeader). |
vendor/k8s.io/klog/v2/textlogger/textlogger.go |
Vendored upstream header/formatting changes. |
vendor/k8s.io/klog/v2/internal/serialize/keyvalues*.go |
Vendored upstream KV formatting/dedup refactor. |
vendor/k8s.io/klog/v2/README.md |
Vendored upstream documentation adjustments. |
cmd/operator/main.go |
Attempts to set new klog flags to opt into non-legacy behavior. |
cmd/webhooks/main.go |
Same klog flag setup as operator. |
cmd/adapter/main.go |
Same klog flag setup as operator/webhooks (but with existing legacy flags). |
CHANGELOG.md |
Adds release note for honoring stderrthreshold with logtostderr. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Opt into the new klog behavior so that -stderrthreshold is honored even | ||
| // when -logtostderr=true (the default). Without this, all log levels are | ||
| // unconditionally sent to stderr and users cannot filter by severity. | ||
| // Requires klog v2.140.0+ (kubernetes/klog#432). | ||
| if err := flag.CommandLine.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | ||
| klog.Fatalf("Failed to set legacy_stderr_threshold_behavior: %v", err) | ||
| } | ||
| if err := flag.CommandLine.Set("stderrthreshold", "INFO"); err != nil { | ||
| klog.Fatalf("Failed to set stderrthreshold: %v", err) | ||
| } |
There was a problem hiding this comment.
flag.CommandLine.Set("legacy_stderr_threshold_behavior", ...) / Set("stderrthreshold", ...) will fail unless the klog flags have been registered on flag.CommandLine first. This binary doesn’t call klog.InitFlags(...) (and the stdlib flag package won’t pick up klog’s internal flagset automatically), so these Set calls will return "no such flag" and the process will exit via klog.Fatalf. Call klog.InitFlags(nil) (or klog.InitFlags(flag.CommandLine)) before setting these values, then add the Go flagset to pflag and parse so users can override them via CLI.
| // Opt into the new klog behavior so that -stderrthreshold is honored even | ||
| // when -logtostderr=true (the default). Without this, all log levels are | ||
| // unconditionally sent to stderr and users cannot filter by severity. | ||
| // Requires klog v2.140.0+ (kubernetes/klog#432). | ||
| if err := flag.CommandLine.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | ||
| klog.Fatalf("Failed to set legacy_stderr_threshold_behavior: %v", err) | ||
| } | ||
| if err := flag.CommandLine.Set("stderrthreshold", "INFO"); err != nil { | ||
| klog.Fatalf("Failed to set stderrthreshold: %v", err) | ||
| } |
There was a problem hiding this comment.
Same issue as in the operator: these flag.CommandLine.Set(...) calls depend on klog flags being registered on the Go flagset first. Without an earlier klog.InitFlags(...), Set will fail with "no such flag" and the program will terminate via klog.Fatalf. Initialize klog flags before these Set calls.
| // Opt into the new klog behavior so that -stderrthreshold is honored even | ||
| // when -logtostderr=true (the default). Without this, all log levels are | ||
| // unconditionally sent to stderr and users cannot filter by severity. | ||
| // Requires klog v2.140.0+ (kubernetes/klog#432). | ||
| if err := flag.CommandLine.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | ||
| klog.Fatalf("Failed to set legacy_stderr_threshold_behavior: %v", err) | ||
| } | ||
| if err := flag.CommandLine.Set("stderrthreshold", "INFO"); err != nil { | ||
| klog.Fatalf("Failed to set stderrthreshold: %v", err) | ||
| } |
There was a problem hiding this comment.
These flag.CommandLine.Set(...) calls will fail unless klog has registered its flags on flag.CommandLine (via klog.InitFlags(...)). This file currently doesn’t initialize klog flags, so the adapter will exit early with "no such flag". Also note that this command already defines a pflag --stderrthreshold (currently documented as a no-op); even after initializing klog flags, users changing the existing pflag won’t affect klog unless you explicitly propagate the parsed stdErrThreshold value into klog (for example, after parsing, call flag.CommandLine.Set("stderrthreshold", stdErrThreshold) when the flag was provided).
zroubalik
left a comment
There was a problem hiding this comment.
@pierluigilenoci could you please double check comments from Copilot, whether these are hallucinations or real issues? Then please fix the problem in the changelog and we are good to go. Thanks!
|
Hi @pierluigilenoci, following up on @zroubalik's review: 1. Copilot's review comments are valid (not hallucinations)I traced through the vendored klog source (
Without calling Fix needed in all three binariesAdd opts := zap.Options{}
opts.BindFlags(flag.CommandLine)
// Register klog flags on flag.CommandLine so they can be set programmatically.
klog.InitFlags(nil)
// Opt into the new klog behavior...
if err := flag.CommandLine.Set("legacy_stderr_threshold_behavior", "false"); err != nil {Files to update:
2. CHANGELOG & merge conflictThe PR also needs a rebase onto 3. Adapter noteFor Thanks for the contribution — the klog fix itself is valuable! |
Update k8s.io/klog/v2 from v2.130.1 to v2.140.0 and opt into the new klog behavior by setting -legacy_stderr_threshold_behavior=false in all three binaries (operator, webhooks, adapter). Previously, when -logtostderr=true (the klog default), the -stderrthreshold flag was completely ignored and all log levels were unconditionally sent to stderr. With this change, users can set -stderrthreshold to WARNING or ERROR to reduce stderr noise. Ref: kubernetes/klog#212, kubernetes/klog#432 Related: kedacore/charts#791, kedacore/charts#696 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Address reviewer feedback: setting legacy_stderr_threshold_behavior=false without also pinning stderrthreshold changes the effective threshold from WARNING to ERROR, silently dropping WARNING messages from stderr. Pin stderrthreshold=INFO to maintain the same observable behavior. Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
klog registers its flags on a private commandLine flagset, not on the stdlib flag.CommandLine. Without calling klog.InitFlags(nil) first, the flag.CommandLine.Set() calls for legacy_stderr_threshold_behavior and stderrthreshold fail with "no such flag", causing the process to exit via klog.Fatalf. Add klog.InitFlags(nil) after opts.BindFlags(flag.CommandLine) and before the flag.CommandLine.Set() calls in all three binaries. Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
32c8f89 to
9069f2e
Compare
|
Update: All fixes have been pushed and the branch has been rebased onto Changes made:
@zroubalik ready for re-review. |
|
Friendly follow-up — @zroubalik all Copilot review comments have been addressed (added |
Summary
k8s.io/klog/v2from v2.130.1 to v2.140.0 which includes the fix for kubernetes/klog#212-legacy_stderr_threshold_behavior=falsein all three binaries (operator, webhooks, adapter)-stderrthresholdtoWARNINGorERRORto reduce stderr noise, even when-logtostderr=true(the default)Background
When
-logtostderr=true(the klog default), the-stderrthresholdflag was completely ignored — all log levels were unconditionally sent to stderr. This was a long-standing klog bug (kubernetes/klog#212) fixed in klog v2.140.0 (kubernetes/klog#432) with a new opt-in flag-legacy_stderr_threshold_behavior.This PR updates klog and opts into the corrected behavior so that
-stderrthresholdis honored regardless of-logtostderr.Changes
go.mod: bumpk8s.io/klog/v2v2.130.1 → v2.140.0cmd/operator/main.go: setlegacy_stderr_threshold_behavior=falsebefore flag parsingcmd/webhooks/main.go: setlegacy_stderr_threshold_behavior=falsebefore flag parsingcmd/adapter/main.go: setlegacy_stderr_threshold_behavior=falsebefore flag parsingvendor/: updated vendored klogRelated: kedacore/charts#791, kedacore/charts#696
Ref: kubernetes/klog#212, kubernetes/klog#432
Fixes: kedacore/charts#791