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

Modify the volume helper logic. #7794

Merged
merged 1 commit into from
May 23, 2024

Conversation

blackpiglet
Copy link
Contributor

@blackpiglet blackpiglet commented May 15, 2024

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

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.

@blackpiglet blackpiglet force-pushed the modify_volume_helper branch from b221ca8 to be31633 Compare May 15, 2024 02:14
@blackpiglet blackpiglet force-pushed the modify_volume_helper branch 2 times, most recently from 863d9b2 to a7f464a Compare May 17, 2024 08:55
@blackpiglet blackpiglet marked this pull request as ready for review May 17, 2024 08:56
@blackpiglet blackpiglet requested review from shubham-pampattiwar and Lyndon-Li and removed request for qiuming-best May 17, 2024 08:56
Copy link

codecov bot commented May 17, 2024

Codecov Report

Attention: Patch coverage is 83.53659% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 58.79%. Comparing base (49eab81) to head (a91d2cb).

Files Patch % Lines
internal/volumehelper/volume_policy_helper.go 84.78% 11 Missing and 3 partials ⚠️
pkg/backup/item_backupper.go 77.08% 9 Missing and 2 partials ⚠️
pkg/backup/backup.go 92.30% 1 Missing ⚠️
pkg/cmd/util/output/backup_describer.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7794      +/-   ##
==========================================
+ Coverage   58.67%   58.79%   +0.11%     
==========================================
  Files         345      345              
  Lines       28743    28764      +21     
==========================================
+ Hits        16866    16911      +45     
+ Misses      10448    10425      -23     
+ Partials     1429     1428       -1     

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

@blackpiglet blackpiglet marked this pull request as draft May 17, 2024 10:47
@blackpiglet blackpiglet force-pushed the modify_volume_helper branch 5 times, most recently from d1d775f to d956114 Compare May 18, 2024 02:57
@blackpiglet blackpiglet marked this pull request as ready for review May 18, 2024 03:18
@blackpiglet blackpiglet force-pushed the modify_volume_helper branch 4 times, most recently from 933d049 to 60e647c Compare May 18, 2024 15:28
@github-actions github-actions bot added the Area/Design Design Documents label May 18, 2024
@blackpiglet blackpiglet force-pushed the modify_volume_helper branch 3 times, most recently from 7063ba9 to 07d2fb4 Compare May 21, 2024 10:43
@blackpiglet blackpiglet force-pushed the modify_volume_helper branch 3 times, most recently from 278d856 to f1aafb6 Compare May 21, 2024 16:53
Copy link
Collaborator

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

Thanks a bunch @blackpiglet Added a few minor comments. In addition to them I think there is one case that we need to cover:

  • If a volume has a matching snapshot action and at the same time the user also specifies defaultVolumesToFSBackups: true then the snapshot would be skipped by CSI plugin, we need to modify the logic here I believe:
    isFSUploaderUsed, err := podvolume.IsPVCDefaultToFSBackup(

pkg/backup/item_backupper.go Outdated Show resolved Hide resolved
pkg/backup/item_backupper.go Outdated Show resolved Hide resolved
pkg/backup/item_backupper.go Outdated Show resolved Hide resolved
@blackpiglet
Copy link
Contributor Author

blackpiglet commented May 21, 2024

Thanks a bunch @blackpiglet Added a few minor comments. In addition to them I think there is one case that we need to cover:

  • If a volume has a matching snapshot action and at the same time the user also specifies defaultVolumesToFSBackups: true then the snapshot would be skipped by CSI plugin, we need to modify the logic here I believe:
    isFSUploaderUsed, err := podvolume.IsPVCDefaultToFSBackup(

Thanks. Modified.

@blackpiglet blackpiglet force-pushed the modify_volume_helper branch from f1aafb6 to cdb4588 Compare May 22, 2024 01:55
@@ -64,14 +63,6 @@ func (p *pvcBackupItemAction) AppliesTo() (velero.ResourceSelector, error) {
}

func (p *pvcBackupItemAction) validateBackup(backup velerov1api.Backup) (valid bool) {
// Do nothing if volume snapshots have not been requested in this backup
Copy link
Contributor

@Lyndon-Li Lyndon-Li May 22, 2024

Choose a reason for hiding this comment

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

This means all the BIA plugins should not check backup.Spec.SnapshotVolumes, we may need to add this to release notes so as to advice the 3rd plugins to remove this check, cc @reasonerjt

@blackpiglet blackpiglet force-pushed the modify_volume_helper branch 4 times, most recently from 2e1c28e to bbeed25 Compare May 22, 2024 11:31
Copy link
Collaborator

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

@blackpiglet PR looks good, just added one comment on removal of unused function.

pkg/util/podvolume/pod_volume.go Outdated Show resolved Hide resolved
@blackpiglet blackpiglet force-pushed the modify_volume_helper branch from bbeed25 to eed8fa1 Compare May 23, 2024 01:41
@blackpiglet blackpiglet force-pushed the modify_volume_helper branch from eed8fa1 to a91d2cb Compare May 23, 2024 01:57
Copy link
Collaborator

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

/lgtm

@blackpiglet blackpiglet merged commit 0e7fb40 into vmware-tanzu:main May 23, 2024
66 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.

3 participants