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

Add check for owner references in backup sync, removing if missing #7032

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

deefdragon
Copy link

Please add a summary of your change

Loops through the list of Ownership References in a backup when synchronizing. For each ownership reference, it checks for the referenced schedule, and if it exists, keeps it, but if it does not exist, removes it, and logs as such.

This is only a draft as I am unsure if this is the best approach/inline with the desired usage of OwnershipReferences in velero. If a seperate approach is desired, I can update as requested.

Does your change fix a particular issue?

Fixes #7031

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

Will update docs as requested where relevant, but I don't think they are relevant anywhere in this particular case?

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (23921e5) 61.00% compared to head (292aa34) 61.16%.
Report is 73 commits behind head on main.

Files Patch % Lines
pkg/controller/backup_sync_controller.go 82.75% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7032      +/-   ##
==========================================
+ Coverage   61.00%   61.16%   +0.15%     
==========================================
  Files         252      258       +6     
  Lines       26898    27327     +429     
==========================================
+ Hits        16409    16714     +305     
- Misses       9326     9430     +104     
- Partials     1163     1183      +20     

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

Copy link
Author

@deefdragon deefdragon left a comment

Choose a reason for hiding this comment

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

request for feedback on a specific section.

@deefdragon
Copy link
Author

Updated the code to check a bit better, and account for ownership references.

I've tried writing some tests, but I'm having difficulty getting the tests to run, and I'm really not understanding how the existing testing works. I cant figure out how Test Backup Sync Reconciler basic function is validating that the end result is as expected.

@ywk253100
Copy link
Contributor

Updated the code to check a bit better, and account for ownership references.

I've tried writing some tests, but I'm having difficulty getting the tests to run, and I'm really not understanding how the existing testing works. I cant figure out how Test Backup Sync Reconciler basic function is validating that the end result is as expected.

How about splitting the newly added logic into a separate function so that you can add unit tests for the function easily?

@ywk253100
Copy link
Contributor

@deefdragon Could you sign off the commit to pass the DCO checking?

@deefdragon
Copy link
Author

sorry. work has kept me super busy the last week. I will get the tests in and signed off hopefully Saturday.

@ywk253100
Copy link
Contributor

@deefdragon Do you have time to handle the PR now? As we are close to the FC date, if you don't have enough time, I'll take it over

@deefdragon
Copy link
Author

I'll try to get to it tonight and clean it up/test it/sign it, but if I don't finish it tonight, feel free to take over.

(I originally ran into this because I'm migrating my cluster, which is taking most of my attention. sorry for the delays.)

@deefdragon
Copy link
Author

Should be signed, split out, and tested. I arguably went overboard on the tests, but I'd rather overdo it than miss something.

@deefdragon deefdragon marked this pull request as ready for review November 16, 2023 10:01
@blackpiglet blackpiglet merged commit c283edf into vmware-tanzu:main Nov 17, 2023
27 checks passed
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.

Backups not syncing to new cluster for migration if owner reference does not exist
3 participants