From 962f5d6859a43fceee872cd92623065a62e81c41 Mon Sep 17 00:00:00 2001 From: Mayank <33252549+mynktl@users.noreply.github.com> Date: Fri, 24 Apr 2020 22:16:20 +0530 Subject: [PATCH] Skipping validation for volumesnapshotlocation for backup if snapshotvolume set to false (#2450) * Disabling validation for volumesnapshotlocation if the backup has snapshotvolume set to false Signed-off-by: mayank * adding a changelog Signed-off-by: mayank * addressing review comment Signed-off-by: mayank --- changelogs/unreleased/2450-mynktl | 1 + pkg/backup/item_backupper.go | 3 +- pkg/controller/backup_controller.go | 7 +++++ pkg/controller/backup_controller_test.go | 40 ++++++++++++++++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/2450-mynktl diff --git a/changelogs/unreleased/2450-mynktl b/changelogs/unreleased/2450-mynktl new file mode 100644 index 000000000..310384a83 --- /dev/null +++ b/changelogs/unreleased/2450-mynktl @@ -0,0 +1 @@ +Support to skip VSL validation for the backup having SnapshotVolumes set to false or created with `--snapshot-volumes=false` diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go index 1f8342913..1640914e8 100644 --- a/pkg/backup/item_backupper.go +++ b/pkg/backup/item_backupper.go @@ -41,6 +41,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/plugin/velero" "github.com/vmware-tanzu/velero/pkg/podexec" "github.com/vmware-tanzu/velero/pkg/restic" + "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/volume" ) @@ -399,7 +400,7 @@ const ( func (ib *defaultItemBackupper) takePVSnapshot(obj runtime.Unstructured, log logrus.FieldLogger) error { log.Info("Executing takePVSnapshot") - if ib.backupRequest.Spec.SnapshotVolumes != nil && !*ib.backupRequest.Spec.SnapshotVolumes { + if boolptr.IsSetToFalse(ib.backupRequest.Spec.SnapshotVolumes) { log.Info("Backup has volume snapshots disabled; skipping volume snapshot action.") return nil } diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index b8a9e0d13..8b255abb4 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -51,6 +51,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/metrics" "github.com/vmware-tanzu/velero/pkg/persistence" "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" + "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/util/collections" "github.com/vmware-tanzu/velero/pkg/util/encode" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" @@ -397,10 +398,16 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg // - a given provider's default location name is added to .spec.volumeSnapshotLocations if one // is not explicitly specified for the provider (if there's only one location for the provider, // it will automatically be used) +// if backup has snapshotVolume disabled then it returns empty VSL func (c *backupController) validateAndGetSnapshotLocations(backup *velerov1api.Backup) (map[string]*velerov1api.VolumeSnapshotLocation, []string) { errors := []string{} providerLocations := make(map[string]*velerov1api.VolumeSnapshotLocation) + // if snapshotVolume is set to false then we don't need to validate volumesnapshotlocation + if boolptr.IsSetToFalse(backup.Spec.SnapshotVolumes) { + return nil, nil + } + for _, locationName := range backup.Spec.VolumeSnapshotLocations { // validate each locationName exists as a VolumeSnapshotLocation location, err := c.snapshotLocationLister.VolumeSnapshotLocations(backup.Namespace).Get(locationName) diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 99bd653b1..24d0af168 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -767,6 +767,46 @@ func TestValidateAndGetSnapshotLocations(t *testing.T) { expectedVolumeSnapshotLocationNames: []string{"aws-us-west-1", "some-name"}, expectedSuccess: true, }, + { + name: "location name does not correspond to any existing location and snapshotvolume disabled; should return empty VSL and no error", + backup: defaultBackup().Phase(velerov1api.BackupPhaseNew).VolumeSnapshotLocations("random-name").SnapshotVolumes(false).Result(), + locations: []*velerov1api.VolumeSnapshotLocation{ + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-east-1").Provider("aws").Result(), + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-west-1").Provider("aws").Result(), + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "some-name").Provider("fake-provider").Result(), + }, + expectedVolumeSnapshotLocationNames: nil, + expectedSuccess: true, + }, + { + name: "duplicate locationName per provider and snapshotvolume disabled; should return empty VSL and no error", + backup: defaultBackup().Phase(velerov1api.BackupPhaseNew).VolumeSnapshotLocations("aws-us-west-1", "aws-us-west-1").SnapshotVolumes(false).Result(), + locations: []*velerov1api.VolumeSnapshotLocation{ + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-east-1").Provider("aws").Result(), + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-west-1").Provider("aws").Result(), + }, + expectedVolumeSnapshotLocationNames: nil, + expectedSuccess: true, + }, + { + name: "no location name for the provider exists, only one VSL created and snapshotvolume disabled; should return empty VSL and no error", + backup: defaultBackup().Phase(velerov1api.BackupPhaseNew).SnapshotVolumes(false).Result(), + locations: []*velerov1api.VolumeSnapshotLocation{ + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-east-1").Provider("aws").Result(), + }, + expectedVolumeSnapshotLocationNames: nil, + expectedSuccess: true, + }, + { + name: "multiple location names for a provider, no default location and backup has no location defined, but snapshotvolume disabled, should return empty VSL and no error", + backup: defaultBackup().Phase(velerov1api.BackupPhaseNew).SnapshotVolumes(false).Result(), + locations: []*velerov1api.VolumeSnapshotLocation{ + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-west-1").Provider("aws").Result(), + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-east-1").Provider("aws").Result(), + }, + expectedVolumeSnapshotLocationNames: nil, + expectedSuccess: true, + }, } for _, test := range tests {