Skip to content

Commit

Permalink
Modify the volume helper logic.
Browse files Browse the repository at this point in the history 
Signed-off-by: Xun Jiang <blackpigletbruce@gmail.com>
  • Loading branch information
@blackpiglet
blackpiglet committed May 16, 2024
1 parent 7e19cdb commit 863d9b2
Show file tree
Hide file tree
Showing 11 changed files with 172 additions and 119 deletions.
1 change: 1 addition & 0 deletions changelogs/unreleased/7794-blackpiglet
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Modify the volume helper logic.
82 changes: 54 additions & 28 deletions internal/volumehelper/volume_policy_helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (

"github.com/vmware-tanzu/velero/internal/resourcepolicies"

kbclient "sigs.k8s.io/controller-runtime/pkg/client"
crclient "sigs.k8s.io/controller-runtime/pkg/client"

"github.com/vmware-tanzu/velero/pkg/util/boolptr"
kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
Expand All @@ -21,17 +21,38 @@ import (
)

type VolumeHelper interface {
GetVolumesForFSBackup(pod *corev1api.Pod, defaultVolumesToFsBackup, backupExcludePVC bool, kbclient kbclient.Client) ([]string, []string, error)
ShouldPerformSnapshot(obj runtime.Unstructured, groupResource schema.GroupResource, kbclient kbclient.Client) (bool, error)
GetVolumesForFSBackup(pod *corev1api.Pod) ([]string, []string, error)
ShouldPerformSnapshot(obj runtime.Unstructured, groupResource schema.GroupResource) (bool, error)
}

type VolumeHelperImpl struct {
VolumePolicy *resourcepolicies.Policies
SnapshotVolumes *bool
Logger logrus.FieldLogger
type volumeHelperImpl struct {
volumePolicy *resourcepolicies.Policies
snapshotVolumes *bool
logger logrus.FieldLogger
client crclient.Client
defaultVolumesToFSBackup bool
backupExcludePVC bool
}

func (v *VolumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, groupResource schema.GroupResource, kbclient kbclient.Client) (bool, error) {
func NewVolumeHelperImpl(
volumePolicy *resourcepolicies.Policies,
snapshotVolumes *bool,
logger logrus.FieldLogger,
client crclient.Client,
defaultVolumesToFSBackup bool,
backupExcludePVC bool,
) VolumeHelper {
return &volumeHelperImpl{
volumePolicy: volumePolicy,
snapshotVolumes: snapshotVolumes,
logger: logger,
client: client,
defaultVolumesToFSBackup: defaultVolumesToFSBackup,
backupExcludePVC: backupExcludePVC,
}
}

func (v *volumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, groupResource schema.GroupResource) (bool, error) {
// check if volume policy exists and also check if the object(pv/pvc) fits a volume policy criteria and see if the associated action is snapshot
// if it is not snapshot then skip the code path for snapshotting the PV/PVC
pvc := new(corev1api.PersistentVolumeClaim)
Expand All @@ -43,7 +64,7 @@ func (v *VolumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, group
return false, err
}

pv, err = kubeutil.GetPVForPVC(pvc, kbclient)
pv, err = kubeutil.GetPVForPVC(pvc, v.client)
if err != nil {
return false, err
}
Expand All @@ -55,38 +76,43 @@ func (v *VolumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, group
}
}

if v.VolumePolicy != nil {
action, err := v.VolumePolicy.GetMatchAction(pv)
if v.volumePolicy != nil {
action, err := v.volumePolicy.GetMatchAction(pv)
if err != nil {
return false, err
}

// Also account for SnapshotVolumes flag on backup CR
if action != nil && action.Type == resourcepolicies.Snapshot && boolptr.IsSetToTrue(v.SnapshotVolumes) {
v.Logger.Infof(fmt.Sprintf("performing snapshot action for pv %s as it satisfies the volume policy criteria and snapshotVolumes is set to true", pv.Name))
if action != nil && action.Type == resourcepolicies.Snapshot {
v.logger.Infof(fmt.Sprintf("performing snapshot action for pv %s as it satisfies the volume policy criteria", pv.Name))
return true, nil
}
v.Logger.Infof(fmt.Sprintf("skipping snapshot action for pv %s possibly due to not satisfying the volume policy criteria or snapshotVolumes is not true", pv.Name))
v.logger.Infof(fmt.Sprintf("skipping snapshot action for pv %s due to not satisfying the volume policy criteria", pv.Name))
return false, nil
} else if !boolptr.IsSetToFalse(v.snapshotVolumes) {
// If the backup.Spec.SnapshotVolumes is not set, or set to true, then should take the snapshot.
v.logger.Infof("performing snapshot action for pv %s as volume policy is nil, but the snapshotVolumes is set to true")
return true, nil
}

v.logger.Infof(fmt.Sprintf("skipping snapshot action for pv %s possibly due to no volume policy setting and snapshotVolumes is false", pv.Name))
return false, nil
}

func (v *VolumeHelperImpl) GetVolumesForFSBackup(pod *corev1api.Pod, defaultVolumesToFsBackup, backupExcludePVC bool, kbclient kbclient.Client) ([]string, []string, error) {
func (v *volumeHelperImpl) GetVolumesForFSBackup(pod *corev1api.Pod) ([]string, []string, error) {
// Check if there is a fs-backup/snapshot volume policy specified by the user, if yes then use the volume policy approach to
// get the list volumes for fs-backup else go via the legacy annotation based approach
var fsBackupVolumePolicyVols, nonFsBackupVolumePolicyVols, volsToProcessByLegacyApproach = make([]string, 0), make([]string, 0), make([]string, 0)
var err error

if v.VolumePolicy != nil {
if v.volumePolicy != nil {
// Get the list of volumes to back up using pod volume backup for the given pod matching fs-backup volume policy action
v.Logger.Infof("Volume Policy specified by the user, using volume policy approach to segregate pod volumes for fs-backup")
// GetVolumesMatchingFSBackupAction return 3 list of Volumes:
v.logger.Infof("Volume Policy specified by the user, using volume policy approach to segregate pod volumes for fs-backup")
// getVolumesMatchingFSBackupAction return 3 list of Volumes:
// fsBackupVolumePolicyVols: Volumes that have a matching fs-backup action from the volume policy specified by the user
// nonFsBackupVolumePolicyVols: Volumes that have an action matching from the volume policy specified by the user, but it is not fs-backup action
// volsToProcessByLegacyApproach: Volumes that did not have any matching action i.e. action was nil from the volume policy specified by the user, these volumes will be processed via the legacy annotations based approach (fallback option)
fsBackupVolumePolicyVols, nonFsBackupVolumePolicyVols, volsToProcessByLegacyApproach, err = v.GetVolumesMatchingFSBackupAction(pod, v.VolumePolicy, backupExcludePVC, kbclient)
fsBackupVolumePolicyVols, nonFsBackupVolumePolicyVols, volsToProcessByLegacyApproach, err = v.getVolumesMatchingFSBackupAction(pod, v.volumePolicy, v.backupExcludePVC, v.client)
if err != nil {
return fsBackupVolumePolicyVols, nonFsBackupVolumePolicyVols, err
}
Expand All @@ -99,9 +125,9 @@ func (v *VolumeHelperImpl) GetVolumesForFSBackup(pod *corev1api.Pod, defaultVolu
// process legacy annotation based approach, this will done when:
// 1. volume policy os specified by the user
// 2. And there are some volumes for which the volume policy approach did not get any supported matching actions
if v.VolumePolicy != nil && len(volsToProcessByLegacyApproach) > 0 {
v.Logger.Infof("volume policy specified by the user but there are volumes with no matching action, using legacy approach based on annotations as a fallback for those volumes")
includedVolumesFromLegacyFallBack, optedOutVolumesFromLegacyFallBack := pdvolumeutil.GetVolumesByPod(pod, defaultVolumesToFsBackup, backupExcludePVC, volsToProcessByLegacyApproach)
if v.volumePolicy != nil && len(volsToProcessByLegacyApproach) > 0 {
v.logger.Infof("volume policy specified by the user but there are volumes with no matching action, using legacy approach based on annotations as a fallback for those volumes")
includedVolumesFromLegacyFallBack, optedOutVolumesFromLegacyFallBack := pdvolumeutil.GetVolumesByPod(pod, v.defaultVolumesToFSBackup, v.backupExcludePVC, volsToProcessByLegacyApproach)
// merge the volumePolicy approach and legacy Fallback lists
fsBackupVolumePolicyVols = append(fsBackupVolumePolicyVols, includedVolumesFromLegacyFallBack...)
nonFsBackupVolumePolicyVols = append(nonFsBackupVolumePolicyVols, optedOutVolumesFromLegacyFallBack...)
Expand All @@ -111,19 +137,19 @@ func (v *VolumeHelperImpl) GetVolumesForFSBackup(pod *corev1api.Pod, defaultVolu
// Get the list of volumes to back up using pod volume backup from the pod's annotations.
// We will also pass the list of volume that did not have any supported volume policy action matched in legacy approach so that
// those volumes get processed via legacy annotation based approach, this is a fallback option on annotation based legacy approach
v.Logger.Infof("fs-backup or snapshot Volume Policy not specified by the user, using legacy approach based on annotations")
includedVolumes, optedOutVolumes := pdvolumeutil.GetVolumesByPod(pod, defaultVolumesToFsBackup, backupExcludePVC, volsToProcessByLegacyApproach)
v.logger.Infof("fs-backup or snapshot Volume Policy not specified by the user, using legacy approach based on annotations")
includedVolumes, optedOutVolumes := pdvolumeutil.GetVolumesByPod(pod, v.defaultVolumesToFSBackup, v.backupExcludePVC, volsToProcessByLegacyApproach)
return includedVolumes, optedOutVolumes, nil
}

// GetVolumesMatchingFSBackupAction returns a list of volume names to backup for the provided pod having fs-backup volume policy action
func (v *VolumeHelperImpl) GetVolumesMatchingFSBackupAction(pod *corev1api.Pod, volumePolicies *resourcepolicies.Policies, backupExcludePVC bool, kbclient kbclient.Client) ([]string, []string, []string, error) {
// getVolumesMatchingFSBackupAction returns a list of volume names to backup for the provided pod having fs-backup volume policy action
func (v *volumeHelperImpl) getVolumesMatchingFSBackupAction(pod *corev1api.Pod, volumePolicies *resourcepolicies.Policies, backupExcludePVC bool, kbclient crclient.Client) ([]string, []string, []string, error) {
FSBackupActionMatchingVols := []string{}
FSBackupNonActionMatchingVols := []string{}
NoActionMatchingVols := []string{}

for i, vol := range pod.Spec.Volumes {
if !v.ShouldIncludeVolumeInBackup(vol, backupExcludePVC) {
if !v.shouldIncludeVolumeInBackup(vol, backupExcludePVC) {
continue
}

Expand Down Expand Up @@ -163,7 +189,7 @@ func (v *VolumeHelperImpl) GetVolumesMatchingFSBackupAction(pod *corev1api.Pod,
return FSBackupActionMatchingVols, FSBackupNonActionMatchingVols, NoActionMatchingVols, nil
}

func (v *VolumeHelperImpl) ShouldIncludeVolumeInBackup(vol corev1api.Volume, backupExcludePVC bool) bool {
func (v *volumeHelperImpl) shouldIncludeVolumeInBackup(vol corev1api.Volume, backupExcludePVC bool) bool {
includeVolumeInBackup := true
// cannot backup hostpath volumes as they are not mounted into /var/lib/kubelet/pods
// and therefore not accessible to the node agent daemon set.
Expand Down
73 changes: 49 additions & 24 deletions internal/volumehelper/volume_policy_helper_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
/*
Copyright the Velero contributors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package volumehelper

import (
Expand Down Expand Up @@ -93,7 +109,7 @@ func TestVolumeHelperImpl_ShouldPerformSnapshot(t *testing.T) {
expectedErr: false,
},
{
name: "Given PV object matches volume policy snapshot action snapshotVolumes flags is false returns false and no error",
name: "Given PV object matches volume policy snapshot action snapshotVolumes flags is false returns true and no error",
obj: PVObjectGP2,
groupResource: kuberesource.PersistentVolumes,
resourcePolicies: resourcepolicies.ResourcePolicies{
Expand All @@ -110,7 +126,7 @@ func TestVolumeHelperImpl_ShouldPerformSnapshot(t *testing.T) {
},
},
snapshotVolumesFlag: pointer.Bool(false),
shouldSnapshot: false,
shouldSnapshot: true,
expectedErr: false,
},
{
Expand Down Expand Up @@ -203,12 +219,13 @@ func TestVolumeHelperImpl_ShouldPerformSnapshot(t *testing.T) {
if err != nil {
t.Fatalf("failed to build policy with error %v", err)
}
vh := &VolumeHelperImpl{
VolumePolicy: p,
SnapshotVolumes: tc.snapshotVolumesFlag,
Logger: velerotest.NewLogger(),
vh := &volumeHelperImpl{
volumePolicy: p,
snapshotVolumes: tc.snapshotVolumesFlag,
logger: velerotest.NewLogger(),
client: fakeClient,
}
ActualShouldSnapshot, actualError := vh.ShouldPerformSnapshot(tc.obj, tc.groupResource, fakeClient)
ActualShouldSnapshot, actualError := vh.ShouldPerformSnapshot(tc.obj, tc.groupResource)
if tc.expectedErr {
assert.NotNil(t, actualError, "Want error; Got nil error")
return
Expand Down Expand Up @@ -354,12 +371,13 @@ func TestVolumeHelperImpl_ShouldIncludeVolumeInBackup(t *testing.T) {
if err != nil {
t.Fatalf("failed to build policy with error %v", err)
}
vh := &VolumeHelperImpl{
VolumePolicy: p,
SnapshotVolumes: pointer.Bool(true),
Logger: velerotest.NewLogger(),
vh := &volumeHelperImpl{
volumePolicy: p,
snapshotVolumes: pointer.Bool(true),
logger: velerotest.NewLogger(),
backupExcludePVC: tc.backupExcludePVC,
}
actualShouldInclude := vh.ShouldIncludeVolumeInBackup(tc.vol, tc.backupExcludePVC)
actualShouldInclude := vh.shouldIncludeVolumeInBackup(tc.vol, vh.backupExcludePVC)
assert.Equalf(t, actualShouldInclude, tc.shouldInclude, "Want shouldInclude as %v; Got actualShouldInclude as %v", tc.shouldInclude, actualShouldInclude)
})
}
Expand Down Expand Up @@ -610,14 +628,18 @@ func TestVolumeHelperImpl_GetVolumesMatchingFSBackupAction(t *testing.T) {
if err != nil {
t.Fatalf("failed to build policy with error %v", err)
}
vh := &VolumeHelperImpl{
VolumePolicy: p,
SnapshotVolumes: pointer.Bool(true),
Logger: velerotest.NewLogger(),
}

objs := []runtime.Object{samplePod, samplePVC1, samplePVC2, samplePV1, samplePV2}
fakeClient := velerotest.NewFakeControllerRuntimeClient(t, objs...)
gotFSBackupActionMatchingVols, gotFSBackupNonActionMatchingVols, gotNoActionMatchingVols, actualError := vh.GetVolumesMatchingFSBackupAction(samplePod, vh.VolumePolicy, tc.backupExcludePVC, fakeClient)

vh := &volumeHelperImpl{
volumePolicy: p,
snapshotVolumes: pointer.Bool(true),
logger: velerotest.NewLogger(),
client: fakeClient,
backupExcludePVC: tc.backupExcludePVC,
}
gotFSBackupActionMatchingVols, gotFSBackupNonActionMatchingVols, gotNoActionMatchingVols, actualError := vh.getVolumesMatchingFSBackupAction(samplePod, vh.volumePolicy, vh.backupExcludePVC, vh.client)
if tc.expectedErr {
assert.NotNil(t, actualError, "Want error; Got nil error")
return
Expand Down Expand Up @@ -681,14 +703,17 @@ func TestVolumeHelperImpl_GetVolumesForFSBackup(t *testing.T) {
if err != nil {
t.Fatalf("failed to build policy with error %v", err)
}
vh := &VolumeHelperImpl{
VolumePolicy: p,
SnapshotVolumes: pointer.Bool(true),
Logger: velerotest.NewLogger(),
}
objs := []runtime.Object{samplePod, samplePVC1, samplePVC2, samplePV1, samplePV2}
fakeClient := velerotest.NewFakeControllerRuntimeClient(t, objs...)
gotIncludedVolumes, gotOptedOutVolumes, actualError := vh.GetVolumesForFSBackup(samplePod, tc.defaultVolumesToFsBackup, tc.backupExcludePVC, fakeClient)
vh := &volumeHelperImpl{
volumePolicy: p,
snapshotVolumes: pointer.Bool(true),
logger: velerotest.NewLogger(),
client: fakeClient,
defaultVolumesToFSBackup: tc.defaultVolumesToFsBackup,
backupExcludePVC: tc.backupExcludePVC,
}
gotIncludedVolumes, gotOptedOutVolumes, actualError := vh.GetVolumesForFSBackup(samplePod)
if tc.expectedErr {
assert.NotNil(t, actualError, "Want error; Got nil error")
return
Expand Down
8 changes: 0 additions & 8 deletions pkg/backup/actions/csi/pvc_action.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,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
if boolptr.IsSetToFalse(backup.Spec.SnapshotVolumes) {
p.log.Infof(
"Volume snapshotting not requested for backup %s/%s",
backup.Namespace, backup.Name)
return false
}

if backup.Status.Phase == velerov1api.BackupPhaseFinalizing ||
backup.Status.Phase == velerov1api.BackupPhaseFinalizingPartiallyFailed {
p.log.WithFields(
Expand Down
5 changes: 0 additions & 5 deletions pkg/backup/actions/csi/pvc_action_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,6 @@ func TestExecute(t *testing.T) {
expectedDataUpload *velerov2alpha1.DataUpload
expectedPVC *corev1.PersistentVolumeClaim
}{
{
name: "Skip PVC handling if SnapshotVolume set to false",
backup: builder.ForBackup("velero", "test").SnapshotVolumes(false).Result(),
expectedErr: nil,
},
{
name: "Skip PVC BIA when backup is in finalizing phase",
backup: builder.ForBackup("velero", "test").Phase(velerov1api.BackupPhaseFinalizing).Result(),
Expand Down
15 changes: 15 additions & 0 deletions pkg/backup/backup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ import (
kbclient "sigs.k8s.io/controller-runtime/pkg/client"

"github.com/vmware-tanzu/velero/internal/hook"
"github.com/vmware-tanzu/velero/internal/resourcepolicies"
"github.com/vmware-tanzu/velero/internal/volume"
"github.com/vmware-tanzu/velero/internal/volumehelper"
velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
velerov2alpha1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1"
"github.com/vmware-tanzu/velero/pkg/client"
Expand Down Expand Up @@ -321,6 +323,11 @@ func (kb *kubernetesBackupper) BackupWithResolvers(
}
backupRequest.Status.Progress = &velerov1api.BackupProgress{TotalItems: len(items)}

var resourcePolicy *resourcepolicies.Policies
if backupRequest.ResPolicies != nil {
resourcePolicy = backupRequest.ResPolicies
}

itemBackupper := &itemBackupper{
backupRequest: backupRequest,
tarWriter: tw,
Expand All @@ -334,6 +341,14 @@ func (kb *kubernetesBackupper) BackupWithResolvers(
PodCommandExecutor: kb.podCommandExecutor,
},
hookTracker: hook.NewHookTracker(),
volumeHelperImpl: volumehelper.NewVolumeHelperImpl(
resourcePolicy,
backupRequest.Spec.SnapshotVolumes,
log,
kb.kbClient,
boolptr.IsSetToTrue(backupRequest.Spec.DefaultVolumesToFsBackup),
!backupRequest.ResourceIncludesExcludes.ShouldInclude(kuberesource.PersistentVolumeClaims.String()),
),
}

// helper struct to send current progress between the main
Expand Down
Loading

0 comments on commit 863d9b2

Please sign in to comment.