Refactor CAPV controller context

[zhanggbj]

/kind feature

Describe the solution you'd like
[A clear and concise description of what you want to happen.]
Currently CAPV has a customized Controller Context which wraps context.Context along with other essential controller-related information as here. This context is propagated across controllers and reconcilers. However, this approach comes with drawbacks:

• Not align with the recommended practices outlined by the containedctx lint, which emphasizes that 'Contexts should not be stored inside a struct type, but instead passed to each function that needs it'.
• Hard to understand the function and may need further investigation for better comprehension about what it needs, for example func (r clusterReconciler) reconcileNormal(ctx *context.ClusterContext)
• contains mixed usage of CAPV controller context and golang context, sometimes capv will initialize new golang context to call some API. It is difficult to enable contextual logging and tracing.

Given the sensitivity of context usage, particularly its impact across controllers, propose a refactoring of the controller context, aiming to split it into golang context and the CAPV controller context. This will be achieved incrementally through the following steps:

• Enable the linter containedctx and use '//nolint:containedctx' to exclude current findings. Refer to Audit golangci-lint config #2058 . (@zhanggbj 🌱 lint: enable containedctx lint #2297 )

• rename all "CAPV context's" in a consistent way: e.g. ControllerContext => controllerCtx, also like imported package alias, function signature (@zhanggbj 🌱 Rename controller context and golang context #2309 )

• Introduce new ctx context.Context parameters to controller and reconciler and underlying functions, such as functions under /pkg, also fix Unit Test. We can break this down by controllers as outlined below, but let's begin and be open to adjustments as needed.

  • controllers/clustermodule_reconciler.go (@sbueringer 🌱 Improve logging of VSphereCluster, ProviderServiceAccount and ServiceDiscovery controllers #2352)
  • controllers/serviceaccount_controller.go (@sbueringer 🌱 Improve logging of VSphereCluster, ProviderServiceAccount and ServiceDiscovery controllers #2352)
  • controllers/servicediscovery_controller.go (@sbueringer 🌱 Improve logging of VSphereCluster, ProviderServiceAccount and ServiceDiscovery controllers #2352)
  • controllers/vspherecluster_controller.go (@zhanggbj ✨ Set up golang context when initiating controller manager #2339)
  • controllers/vspherecluster_reconciler.go (@zhanggbj ✨ Set up golang context when initiating controller manager #2339)
  • controllers/vsphereclusteridentity_controller.go (@MrDXY) ✨ refactor context for vsphereclusteridentity controller #2374
  • controllers/vspheredeploymentzone_controller.go (@zhanggbj ✨ refactor context for vspheredeploymentzone controller #2369)
  • controllers/vspheredeploymentzone_controller_domain.go (@zhanggbj ✨ refactor context for vspheredeploymentzone controller #2369)
  • controllers/vspheremachine_controller.go (@Ankitasw) ✨ Refactor context for vspheremachine controller #2376
  • controllers/vspherevm_controller.go (@xiujuanx) ✨ Refactor context for vspherevm controller #2398
  • controllers/vspherevm_ipaddress_reconciler.go (@xiujuanx) ✨ Refactor context for vspherevm controller #2398
  • controllers/vmware/vspherecluster_reconciler.go
  • controller initiation in main.go (✨ Set up golang context when initiating controller manager #2339)

• get rid of our containedctx's exception and removed context.Context from capv customized controller Context (@zhanggbj 🌱 Remove context from capv customized controller context #2431)

• check that there are no leftover context.TODO()'s (indicates we missed something to refactor above) (@zhanggbj 🌱 Remove context from capv customized controller context #2431)

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

• Cluster-api-provider-vsphere version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

[sbueringer]

Sounds perfect. Thank you!

[sbueringer]

Introduce new ctx context.Context parameters to funcs, also fix Unit Test. We can break this down by controllers as outlined below, but let's begin and be open to adjustments as needed.

Just stating the obvious probably. The idea would be basically to use the ctx that controller-runtime hands us over in the Reconcile func and then pass it down to wherever we need it. This would be:

• places where today we use the ctx from our CAPV contexts
• places where we use context.TODO/context.Background (in a lot of cases this is when we make a Get/List/... call via the controller-runtime client)

[zhanggbj]

/assign
/help wanted

[zhanggbj]

The first PR to pass down context to controller/reconciler are in review #2339.

Please feel free to pick up other tasks listed above once it is merged. We can assign this issue to multiple assignees :) Thanks!

[sbueringer]

Let's hold this for a bit (hopefully only a few days). I started to unblock the logging work and had to add more context handling across half the repo. Let's get this one in first to avoid duplicate work: #2352

[sbueringer]

Updated the tasks above with the controllers already refactored via #2352

[chrischdi]

Added task:

• check that there are no leftover context.TODO()'s (indicates we missed something to refactor above)

[sbueringer]

#2352 merged

@zhanggbj I think we can continue here

cc @Madhur97 @Ankitasw If you have time to pick up some of the tasks here (let us know if you need more context)

[zhanggbj]

Also CC @MrDXY @xiujuanx feel free to pick up tasks, this is fundamental work for tracing and logging :-)

[MrDXY]

I'll take this one! 😄

• controllers/vsphereclusteridentity_controller.go

[Ankitasw]

I can pick controllers/vspheremachine_controller.go

[zhanggbj]

/assign @Ankitasw

[zhanggbj]

I'll continue with this one.

• controllers/vspheredeploymentzone_controller.go

[xiujuanx]

I would like to help on this one:

• controllers/vspherevm_controller.go

[sbueringer]

Thx folks! I've updated the list above accordingly

[Madhur97]

I can pick controllers/vspherevm_ipaddress_reconciler.go

[zhanggbj]

/assign @xiujuanx

[zhanggbj]

/assign @Madhur97

[zhanggbj]

/assign @MrDXY

[zhanggbj]

About the logger which may relate to context refactoring for each controller, I think we could leave them for separate PRs to keep our PR easy to review, and logging refactoring are tracking by #2076
CC @MrDXY @Ankitasw @xiujuanx @Madhur97

[sbueringer]

Absolutely. Let's not start using contextual logging in these PRs.

[chrischdi]

Re-assigning controllers/vspherevm_ipaddress_reconciler.go to @xiujuanx because its included in #2398 .

[adityabhatia]

I believe first 3 entries: controllers/clustermodule_reconciler.go, controllers/serviceaccount_controller.go, controllers/servicediscovery_controller.go were already fixed in 09ff688

[zhanggbj]

@adityabhatia yes, just linked them in case of any confusion.

[chrischdi]

#2398 should be merging now too.

Afterwards we can do the final steps and verify that we didn't miss a case :-)

[chrischdi]

Thanks for taking the next tasks @zhanggbj !

[zhanggbj]

As we're holding #2462 and trying to reproduce the flaky error, it may take a long time, so move it out of this list to #2506 for tracking.
I'll close this, thanks a lot for all the contributors and also reviewers! :-)

[sbueringer]

Sounds good, thank you!