🌱 Switch logging implementation to klog #2810
Conversation
metal3-io-bot
added
the
size/L
fmuyassarov
force-pushed
the
devel/klog
branch
from
2060219 to
e62fb84
Compare
|
/assign @kashifest |
|
Fix the DCO and squash the commit, please. |
Use /cc for requesting reviews. |
I would prefer to keep them as they are actually unless there is a valid reason to squash them. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the logging implementation from Zap to klog v2 to align with logging approaches used in other Metal3 controllers (CAPM3, Ironic, and IPAM), ensuring consistent logging format and behavior across the Metal3 ecosystem.
- Replaces Zap logger initialization with klog v2 throughout the codebase
- Removes Zap-specific configuration including development mode and time encoder settings
- Updates logger naming hierarchy by removing intermediate "controllers" namespace level
- Simplifies variable declarations in main.go by grouping them in a single var block
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| main.go | Removes Zap initialization and configuration, replaces with klog.NewKlogr(), updates logger naming for controllers, and converts setupLog usage to direct klog calls in several places |
| pkg/provisioner/ironic/factory.go | Adds newline prefixes to logging key names for multi-line formatting of ironic configuration settings |
| pkg/provisioner/fixture/fixture.go | Replaces Zap logger initialization with klog.NewKlogr() for the fixture provisioner |
| go.mod | Moves go.uber.org/zap from direct dependency to indirect dependency since it's no longer directly used |
Comments suppressed due to low confidence (1)
main.go:143
- The
devLoggingvariable is declared and set via the--devflag but is no longer used after removing the Zap configuration. This flag should either be removed if not needed, or the klog configuration should respect it (e.g., by setting verbosity level based on this flag).
devLogging bool
runInTestMode bool
runInDemoMode bool
webhookPort int
restConfigQPS float64
restConfigBurst int
controllerConcurrency int
leaseDurationSeconds string
renewDeadlineSeconds string
retryPeriodSeconds string
)
// From CAPI point of view, BMO should be able to watch all namespaces
// in case of a deployment that is not multi-tenant. If the deployment
// is for multi-tenancy, then the BMO should watch only the provided
// namespace.
flag.StringVar(&watchNamespace, "namespace", os.Getenv("WATCH_NAMESPACE"),
"Namespace that the controller watches to reconcile host resources.")
flag.StringVar(&metricsBindAddr, "metrics-addr", ":8443",
"The address the metric endpoint binds to.")
flag.BoolVar(&enableLeaderElection, "enable-leader-election", false,
"Enable leader election for controller manager. "+
"Enabling this will ensure there is only one active controller manager.")
flag.BoolVar(&preprovImgEnable, "build-preprov-image", false, "enable integration with the PreprovisioningImage API")
flag.BoolVar(&devLogging, "dev", false, "enable developer logging")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I don't think the amount of code in them warrants multiple commits. |
fmuyassarov
force-pushed
the
devel/klog
branch
from
e62fb84 to
f0a713e
Compare
metal3-io-bot
added
size/M
I also don't think that amount of the code is the factor for dividing PRs into commits, but rather commits should be self-contained and functional on their own. |
|
@tuminoid it is a single commit now + I've dropped the unrelated commit from the list. |
|
/approve I am honestly not sure if the unresolved copilot review needs to be addressed or not. Looks ok to me. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
metal3-io-bot
added
approved
fmuyassarov
force-pushed
the
devel/klog
branch
from
f0a713e to
d851f5d
Compare
metal3-io-bot
removed
the
needs-rebase
that's something tiny but I fixed now + rebased the tree. |
fmuyassarov
force-pushed
the
devel/klog
branch
from
d851f5d to
83b4813
Compare
@kashifest actually it was a bad idea to fix what copilot asked 😄 , because it had no context and it broke the unit tests since they have some expectation for certain error messages in certain string. I reverted it to use |
|
Could you comment on the resolved copilot comments that you didn't do anything about? It would be very helpful for the review to know why the comment was not addressed. I may have similar comments otherwise 😉 |
Sure. The reason I skipped those suggestions is that I want to maintain the same ordering and structure of the logging context. In the current implementation, the setup context is only added for certain log messages, not all of them. If you look at the PR description, you can see how it behaved previously with zap and how it works now with klog. I tried to keep the context consistent with the original behavior, which is why I skipped all of those Copilot comments. |
lentzi90
left a comment
lentzi90
left a comment
There was a problem hiding this comment.
Honestly, to me the example output looks much better with zap. That is structured logging that can be parsed easily to filter out specific fields and so on.
|
It feels to me like we are here trying to switch to a different logger, that is supposed to behave differently, but we still want to keep the behavior the way it was. Not adjusting unit tests, etc. This is not a good idea. |
metal3-io-bot
added
the
do-not-merge/hold
Sure, but what shall we fix? I don't get it. All CI tests have passed, no? |
|
How about this:
You replied to this that it doesn't make sense because it breaks unit tests. So my interpretation is that we are here trying to keep the behavior even when that is not idiomatic or recommended. This is what I mean we should fix. Either update the unit tests and address the comment or explain why this comment is wrong. |
I can try and I think I replied here that it broke the CI due to it expecting specific string. Anyways, If I address and use the suggestion of copilot, diff --git a/main.go b/main.go
index 15652755..2eacfd59 100644
--- a/main.go
+++ b/main.go
@@ -447,7 +447,7 @@ func GetTLSOptionOverrideFuncs(options TLSOptions) ([]func(*tls.Config), error)
for _, cipher := range tlsCipherSuites {
for _, insecureCipherName := range insecureCipherValues {
if insecureCipherName == cipher {
- klog.Infof("warning: use of insecure cipher '%s' detected.", cipher)
+ klog.Info("warning: use of insecure cipher detected", "cipher", cipher)
}
}
}
(END)It breaks the test because we are checking for string value in the error message. However, go test -coverprofile=cover.out ./...
--- FAIL: TestTLSInsecureCiperSuite (0.00s)
--- FAIL: TestTLSInsecureCiperSuite/test_insecure_cipher_suite_passed_as_TLS_flag (0.00s)
main_test.go:45:
Expected
<string>: I1128 12:17:53.257404 54613 main.go:450] warning: use of insecure cipher detectedcipherTLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
to contain substring
<string>: use of insecure cipher 'TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256' detected.
FAIL |
|
It looks like we are missing some config options to make klog format the messages properly. |
fmuyassarov
force-pushed
the
devel/klog
branch
from
83b4813 to
f002e40
Compare
|
@lentzi90 I've addressed your comments. PTAL. |
This commit removes the dependencies on go.uber.org/zap and sigs.k8s.io/controller-runtime/pkg/log/zap, replacing them with klog v2. Other Metal3 controllers—including CAPM3, Ironic, and IPAM— already use klog v2, so this change aligns the logging approach across the project and ensures a consistent logging format. Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov
force-pushed
the
devel/klog
branch
from
f002e40 to
2164f20
Compare
|
/retest |
lentzi90
left a comment
lentzi90
left a comment
There was a problem hiding this comment.
There are still three places where setupLog is changed to klog. I think we should stick to setupLog for those.
I am still also concerned about the change in format. Changing to klog is fine and for the better, but we should configure it to have similar output format. I have no issue with the change in the timestamp format, but we should keep the nice json format that makes it easy to parse with tools.
Current main:
{"level":"info","ts":1764817462.027348,"logger":"setup","msg":"Go Version: go1.24.11"}
{"level":"info","ts":1764817462.027371,"logger":"setup","msg":"Go OS/Arch: linux/amd64"}
{"level":"info","ts":1764817462.0273771,"logger":"setup","msg":"Git commit: unknown"}
{"level":"info","ts":1764817462.0273826,"logger":"setup","msg":"Build time: unknown"}
{"level":"info","ts":1764817462.027388,"logger":"setup","msg":"Component: metal3-io/baremetal-operator was not built with version info"}
{"level":"info","ts":1764817462.0274692,"logger":"setup","msg":"Manager set up with multiple namespaces to watch","namespaces":"basic-ops,external-inspection,externally-provisioned,inspection,live-iso-ops,provisioning-ops,re-inspection,automated-cleaning,upgrade-bmo,upgrade-ironic"}
...
{"level":"info","ts":1764817463.04442,"msg":"Starting Controller","controller":"bmceventsubscription","controllerGroup":"metal3.io","controllerKind":"BMCEventSubscription"}
{"level":"info","ts":1764817463.0443861,"msg":"Starting Controller","controller":"hostfirmwaresettings","controllerGroup":"metal3.io","controllerKind":"HostFirmwareSettings"}
{"level":"info","ts":1764817463.0444293,"msg":"Starting workers","controller":"hostfirmwaresettings","controllerGroup":"metal3.io","controllerKind":"HostFirmwareSettings","worker count":8}
{"level":"info","ts":1764817463.0444257,"msg":"Starting workers","controller":"bmceventsubscription","controllerGroup":"metal3.io","controllerKind":"BMCEventSubscription","worker count":8}
This PR:
I1203 08:36:54.537138 1 main.go:82] "Go Version: go1.24.9" logger="setup"
I1203 08:36:54.537240 1 main.go:83] "Go OS/Arch: linux/amd64" logger="setup"
I1203 08:36:54.537245 1 main.go:84] "Git commit: unknown" logger="setup"
I1203 08:36:54.537249 1 main.go:85] "Build time: unknown" logger="setup"
I1203 08:36:54.537254 1 main.go:86] "Component: metal3-io/baremetal-operator was not built with version info" logger="setup"
I1203 08:36:54.537358 1 main.go:200] "Manager set up with multiple namespaces to watch" logger="setup" namespaces="basic-ops,external-inspection,externally-provisioned,inspection,live-iso-ops,provisioning-ops,re-inspection,automated-cleaning,upgrade-bmo,upgrade-ironic"
...
I1203 08:36:55.555709 1 controller.go:289] "Starting workers" controller="baremetalhost" controllerGroup="metal3.io" controllerKind="BareMetalHost" worker count=8
I1203 08:36:55.555714 1 controller.go:289] "Starting workers" controller="hostfirmwarecomponents" controllerGroup="metal3.io" controllerKind="HostFirmwareComponents" worker count=8
I1203 08:36:55.555661 1 controller.go:286] "Starting Controller" controller="hostfirmwaresettings" controllerGroup="metal3.io" controllerKind="HostFirmwareSettings"
I1203 08:36:55.555661 1 controller.go:286] "Starting Controller" controller="bmceventsubscription" controllerGroup="metal3.io" controllerKind="BMCEventSubscription"
|
Honestly, I'd rather have the format parseable by humans by default (i.e. klog) but maybe it's only me. |
|
In my opinion,klog is batter than zag. |
|
I am also fine, and supportive of switching to klog. I would however prefer to not change the format at the same time. But if the community wants to change the format, fine. |
tuminoid
left a comment
tuminoid
left a comment
There was a problem hiding this comment.
How is the consistency of the logs in the org right now? We don't want to be logging four different ways and formats.
unfortunately that's already the case and i want to fix that as well a long the way. |
|
If people are really keen on parsing the logs of BMO, I will check how to have the JSON format. Don't recall if there is a flag but will test and confirm. |
For that reason we need consensus where to take the logging. :) |
7 participants
What this PR does / why we need it:
Migrate the logging implementation from Zap to klog v2, aligning with the logging approach used by other Metal3 controllers (CAPM3, Ironic, and IPAM). This ensures consistent logging format and behavior.
With Zap
With klog