Skip to content

fix: correct 17 klog format-string bugs across the codebase#1134

Merged
luomingmeng merged 1 commit intokubewharf:mainfrom
pierluigilenoci:fix/honor-stderrthreshold
May 5, 2026
Merged

fix: correct 17 klog format-string bugs across the codebase#1134
luomingmeng merged 1 commit intokubewharf:mainfrom
pierluigilenoci:fix/honor-stderrthreshold

Conversation

@pierluigilenoci
Copy link
Copy Markdown
Contributor

@pierluigilenoci pierluigilenoci commented Apr 22, 2026

Summary

Fix 17 pre-existing klog format-string bugs across the codebase. These calls use klog.Info, klog.Warning, klog.Error (non-formatting variants) with %s/%v placeholders, or use the f-suffixed variants with mismatched argument counts — causing log messages to be silently malformed at runtime.

Categories of bugs fixed

Bug type Count Example
klog.Info(...) with format verbs (should be klog.Infof) 2 klog.Info("pod %v added with target scraping", pod.Name)
klog.Warning(...) with format verbs (should be klog.Warningf) 3 klog.Warning("failed to patch %s ...", config.Name, err)
klog.Error(...) with format verbs (should be klog.Errorf) 1 klog.Error("GetOvercommitRatio getNode fail: %v", err)
klog.Warningf/klog.Errorf with wrong argument count 7 klog.Warningf("[%s] skip resource: %s with total: %.2f", b.pluginName, total) — missing resourceName
klog.Errorf with format string but no arguments 4 klog.Errorf("unable to sync caches for %s webhook") — missing podWebhookName

Files changed

  • pkg/agent/evictionmanager/plugin/resource/resources.go
  • pkg/agent/orm/manager_nri.go
  • pkg/agent/orm/resourceprovider.go
  • pkg/agent/sysadvisor/plugin/overcommitmentaware/realtime/realtime.go
  • pkg/controller/monitor/cnr.go
  • pkg/controller/overcommit/node/node.go
  • pkg/controller/vpa/vpa.go
  • pkg/custom-metric/collector/prometheus/collector_promethes.go
  • pkg/scheduler/eventhandlers/handler.go
  • pkg/scheduler/plugins/noderesourcetopology/filter_native.go
  • pkg/scheduler/plugins/noderesourcetopology/reserve.go
  • pkg/util/general/healthz.go
  • pkg/webhook/mutating/pod/pod.go
  • pkg/webhook/mutating/pod/resource_mutator.go

Note: This PR was originally scoped to also upgrade k8s.io/klog/v2 to v2.140.0 to honor -stderrthreshold when -logtostderr=true. That version bump caused 3400+ typecheck CI failures because it requires a coordinated dependency upgrade. The klog bump has been reverted; the format-string fixes stand on their own as valuable correctness improvements.

Signed-off-by: Pierluigi Lenoci pierluigi.lenoci@gmail.com

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 22, 2026

CLA assistant check
All committers have signed the CLA.

@pierluigilenoci
Copy link
Copy Markdown
Contributor Author

Pushed a second commit to fix the CI failures:

Vet / Unit Test failures — klog v2.140.0 adds proper printf format-checking annotations (//go:printf or equivalent), which exposed 17 pre-existing format-string bugs across the codebase. These are all bugs in the existing code, not introduced by this PR. Fixed all of them:

Category Files
Wrong function (Info/Warning/Error instead of Infof/Warningf/Errorf) cnr.go, realtime.go, collector_promethes.go, filter_native.go, node.go (×2), vpa.go, healthz.go, resource_mutator.go
Missing format arguments resources.go, handler.go, reserve.go, node.go, pod.go
Mismatched format verbs / wrong arg count manager_nri.go, resourceprovider.go (×2)

Lint / Parallel failures — Removed the toolchain go1.24.6 directive from go.mod. This directive caused Go to auto-download Go 1.24.6, which produces export data in a newer format (version 2) that golangci-lint v1.52.2 and paralleltest v1.0.10 cannot parse (panic: unsupported version: 2).

@pierluigilenoci pierluigilenoci changed the title fix: honor -stderrthreshold when -logtostderr=true fix: correct 17 klog format-string bugs across the codebase Apr 27, 2026
@pierluigilenoci
Copy link
Copy Markdown
Contributor Author

Pushed a third commit that reverts the klog v2.80.1 → v2.140.0 upgrade and the associated stderrthreshold flag changes.

Root cause of CI failures: The klog version jump pulled in go-logr/logr v1.4.1 and required go 1.21, which caused 3,400+ typecheck errors in golangci-lint (e.g., undefined: klog, missing struct fields in downstream packages). The unit test failures in pkg/agent/qrm-plugins/cpu/dynamicpolicy were also caused by the dependency incompatibility.

What remains in this PR: Only the 17 format-string bug fixes — these are real correctness issues where klog.Info/Warning/Error (non-formatting) were called with %s/%v placeholders, or klog.Infof/Warningf/Errorf had mismatched argument counts. These fixes are valid and valuable regardless of klog version.

The stderrthreshold feature requires klog >= v2.140.0, which needs a broader coordinated dependency upgrade. I can open a separate PR for that if there is interest.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026

Codecov Report

❌ Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.67%. Comparing base (1f061d6) to head (a9605b6).
⚠️ Report is 43 commits behind head on main.

Files with missing lines Patch % Lines
pkg/agent/orm/resourceprovider.go 0.00% 2 Missing ⚠️
pkg/controller/overcommit/node/node.go 33.33% 2 Missing ⚠️
...agent/evictionmanager/plugin/resource/resources.go 0.00% 1 Missing ⚠️
pkg/agent/orm/manager_nri.go 0.00% 1 Missing ⚠️
...or/plugin/overcommitmentaware/realtime/realtime.go 0.00% 1 Missing ⚠️
pkg/controller/monitor/cnr.go 0.00% 1 Missing ⚠️
...metric/collector/prometheus/collector_promethes.go 0.00% 1 Missing ⚠️
pkg/util/general/healthz.go 0.00% 1 Missing ⚠️
pkg/webhook/mutating/pod/pod.go 0.00% 1 Missing ⚠️
pkg/webhook/mutating/pod/resource_mutator.go 0.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (14.28%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1134      +/-   ##
==========================================
+ Coverage   61.50%   61.67%   +0.17%     
==========================================
  Files         785      786       +1     
  Lines       73413    73689     +276     
==========================================
+ Hits        45152    45449     +297     
+ Misses      23241    23215      -26     
- Partials     5020     5025       +5     
Flag Coverage Δ
unittest 61.67% <14.28%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@luomingmeng
Copy link
Copy Markdown
Collaborator

Pushed a third commit that reverts the klog v2.80.1 → v2.140.0 upgrade and the associated stderrthreshold flag changes.

Root cause of CI failures: The klog version jump pulled in go-logr/logr v1.4.1 and required go 1.21, which caused 3,400+ typecheck errors in golangci-lint (e.g., undefined: klog, missing struct fields in downstream packages). The unit test failures in pkg/agent/qrm-plugins/cpu/dynamicpolicy were also caused by the dependency incompatibility.

What remains in this PR: Only the 17 format-string bug fixes — these are real correctness issues where klog.Info/Warning/Error (non-formatting) were called with %s/%v placeholders, or klog.Infof/Warningf/Errorf had mismatched argument counts. These fixes are valid and valuable regardless of klog version.

The stderrthreshold feature requires klog >= v2.140.0, which needs a broader coordinated dependency upgrade. I can open a separate PR for that if there is interest.

Can you squash the commits?

Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
@pierluigilenoci pierluigilenoci force-pushed the fix/honor-stderrthreshold branch from 1b3d170 to a9605b6 Compare May 3, 2026 14:13
@pierluigilenoci
Copy link
Copy Markdown
Contributor Author

Done — the PR already contains a single commit (a9605b6). Ready for review!

@luomingmeng luomingmeng merged commit 6a36eaa into kubewharf:main May 5, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants