build(deps): bump k8s.io/klog/v2 from v2.130.1 to v2.140.0#385
build(deps): bump k8s.io/klog/v2 from v2.130.1 to v2.140.0#385pierluigilenoci wants to merge 2 commits intomozillazg:masterfrom
Conversation
klog v2 defaults -logtostderr to true, which silently ignores -stderrthreshold — all log levels go to stderr unconditionally. This has been an open issue since 2020 (kubernetes/klog#212). klog v2.140.0 introduced a fix behind an opt-in flag (legacy_stderr_threshold_behavior). This commit bumps klog to v2.140.0 and enables the fix so that -stderrthreshold is honored, while preserving the current default behavior (stderrthreshold=INFO means all logs still go to stderr unless the user overrides it on the command line). Ref: kubernetes/klog#212 Ref: kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpgrades k8s.io/klog/v2 to v2.140.0; refactors key/value serialization to a multi-slice FormatKVs with deduplication; adjusts klog flag/threshold and stderr-routing behavior; minor change in cmd/log.go to explicitly ignore flag.Set errors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/log.go (1)
43-54:⚠️ Potential issue | 🟠 Major
silenceKlog()behavior regressed—now allows INFO+ messages on stderr instead of silencing themTwo critical issues:
legacy_stderr_threshold_behavior=falseis dead code here. This flag only affects behavior whenlogtostderr=true(in theif l.toStderrbranch per klog source code). Since the function immediately setslogtostderr=false, that branch is never taken. The referenced PR#432fixes filtering forlogtostderr=truecases, but this codebase useslogtostderr=false.
stderrthreshold=INFObreaks silencing semantics. Before this change, the defaultstderrThresholdofERRORfiltered INFO/WARNING messages to stderr. This change explicitly sets it toINFO, causing all INFO+ messages from dependencies (cri-api,cri-client, etc.) to reach stderr when running at--log-level=debugor--log-level=info. This contradicts the function's name and the PR description's claim to "preserve the current default behavior."Fix: Revert
stderrthresholdfromINFOtoERROR(or remove the line to rely on the default). If surfacing INFO+ messages on stderr is intentional, update the PR description, rename the function, and document this behavior change clearly.Minor: Standardize error handling—either all
flags.Set()calls should use_ =to discard errors, or none should, for consistency and visibility of any future flag misconfigurations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/log.go` around lines 43 - 54, silenceKlog currently enables stderr for INFO+ and sets a dead flag; change it to restore previous silencing by reverting stderrthreshold to "ERROR" (or remove the flags.Set("stderrthreshold", ...) call so the default ERROR is used) and remove the pointless legacy_stderr_threshold_behavior flag setting since it only affects the logtostderr=true branch (we set logtostderr=false); also make the flags.Set() error handling consistent (either prefix all with `_ = ` or none) so flag errors are treated uniformly; update references to klog and the silenceKlog function accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/log.go`:
- Around line 43-54: silenceKlog currently enables stderr for INFO+ and sets a
dead flag; change it to restore previous silencing by reverting stderrthreshold
to "ERROR" (or remove the flags.Set("stderrthreshold", ...) call so the default
ERROR is used) and remove the pointless legacy_stderr_threshold_behavior flag
setting since it only affects the logtostderr=true branch (we set
logtostderr=false); also make the flags.Set() error handling consistent (either
prefix all with `_ = ` or none) so flag errors are treated uniformly; update
references to klog and the silenceKlog function accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aff1e8c8-3702-4dba-bb05-28eab475ce58
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/log.gogo.modvendor/k8s.io/klog/v2/README.mdvendor/k8s.io/klog/v2/internal/serialize/keyvalues.govendor/k8s.io/klog/v2/internal/serialize/keyvalues_no_slog.govendor/k8s.io/klog/v2/internal/serialize/keyvalues_slog.govendor/k8s.io/klog/v2/klog.govendor/k8s.io/klog/v2/klogr.govendor/k8s.io/klog/v2/klogr_slog.govendor/modules.txt
💤 Files with no reviewable changes (1)
- vendor/k8s.io/klog/v2/README.md
CodeRabbit correctly flagged two issues in silenceKlog(): 1. `legacy_stderr_threshold_behavior=false` is dead code: this flag only affects behavior in the `if l.toStderr` branch (klog.go:916), which is never reached because we set logtostderr=false. 2. `stderrthreshold=INFO` breaks silencing: with logtostderr=false, klog's output path (klog.go:928-937) copies messages to stderr when severity >= stderrThreshold. The default threshold is ERROR, so only errors leak. Setting it to INFO causes ALL log messages to appear on stderr, defeating the purpose of silenceKlog(). Fix: remove both flags and keep the function as a clean klog silencer. Also add `_ =` to the remaining flags.Set() calls for consistent error-discard style. The klog v2.130.1 -> v2.140.0 upgrade is preserved as-is. Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
|
@coderabbitai Good catch on both points. Fixed in 1e31871:
The function is now a clean klog silencer again: route file output to |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
What changed
Bumps k8s.io/klog/v2 from v2.130.1 to v2.140.0, which brings:
legacy_stderr_threshold_behaviorflag) for callers that uselogtostderr=true(kubernetes/klog#432)FormatKVs)silenceKlog() — kept minimal
silenceKlog()disables klog output by routing file logs toio.Discardand settinglogtostderr=false. The previous revision addedstderrthreshold=INFOandlegacy_stderr_threshold_behavior=false, but as @coderabbitai correctly identified:legacy_stderr_threshold_behavior=falseis dead code whenlogtostderr=false— it only affects theif l.toStderrbranch.stderrthreshold=INFOactively broke silencing — withlogtostderr=false, klog copies messages at or abovestderrThresholdto stderr, so INFO makes all messages leak.Both lines have been removed. The function now only sets the two flags that matter (
logtostderr=false,alsologtostderr=false) with consistent_ =error-discard style.References
Summary by CodeRabbit
Dependencies
Bug Fixes
Documentation