🌱 Improve logging for VSphereMachine #2395

Ankitasw · 2023-09-26T14:51:23Z

What this PR does / why we need it:
Improve logging for VSphereMachine controller and underlying packages.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #2076

codecov · 2023-09-26T14:55:39Z

Codecov Report

Attention: 42 lines in your changes are missing coverage. Please review.

Comparison is base (3b7fbc6) 60.79% compared to head (a3f651f) 60.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2395      +/-   ##
==========================================
+ Coverage   60.79%   60.86%   +0.07%     
==========================================
  Files         164      164              
  Lines        9473     9435      -38     
==========================================
- Hits         5759     5743      -16     
+ Misses       3298     3274      -24     
- Partials      416      418       +2

Files	Coverage Δ
pkg/context/fake/fake_machine_context.go	`94.11% <ø> (-0.09%)`	⬇️
pkg/context/machine_context.go	`85.71% <100.00%> (+10.71%)`	⬆️
pkg/context/vmware/machine_context.go	`100.00% <100.00%> (ø)`
pkg/util/testutil.go	`100.00% <ø> (ø)`
controllers/vspheremachine_controller.go	`76.47% <90.62%> (-0.09%)`	⬇️
pkg/services/vmoperator/vmopmachine.go	`68.68% <68.62%> (+0.15%)`	⬆️
pkg/services/vimmachine.go	`60.96% <57.40%> (+3.95%)`	⬆️

... and 3 files with indirect coverage changes

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

controllers/servicediscovery_controller.go

pkg/services/vimmachine.go

chrischdi

What a great refactoring PR.

Some smaller nits/questions :-)

pkg/services/vmoperator/vmopmachine.go

pkg/services/vmoperator/vmopmachine_test.go

controllers/vspheremachine_controller.go

pkg/services/vimmachine.go

chrischdi

I think last round then it should be in great shape!

controllers/vspheremachine_controller.go

pkg/services/vimmachine.go

chrischdi

One very last nit

With that lgtm/approve from my side 👍

Great job 🎉

chrischdi · 2023-09-29T14:22:10Z

/lgtm

/assign @killianmuldoon

k8s-ci-robot · 2023-09-29T14:22:16Z

LGTM label has been added.

Git tree hash: b3dc1a9e4fb06a2ca8216b2f5f6afcfacf79226e

k8s-ci-robot · 2023-09-29T15:23:45Z

@Ankitasw: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-vsphere-apidiff-main	`a3f651f`	link	false	`/test pull-cluster-api-provider-vsphere-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

killianmuldoon · 2023-10-02T10:11:17Z

pkg/services/vmoperator/vmopmachine.go

-	if supervisorMachineCtx.Logger.V(5).Enabled() {
-		vms, err := getVirtualMachinesInCluster(supervisorMachineCtx)
-		supervisorMachineCtx.Logger.Info("Trace Destroy PRE: VirtualMachines", "vmcount", len(vms), "error", err)
+	if log.V(5).Enabled() {


I'm fine with keeping this as-is for now - but let's add a TODO to change the way this logging works - we shouldn't be using if log.V(5).Enabled() IMO, and we should avoid deferred logging if possible

killianmuldoon

Great work - I have a couple of comments, but let's just add some items to #2076

Thanks!

/lgtm
/approve

killianmuldoon · 2023-10-02T10:13:17Z

pkg/services/vmoperator/vmopmachine.go

 	providerID := fmt.Sprintf("vsphere://%s", vm.Status.BiosUUID)

 	if supervisorMachineCtx.VSphereMachine.Spec.ProviderID == nil || *supervisorMachineCtx.VSphereMachine.Spec.ProviderID != providerID {
 		supervisorMachineCtx.VSphereMachine.Spec.ProviderID = &providerID
-		supervisorMachineCtx.Logger.Info("Updated provider ID for machine", "machine", supervisorMachineCtx.VSphereMachine.Name, "provider-id", providerID)
+		log.Info("Updated provider ID", "providerID", providerID)


Fine with this for now - keeping parity with the current state - but we should re-evaluate what we use in K/V pairs. This isn't put into the context so it's only going to appear in one log line.

k8s-ci-robot · 2023-10-02T10:19:06Z

LGTM label has been added.

Git tree hash: 3ff45bd14dffed89ea395d9fb7573cdff34a5254

k8s-ci-robot · 2023-10-02T10:19:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: killianmuldoon

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:

~~OWNERS~~ [killianmuldoon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

killianmuldoon · 2023-10-02T10:19:46Z

Also - let's focus on context refactoring instead of logging for now - that will make these PRs easier to review going forward.

Ankitasw · 2023-10-02T14:30:31Z

@killianmuldoon actually the context refactoring for vspheremachine controller is already done, that's why picked up this logging improvement.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 26, 2023

k8s-ci-robot requested review from MaxRink and vrabbi September 26, 2023 14:51

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 26, 2023

Ankitasw force-pushed the vspheremachine-logging branch from 8eca562 to 80ab893 Compare September 28, 2023 11:50

Ankitasw changed the title ~~🌱 [WIP] Improve logging for VSphereMachine~~ 🌱 Improve logging for VSphereMachine Sep 28, 2023

Ankitasw force-pushed the vspheremachine-logging branch from 80ab893 to ce205df Compare September 28, 2023 12:54

chrischdi reviewed Sep 28, 2023

View reviewed changes

controllers/servicediscovery_controller.go Outdated Show resolved Hide resolved

chrischdi reviewed Sep 28, 2023

View reviewed changes

pkg/services/vimmachine.go Outdated Show resolved Hide resolved

chrischdi reviewed Sep 28, 2023

View reviewed changes

pkg/services/vmoperator/vmopmachine.go Outdated Show resolved Hide resolved

pkg/services/vmoperator/vmopmachine_test.go Outdated Show resolved Hide resolved

Ankitasw force-pushed the vspheremachine-logging branch from ce205df to 3de0f9c Compare September 29, 2023 06:52

Ankitasw requested a review from chrischdi September 29, 2023 06:54

chrischdi reviewed Sep 29, 2023

View reviewed changes

Ankitasw force-pushed the vspheremachine-logging branch from 3de0f9c to 38a7116 Compare September 29, 2023 11:25

Ankitasw requested a review from chrischdi September 29, 2023 13:14

chrischdi reviewed Sep 29, 2023

View reviewed changes

Ankitasw force-pushed the vspheremachine-logging branch from 38a7116 to b87781d Compare September 29, 2023 13:30

Ankitasw requested a review from chrischdi September 29, 2023 13:35

chrischdi reviewed Sep 29, 2023

View reviewed changes

pkg/services/vimmachine.go Outdated Show resolved Hide resolved

chrischdi reviewed Sep 29, 2023

View reviewed changes

Ankitasw force-pushed the vspheremachine-logging branch from b87781d to 32c4b37 Compare September 29, 2023 14:21

Ankitasw requested a review from chrischdi September 29, 2023 14:21

k8s-ci-robot assigned killianmuldoon and chrischdi Sep 29, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2023

Ankitasw force-pushed the vspheremachine-logging branch from 32c4b37 to 83a94e2 Compare September 29, 2023 15:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2023

k8s-ci-robot requested a review from killianmuldoon September 29, 2023 15:14

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2023

Improve logging for VSphereMachine

a3f651f

Ankitasw force-pushed the vspheremachine-logging branch from 83a94e2 to a3f651f Compare September 29, 2023 15:18

killianmuldoon reviewed Oct 2, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2023

k8s-ci-robot merged commit 665d00b into kubernetes-sigs:main Oct 2, 2023
10 checks passed

k8s-ci-robot added this to the v1.9 milestone Oct 2, 2023

killianmuldoon mentioned this pull request Oct 2, 2023

Improve CAPV logging #2076

Closed

12 tasks

Ankitasw deleted the vspheremachine-logging branch October 2, 2023 14:29

Ankitasw mentioned this pull request Oct 4, 2023

🌱 Removal of if block usage wrt logging in vsphere machine controller #2413

Merged

rikatz mentioned this pull request Oct 30, 2023

🐛 Re-add return on vimmachine error #2473

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Improve logging for VSphereMachine #2395

🌱 Improve logging for VSphereMachine #2395

Ankitasw commented Sep 26, 2023

codecov bot commented Sep 26, 2023 •

edited

Loading

chrischdi left a comment

chrischdi left a comment

chrischdi left a comment

chrischdi commented Sep 29, 2023

k8s-ci-robot commented Sep 29, 2023

k8s-ci-robot commented Sep 29, 2023 •

edited

Loading

killianmuldoon Oct 2, 2023

killianmuldoon left a comment

killianmuldoon Oct 2, 2023

k8s-ci-robot commented Oct 2, 2023

k8s-ci-robot commented Oct 2, 2023

killianmuldoon commented Oct 2, 2023

Ankitasw commented Oct 2, 2023

🌱 Improve logging for VSphereMachine #2395

🌱 Improve logging for VSphereMachine #2395

Conversation

Ankitasw commented Sep 26, 2023

codecov bot commented Sep 26, 2023 • edited Loading

Codecov Report

chrischdi left a comment

Choose a reason for hiding this comment

chrischdi left a comment

Choose a reason for hiding this comment

chrischdi left a comment

Choose a reason for hiding this comment

chrischdi commented Sep 29, 2023

k8s-ci-robot commented Sep 29, 2023

k8s-ci-robot commented Sep 29, 2023 • edited Loading

killianmuldoon Oct 2, 2023

Choose a reason for hiding this comment

killianmuldoon left a comment

Choose a reason for hiding this comment

killianmuldoon Oct 2, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 2, 2023

k8s-ci-robot commented Oct 2, 2023

killianmuldoon commented Oct 2, 2023

Ankitasw commented Oct 2, 2023

codecov bot commented Sep 26, 2023 •

edited

Loading

k8s-ci-robot commented Sep 29, 2023 •

edited

Loading