fix: Honor stderrthreshold when logtostderr is enabled#2016
fix: Honor stderrthreshold when logtostderr is enabled#2016pierluigilenoci wants to merge 3 commits intokubernetes-sigs:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
@pierluigilenoci: The label(s) 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. |
|
[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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2016 +/- ##
==========================================
- Coverage 22.07% 22.04% -0.03%
==========================================
Files 57 57
Lines 3198 3202 +4
==========================================
Hits 706 706
- Misses 2400 2404 +4
Partials 92 92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5d0743d to
2df9bff
Compare
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
cc @aramase @ritazh — would you be able to review this when you get a chance? All CI checks are green. This opts into the klog fix for kubernetes/klog#212 so that |
|
/retest release-secrets-store-csi-driver-e2e-aws |
|
/test release-secrets-store-csi-driver-e2e-aws |
1 similar comment
|
/test release-secrets-store-csi-driver-e2e-aws |
|
@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. |
|
/retest release-secrets-store-csi-driver-e2e-aws |
|
@pierluigilenoci: The The following commands are available to trigger optional jobs: Use 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. |
There was a problem hiding this comment.
Chart changes should only be made in the manifest_staging directory. Revert changes to charts/ and make them in manifest_staging/charts/secrets-store-csi-driver/ instead.
| flag.Set("legacy_stderr_threshold_behavior", "false") //nolint:errcheck | ||
| flag.Set("stderrthreshold", "INFO") //nolint:errcheck |
There was a problem hiding this comment.
//nolint:errcheck is not used anywhere else in this repo. flag.Set returns an error if the flag doesn't exist — which would happen if the klog dep bump is reverted but this code stays. Handle the errors:
| 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.ErrorS(err, "failed to set legacy_stderr_threshold_behavior flag") | |
| } | |
| if err := flag.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.ErrorS(err, "failed to set stderrthreshold flag") | |
| } |
| # stderr when -logtostderr=true (the default). Requires klog v2.140.0+ with | ||
| # -legacy_stderr_threshold_behavior=false which is set by the driver. | ||
| # Valid values: INFO, WARNING, ERROR, FATAL (default: INFO). | ||
| # stderrThreshold: "" |
There was a problem hiding this comment.
If adding stderrThreshold to values.yaml, the README.md table in manifest_staging/charts/secrets-store-csi-driver/ needs a corresponding entry (see the logVerbosity and logFormatJSON rows for the pattern).
|
/retitle fix: Honor stderrthreshold when logtostderr is enabled |
|
Hi @aramase — I've addressed all your review feedback in the latest commit:
The only remaining CI failure is Thank you for the thorough review! |
|
Hi @aramase — I've addressed all three points from your review:
Could you take another look when you get a chance? Thank you! |
|
Hi @aramase — friendly ping. All review feedback has been addressed (chart path, error handling, README update) and CI is fully green except for the Could you take another look when you have a moment? Thank you! |
Update klog from v2.120.1 to v2.140.0 which includes the fix for kubernetes/klog#212 (stderrthreshold not honored when logtostderr is set). Opt into the new behavior by setting -legacy_stderr_threshold_behavior=false after klog.InitFlags(nil). Also set -stderrthreshold=INFO to preserve backward-compatible behavior (all logs still appear on stderr by default). Users can now override -stderrthreshold to WARNING or ERROR to reduce stderr noise, which was previously impossible with -logtostderr=true. The Helm chart exposes a new optional stderrThreshold value to configure this via the chart. Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
The e2eprovider module has a replace directive pointing to the main module. Run go mod tidy to sync the klog dependency bump. Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
- Move Helm chart changes from charts/ to manifest_staging/ - Handle flag.Set errors with klog.ErrorS instead of nolint:errcheck - Add stderrThreshold documentation to Helm chart README Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
95f2027 to
4200ec4
Compare
|
Hi @aramase — friendly follow-up. All 3 review items have been addressed (charts moved to manifest_staging/, nolint replaced with proper error handling, stderrThreshold added to README). CI is fully green. Would you be able to take another look when you get a chance? Thanks! |
Summary
k8s.io/klog/v2from v2.120.1 to v2.140.0 which includes the fix for kubernetes/klog#212 (stderrthreshold not honored when logtostderr is set, fixed by kubernetes/klog#432)-legacy_stderr_threshold_behavior=falseafterklog.InitFlags(nil)inmain.go-stderrthreshold=INFOto preserve backward-compatible behavior (all logs still appear on stderr by default)-stderrthresholdtoWARNINGorERRORto reduce stderr noise, which was previously impossible with-logtostderr=truestderrThresholdHelm value for chart-based configurationMotivation
With the default klog configuration (
-logtostderr=true), the-stderrthresholdflag was completely ignored and all log messages of every severity were written to stderr. This caused excessive log noise and increased storage costs for log aggregation systems (see kubernetes/klog#212, Azure/secrets-store-csi-driver-provider-azure#387).klog v2.140.0 introduced a new
-legacy_stderr_threshold_behaviorflag that, when set tofalse, makes-stderrthresholdwork correctly even with-logtostderr=true.Changes
go.mod/go.sumcmd/secrets-store-csi-driver/main.golegacy_stderr_threshold_behavior=falseandstderrthreshold=INFOafterklog.InitFlags(nil)charts/.../values.yamlstderrThresholdvalue (commented out)charts/.../secrets-store-csi-driver.yaml-stderrthresholdif configuredcharts/.../secrets-store-csi-driver-windows.yamlBackward Compatibility
This change is fully backward compatible:
stderrthreshold=INFOmeans all logs still go to stderr-stderrthreshold=WARNINGor-stderrthreshold=ERRORlegacy_stderr_threshold_behaviorflag can still be overridden via command lineTest plan
go build ./...passespkg/secrets-storeare pre-existing macOS socket path issues)-stderrthreshold=ERRORand verify only ERROR+ messages appear on stderrstderrThreshold: WARNINGand verify only WARNING+ messages appear on stderr