diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index 712ed95a0..fae2b44a6 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -510,9 +510,11 @@ func (r *backupDeletionReconciler) deletePodVolumeSnapshots(ctx context.Context, return []error{err} } - return r.batchDeleteSnapshots(ctx, directSnapshots, backup) + return batchDeleteSnapshots(ctx, r.repoEnsurer, r.repoMgr, directSnapshots, backup, r.logger) } +var batchDeleteSnapshotFunc = batchDeleteSnapshots + func (r *backupDeletionReconciler) deleteMovedSnapshots(ctx context.Context, backup *velerov1api.Backup) []error { if r.repoMgr == nil { return nil @@ -532,34 +534,50 @@ func (r *backupDeletionReconciler) deleteMovedSnapshots(ctx context.Context, bac directSnapshots := map[string][]repository.SnapshotIdentifier{} for i := range list.Items { cm := list.Items[i] - snapshot := repository.SnapshotIdentifier{} + if cm.Data == nil || len(cm.Data) == 0 { + errs = append(errs, errors.New("no snapshot info in config")) + continue + } + b, err := json.Marshal(cm.Data) if err != nil { errs = append(errs, errors.Wrapf(err, "fail to marshal the snapshot info into JSON")) continue } + + snapshot := repository.SnapshotIdentifier{} if err := json.Unmarshal(b, &snapshot); err != nil { errs = append(errs, errors.Wrapf(err, "failed to unmarshal snapshot info")) continue } + if snapshot.SnapshotID == "" || snapshot.VolumeNamespace == "" || snapshot.RepositoryType == "" { + errs = append(errs, errors.Errorf("invalid snapshot, ID %s, namespace %s, repository %s", snapshot.SnapshotID, snapshot.VolumeNamespace, snapshot.RepositoryType)) + continue + } + if directSnapshots[snapshot.VolumeNamespace] == nil { directSnapshots[snapshot.VolumeNamespace] = []repository.SnapshotIdentifier{} } directSnapshots[snapshot.VolumeNamespace] = append(directSnapshots[snapshot.VolumeNamespace], snapshot) - r.logger.Infof("Deleted snapshot %s, namespace: %s, repo type: %s", snapshot.SnapshotID, snapshot.VolumeNamespace, snapshot.RepositoryType) + r.logger.Infof("Deleting snapshot %s, namespace: %s, repo type: %s", snapshot.SnapshotID, snapshot.VolumeNamespace, snapshot.RepositoryType) + } + + for i := range list.Items { + cm := list.Items[i] if err := r.Client.Delete(ctx, &cm); err != nil { r.logger.Warnf("Failed to delete snapshot info configmap %s/%s: %v", cm.Namespace, cm.Name, err) } } - if len(errs) > 0 { - return errs + if len(directSnapshots) > 0 { + deleteErrs := batchDeleteSnapshotFunc(ctx, r.repoEnsurer, r.repoMgr, directSnapshots, backup, r.logger) + errs = append(errs, deleteErrs...) } - return r.batchDeleteSnapshots(ctx, directSnapshots, backup) + return errs } func (r *backupDeletionReconciler) patchDeleteBackupRequest(ctx context.Context, req *velerov1api.DeleteBackupRequest, mutate func(*velerov1api.DeleteBackupRequest)) (*velerov1api.DeleteBackupRequest, error) { @@ -615,7 +633,8 @@ func getSnapshotsInBackup(ctx context.Context, backup *velerov1api.Backup, kbCli return podvolume.GetSnapshotIdentifier(podVolumeBackups), nil } -func (r *backupDeletionReconciler) batchDeleteSnapshots(ctx context.Context, directSnapshots map[string][]repository.SnapshotIdentifier, backup *velerov1api.Backup) []error { +func batchDeleteSnapshots(ctx context.Context, repoEnsurer *repository.Ensurer, repoMgr repository.Manager, + directSnapshots map[string][]repository.SnapshotIdentifier, backup *velerov1api.Backup, logger logrus.FieldLogger) []error { var errs []error for volumeNamespace, snapshots := range directSnapshots { batchForget := []string{} @@ -624,18 +643,19 @@ func (r *backupDeletionReconciler) batchDeleteSnapshots(ctx context.Context, dir } // For volumes in one backup, the BSL and repositoryType should always be the same - repo, err := r.repoEnsurer.EnsureRepo(ctx, backup.Namespace, volumeNamespace, backup.Spec.StorageLocation, snapshots[0].RepositoryType) + repoType := snapshots[0].RepositoryType + repo, err := repoEnsurer.EnsureRepo(ctx, backup.Namespace, volumeNamespace, backup.Spec.StorageLocation, repoType) if err != nil { - errs = append(errs, errors.Wrapf(err, "error to ensure repo %s-%s-%s, skip deleting PVB snapshots %v", backup.Spec.StorageLocation, volumeNamespace, snapshots[0].RepositoryType, batchForget)) + errs = append(errs, errors.Wrapf(err, "error to ensure repo %s-%s-%s, skip deleting PVB snapshots %v", backup.Spec.StorageLocation, volumeNamespace, repoType, batchForget)) continue } - if forgetErrs := r.repoMgr.BatchForget(ctx, repo, batchForget); len(forgetErrs) > 0 { + if forgetErrs := repoMgr.BatchForget(ctx, repo, batchForget); len(forgetErrs) > 0 { errs = append(errs, forgetErrs...) continue } - r.logger.Infof("Batch deleted snapshots %v", batchForget) + logger.Infof("Batch deleted snapshots %v", batchForget) } return errs diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index 2a0fb9542..e0f1f68aa 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -18,6 +18,8 @@ package controller import ( "bytes" + "encoding/json" + "errors" "fmt" "io" "reflect" @@ -54,6 +56,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" pluginmocks "github.com/vmware-tanzu/velero/pkg/plugin/mocks" "github.com/vmware-tanzu/velero/pkg/repository" + repomocks "github.com/vmware-tanzu/velero/pkg/repository/mocks" velerotest "github.com/vmware-tanzu/velero/pkg/test" ) @@ -850,26 +853,178 @@ func TestGetSnapshotsInBackup(t *testing.T) { assert.NoError(t, err) assert.True(t, reflect.DeepEqual(res, test.expected)) - - // for k, v := range res { - - // } - - // // sort to ensure good compare of slices - // less := func(snapshots []repository.SnapshotIdentifier) func(i, j int) bool { - // return func(i, j int) bool { - // if snapshots[i].VolumeNamespace == snapshots[j].VolumeNamespace { - // return snapshots[i].SnapshotID < snapshots[j].SnapshotID - // } - // return snapshots[i].VolumeNamespace < snapshots[j].VolumeNamespace - // } - - // } - - // sort.Slice(test.expected, less(test.expected)) - // sort.Slice(res, less(res)) - - // assert.Equal(t, test.expected, res) + }) + } +} + +func batchDeleteSucceed(ctx context.Context, repoEnsurer *repository.Ensurer, repoMgr repository.Manager, directSnapshots map[string][]repository.SnapshotIdentifier, backup *velerov1api.Backup, logger logrus.FieldLogger) []error { + return nil +} + +func batchDeleteFail(ctx context.Context, repoEnsurer *repository.Ensurer, repoMgr repository.Manager, directSnapshots map[string][]repository.SnapshotIdentifier, backup *velerov1api.Backup, logger logrus.FieldLogger) []error { + return []error{ + errors.New("fake-delete-1"), + errors.New("fake-delete-2"), + } +} + +func generateSnapshotData(snapshot *repository.SnapshotIdentifier) (map[string]string, error) { + if snapshot == nil { + return nil, nil + } + + b, err := json.Marshal(snapshot) + if err != nil { + return nil, err + } + + data := make(map[string]string) + if err := json.Unmarshal(b, &data); err != nil { + return nil, err + } + + return data, nil +} + +func TestDeleteMovedSnapshots(t *testing.T) { + tests := []struct { + name string + repoMgr repository.Manager + batchDeleteSucceed bool + backupName string + snapshots []*repository.SnapshotIdentifier + expected []string + }{ + { + name: "repoMgr is nil", + }, + { + name: "no cm", + repoMgr: repomocks.NewManager(t), + }, + { + name: "bad cm info", + repoMgr: repomocks.NewManager(t), + backupName: "backup-01", + snapshots: []*repository.SnapshotIdentifier{nil}, + expected: []string{"no snapshot info in config"}, + }, + { + name: "invalid snapshots", + repoMgr: repomocks.NewManager(t), + backupName: "backup-01", + snapshots: []*repository.SnapshotIdentifier{ + { + RepositoryType: "repo-1", + VolumeNamespace: "ns-1", + }, + { + SnapshotID: "snapshot-1", + VolumeNamespace: "ns-1", + }, + { + SnapshotID: "snapshot-1", + RepositoryType: "repo-1", + }, + }, + batchDeleteSucceed: true, + expected: []string{ + "invalid snapshot, ID , namespace ns-1, repository repo-1", + "invalid snapshot, ID snapshot-1, namespace ns-1, repository ", + "invalid snapshot, ID snapshot-1, namespace , repository repo-1", + }, + }, + { + name: "batch delete succeed", + repoMgr: repomocks.NewManager(t), + backupName: "backup-01", + snapshots: []*repository.SnapshotIdentifier{ + + { + SnapshotID: "snapshot-1", + RepositoryType: "repo-1", + VolumeNamespace: "ns-1", + }, + }, + batchDeleteSucceed: true, + expected: []string{}, + }, + { + name: "batch delete fail", + repoMgr: repomocks.NewManager(t), + backupName: "backup-01", + snapshots: []*repository.SnapshotIdentifier{ + { + RepositoryType: "repo-1", + VolumeNamespace: "ns-1", + }, + { + SnapshotID: "snapshot-1", + RepositoryType: "repo-1", + VolumeNamespace: "ns-1", + }, + }, + expected: []string{"invalid snapshot, ID , namespace ns-1, repository repo-1", "fake-delete-1", "fake-delete-2"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + objs := []runtime.Object{} + for i, snapshot := range test.snapshots { + snapshotData, err := generateSnapshotData(snapshot) + require.NoError(t, err) + + cm := corev1api.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1api.SchemeGroupVersion.String(), + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "velero", + Name: fmt.Sprintf("du-info-%d", i), + Labels: map[string]string{ + velerov1api.BackupNameLabel: test.backupName, + velerov1api.DataUploadSnapshotInfoLabel: "true", + }, + }, + Data: snapshotData, + } + + objs = append(objs, &cm) + } + + veleroBackup := &velerov1api.Backup{} + controller := NewBackupDeletionReconciler( + velerotest.NewLogger(), + velerotest.NewFakeControllerRuntimeClient(t, objs...), + NewBackupTracker(), + test.repoMgr, + metrics.NewServerMetrics(), + nil, // discovery helper + func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + NewFakeSingleObjectBackupStoreGetter(backupStore), + velerotest.NewFakeCredentialsFileStore("", nil), + nil, + ) + + veleroBackup.Name = test.backupName + + if test.batchDeleteSucceed { + batchDeleteSnapshotFunc = batchDeleteSucceed + } else { + batchDeleteSnapshotFunc = batchDeleteFail + } + + errs := controller.deleteMovedSnapshots(context.Background(), veleroBackup) + if test.expected == nil { + assert.Nil(t, errs) + } else { + assert.Equal(t, len(test.expected), len(errs)) + for i := range test.expected { + assert.EqualError(t, errs[i], test.expected[i]) + } + } }) } } diff --git a/pkg/datamover/dataupload_delete_action.go b/pkg/datamover/dataupload_delete_action.go index 3e8d5b903..18501719d 100644 --- a/pkg/datamover/dataupload_delete_action.go +++ b/pkg/datamover/dataupload_delete_action.go @@ -56,7 +56,7 @@ func genConfigmap(bak *velerov1.Backup, du velerov2alpha1.DataUpload) *corev1api VolumeNamespace: du.Spec.SourceNamespace, BackupStorageLocation: bak.Spec.StorageLocation, SnapshotID: du.Status.SnapshotID, - RepositoryType: GetUploaderType(du.Spec.DataMover), + RepositoryType: velerov1.BackupRepositoryTypeKopia, UploaderType: GetUploaderType(du.Spec.DataMover), Source: GetRealSource(du.Spec.SourceNamespace, du.Spec.SourcePVC), } diff --git a/pkg/repository/provider/unified_repo.go b/pkg/repository/provider/unified_repo.go index 178c08a7f..a72ecdad4 100644 --- a/pkg/repository/provider/unified_repo.go +++ b/pkg/repository/provider/unified_repo.go @@ -350,7 +350,7 @@ func (urp *unifiedRepoProvider) BatchForget(ctx context.Context, snapshotIDs []s for _, snapshotID := range snapshotIDs { err = bkRepo.DeleteManifest(ctx, udmrepo.ID(snapshotID)) if err != nil { - errs = append(errs, errors.Wrap(err, "error to delete manifest")) + errs = append(errs, errors.Wrapf(err, "error to delete manifest %s", snapshotID)) } } @@ -361,7 +361,7 @@ func (urp *unifiedRepoProvider) BatchForget(ctx context.Context, snapshotIDs []s log.Debug("Forget snapshot complete") - return nil + return errs } func (urp *unifiedRepoProvider) DefaultMaintenanceFrequency(ctx context.Context, param RepoParam) time.Duration { diff --git a/pkg/repository/provider/unified_repo_test.go b/pkg/repository/provider/unified_repo_test.go index 9a7dd84da..44d7301c4 100644 --- a/pkg/repository/provider/unified_repo_test.go +++ b/pkg/repository/provider/unified_repo_test.go @@ -857,6 +857,161 @@ func TestForget(t *testing.T) { } } +func TestBatchForget(t *testing.T) { + var backupRepo *reposervicenmocks.BackupRepo + + testCases := []struct { + name string + funcTable localFuncTable + getter *credmock.SecretStore + repoService *reposervicenmocks.BackupRepoService + backupRepo *reposervicenmocks.BackupRepo + retFuncOpen []interface{} + retFuncDelete interface{} + retFuncFlush interface{} + credStoreReturn string + credStoreError error + snapshots []string + expectedErr []string + }{ + { + name: "get repo option fail", + expectedErr: []string{"error to get repo options: error to get repo password: invalid credentials interface"}, + }, + { + name: "repo open fail", + getter: new(credmock.SecretStore), + credStoreReturn: "fake-password", + funcTable: localFuncTable{ + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + return map[string]string{}, nil + }, + getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { + return map[string]string{}, nil + }, + }, + repoService: new(reposervicenmocks.BackupRepoService), + retFuncOpen: []interface{}{ + func(context.Context, udmrepo.RepoOptions) udmrepo.BackupRepo { + return backupRepo + }, + + func(context.Context, udmrepo.RepoOptions) error { + return errors.New("fake-error-2") + }, + }, + expectedErr: []string{"error to open backup repo: fake-error-2"}, + }, + { + name: "delete fail", + getter: new(credmock.SecretStore), + credStoreReturn: "fake-password", + funcTable: localFuncTable{ + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + return map[string]string{}, nil + }, + getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { + return map[string]string{}, nil + }, + }, + repoService: new(reposervicenmocks.BackupRepoService), + backupRepo: new(reposervicenmocks.BackupRepo), + retFuncOpen: []interface{}{ + func(context.Context, udmrepo.RepoOptions) udmrepo.BackupRepo { + return backupRepo + }, + + func(context.Context, udmrepo.RepoOptions) error { + return nil + }, + }, + retFuncDelete: func(context.Context, udmrepo.ID) error { + return errors.New("fake-error-3") + }, + snapshots: []string{"snapshot-1", "snapshot-2"}, + expectedErr: []string{"error to delete manifest snapshot-1: fake-error-3", "error to delete manifest snapshot-2: fake-error-3"}, + }, + { + name: "flush fail", + getter: new(credmock.SecretStore), + credStoreReturn: "fake-password", + funcTable: localFuncTable{ + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + return map[string]string{}, nil + }, + getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { + return map[string]string{}, nil + }, + }, + repoService: new(reposervicenmocks.BackupRepoService), + backupRepo: new(reposervicenmocks.BackupRepo), + retFuncOpen: []interface{}{ + func(context.Context, udmrepo.RepoOptions) udmrepo.BackupRepo { + return backupRepo + }, + + func(context.Context, udmrepo.RepoOptions) error { + return nil + }, + }, + retFuncDelete: func(context.Context, udmrepo.ID) error { + return nil + }, + retFuncFlush: func(context.Context) error { + return errors.New("fake-error-4") + }, + expectedErr: []string{"error to flush repo: fake-error-4"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + funcTable = tc.funcTable + + var secretStore velerocredentials.SecretStore + if tc.getter != nil { + tc.getter.On("Get", mock.Anything, mock.Anything).Return(tc.credStoreReturn, tc.credStoreError) + secretStore = tc.getter + } + + urp := unifiedRepoProvider{ + credentialGetter: velerocredentials.CredentialGetter{ + FromSecret: secretStore, + }, + repoService: tc.repoService, + log: velerotest.NewLogger(), + } + + backupRepo = tc.backupRepo + + if tc.repoService != nil { + tc.repoService.On("Open", mock.Anything, mock.Anything).Return(tc.retFuncOpen[0], tc.retFuncOpen[1]) + } + + if tc.backupRepo != nil { + backupRepo.On("DeleteManifest", mock.Anything, mock.Anything).Return(tc.retFuncDelete) + backupRepo.On("Flush", mock.Anything).Return(tc.retFuncFlush) + backupRepo.On("Close", mock.Anything).Return(nil) + } + + errs := urp.BatchForget(context.Background(), tc.snapshots, RepoParam{ + BackupLocation: &velerov1api.BackupStorageLocation{}, + BackupRepo: &velerov1api.BackupRepository{}, + }) + + if tc.expectedErr == nil { + assert.Equal(t, 0, len(errs)) + } else { + assert.Equal(t, len(tc.expectedErr), len(errs)) + + for i := range tc.expectedErr { + assert.EqualError(t, errs[i], tc.expectedErr[i]) + } + } + }) + } +} + func TestInitRepo(t *testing.T) { testCases := []struct { name string