Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve CAPV logging #2076

Closed
12 tasks done
sbueringer opened this issue Jul 24, 2023 · 7 comments · Fixed by #2527
Closed
12 tasks done

Improve CAPV logging #2076

sbueringer opened this issue Jul 24, 2023 · 7 comments · Fixed by #2527
Assignees

Comments

@sbueringer
Copy link
Member

sbueringer commented Jul 24, 2023

Logging is a crucial to make it easier to understand controller behavior and troubleshoot issues.

While onboarding to CAPV we identified a few quickwins which should improve log quality significantly:

  • It is a best practice in Kubernetes to add k/v pairs of the involved objects to a log line (https://cluster-api.sigs.k8s.io/developer/logging.html#keyvalue-pairs)
    • We should also add k/v pairs for all involved objects (and their owner hierarchy) that we retrieve at the beginning of the Reconcile
  • We should also consistently use contextual logging (i.e. taking the logger from the ctx). This allows propagation of k/v pairs across the entire call stack
    • This also includes using the ctx that controller-runtime passes into the Reconcile func instead of some global ctx that is the same across Reconcile calls.

The overall goal of this issue is to improve the logs so troubleshooting becomes easier. Adding k/v pairs across the board will make it very easy to correlate logs e.g. for a specific Machine across controllers.

Additional notes:

  • Let's take a look at how event recorders are setup in core CAPI vs CAPV
  • Take a look at controller logger setup (e.g. regarding name). IIRC we shouldn't need any logger on the controllers though
  • Audit all log calls for additional k/v pairs
  • Ensure "Named" is set correctly on all controllers
  • Let's double check the k/v pairs we add in core CAPI to ensure we can cross-reference everything

Prior art in core CAPI:

Tasks

Concrete tasks for now (I have some follow-ups to audit, but let's do this afterwards):

I took a quick look and there should be no overlap, so every tasks should ideally be a separate PR.

Tasks per controller:

  • Adjust controller setup:
    • ControllerContext & Logger fields should be dropped from the Reconciler struct
    • Add Client & Recorder fields instead
    • If necessary we can add ControllerManagerContext (with a field name, no embedding to make the usage of the context explicit)
  • Use logger from context.Context. Drop client & logger fields from structs like ClusterContext, MachineContext, ...
    • If there is currently no ctx available to get the logger from, add a ctx parameter to the current func
    • This will probably also lead to some compile errors when e.g. a MachineContext is passed into a client.Get. Please then pass in a context.Context instead.
  • Add k/v pairs where appropriate and for all related object we "get" early in Reconcile

For more details see #2352

@sbueringer
Copy link
Member Author

cc @chrischdi @killianmuldoon @randomvariable @srm09 @yastij fyi

@sbueringer
Copy link
Member Author

/assign

@Ankitasw
Copy link
Member

Will pick vspheremachine_controller.go once this PR is merged.

@Ankitasw
Copy link
Member

Ankitasw commented Sep 28, 2023

Will pick vspheredeploymentzone_controller.go and vspheredeploymentzone_controller_domain.go after PR merges.

@killianmuldoon
Copy link
Contributor

Two follow up tasks / things to check in logging:

  • Remove defer logs where possible
  • Remove checking log levels explicitly - e.g. log.V(5).Enabled()

Examples of this here: #2395 (comment)

@chrischdi
Copy link
Member

Thanks for picking vsphereclusteridentity_controller.go @Madhur97 👍

@adityabhatia
Copy link
Contributor

I can pickup vspherevm_controller.go & vspherevm_ipaddress_reconciler.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants