Migrate from klog v1 to klog/v2 and honor stderrthreshold#69
Migrate from klog v1 to klog/v2 and honor stderrthreshold#69pierluigilenoci wants to merge 2 commits intovolcano-sh:masterfrom
Conversation
Replace all imports of k8s.io/klog with k8s.io/klog/v2, update go.mod dependencies, and configure stderrthreshold to ensure log messages are properly written to stderr. Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci <[email protected]>
|
cc @wackxu @kevin-wangzefeng for review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the project to Go 1.21 and migrates the logging infrastructure from klog v1 to v2. The changes include updating imports across multiple packages, refreshing the go.mod and go.sum files, and adding the necessary vendored dependencies. Additionally, the gene-controller main function now explicitly configures klog flags. Feedback was provided to improve error handling when setting these flags to ensure the application does not start with an invalid logging configuration.
cmd/kube-dag/gene-controller.go
Outdated
| _ = goflag.Set("legacy_stderr_threshold_behavior", "false") | ||
| _ = goflag.Set("stderrthreshold", "INFO") |
There was a problem hiding this comment.
It's important to check the errors returned by goflag.Set. If these flags fail to set, for example if the flag names change in a future version of klog, the logging behavior will not be as expected. This could make debugging issues in production very difficult. I suggest logging a fatal error if setting these flags fails to ensure the application starts with the correct logging configuration.
| _ = goflag.Set("legacy_stderr_threshold_behavior", "false") | |
| _ = goflag.Set("stderrthreshold", "INFO") | |
| if err := goflag.Set("legacy_stderr_threshold_behavior", "false"); err != nil { | |
| klog.Fatalf("failed to set klog flag 'legacy_stderr_threshold_behavior': %v", err) | |
| } | |
| if err := goflag.Set("stderrthreshold", "INFO"); err != nil { | |
| klog.Fatalf("failed to set klog flag 'stderrthreshold': %v", err) | |
| } |
Replace discarded error returns (_ = goflag.Set(...)) with proper error handling that calls klog.Fatalf on failure, so misconfigured logging flags are surfaced immediately instead of being silently ignored. Signed-off-by: Pierluigi Lenoci <[email protected]>
|
Hi @wackxu @kevin-wangzefeng — friendly follow-up on this PR. It migrates the logging from klog v1 to v2 and updates the Go version. All CI checks are passing (DCO passed, tide pending labels as expected). Would you be able to review when you get a chance? Thank you! |
1 participant
Summary
k8s.io/klogimports withk8s.io/klog/v2across all non-vendor Go source filesgo.modto depend onk8s.io/klog/v2 v2.140.0and rungo mod tidy+go mod vendorstderrthreshold=INFOandlegacy_stderr_threshold_behavior=falseafterklog.InitFlags()in the main entry point so that log messages are properly written to stderrMotivation
k8s.io/klogv1 is deprecated and unmaintained. Migrating tok8s.io/klog/v2brings bug fixes, better performance, and alignment with the broader Kubernetes ecosystem.Additionally, klog's default
stderrthresholdbehavior silently drops INFO/WARNING messages from stderr unless explicitly configured. This patch ensures all log levels are emitted to stderr as expected.Ref: kubernetes/klog#212, kubernetes/klog#432
Test plan
go build ./cmd/kube-dag/...compiles successfullygo test ./...