Volume Policy - miss match behavior with Velero for SnapshotVolume

vmware-tanzu / velero

Backup and migrate Kubernetes applications and their persistent volumes

https://velero.io

Apache License 2.0

8.63k stars 1.39k forks source link

Volume Policy - miss match behavior with Velero for SnapshotVolume #7782

Closed Lyndon-Li closed 4 months ago

Lyndon-Li commented 4 months ago

For --snapshot-volume flag, if it is not set, Velero's existing behavior treat SnapshotVolume as true, that is, volumes are backed up by CSI snapshot (data movement) or native snapshot; However, for volume policy, if the flag is not set, it disable the snapshot action even if the condition is met.

Steps:

Create a volume policy configuration with a snapshot action and a condition meeting a volume being backed up
Run a Velero backup using the volume policy but don't set --snapshot-volume flag explictily
[Result] The volume is still skipped by volume policy even though the condition is met

Lyndon-Li commented 4 months ago

@shubham-pampattiwar The issue is about a mismatching behavior to check --snapshot-volume flag by volume policy.

However, I discussed further with @reasonerjt for this issue, we think volume policy may not need to respect --snapshot-volume flag, since volume policy always take the preference, if the condition is met which means volume policy voting the snapshot action, the backup should go with snapshot action not matter how snapshot-volume is set. So we may need to change the existing code of Velero where snapshot-volume is being checked.

shubham-pampattiwar commented 4 months ago

@Lyndon-Li I think not respecting snapshotVolmes as true may not be the best idea here. The CSI plugin depends on this value to perform snapshots: https://github.com/vmware-tanzu/velero/blob/0d85a647b5a4d3b812d3669056eff3a0f539e282/pkg/backup/actions/csi/pvc_action.go#L68

Lyndon-Li commented 4 months ago

@Lyndon-Li I think not respecting snapshotVolmes as true may not be the best idea here. The CSI plugin depends on this value to perform snapshots:

https://github.com/vmware-tanzu/velero/blob/0d85a647b5a4d3b812d3669056eff3a0f539e282/pkg/backup/actions/csi/pvc_action.go#L68

@shubham-pampattiwar Actually the idea we discussed yesterday is that once the volume policy chooses snapshot for a volume, any other places (including CSI plugin) should not check for the SnapshotVolumes again. This introduces more changes than the code for volume policy but it makes things more clear.

shubham-pampattiwar commented 4 months ago

Created a PR for this issue: https://github.com/vmware-tanzu/velero/pull/7786

Lyndon-Li commented 4 months ago

Fixed by #7794