-
Notifications
You must be signed in to change notification settings - Fork 300
fix: use existing context when ctx is available #9773
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
fix: use existing context when ctx is available #9773
Conversation
|
Hi @anndono. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
nilo19
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran go test ./... locally; all tests passed.
This looks like a solid step toward Kubernetes contextual logging guidance (ref: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md): prefer deriving the logger from the provided ctx (via klog.FromContext / log.FromContextOrBackground) instead of using a global/background logger, and initialize the logger in the correct scope (e.g., inside leader-election callbacks).
One follow-up for consistency: there are still a few functions that accept ctx but use klog.Background() instead of the context logger (e.g. pkg/provider/azure_vmss.go EnsureHostInPool / ensureBackendPoolDeletedFromNode, and pkg/provider/azure_loadbalancer.go getEligibleLoadBalancersForService). Consider switching those to log.FromContextOrBackground(ctx) (or klog.FromContext(ctx)) so request/controller-scoped values make it into logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves logger initialization throughout the codebase by replacing log.Background() with log.FromContextOrBackground(ctx) when a context is available. This allows loggers to inherit contextual information from request contexts, providing better traceability and debugging capabilities.
Key Changes:
- Replace
log.Background()withlog.FromContextOrBackground(ctx)in functions that have context available - Rename unused context parameters from
_toctxto enable logger context usage - Move logger initialization to appropriate scopes, particularly in cache getter functions where context becomes available
- Move some
context.Background()initializations earlier in functions to enable context reuse
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/provider/subnet/subnet.go | Updated logger initialization in CreateOrUpdate to use context |
| pkg/provider/storage/storage_account.go | Updated logger in GetStorageAccesskey to use context |
| pkg/provider/storage/fileservice/fileservice_repo.go | Updated logger in Get to use context |
| pkg/provider/storage/azure_storageaccount.go | Updated loggers in multiple storage account methods to use context |
| pkg/provider/securitygroup/azure_securitygroup_repo.go | Moved logger initialization into getter closure and updated CreateOrUpdateSecurityGroup |
| pkg/provider/azure_zones.go | Updated loggers in zone-related methods to use context |
| pkg/provider/azure_vmssflex_cache.go | Updated loggers in VMSS Flex cache methods to use context |
| pkg/provider/azure_vmssflex.go | Updated loggers in VMSS Flex methods to use context |
| pkg/provider/azure_vmss_repo.go | Moved logger initialization after context creation in CreateOrUpdateVMSS |
| pkg/provider/azure_vmss_cache.go | Moved logger initialization into cache getter closures |
| pkg/provider/azure_vmss.go | Updated loggers and moved initialization after context creation where applicable |
| pkg/provider/azure_vmsets_repo.go | Updated loggers and moved initialization into cache getter closure |
| pkg/provider/azure_standard.go | Updated loggers in availability set methods to use context |
| pkg/provider/azure_routes.go | Updated loggers in route methods and renamed parameter from _ to ctx in DeleteRoute |
| pkg/provider/azure_privatelinkservice.go | Updated loggers in private link service methods to use context |
| pkg/provider/azure_local_services.go | Updated loggers in load balancer backend pool methods to use context |
| pkg/provider/azure_loadbalancer_repo.go | Updated loggers and moved initialization into cache getter closure |
| pkg/provider/azure_loadbalancer.go | Updated loggers in load balancer methods to use context |
| pkg/provider/azure_interface_repo.go | Updated logger in CreateOrUpdateInterface to use context |
| pkg/provider/azure_instances_v2.go | Updated loggers in InstancesV2 methods to use context |
| pkg/provider/azure_instances_v1.go | Updated loggers in InstancesV1 methods to use context |
| pkg/provider/azure_instance_metadata.go | Renamed parameter from _ to ctx and updated loggers in metadata methods |
| pkg/provider/azure_controller_vmssflex.go | Updated loggers in VMSS Flex controller methods to use context |
| pkg/provider/azure_controller_vmss.go | Updated loggers in VMSS controller methods to use context |
| pkg/provider/azure_controller_standard.go | Renamed parameter from _ to ctx and updated logger in DeleteCacheForNode |
| pkg/provider/azure.go | Updated logger in InitializeCloudFromConfig to use context |
| pkg/nodemanager/nodemanager.go | Updated loggers in node manager methods to use context |
| pkg/nodeipam/node_ipam_controller.go | Updated logger in Run to use context |
| pkg/nodeipam/ipam/range_allocator.go | Updated logger in Run to use context |
| pkg/nodeipam/ipam/cloud_cidr_allocator.go | Created context earlier and updated loggers to use it |
| pkg/credentialprovider/identity_bindings_credentials.go | Moved logger into DialContext closure and updated GetToken logger |
| pkg/credentialprovider/azure_credentials.go | Created context earlier and updated loggers to use it |
| cmd/cloud-node-manager/app/nodemanager.go | Updated loggers in Run and startControllers to use context |
| cmd/cloud-controller-manager/app/core.go | Updated logger in startRouteController to use context |
| cmd/cloud-controller-manager/app/controllermanager.go | Moved logger initialization into RunWrapper and runAsync closures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/ok-to-test |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anndono, nilo19 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes logger initialization to correctly derive loggers from the request context when available.
Changes:
Which issue(s) this PR fixes:
Fixes #
ref: #1575
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: