Skip to content

Conversation

@anndono
Copy link
Contributor

@anndono anndono commented Dec 22, 2025

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR standardizes error logging by migrating from a mix of plaintext and structured logging to contextual logging, improving consistency, clarity, and observability across the codebase.

Changes:

  • Replaced klog.Error, klog.Errorf, klog.ErrorS with logger.Error
  • Replaced klog.V(n).ErrorS with logger.V(n).Error
  • Replaced klog.Fatalf with logger.Error followed by klog.FlushAndExit(klog.ExitFlushTimeout, 1)
  • In sub modules, replaced klog.Fatalf with klog.ErrorS followed by klog.FlushAndExit(klog.ExitFlushTimeout, 1)
  • Modified some error messages to remove redundancy and improve clarity
  • Added logger and renamed some of the unused parameters from _ to ctx because now that the context is actively used for logging
  • Updated a small number of log statements that used log.Background to use existing logger for consistency

Which issue(s) this PR fixes:

Fixes #
ref: #1575

Special notes for your reviewer:

Does this PR introduce a user-facing change?

ACTION REQUIRED: The exit behavior has changed. Fatal errors that previously used `klog.Fatalf` now exit with status code 1 instead of 255.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 22, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anndono
Once this PR has been reviewed and has the lgtm label, please assign joelspeed for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

@github-actions github-actions bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 22, 2025
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 22, 2025
@nilo19
Copy link
Contributor

nilo19 commented Dec 22, 2025

please resolve linting errors.

@nilo19
Copy link
Contributor

nilo19 commented Dec 23, 2025

Please don't fix #1575 in this pr because we have other cherry-picks related to 1575. We can close the issue after all completions.

zones, innerErr = az.zoneRepo.ListZones(ctx)
if innerErr != nil {
klog.ErrorS(err, "Failed to list zones")
logger.Error(err, "Failed to list zones")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related to your change, but we should change it to innerErr here instead of err. please check why the unit test doesn't cover this bug if you have time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit test doesn't cover this because it only verifies the returned error value instead of log messages.

@nilo19 nilo19 requested a review from Copilot December 23, 2025 04:51
@nilo19
Copy link
Contributor

nilo19 commented Dec 23, 2025

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 23, 2025
Copy link

Copilot AI left a 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 migrates error logging from klog (plaintext and structured) to contextual logging using logger.Error for improved consistency and observability across the codebase.

Key Changes:

  • Replaced klog.Error, klog.Errorf, klog.ErrorS with logger.Error
  • Replaced klog.Fatalf with logger.Error followed by klog.FlushAndExit(klog.ExitFlushTimeout, 1) for consistent exit behavior
  • Renamed unused _ context parameters to ctx where the context is now used for logging
  • Fixed spelling errors ("paring" → "parsing" in most places)

Reviewed changes

Copilot reviewed 51 out of 52 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
tests/e2e/e2e_test.go Migrated Fatalf to logger.Error + FlushAndExit; added error message to kusto ingest failure
pkg/util/controller/testutil/test_utils.go Migrated all klog.Errorf calls to logger.Error with structured logging; added context parameter
pkg/util/controller/node/controller_utils.go Migrated klog.Errorf to logger.Error in node delete handler
pkg/provider/subnet/subnet.go Migrated subnet operation errors to logger.Error
pkg/provider/storage/azure_storageaccount.go Migrated storage account errors to logger.Error
pkg/provider/securitygroup/azure_securitygroup_repo.go Migrated security group cache creation errors to logger.Error
pkg/provider/azure_zones.go Fixed error variable in zones list logging
pkg/provider/azure_vmssflex_cache.go Migrated VMSS flex cache errors to logger.Error
pkg/provider/azure_vmssflex.go Comprehensive migration of VMSS flex operations to logger.Error
pkg/provider/azure_vmss_repo.go Migrated VMSS repository errors to logger.Error
pkg/provider/azure_vmss_cache.go Migrated VMSS cache errors to logger.Error
pkg/provider/azure_vmss.go Extensive migration of VMSS operations to logger.Error
pkg/provider/azure_vmsets_repo.go Migrated VM sets repository errors to logger.Error
pkg/provider/azure_standard.go Migrated availability set errors to logger.Error
pkg/provider/azure_routes.go Migrated routing errors to logger.Error; changed warning to info
pkg/provider/azure_publicip_repo.go Migrated public IP errors to logger.Error
pkg/provider/azure_privatelinkservice.go Migrated private link service errors to logger.Error
pkg/provider/azure_local_services.go Migrated endpoint slice conversion errors to logger.Error
pkg/provider/azure_loadbalancer_*.go Comprehensive migration of load balancer operations to logger.Error
pkg/provider/azure_interface_repo.go Migrated interface errors to logger.Error
pkg/provider/azure_instances_v2.go Migrated instance operations to logger.Error
pkg/provider/azure_instance_metadata.go Migrated IMDS errors to logger.Error
pkg/provider/azure_controller_*.go Migrated controller errors to logger.Error
pkg/provider/azure.go Migrated cloud provider initialization errors to logger.Error + FlushAndExit
pkg/nodemanager/nodemanager.go Migrated node manager errors to logger.Error
pkg/nodeipam/*.go Migrated IPAM errors to logger.Error; replaced Fatalf with FlushAndExit pattern
pkg/node/*.go Migrated node provider initialization to logger.Error + FlushAndExit
pkg/credentialprovider/azure_credentials.go Migrated ACR credential provider errors to logger.Error
cmd/**/*.go Migrated command initialization and controller startup errors to logger.Error + FlushAndExit
health-probe-proxy/main.go Migrated proxy errors to logger.Error
kubetest2-aks/**/*.go Migrated test deployment errors to klog.ErrorS + FlushAndExit; updated klog import

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@coveralls
Copy link

Coverage Status

coverage: 74.881% (-0.06%) from 74.942%
when pulling 873d155 on anndono:chengan/migrate-to-contextual-logging-error
into 432deb3 on kubernetes-sigs:master.

@anndono anndono force-pushed the chengan/migrate-to-contextual-logging-error branch from 1d78445 to db156ca Compare December 28, 2025 23:10
@k8s-ci-robot
Copy link
Contributor

@anndono: The following tests 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-cloud-provider-azure-e2e-ccm-vmss-shared-probe-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-vmss-shared-probe-capz
pull-cloud-provider-azure-e2e-ccm-dualstack-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-dualstack-capz
pull-cloud-provider-azure-e2e-ccm-ipv6-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-ipv6-capz
pull-cloud-provider-azure-e2e-ccm-dualstack-vmss-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-dualstack-vmss-capz
pull-cloud-provider-azure-e2e-ccm-vmss-multi-slb-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-vmss-multi-slb-capz
pull-cloud-provider-azure-e2e-ccm-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-capz
pull-cloud-provider-azure-e2e-ccm-vmssflex-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-vmssflex-capz
pull-cloud-provider-azure-e2e-capz db156ca link true /test pull-cloud-provider-azure-e2e-capz
pull-cloud-provider-azure-e2e-ccm-vmss-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-vmss-capz
pull-cloud-provider-azure-e2e-ccm-vmss-ip-lb-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-vmss-ip-lb-capz
pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz db156ca link true /test pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz

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.

Details

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-sigs/prow repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 5, 2026
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Details

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants