fix: honor -stderrthreshold when -logtostderr=true#2494
fix: honor -stderrthreshold when -logtostderr=true#2494pierluigilenoci wants to merge 1 commit intokubernetes-sigs:masterfrom
Conversation
klog v2 defaults -logtostderr to true, which silently ignores -stderrthreshold — all severities are unconditionally sent to stderr. Opt into the fixed behavior introduced in klog v2.140.0 by setting legacy_stderr_threshold_behavior=false so that -stderrthreshold is respected. The default is set to INFO (preserving current behavior); users can now override it on the command line. Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com> Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[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
size/S
|
@pierluigilenoci: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Thanks for this, Pierluigi — and for driving the upstream klog fix in the first place. The intent is exactly right and I want to land it.
Two things to sort before merge:
go.modstill pinsk8s.io/klog/v2 v2.130.1, butlegacy_stderr_threshold_behavioronly exists from v2.140.0. Right now everySetreturnsno such flagand the_ =swallows it — the fix is a no-op as shipped. Please bump klog in the same PR.- In
pkg/utils/klog/klog.go, the override is reverted at runtime byMergeKlogConfiguration(used bynfd-masterandnfd-worker— the primary daemons). Details inline.
Smaller asks: don't swallow Set errors, and consider extracting the 4-line block into a klogutils helper so we have one place to evolve. A table test asserting -stderrthreshold=WARNING takes effect after init would pin the behavior and would have caught both blockers.
Happy to pair on the merge-path fix if easier.
| // Opt into the new klog behavior so that -stderrthreshold is honored even | ||
| // when -logtostderr=true (the default). | ||
| // Ref: kubernetes/klog#212, kubernetes/klog#432 | ||
| _ = flags.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck |
There was a problem hiding this comment.
Subtle one: setting on the inner flags FlagSet is undone later by MergeKlogConfiguration (called in pkg/nfd-master/nfd-master.go:1206 and pkg/nfd-worker/nfd-worker.go:386). That path calls KlogFlagVal.DefValue(), which returns k.flag.DefValue — klog's declared default ("true"), not the value we just Set. So unless the user explicitly passes -legacy_stderr_threshold_behavior=false on the CLI, the merge reverts it and nfd-master / nfd-worker silently regress.
Simplest fix: call Set on the outer flagset after VisitAll, so KlogFlagVal.Set is invoked and isSetFromCmdLine=true. The merge then leaves it alone.
| // when -logtostderr=true (the default). | ||
| // Ref: kubernetes/klog#212, kubernetes/klog#432 | ||
| _ = flags.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck | ||
| _ = flags.Set("stderrthreshold", "INFO") //nolint:errcheck |
There was a problem hiding this comment.
INFO is already klog's default, so this line is a no-op at runtime. I'd drop it; if the intent is documentation, a one-line comment ("we rely on klog's default stderrthreshold=INFO") reads more clearly.
| // Opt into the new klog behavior so that -stderrthreshold is honored even | ||
| // when -logtostderr=true (the default). | ||
| // Ref: kubernetes/klog#212, kubernetes/klog#432 | ||
| _ = flagset.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck |
There was a problem hiding this comment.
Two things here:
- This flag was added in klog v2.140.0;
go.modpins v2.130.1, soSetcurrently returnsno such flag. Needs a klog bump in this PR. - Even after the bump, I'd prefer not to swallow the error. If klog ever renames or removes the flag we want a loud failure — a
klog.Fatalf(or at minimumklog.Warningf) on non-nil gives us that signal. Same applies to the equivalent block incmd/nfd-topology-updater/main.go.
3 participants
What
Opt into the fixed klog behavior so that
-stderrthresholdis respected even when-logtostderr=true(the default).Why
klog v2 defaults
-logtostderrtotrue. When active, the-stderrthresholdflag is silently ignored — all log severities are unconditionally written to stderr. This makes it impossible for log-aggregation systems to filter by severity.This has been an open issue since 2020: kubernetes/klog#212
The fix was merged in klog v2.140.0 via kubernetes/klog#432 (authored by me) and introduces an opt-in flag
legacy_stderr_threshold_behavior. Setting it tofalseenables the corrected behavior where-stderrthresholdis honored.What changes
After
klog.InitFlags(), two flags are set:legacy_stderr_threshold_behavior=false— enables the fixstderrthreshold=INFO— preserves current behavior (all severities still go to stderr by default)Since these are set before flag parsing, users can now override
-stderrthreshold=WARNINGor-stderrthreshold=ERRORon the command line and it will actually work.The fix is applied in three locations:
pkg/utils/klog/klog.go— centralized klog wrapper used bynfd-masterandnfd-workercmd/nfd-gc/main.go— garbage collector (callsklog.InitFlagsdirectly)cmd/nfd-topology-updater/main.go— topology updater (callsklog.InitFlagsdirectly)References