Skip to content

Conversation

@priyansh17
Copy link
Collaborator

@priyansh17 priyansh17 commented Aug 8, 2025

Signed-off-by: Priyansh Choudhary [email protected]

Please add a summary of your change

  • Do not add the logger to any struct that is marshaled.
  • Always pass the logger via context.Context using WithLogger and LoggerFromContext.
  • Use this pattern for all backends for consistent behavior.

Fix #9176

Please indicate you've done the following:

@Lyndon-Li Lyndon-Li added this to the v1.17 milestone Aug 11, 2025
@Lyndon-Li
Copy link
Contributor

@priyansh17
Thanks for the fix! Please help to add a changelog for this PR.

Signed-off-by: Priyansh Choudhary <[email protected]>
@codecov
Copy link

codecov bot commented Aug 11, 2025

Codecov Report

❌ Patch coverage is 25.00000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.66%. Comparing base (d295314) to head (8d4203e).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...po/kopialib/backend/azure/azure_storage_wrapper.go 0.00% 7 Missing ⚠️
...sitory/udmrepo/kopialib/backend/logging/context.go 0.00% 7 Missing ⚠️
pkg/repository/udmrepo/kopialib/backend/azure.go 0.00% 1 Missing ⚠️
...repository/udmrepo/kopialib/backend/file_system.go 66.66% 1 Missing ⚠️
pkg/repository/udmrepo/kopialib/backend/gcs.go 66.66% 1 Missing ⚠️
pkg/repository/udmrepo/kopialib/backend/s3.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9168      +/-   ##
==========================================
- Coverage   59.68%   59.66%   -0.02%     
==========================================
  Files         381      382       +1     
  Lines       43879    43900      +21     
==========================================
+ Hits        26189    26194       +5     
- Misses      16145    16161      +16     
  Partials     1545     1545              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

This PR introduces duplicate functionality - Velero already uses Kopia's logging.WithLogger/LoggerFromContext in pkg/kopia/kopia_log.go for context-based logging in Kopia integrations. Since all modified backends are under kopialib/ and return Kopia storage implementations, consider reusing Kopia's existing logging utilities rather than creating a parallel implementation in pkg/repository/udmrepo/logging/.

@Lyndon-Li
Copy link
Contributor

Lyndon-Li commented Aug 12, 2025

@kaovilai
This PR is to fix the problem introduced by #8875, which causes Velero doesn't work with Azure Blob storage since 1.16. So if we want to remove this extra log mechanism, we should revert the changes in #8875, which is to fix #8827.
I think the logs mentioned in the problem belongs to Velero code and there is no Kopia log available around that code (Kopia code doesn't define logger parameter for the related functions/interfaces), and moreover, Kopia log doesn't respect to --json either (notice that Kopia logs are also in Velero log files).

@Lyndon-Li
Copy link
Contributor

FYI, I have opened a dedicated issue #9176, let's discuss in the issue

@anshulahuja98
Copy link
Collaborator

Tag: @kaovilai for any review.

@Lyndon-Li Lyndon-Li merged commit 1a52994 into vmware-tanzu:main Aug 15, 2025
44 of 46 checks passed
@Lyndon-Li
Copy link
Contributor

@priyansh17 Could you help to cherry-pick this PR to 1.16 branch?

@priyansh17
Copy link
Collaborator Author

@priyansh17 Could you help to cherry-pick this PR to 1.16 branch?

Sure, will raise another PR.

@priyansh17
Copy link
Collaborator Author

priyansh17 commented Aug 15, 2025

@kaovilai
Copy link
Collaborator

@priyansh17 Do you mean to raise a PR to release-1.16 branch so that #9168 make it into release-1.16?

@priyansh17
Copy link
Collaborator Author

priyansh17 commented Aug 15, 2025

@kaovilai I meant @Lyndon-Li mentioned to add this PR into 1.16 but this is a fix for #8875, if the issue isn't in the release there's no need for the solution to go in.

@Lyndon-Li
Copy link
Contributor

@kaovilai I meant @Lyndon-Li mentioned to add this PR into 1.16 but this is a fix for #8875, if the issue isn't in the release there's no need for the solution to go in.

Thanks for the confirmation! My mistake, I previous thought the problematic PR is also in 1.16.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Velero to Azure Blob is broken since 1.16

5 participants