🌱 Improve logging of VSphereCluster, ProviderServiceAccount and ServiceDiscovery controllers

[sbueringer]

Set up golang context when initiating controller manager and also pass it down
to VSphereCluster controller

Signed-off-by: Gong Zhang [email protected]

What this PR does / why we need it:
Have to do a few more things + the underlying PR has to be merged first

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]

Codecov Report

Patch coverage: 74.80% and project coverage change: +0.59% 🎉

Comparison is base (d63e2bc) 60.37% compared to head (09ff688) 60.97%.
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2352      +/-   ##
==========================================
+ Coverage   60.37%   60.97%   +0.59%     
==========================================
  Files         164      164              
  Lines        9442     9465      +23     
==========================================
+ Hits         5701     5771      +70     
+ Misses       3324     3284      -40     
+ Partials      417      410       -7     

Files Changed	Coverage Δ
controllers/vsphereclusteridentity_controller.go	53.84% <0.00%> (+0.90%)	⬆️
pkg/clustermodule/session.go	29.78% <20.00%> (ø)
pkg/clustermodule/service.go	20.61% <32.43%> (+3.22%)	⬆️
pkg/context/fake/fake_guest_cluster_context.go	71.42% <50.00%> (ø)
pkg/context/fake/fake_vmware_cluster_context.go	71.42% <50.00%> (-3.58%)	⬇️
controllers/clustermodule_reconciler.go	61.46% <58.82%> (-0.98%)	⬇️
controllers/vspheredeploymentzone_controller.go	76.00% <60.00%> (-2.67%)	⬇️
pkg/manager/network.go	58.82% <60.00%> (+5.49%)	⬆️
pkg/context/vmware/cluster_context.go	83.33% <66.66%> (ø)
controllers/vspherecluster_reconciler.go	62.03% <72.00%> (+1.94%)	⬆️
... and 20 more

... and 5 files with indirect coverage changes

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

[sbueringer]

/test pull-cluster-api-provider-vsphere-e2e-full-main

[sbueringer]

/test pull-cluster-api-provider-vsphere-e2e-full-main

[sbueringer]

/test pull-cluster-api-provider-vsphere-e2e-full-main

[sbueringer]

/test pull-cluster-api-provider-vsphere-e2e-full-main

[randomvariable]

Whilst you're doing this, i would enable loggercheck in golangci-lint to catch when someone hasn't completed kv pairs.

[sbueringer]

Whilst you're doing this, i would enable loggercheck in golangci-lint to catch when someone hasn't completed kv pairs.

Thx, nice one! Will do

[killianmuldoon]

/test pull-cluster-api-provider-vsphere-e2e-full-main

[killianmuldoon]

/lgtm remove

Should be squashed before merge.

[killianmuldoon]

/lgtm cancel

[sbueringer]

I would generally prefer if we tried to keep changes like this more atomic in future - this PR has so many small changes I think it's very difficult to review.

I agree. This escalated a bit and it was a mixture of keeping the PR safe and in some cases fixing up things while I'm already there.

A majority of the "scope creep" was because the only way to safely refactor this was to remove the *context.ControllerContext and Logger entirely from ClusterContext. The result was that I had to refactor some other controllers as well, the same applies to packages like network & vmoperator that are used by the controllers.

I had an intermediate version where I only tried to stop using these fields and not setting them in some controller but this blew up pretty badly in some cases I missed in unit tests (panic's all over the place). Because I couldn't be sure that we have full coverage I thought it's the safer option to play it safe and drop these fields entirely instead of hoping that I got all relevant places and then risking panic's in rare edge cases in production later. This way the compiler took care of ensuring that there are no panics when ControllerContext & Logger are accessed (because the fields don't exist anymore)

[sbueringer]

Pushed some fixes. Will squash later after @chrischdi 's review and corresponding fixes

[sbueringer]

codecov failed because of the flake in our tests

[sbueringer]

/test pull-cluster-api-provider-vsphere-e2e-full-main

[chrischdi]

One thing to maybe take another look (I may missed something there?!)

A lot of awesome small fixes are included too! 🎉

[chrischdi]

not in scope of this PR, but wondering if we should splitup this file to netop_provider_test.go and nsxt_provider_test.go

[sbueringer]

Feel free to open an issue/PR for it. Probably it should be one file per provider (including dummy)

[chrischdi]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 2231c60b5e8f9411512e105d14ac77b86fdcebde

[sbueringer]

/test pull-cluster-api-provider-vsphere-e2e-full-main

[sbueringer]

/test ?

[sbueringer]

/test pull-cluster-api-provider-vsphere-conformance-main

[k8s-ci-robot]

@sbueringer: 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	09ff688	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.

[sbueringer]

/hold cancel