From 386fbb1ea6d087ff35ff38aa60a4a67054d3cdf4 Mon Sep 17 00:00:00 2001 From: Scott Seago Date: Fri, 12 Jul 2024 10:11:20 -0400 Subject: [PATCH 1/3] Reuse existing plugin manager for get/put volume info Signed-off-by: Scott Seago --- changelogs/unreleased/8012-sseago | 1 + pkg/backup/backup.go | 27 +++++-------------- pkg/backup/backup_test.go | 2 +- pkg/controller/backup_controller_test.go | 1 + pkg/controller/backup_finalizer_controller.go | 1 + .../backup_finalizer_controller_test.go | 2 +- 6 files changed, 12 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/8012-sseago diff --git a/changelogs/unreleased/8012-sseago b/changelogs/unreleased/8012-sseago new file mode 100644 index 000000000..6f56e867a --- /dev/null +++ b/changelogs/unreleased/8012-sseago @@ -0,0 +1 @@ +Reuse existing plugin manager for get/put volume info diff --git a/pkg/backup/backup.go b/pkg/backup/backup.go index 10ef71f39..ecb2aee91 100644 --- a/pkg/backup/backup.go +++ b/pkg/backup/backup.go @@ -95,6 +95,7 @@ type Backupper interface { outBackupFile io.Writer, backupItemActionResolver framework.BackupItemActionResolverV2, asyncBIAOperations []*itemoperation.BackupOperation, + backupStore persistence.BackupStore, ) error } @@ -610,6 +611,7 @@ func (kb *kubernetesBackupper) FinalizeBackup( outBackupFile io.Writer, backupItemActionResolver framework.BackupItemActionResolverV2, asyncBIAOperations []*itemoperation.BackupOperation, + backupStore persistence.BackupStore, ) error { gzw := gzip.NewWriter(outBackupFile) defer gzw.Close() @@ -726,7 +728,7 @@ func (kb *kubernetesBackupper) FinalizeBackup( }).Infof("Updated %d items out of an estimated total of %d (estimate will change throughout the backup finalizer)", len(backupRequest.BackedUpItems), totalItems) } - backupStore, volumeInfos, err := kb.getVolumeInfos(*backupRequest.Backup, log) + volumeInfos, err := kb.getVolumeInfos(*backupRequest.Backup, backupStore, log) if err != nil { log.WithError(err).Errorf("fail to get the backup VolumeInfos for backup %s", backupRequest.Name) return err @@ -812,30 +814,15 @@ type tarWriter interface { func (kb *kubernetesBackupper) getVolumeInfos( backup velerov1api.Backup, + backupStore persistence.BackupStore, log logrus.FieldLogger, -) (persistence.BackupStore, []*volume.BackupVolumeInfo, error) { - location := &velerov1api.BackupStorageLocation{} - if err := kb.kbClient.Get(context.Background(), kbclient.ObjectKey{ - Namespace: backup.Namespace, - Name: backup.Spec.StorageLocation, - }, location); err != nil { - return nil, nil, errors.WithStack(err) - } - - pluginManager := kb.pluginManager(log) - defer pluginManager.CleanupClients() - - backupStore, storeErr := kb.backupStoreGetter.Get(location, pluginManager, log) - if storeErr != nil { - return nil, nil, storeErr - } - +) ([]*volume.BackupVolumeInfo, error) { volumeInfos, err := backupStore.GetBackupVolumeInfos(backup.Name) if err != nil { - return nil, nil, err + return nil, err } - return backupStore, volumeInfos, nil + return volumeInfos, nil } // updateVolumeInfos update the VolumeInfos according to the AsyncOperations diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index 86aa2ee43..bcb58a78a 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -4532,7 +4532,7 @@ func TestGetVolumeInfos(t *testing.T) { bsl := builder.ForBackupStorageLocation("velero", "default").Result() require.NoError(t, h.backupper.kbClient.Create(context.Background(), bsl)) - _, _, err := h.backupper.getVolumeInfos(*backup, h.log) + _, err := h.backupper.getVolumeInfos(*backup, backupStore, h.log) require.NoError(t, err) } diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index ad7677923..fb6b55631 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -85,6 +85,7 @@ func (b *fakeBackupper) FinalizeBackup( outBackupFile io.Writer, backupItemActionResolver framework.BackupItemActionResolverV2, asyncBIAOperations []*itemoperation.BackupOperation, + backupStore persistence.BackupStore, ) error { args := b.Called(logger, backup, inBackupFile, outBackupFile, backupItemActionResolver, asyncBIAOperations) return args.Error(0) diff --git a/pkg/controller/backup_finalizer_controller.go b/pkg/controller/backup_finalizer_controller.go index 2bf17e9bf..74386b1cd 100644 --- a/pkg/controller/backup_finalizer_controller.go +++ b/pkg/controller/backup_finalizer_controller.go @@ -184,6 +184,7 @@ func (r *backupFinalizerReconciler) Reconcile(ctx context.Context, req ctrl.Requ outBackupFile, backupItemActionsResolver, operations, + backupStore, ) if err != nil { log.WithError(err).Error("error finalizing Backup") diff --git a/pkg/controller/backup_finalizer_controller_test.go b/pkg/controller/backup_finalizer_controller_test.go index 8c1a5e361..898c5c28d 100644 --- a/pkg/controller/backup_finalizer_controller_test.go +++ b/pkg/controller/backup_finalizer_controller_test.go @@ -225,7 +225,7 @@ func TestBackupFinalizerReconcile(t *testing.T) { backupStore.On("GetBackupVolumeInfos", mock.Anything).Return(nil, nil) backupStore.On("PutBackupVolumeInfos", mock.Anything, mock.Anything).Return(nil) pluginManager.On("GetBackupItemActionsV2").Return(nil, nil) - backupper.On("FinalizeBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, framework.BackupItemActionResolverV2{}, mock.Anything).Return(nil) + backupper.On("FinalizeBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, framework.BackupItemActionResolverV2{}, mock.Anything, mock.Anything).Return(nil) _, err := reconciler.Reconcile(context.TODO(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: test.backup.Namespace, Name: test.backup.Name}}) gotErr := err != nil assert.Equal(t, test.expectError, gotErr) From 54df263094fd2de2b69763330c5a83d44af78126 Mon Sep 17 00:00:00 2001 From: lyndon-li <98304688+Lyndon-Li@users.noreply.github.com> Date: Mon, 15 Jul 2024 21:48:10 +0800 Subject: [PATCH 2/3] fix linter check error (#8014) Signed-off-by: Lyndon-Li Signed-off-by: Scott Seago --- pkg/backup/backup.go | 15 +-------------- pkg/backup/backup_test.go | 19 ------------------- 2 files changed, 1 insertion(+), 33 deletions(-) diff --git a/pkg/backup/backup.go b/pkg/backup/backup.go index ecb2aee91..c0ad81cf6 100644 --- a/pkg/backup/backup.go +++ b/pkg/backup/backup.go @@ -728,7 +728,7 @@ func (kb *kubernetesBackupper) FinalizeBackup( }).Infof("Updated %d items out of an estimated total of %d (estimate will change throughout the backup finalizer)", len(backupRequest.BackedUpItems), totalItems) } - volumeInfos, err := kb.getVolumeInfos(*backupRequest.Backup, backupStore, log) + volumeInfos, err := backupStore.GetBackupVolumeInfos(backupRequest.Backup.Name) if err != nil { log.WithError(err).Errorf("fail to get the backup VolumeInfos for backup %s", backupRequest.Name) return err @@ -812,19 +812,6 @@ type tarWriter interface { WriteHeader(*tar.Header) error } -func (kb *kubernetesBackupper) getVolumeInfos( - backup velerov1api.Backup, - backupStore persistence.BackupStore, - log logrus.FieldLogger, -) ([]*volume.BackupVolumeInfo, error) { - volumeInfos, err := backupStore.GetBackupVolumeInfos(backup.Name) - if err != nil { - return nil, err - } - - return volumeInfos, nil -} - // updateVolumeInfos update the VolumeInfos according to the AsyncOperations func updateVolumeInfos( volumeInfos []*volume.BackupVolumeInfo, diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index bcb58a78a..8dcbc5090 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -54,8 +54,6 @@ import ( "github.com/vmware-tanzu/velero/pkg/kuberesource" "github.com/vmware-tanzu/velero/pkg/persistence" persistencemocks "github.com/vmware-tanzu/velero/pkg/persistence/mocks" - "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" - pluginmocks "github.com/vmware-tanzu/velero/pkg/plugin/mocks" "github.com/vmware-tanzu/velero/pkg/plugin/velero" biav2 "github.com/vmware-tanzu/velero/pkg/plugin/velero/backupitemaction/v2" vsv1 "github.com/vmware-tanzu/velero/pkg/plugin/velero/volumesnapshotter/v1" @@ -4519,23 +4517,6 @@ func TestBackupNamespaces(t *testing.T) { } } -func TestGetVolumeInfos(t *testing.T) { - h := newHarness(t) - pluginManager := new(pluginmocks.Manager) - backupStore := new(persistencemocks.BackupStore) - h.backupper.pluginManager = func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager } - h.backupper.backupStoreGetter = NewFakeSingleObjectBackupStoreGetter(backupStore) - backupStore.On("GetBackupVolumeInfos", "backup-01").Return([]*volume.BackupVolumeInfo{}, nil) - pluginManager.On("CleanupClients").Return() - - backup := builder.ForBackup("velero", "backup-01").StorageLocation("default").Result() - bsl := builder.ForBackupStorageLocation("velero", "default").Result() - require.NoError(t, h.backupper.kbClient.Create(context.Background(), bsl)) - - _, err := h.backupper.getVolumeInfos(*backup, backupStore, h.log) - require.NoError(t, err) -} - func TestUpdateVolumeInfos(t *testing.T) { timeExample := time.Date(2014, 6, 5, 11, 56, 45, 0, time.Local) now := metav1.NewTime(timeExample) From 89a536382b00733160299a0d56ea76bcb63660a7 Mon Sep 17 00:00:00 2001 From: Scott Seago Date: Mon, 15 Jul 2024 14:26:56 -0400 Subject: [PATCH 3/3] update changelog filename Signed-off-by: Scott Seago --- changelogs/unreleased/{8012-sseago => 8016-sseago} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelogs/unreleased/{8012-sseago => 8016-sseago} (100%) diff --git a/changelogs/unreleased/8012-sseago b/changelogs/unreleased/8016-sseago similarity index 100% rename from changelogs/unreleased/8012-sseago rename to changelogs/unreleased/8016-sseago