From 9743a7ce5655f6b65b27a22e79b9104ba8109c87 Mon Sep 17 00:00:00 2001 From: Xun Jiang/Bruce Jiang <59276555+blackpiglet@users.noreply.github.com> Date: Thu, 8 Jun 2023 17:05:17 +0800 Subject: [PATCH] Fix #6118: Do not persist VolumeSnapshot and VolumeSnapshotContent for snapshot DataMover case. (#6366) 1. Because VolumeSnapshot and VolumeSnapshotContent CRs are not kept after backup completed, don't persist them in the backup metadata. 2. Add some builder methods needed by CSI plugin. Signed-off-by: Xun Jiang --- changelogs/unreleased/6366-blackpiglet | 1 + .../persistent_volume_claim_builder.go | 6 + pkg/builder/storage_class_builder.go | 6 + pkg/builder/volume_snapshot_class_builder.go | 62 +++++++ pkg/controller/backup_controller.go | 4 +- pkg/controller/backup_controller_test.go | 154 +++++++++++++++++- pkg/podvolume/restorer_test.go | 10 +- 7 files changed, 237 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/6366-blackpiglet create mode 100644 pkg/builder/volume_snapshot_class_builder.go diff --git a/changelogs/unreleased/6366-blackpiglet b/changelogs/unreleased/6366-blackpiglet new file mode 100644 index 000000000..990dd7736 --- /dev/null +++ b/changelogs/unreleased/6366-blackpiglet @@ -0,0 +1 @@ +Do not persist VolumeSnapshot and VolumeSnapshotContent for snapshot DataMover case. \ No newline at end of file diff --git a/pkg/builder/persistent_volume_claim_builder.go b/pkg/builder/persistent_volume_claim_builder.go index a96cdcfa0..376d71444 100644 --- a/pkg/builder/persistent_volume_claim_builder.go +++ b/pkg/builder/persistent_volume_claim_builder.go @@ -67,3 +67,9 @@ func (b *PersistentVolumeClaimBuilder) StorageClass(name string) *PersistentVolu b.object.Spec.StorageClassName = &name return b } + +// Phase sets the PersistentVolumeClaim's status Phase. +func (b *PersistentVolumeClaimBuilder) Phase(phase corev1api.PersistentVolumeClaimPhase) *PersistentVolumeClaimBuilder { + b.object.Status.Phase = phase + return b +} diff --git a/pkg/builder/storage_class_builder.go b/pkg/builder/storage_class_builder.go index 8ffe4afd1..9e83ee957 100644 --- a/pkg/builder/storage_class_builder.go +++ b/pkg/builder/storage_class_builder.go @@ -81,3 +81,9 @@ func ForStorageClassSlice(names ...string) *StorageClassBuilder { func (b *StorageClassBuilder) SliceResult() []*storagev1api.StorageClass { return b.objectSlice } + +// Provisioner sets StorageClass's provisioner. +func (b *StorageClassBuilder) Provisioner(provisioner string) *StorageClassBuilder { + b.object.Provisioner = provisioner + return b +} diff --git a/pkg/builder/volume_snapshot_class_builder.go b/pkg/builder/volume_snapshot_class_builder.go new file mode 100644 index 000000000..9a5911e0f --- /dev/null +++ b/pkg/builder/volume_snapshot_class_builder.go @@ -0,0 +1,62 @@ +/* +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 builder + +import ( + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// VolumeSnapshotClassBuilder builds VolumeSnapshotClass objects. +type VolumeSnapshotClassBuilder struct { + object *snapshotv1api.VolumeSnapshotClass +} + +// ForVolumeSnapshotClass is the constructor of VolumeSnapshotClassBuilder. +func (b *VolumeSnapshotClassBuilder) ForVolumeSnapshotClass(name string) *VolumeSnapshotClassBuilder { + return &VolumeSnapshotClassBuilder{ + object: &snapshotv1api.VolumeSnapshotClass{ + TypeMeta: metav1.TypeMeta{ + Kind: "VolumeSnapshotClass", + APIVersion: snapshotv1api.SchemeGroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + }, + } +} + +// Result returns the built VolumeSnapshotClass. +func (b *VolumeSnapshotClassBuilder) Result() *snapshotv1api.VolumeSnapshotClass { + return b.object +} + +// Driver sets the driver of built VolumeSnapshotClass. +func (b *VolumeSnapshotClassBuilder) Driver(driver string) *VolumeSnapshotClassBuilder { + b.object.Driver = driver + return b +} + +// ObjectMeta applies functional options to the VolumeSnapshotClass's ObjectMeta. +func (b *VolumeSnapshotClassBuilder) ObjectMeta(opts ...ObjectMetaOpt) *VolumeSnapshotClassBuilder { + for _, opt := range opts { + opt(b.object) + } + + return b +} diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 41bac4d85..e04cd3902 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -648,7 +648,9 @@ func (b *backupReconciler) runBackup(backup *pkgbackup.Request) error { var volumeSnapshots []snapshotv1api.VolumeSnapshot var volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent var volumeSnapshotClasses []snapshotv1api.VolumeSnapshotClass - if features.IsEnabled(velerov1api.CSIFeatureFlag) { + if boolptr.IsSetToTrue(backup.Spec.SnapshotMoveData) { + backupLog.Info("backup SnapshotMoveData is set to true, skip VolumeSnapshot resource persistence.") + } else if features.IsEnabled(velerov1api.CSIFeatureFlag) { selector := label.NewSelectorForBackup(backup.Name) vscList := &snapshotv1api.VolumeSnapshotContentList{} diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 37278ebcf..ee0083271 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -26,6 +26,9 @@ import ( "testing" "time" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + snapshotfake "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake" + snapshotinformers "github.com/kubernetes-csi/external-snapshotter/client/v4/informers/externalversions" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -44,6 +47,7 @@ import ( pkgbackup "github.com/vmware-tanzu/velero/pkg/backup" "github.com/vmware-tanzu/velero/pkg/builder" "github.com/vmware-tanzu/velero/pkg/discovery" + "github.com/vmware-tanzu/velero/pkg/features" "github.com/vmware-tanzu/velero/pkg/itemoperation" "github.com/vmware-tanzu/velero/pkg/metrics" "github.com/vmware-tanzu/velero/pkg/persistence" @@ -581,6 +585,7 @@ func TestProcessBackupCompletions(t *testing.T) { expectedResult *velerov1api.Backup backupExists bool existenceCheckError error + volumeSnapshot *snapshotv1api.VolumeSnapshot }{ // Finalizing { @@ -1000,16 +1005,139 @@ func TestProcessBackupCompletions(t *testing.T) { }, }, }, + { + name: "backup with snapshot data movement when CSI feature is enabled", + backup: defaultBackup().SnapshotMoveData(true).Result(), + backupLocation: defaultBackupLocation, + defaultVolumesToFsBackup: false, + expectedResult: &velerov1api.Backup{ + TypeMeta: metav1.TypeMeta{ + Kind: "Backup", + APIVersion: "velero.io/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: velerov1api.DefaultNamespace, + Name: "backup-1", + Annotations: map[string]string{ + "velero.io/source-cluster-k8s-major-version": "1", + "velero.io/source-cluster-k8s-minor-version": "16", + "velero.io/source-cluster-k8s-gitversion": "v1.16.4", + "velero.io/resource-timeout": "0s", + }, + Labels: map[string]string{ + "velero.io/storage-location": "loc-1", + }, + }, + Spec: velerov1api.BackupSpec{ + StorageLocation: defaultBackupLocation.Name, + DefaultVolumesToFsBackup: boolptr.False(), + SnapshotMoveData: boolptr.True(), + }, + Status: velerov1api.BackupStatus{ + Phase: velerov1api.BackupPhaseFinalizing, + Version: 1, + FormatVersion: "1.1.0", + StartTimestamp: ×tamp, + Expiration: ×tamp, + CSIVolumeSnapshotsAttempted: 0, + CSIVolumeSnapshotsCompleted: 0, + }, + }, + volumeSnapshot: builder.ForVolumeSnapshot("velero", "testVS").ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1")).Result(), + }, + { + name: "backup with snapshot data movement set to false when CSI feature is enabled", + backup: defaultBackup().SnapshotMoveData(false).Result(), + //backup: defaultBackup().Result(), + backupLocation: defaultBackupLocation, + defaultVolumesToFsBackup: false, + expectedResult: &velerov1api.Backup{ + TypeMeta: metav1.TypeMeta{ + Kind: "Backup", + APIVersion: "velero.io/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: velerov1api.DefaultNamespace, + Name: "backup-1", + Annotations: map[string]string{ + "velero.io/source-cluster-k8s-major-version": "1", + "velero.io/source-cluster-k8s-minor-version": "16", + "velero.io/source-cluster-k8s-gitversion": "v1.16.4", + "velero.io/resource-timeout": "0s", + }, + Labels: map[string]string{ + "velero.io/storage-location": "loc-1", + }, + }, + Spec: velerov1api.BackupSpec{ + StorageLocation: defaultBackupLocation.Name, + DefaultVolumesToFsBackup: boolptr.False(), + SnapshotMoveData: boolptr.False(), + }, + Status: velerov1api.BackupStatus{ + Phase: velerov1api.BackupPhaseFinalizing, + Version: 1, + FormatVersion: "1.1.0", + StartTimestamp: ×tamp, + Expiration: ×tamp, + CSIVolumeSnapshotsAttempted: 1, + CSIVolumeSnapshotsCompleted: 0, + }, + }, + volumeSnapshot: builder.ForVolumeSnapshot("velero", "testVS").ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1")).Result(), + }, + { + name: "backup with snapshot data movement not set when CSI feature is enabled", + backup: defaultBackup().Result(), + backupLocation: defaultBackupLocation, + defaultVolumesToFsBackup: false, + expectedResult: &velerov1api.Backup{ + TypeMeta: metav1.TypeMeta{ + Kind: "Backup", + APIVersion: "velero.io/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: velerov1api.DefaultNamespace, + Name: "backup-1", + Annotations: map[string]string{ + "velero.io/source-cluster-k8s-major-version": "1", + "velero.io/source-cluster-k8s-minor-version": "16", + "velero.io/source-cluster-k8s-gitversion": "v1.16.4", + "velero.io/resource-timeout": "0s", + }, + Labels: map[string]string{ + "velero.io/storage-location": "loc-1", + }, + }, + Spec: velerov1api.BackupSpec{ + StorageLocation: defaultBackupLocation.Name, + DefaultVolumesToFsBackup: boolptr.False(), + }, + Status: velerov1api.BackupStatus{ + Phase: velerov1api.BackupPhaseFinalizing, + Version: 1, + FormatVersion: "1.1.0", + StartTimestamp: ×tamp, + Expiration: ×tamp, + CSIVolumeSnapshotsAttempted: 1, + CSIVolumeSnapshotsCompleted: 0, + }, + }, + volumeSnapshot: builder.ForVolumeSnapshot("velero", "testVS").ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1")).Result(), + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { formatFlag := logging.FormatText var ( - logger = logging.DefaultLogger(logrus.DebugLevel, formatFlag) - pluginManager = new(pluginmocks.Manager) - backupStore = new(persistencemocks.BackupStore) - backupper = new(fakeBackupper) + logger = logging.DefaultLogger(logrus.DebugLevel, formatFlag) + pluginManager = new(pluginmocks.Manager) + backupStore = new(persistencemocks.BackupStore) + backupper = new(fakeBackupper) + snapshotClient = snapshotfake.NewSimpleClientset() + sharedInformer = snapshotinformers.NewSharedInformerFactory(snapshotClient, 0) + snapshotLister = sharedInformer.Snapshot().V1().VolumeSnapshots().Lister() ) var fakeClient kbclient.Client @@ -1020,6 +1148,12 @@ func TestProcessBackupCompletions(t *testing.T) { fakeClient = velerotest.NewFakeControllerRuntimeClient(t) } + if test.volumeSnapshot != nil { + snapshotClient.SnapshotV1().VolumeSnapshots(test.volumeSnapshot.Namespace).Create(context.Background(), test.volumeSnapshot, metav1.CreateOptions{}) + sharedInformer.Snapshot().V1().VolumeSnapshots().Informer().GetStore().Add(test.volumeSnapshot) + sharedInformer.WaitForCacheSync(make(chan struct{})) + } + apiServer := velerotest.NewAPIServer(t) apiServer.DiscoveryClient.FakedServerVersion = &version.Info{ @@ -1050,6 +1184,8 @@ func TestProcessBackupCompletions(t *testing.T) { backupStoreGetter: NewFakeSingleObjectBackupStoreGetter(backupStore), backupper: backupper, formatFlag: formatFlag, + volumeSnapshotClient: snapshotClient, + volumeSnapshotLister: snapshotLister, } pluginManager.On("GetBackupItemActionsV2").Return(nil, nil) @@ -1079,10 +1215,20 @@ func TestProcessBackupCompletions(t *testing.T) { // add the default backup storage location to the clientset and the informer/lister store require.NoError(t, fakeClient.Create(context.Background(), defaultBackupLocation)) + // Enable CSI feature flag for SnapshotDataMovement test. + if strings.Contains(test.name, "backup with snapshot data movement") { + features.Enable(velerov1api.CSIFeatureFlag) + } + actualResult, err := c.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: test.backup.Namespace, Name: test.backup.Name}}) assert.Equal(t, actualResult, ctrl.Result{}) assert.Nil(t, err) + // Disable CSI feature to not impact other test cases. + if strings.Contains(test.name, "backup with snapshot data movement") { + features.Disable(velerov1api.CSIFeatureFlag) + } + res := &velerov1api.Backup{} err = c.kbClient.Get(context.Background(), kbclient.ObjectKey{Namespace: test.backup.Namespace, Name: test.backup.Name}, res) require.NoError(t, err) diff --git a/pkg/podvolume/restorer_test.go b/pkg/podvolume/restorer_test.go index ae721ec96..0202904d8 100644 --- a/pkg/podvolume/restorer_test.go +++ b/pkg/podvolume/restorer_test.go @@ -460,7 +460,15 @@ func TestRestorePodVolumes(t *testing.T) { assert.Equal(t, test.errs[i].err, errMsg) } else { - assert.EqualError(t, errs[i], test.errs[i].err) + for i := 0; i < len(errs); i++ { + j := 0 + for ; j < len(test.errs); j++ { + if errs[i].Error() == test.errs[j].err { + break + } + } + assert.Equal(t, true, j < len(test.errs)) + } } } }