fix: honor --stderrthreshold flag when --logtostderr is enabled#1485
fix: honor --stderrthreshold flag when --logtostderr is enabled#1485pierluigilenoci wants to merge 3 commits intoopenshift:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
Walkthroughklog was bumped and multiple command entrypoints now programmatically set klog flags during startup: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
Hi @pierluigilenoci. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
bot
added
the
needs-ok-to-test
|
cc @JoelSpeed @damdo — This is a straightforward klog fix to honor the `--stderrthreshold` flag when `--logtostderr` is enabled. Part of a broader campaign to fix this across the Kubernetes ecosystem (see kubernetes/klog#432). Would appreciate a review when you get a chance! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/machine-healthcheck/main.go`:
- Around line 81-89: After calling klog.InitFlags(nil) in main (klog.InitFlags),
set the logtostderr flag to "true" like the other entry points do so the stderr
threshold behavior is applied consistently; add a flag.Set("logtostderr",
"true") immediately after klog.InitFlags(nil) alongside the existing flag.Set
calls for "legacy_stderr_threshold_behavior" and "stderrthreshold" to ensure
klog writes logs to stderr as expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a5fba2b-55e2-4542-9ca5-20c81ca353ad
⛔ Files ignored due to path filters (11)
go.sumis excluded by!**/*.sumvendor/k8s.io/klog/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_no_slog.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_slog.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/klog.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/klogr.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/klogr_slog.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/textlogger/options.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/textlogger/textlogger.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
cmd/machine-api-operator/start.gocmd/machine-api-operator/version.gocmd/machine-healthcheck/main.gocmd/machineset/main.gocmd/nodelink-controller/main.gocmd/vsphere/main.gogo.mod
pierluigilenoci
force-pushed
the
fix/honor-stderrthreshold
branch
from
e6831c9 to
994d225
Compare
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
@pierluigilenoci Vendor is inconsistent, please run |
|
@JoelSpeed Done — I've run |
|
Changes LGTM assuming the E2Es run through ok and we can still see the expected logs |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/verified by @huali9 Based on regression test ci/prow/regression-clusterinfra-aws-ipi-mapi passed |
openshift-ci-robot
added
the
verified
|
@huali9: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
lunarwhite
left a comment
lunarwhite
left a comment
There was a problem hiding this comment.
@pierluigilenoci Would you kindly please share how you verify this (e2e steps and results) as per your PR description?
After this change, users can set
--stderrthreshold=WARNING(or any level) and it will work as expected.
...
Verified--stderrthreshold=WARNINGnow correctly filters output when--logtostderris enabled
|
@lunarwhite Thanks for the question. Here's how the fix can be verified: Verification steps:
The change is purely in flag initialization at startup — it sets Note that this PR has already passed all CI checks including the regression-clusterinfra-aws-ipi-mapi e2e run, and has been Separately, this PR appears to be blocked by tide due to the missing |
|
Hi @JoelSpeed, @damdo — thanks again for the review and approval! All CI checks are green, and the PR has Could you help with getting a Jira ticket linked to this PR so it can proceed to merge? Happy to update the PR title/commit with the ticket reference if you can point me to the right Jira project or create one. Thanks for your time! |
|
@pierluigilenoci I was specifically asking how you actually verified this from an end-user perspective. The example you shared involves running the bin locally, but would it actually be usable in a real prod setup? MAO doesn't expose logLevel / logFlag fields that users can configure to control verbosity per component. Even if someone manually modified the Deployment to add these flags - can you confirm if the operator would reconcile those changes away during the next sync loop? This PR appears to enable a code path that isn't realistically reachable in production.
A green CI won't help in demonstrating this is a valid or practical scenario. We should be careful about what practical benefit and potential impact would be made to users /w each change. |
|
@lunarwhite — great question, and you raise a fair point about production reachability. You're right that MAO doesn't currently expose
To directly answer your question about reconciliation: yes, if you manually edit the Deployment to add Happy to discuss further if you have concerns about the approach. |
|
New changes are detected. LGTM label has been removed. |
|
Hi — it looks like the LGTM label was removed after the recent merge from main. The branch is now up to date and CI is green. Could a reviewer re-apply |
|
/retest |
|
@pierluigilenoci we don't do merge commits here. Could you rebase instead? Thanks! |
|
/retest |
|
/retest |
All five entry points (vsphere, machineset, machine-api-operator start/version, nodelink-controller) set logtostderr=true, which causes klog's legacy behavior to silently ignore the stderrthreshold flag. This opts into the fixed behavior introduced in klog v2.130.0 by setting legacy_stderr_threshold_behavior=false in all entry points. See: kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
The machine-healthcheck binary was missing the logtostderr and stderrthreshold flag.Set calls that were added to all other binaries. Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
pierluigilenoci
force-pushed
the
fix/honor-stderrthreshold
branch
from
7d10ee5 to
bfbb9ff
Compare
|
@damdo Done! The merge commit has been replaced with a clean rebase onto |
|
@pierluigilenoci: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
6 participants
What this PR does
When
--logtostderris enabled (which is the default in klog and explicitly set in all entry points), klog's legacy behavior silently ignores the--stderrthresholdflag. This means users cannot filter log output by severity level, even though the flag appears to be available.This PR opts into the fixed behavior introduced in klog v2.140.0 by setting:
legacy_stderr_threshold_behavior=false— disables the legacy overridestderrthreshold=INFO— preserves the current default behaviorAfter this change, users can set
--stderrthreshold=WARNING(or any level) and it will work as expected.All entry points in
cmd/are updated:cmd/machinesetcmd/nodelink-controllercmd/vspherecmd/machine-api-operator(start + version subcommands)cmd/machine-healthcheckklog dependency updated from v2.130.1 to v2.140.0 (which introduces the
legacy_stderr_threshold_behaviorflag).Why
See upstream fix: kubernetes/klog#432
Testing
--stderrthreshold=WARNINGnow correctly filters output when--logtostderris enabledSummary by CodeRabbit
Bug Fixes
Dependencies