Remove snapshotVolumes flag necessity for volume policy snapshot action

[shubham-pampattiwar]

Thank you for contributing to Velero!

Please add a summary of your change

Remove snapshotVolumes flag necessity for volume policy snapshot action

Does your change fix a particular issue?

Fixes #7782

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.65%. Comparing base (0d85a64) to head (9f10b0e).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7786      +/-   ##
==========================================
- Coverage   58.66%   58.65%   -0.01%     
==========================================
  Files         344      344              
  Lines       28731    28730       -1     
==========================================
- Hits        16854    16853       -1     
  Misses      10448    10448              
  Partials     1429     1429              

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

[Lyndon-Li]

@shubham-pampattiwar
Thanks for the PR, we are making good progress on the understanding of the problem. But I am afraid there are still gaps to fix it:

1. We cannot simply check VolumePolicy != nil when we want to ignore snapshotVolume. VolumePolicy != nil means there are some conditions, however, it doesn't mean these conditions vote for snapshot action. Instead, I think we need to purely call VolumeHelperImpl.ShouldPerformSnapshot in all the places we need to check the snapshot action and inside VolumeHelperImpl.ShouldPerformSnapshot we also consider the snapshotVolume flag --- if we find a met condition voting for snapshot action, we ignore the flag; otherwise, we return snapshotVolume. This is just like what we do in GetVolumesForFSBackup for defaultVolumesToFsBackup
2. Besides backup, we have some places in restore that checks snapshotVolume, i.e.,

   velero/pkg/restore/pv_restorer.go

   Line 56 in 7563a45

   if boolptr.IsSetToFalse(r.snapshotVolumes) {

   . We need to do some test to decide if we can simply remove it or use more reliable way to check
3. For backup, we have some more places to cover:

velero/pkg/backup/item_backupper.go

Line 410 in 7563a45

} else if (actionName == csiBIAPluginName || actionName == vsphereBIAPluginName) && !boolptr.IsSetToFalse(ib.backupRequest.Backup.Spec.SnapshotVolumes) {

velero/pkg/controller/backup_controller.go

Line 508 in 7563a45

if boolptr.IsSetToFalse(backup.Spec.SnapshotVolumes) {

4. Looks like this is a gap during the design discussion, as I see the design is aligned with the current code, so we also need to change the design as well.

If you need a live discussion or need any help from us, just let us know. Thanks.

[blackpiglet]

/cc @anshulahuja98 @vmware-tanzu/velero-maintainers

[Lyndon-Li]

@shubham-pampattiwar
One more follow up for 1 in the above comment:
If no matching condition for snapshot action and snapshotVolume is set to true, we still need to check defaultVolumesToFsBackup flag to decide whether ShouldPerformSnapshot should return true. Am I right?

[shubham-pampattiwar]

closing this PR in favor of #7794
Thanks @blackpiglet
cc: @Lyndon-Li @reasonerjt