From f1fcec35142cd8cb1574242669051b8c944602c0 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 3 Oct 2023 14:14:48 -0700 Subject: [PATCH 1/5] Fix issue 6913 Signed-off-by: Shubham Pampattiwar add changelog file Signed-off-by: Shubham Pampattiwar keep canceling phase const Signed-off-by: Shubham Pampattiwar fix data download as well Signed-off-by: Shubham Pampattiwar address PR feedback Signed-off-by: Shubham Pampattiwar minor fixes Signed-off-by: Shubham Pampattiwar (cherry picked from commit ee271b7b69171a5be25d0e8bacf03557dac23d13) update changelog filename Signed-off-by: Shubham Pampattiwar --- changelogs/unreleased/7025-shubham-pampattiwar | 1 + pkg/controller/data_download_controller.go | 4 ++-- pkg/controller/data_upload_controller.go | 12 +++++++----- 3 files changed, 10 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/7025-shubham-pampattiwar diff --git a/changelogs/unreleased/7025-shubham-pampattiwar b/changelogs/unreleased/7025-shubham-pampattiwar new file mode 100644 index 000000000..c95688b39 --- /dev/null +++ b/changelogs/unreleased/7025-shubham-pampattiwar @@ -0,0 +1 @@ +Fix issue 6913: Velero Built-in Datamover: Backup stucks in phase WaitingForPluginOperations when Node Agent pod gets restarted \ No newline at end of file diff --git a/pkg/controller/data_download_controller.go b/pkg/controller/data_download_controller.go index cc60cacef..0212fa00f 100644 --- a/pkg/controller/data_download_controller.go +++ b/pkg/controller/data_download_controller.go @@ -287,12 +287,13 @@ func (r *DataDownloadReconciler) Reconcile(ctx context.Context, req ctrl.Request } else if dd.Status.Phase == velerov2alpha1api.DataDownloadPhaseInProgress { log.Info("Data download is in progress") if dd.Spec.Cancel { + log.Info("Data download is being canceled") fsRestore := r.dataPathMgr.GetAsyncBR(dd.Name) if fsRestore == nil { + r.OnDataDownloadCancelled(ctx, dd.GetNamespace(), dd.GetName()) return ctrl.Result{}, nil } - log.Info("Data download is being canceled") // Update status to Canceling. original := dd.DeepCopy() dd.Status.Phase = velerov2alpha1api.DataDownloadPhaseCanceling @@ -300,7 +301,6 @@ func (r *DataDownloadReconciler) Reconcile(ctx context.Context, req ctrl.Request log.WithError(err).Error("error updating data download status") return ctrl.Result{}, err } - fsRestore.Cancel() return ctrl.Result{}, nil } diff --git a/pkg/controller/data_upload_controller.go b/pkg/controller/data_upload_controller.go index 8bc650f5f..ec3f4d154 100644 --- a/pkg/controller/data_upload_controller.go +++ b/pkg/controller/data_upload_controller.go @@ -292,13 +292,15 @@ func (r *DataUploadReconciler) Reconcile(ctx context.Context, req ctrl.Request) } else if du.Status.Phase == velerov2alpha1api.DataUploadPhaseInProgress { log.Info("Data upload is in progress") if du.Spec.Cancel { - fsBackup := r.dataPathMgr.GetAsyncBR(du.Name) - if fsBackup == nil { - return ctrl.Result{}, nil - } log.Info("Data upload is being canceled") - // Update status to Canceling. + fsBackup := r.dataPathMgr.GetAsyncBR(du.Name) + if fsBackup == nil { + r.OnDataUploadCancelled(ctx, du.GetNamespace(), du.GetName()) + return ctrl.Result{}, nil + } + + // Update status to Canceling original := du.DeepCopy() du.Status.Phase = velerov2alpha1api.DataUploadPhaseCanceling if err := r.client.Patch(ctx, du, client.MergeFrom(original)); err != nil { From 7b76047596af4936678e47b0698676f54ce97419 Mon Sep 17 00:00:00 2001 From: Anshul Ahuja Date: Fri, 27 Oct 2023 02:10:43 +0530 Subject: [PATCH 2/5] Make configmapref check case insensitive (#6804) (#7023) * Make configmapref check case insensitive * update resourcemodfier test case to validate case --------- Signed-off-by: Anshul Ahuja Signed-off-by: Anshul Ahuja Co-authored-by: Anshul Ahuja --- pkg/controller/backup_controller.go | 3 ++- pkg/controller/restore_controller.go | 3 ++- pkg/controller/restore_controller_test.go | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index a55571a8d..f4a4147cb 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -21,6 +21,7 @@ import ( "context" "fmt" "os" + "strings" "time" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" @@ -476,7 +477,7 @@ func (b *backupReconciler) prepareBackupRequest(backup *velerov1api.Backup, logg request.Status.ValidationErrors = append(request.Status.ValidationErrors, "encountered labelSelector as well as orLabelSelectors in backup spec, only one can be specified") } - if request.Spec.ResourcePolicy != nil && request.Spec.ResourcePolicy.Kind == resourcepolicies.ConfigmapRefType { + if request.Spec.ResourcePolicy != nil && strings.EqualFold(request.Spec.ResourcePolicy.Kind, resourcepolicies.ConfigmapRefType) { policiesConfigmap := &corev1api.ConfigMap{} err := b.kbClient.Get(context.Background(), kbclient.ObjectKey{Namespace: request.Namespace, Name: request.Spec.ResourcePolicy.Name}, policiesConfigmap) if err != nil { diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 0e11d7ced..5d6ed505e 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -25,6 +25,7 @@ import ( "io" "os" "sort" + "strings" "time" "github.com/pkg/errors" @@ -376,7 +377,7 @@ func (r *restoreReconciler) validateAndComplete(restore *api.Restore) (backupInf } var resourceModifiers *resourcemodifiers.ResourceModifiers = nil - if restore.Spec.ResourceModifier != nil && restore.Spec.ResourceModifier.Kind == resourcemodifiers.ConfigmapRefType { + if restore.Spec.ResourceModifier != nil && strings.EqualFold(restore.Spec.ResourceModifier.Kind, resourcemodifiers.ConfigmapRefType) { ResourceModifierConfigMap := &corev1api.ConfigMap{} err := r.kbClient.Get(context.Background(), client.ObjectKey{Namespace: restore.Namespace, Name: restore.Spec.ResourceModifier.Name}, ResourceModifierConfigMap) if err != nil { diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index e3ffaf4df..b7f6fbd25 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -781,7 +781,8 @@ func TestValidateAndCompleteWithResourceModifierSpecified(t *testing.T) { Spec: velerov1api.RestoreSpec{ BackupName: "backup-1", ResourceModifier: &corev1.TypedLocalObjectReference{ - Kind: resourcemodifiers.ConfigmapRefType, + // intentional to ensure case insensitivity works as expected + Kind: "confIGMaP", Name: "test-configmap-invalid", }, }, From dd825ef8bb5ce777815c20713581c3b6e5996701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wenkai=20Yin=28=E5=B0=B9=E6=96=87=E5=BC=80=29?= Date: Mon, 30 Oct 2023 16:54:21 +0800 Subject: [PATCH 3/5] Read information from the credential specified by BSL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Read information from the credential specified by BSL Signed-off-by: Wenkai Yin(尹文开) --- changelogs/unreleased/7033-ywk253100 | 1 + pkg/repository/provider/unified_repo.go | 15 ++++++-- pkg/repository/provider/unified_repo_test.go | 37 ++++++++++---------- 3 files changed, 32 insertions(+), 21 deletions(-) create mode 100644 changelogs/unreleased/7033-ywk253100 diff --git a/changelogs/unreleased/7033-ywk253100 b/changelogs/unreleased/7033-ywk253100 new file mode 100644 index 000000000..b4ce2bbab --- /dev/null +++ b/changelogs/unreleased/7033-ywk253100 @@ -0,0 +1 @@ +Read information from the credential specified by BSL \ No newline at end of file diff --git a/pkg/repository/provider/unified_repo.go b/pkg/repository/provider/unified_repo.go index d7522ec86..768108181 100644 --- a/pkg/repository/provider/unified_repo.go +++ b/pkg/repository/provider/unified_repo.go @@ -54,7 +54,7 @@ var getS3BucketRegion = repoconfig.GetAWSBucketRegion var getAzureStorageDomain = repoconfig.GetAzureStorageDomain type localFuncTable struct { - getStorageVariables func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) + getStorageVariables func(*velerov1api.BackupStorageLocation, string, string, credentials.FileStore) (map[string]string, error) getStorageCredentials func(*velerov1api.BackupStorageLocation, credentials.FileStore) (map[string]string, error) } @@ -345,7 +345,7 @@ func (urp *unifiedRepoProvider) GetStoreOptions(param interface{}) (map[string]s return map[string]string{}, errors.Errorf("invalid parameter, expect %T, actual %T", RepoParam{}, param) } - storeVar, err := funcTable.getStorageVariables(repoParam.BackupLocation, urp.repoBackend, repoParam.BackupRepo.Spec.VolumeNamespace) + storeVar, err := funcTable.getStorageVariables(repoParam.BackupLocation, urp.repoBackend, repoParam.BackupRepo.Spec.VolumeNamespace, urp.credentialGetter.FromFile) if err != nil { return map[string]string{}, errors.Wrap(err, "error to get storage variables") } @@ -450,7 +450,8 @@ func getStorageCredentials(backupLocation *velerov1api.BackupStorageLocation, cr return result, nil } -func getStorageVariables(backupLocation *velerov1api.BackupStorageLocation, repoBackend string, repoName string) (map[string]string, error) { +func getStorageVariables(backupLocation *velerov1api.BackupStorageLocation, repoBackend string, repoName string, + credentialFileStore credentials.FileStore) (map[string]string, error) { result := make(map[string]string) backendType := repoconfig.GetBackendType(backupLocation.Spec.Provider, backupLocation.Spec.Config) @@ -463,6 +464,14 @@ func getStorageVariables(backupLocation *velerov1api.BackupStorageLocation, repo config = map[string]string{} } + if backupLocation.Spec.Credential != nil { + credsFile, err := credentialFileStore.Path(backupLocation.Spec.Credential) + if err != nil { + return map[string]string{}, errors.WithStack(err) + } + config[repoconfig.CredentialsFileKey] = credsFile + } + bucket := strings.Trim(config["bucket"], "/") prefix := strings.Trim(config["prefix"], "/") if backupLocation.Spec.ObjectStorage != nil { diff --git a/pkg/repository/provider/unified_repo_test.go b/pkg/repository/provider/unified_repo_test.go index 08f33eb57..d1b4c3375 100644 --- a/pkg/repository/provider/unified_repo_test.go +++ b/pkg/repository/provider/unified_repo_test.go @@ -521,12 +521,13 @@ func TestGetStorageVariables(t *testing.T) { }, } + credFileStore := new(credmock.FileStore) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { getS3BucketRegion = tc.getS3BucketRegion getAzureStorageDomain = tc.getAzureStorageDomain - actual, err := getStorageVariables(&tc.backupLocation, tc.repoBackend, tc.repoName) + actual, err := getStorageVariables(&tc.backupLocation, tc.repoBackend, tc.repoName, credFileStore) require.Equal(t, tc.expected, actual) @@ -615,7 +616,7 @@ func TestGetStoreOptions(t *testing.T) { BackupRepo: &velerov1api.BackupRepository{}, }, funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, errors.New("fake-error-2") }, }, @@ -629,7 +630,7 @@ func TestGetStoreOptions(t *testing.T) { BackupRepo: &velerov1api.BackupRepository{}, }, funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -689,7 +690,7 @@ func TestPrepareRepo(t *testing.T) { repoService: new(reposervicenmocks.BackupRepoService), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, errors.New("fake-store-option-error") }, }, @@ -700,7 +701,7 @@ func TestPrepareRepo(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -720,7 +721,7 @@ func TestPrepareRepo(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -797,7 +798,7 @@ func TestForget(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -821,7 +822,7 @@ func TestForget(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -849,7 +850,7 @@ func TestForget(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -941,7 +942,7 @@ func TestInitRepo(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -959,7 +960,7 @@ func TestInitRepo(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -1029,7 +1030,7 @@ func TestConnectToRepo(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -1047,7 +1048,7 @@ func TestConnectToRepo(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -1121,7 +1122,7 @@ func TestBoostRepoConnect(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -1148,7 +1149,7 @@ func TestBoostRepoConnect(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -1174,7 +1175,7 @@ func TestBoostRepoConnect(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -1261,7 +1262,7 @@ func TestPruneRepo(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { @@ -1279,7 +1280,7 @@ func TestPruneRepo(t *testing.T) { getter: new(credmock.SecretStore), credStoreReturn: "fake-password", funcTable: localFuncTable{ - getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) { + getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string, velerocredentials.FileStore) (map[string]string, error) { return map[string]string{}, nil }, getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) { From 7e35fd32612f11beabfbaa39cbdffd83bdcaa9f7 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 2 Nov 2023 15:53:47 -0400 Subject: [PATCH 4/5] restore: Use warning when Create IsAlreadyExist and Get error (#7004) Signed-off-by: Tiger Kaovilai --- changelogs/unreleased/7054-kaovilai | 1 + pkg/restore/restore.go | 11 +++++------ 2 files changed, 6 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/7054-kaovilai diff --git a/changelogs/unreleased/7054-kaovilai b/changelogs/unreleased/7054-kaovilai new file mode 100644 index 000000000..3b85df196 --- /dev/null +++ b/changelogs/unreleased/7054-kaovilai @@ -0,0 +1 @@ +restore: Use warning when Create IsAlreadyExist and Get error \ No newline at end of file diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 835553330..e18e4d3e3 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1524,18 +1524,17 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } if restoreErr != nil { - // check for the existence of the object in cluster, if no error then it implies that object exists - // and if err then we want to judge whether there is an existing error in the previous creation. - // if so, we will return the 'get' error. - // otherwise, we will return the original creation error. + // check for the existence of the object that failed creation due to alreadyExist in cluster, if no error then it implies that object exists. + // and if err then itemExists remains false as we were not able to confirm the existence of the object via Get call or creation call. + // We return the get error as a warning to notify the user that the object could exist in cluster and we were not able to confirm it. if !ctx.disableInformerCache { fromCluster, err = ctx.getResource(groupResource, obj, namespace, name) } else { fromCluster, err = resourceClient.Get(name, metav1.GetOptions{}) } if err != nil && isAlreadyExistsError { - ctx.log.Errorf("Error retrieving in-cluster version of %s: %v", kube.NamespaceAndName(obj), err) - errs.Add(namespace, err) + ctx.log.Warnf("Unable to retrieve in-cluster version of %s: %v, object won't be restored by velero or have restore labels, and existing resource policy is not applied", kube.NamespaceAndName(obj), err) + warnings.Add(namespace, err) return warnings, errs, itemExists } } From bb4f9094fd92fc93d2feb36709fd2aed07273571 Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Tue, 31 Oct 2023 12:08:48 +0800 Subject: [PATCH 5/5] issue 7027: backup exposer -- don't assume first volume as the backup volume Signed-off-by: Lyndon-Li --- changelogs/unreleased/7060-Lyndon-Li | 1 + pkg/exposer/csi_snapshot.go | 16 ++- pkg/exposer/csi_snapshot_test.go | 179 +++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/7060-Lyndon-Li diff --git a/changelogs/unreleased/7060-Lyndon-Li b/changelogs/unreleased/7060-Lyndon-Li new file mode 100644 index 000000000..f2a3a9ae2 --- /dev/null +++ b/changelogs/unreleased/7060-Lyndon-Li @@ -0,0 +1 @@ +Fix issue #7027, data mover backup exposer should not assume the first volume as the backup volume in backup pod \ No newline at end of file diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 85f511524..8b501e648 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -190,6 +190,7 @@ func (e *csiSnapshotExposer) GetExposed(ctx context.Context, ownerObject corev1. backupPodName := ownerObject.Name backupPVCName := ownerObject.Name + volumeName := string(ownerObject.UID) curLog := e.log.WithFields(logrus.Fields{ "owner": ownerObject.Name, @@ -218,7 +219,20 @@ func (e *csiSnapshotExposer) GetExposed(ctx context.Context, ownerObject corev1. curLog.WithField("backup pvc", backupPVCName).Info("Backup PVC is bound") - return &ExposeResult{ByPod: ExposeByPod{HostingPod: pod, VolumeName: pod.Spec.Volumes[0].Name}}, nil + i := 0 + for i = 0; i < len(pod.Spec.Volumes); i++ { + if pod.Spec.Volumes[i].Name == volumeName { + break + } + } + + if i == len(pod.Spec.Volumes) { + return nil, errors.Errorf("backup pod %s doesn't have the expected backup volume", pod.Name) + } + + curLog.WithField("pod", pod.Name).Infof("Backup volume is found in pod at index %v", i) + + return &ExposeResult{ByPod: ExposeByPod{HostingPod: pod, VolumeName: volumeName}}, nil } func (e *csiSnapshotExposer) CleanUp(ctx context.Context, ownerObject corev1.ObjectReference, vsName string, sourceNamespace string) { diff --git a/pkg/exposer/csi_snapshot_test.go b/pkg/exposer/csi_snapshot_test.go index 7ea6d5bf0..d57d78ed4 100644 --- a/pkg/exposer/csi_snapshot_test.go +++ b/pkg/exposer/csi_snapshot_test.go @@ -37,6 +37,8 @@ import ( velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" velerotest "github.com/vmware-tanzu/velero/pkg/test" "github.com/vmware-tanzu/velero/pkg/util/boolptr" + + clientFake "sigs.k8s.io/controller-runtime/pkg/client/fake" ) type reactor struct { @@ -384,3 +386,180 @@ func TestExpose(t *testing.T) { }) } } + +func TestGetExpose(t *testing.T) { + backup := &velerov1.Backup{ + TypeMeta: metav1.TypeMeta{ + APIVersion: velerov1.SchemeGroupVersion.String(), + Kind: "Backup", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: velerov1.DefaultNamespace, + Name: "fake-backup", + UID: "fake-uid", + }, + } + + backupPod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: backup.Name, + }, + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + { + Name: "fake-volume", + }, + { + Name: "fake-volume-2", + }, + { + Name: string(backup.UID), + }, + }, + }, + } + + backupPodWithoutVolume := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: backup.Name, + }, + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + { + Name: "fake-volume-1", + }, + { + Name: "fake-volume-2", + }, + }, + }, + } + + backupPVC := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: backup.Name, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + VolumeName: "fake-pv-name", + }, + } + + backupPV := &corev1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-pv-name", + }, + } + + scheme := runtime.NewScheme() + corev1.AddToScheme(scheme) + + tests := []struct { + name string + kubeClientObj []runtime.Object + ownerBackup *velerov1.Backup + exposeWaitParam CSISnapshotExposeWaitParam + Timeout time.Duration + err string + expectedResult *ExposeResult + }{ + { + name: "backup pod is not found", + ownerBackup: backup, + exposeWaitParam: CSISnapshotExposeWaitParam{ + NodeName: "fake-node", + }, + }, + { + name: "wait pvc bound fail", + ownerBackup: backup, + exposeWaitParam: CSISnapshotExposeWaitParam{ + NodeName: "fake-node", + }, + kubeClientObj: []runtime.Object{ + backupPod, + }, + Timeout: time.Second, + err: "error to wait backup PVC bound, fake-backup: error to wait for rediness of PVC: error to get pvc velero/fake-backup: persistentvolumeclaims \"fake-backup\" not found", + }, + { + name: "backup volume not found in pod", + ownerBackup: backup, + exposeWaitParam: CSISnapshotExposeWaitParam{ + NodeName: "fake-node", + }, + kubeClientObj: []runtime.Object{ + backupPodWithoutVolume, + backupPVC, + backupPV, + }, + Timeout: time.Second, + err: "backup pod fake-backup doesn't have the expected backup volume", + }, + { + name: "succeed", + ownerBackup: backup, + exposeWaitParam: CSISnapshotExposeWaitParam{ + NodeName: "fake-node", + }, + kubeClientObj: []runtime.Object{ + backupPod, + backupPVC, + backupPV, + }, + Timeout: time.Second, + expectedResult: &ExposeResult{ + ByPod: ExposeByPod{ + HostingPod: backupPod, + VolumeName: string(backup.UID), + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + fakeKubeClient := fake.NewSimpleClientset(test.kubeClientObj...) + + fakeClientBuilder := clientFake.NewClientBuilder() + fakeClientBuilder = fakeClientBuilder.WithScheme(scheme) + + fakeClient := fakeClientBuilder.WithRuntimeObjects(test.kubeClientObj...).Build() + + exposer := csiSnapshotExposer{ + kubeClient: fakeKubeClient, + log: velerotest.NewLogger(), + } + + var ownerObject corev1.ObjectReference + if test.ownerBackup != nil { + ownerObject = corev1.ObjectReference{ + Kind: test.ownerBackup.Kind, + Namespace: test.ownerBackup.Namespace, + Name: test.ownerBackup.Name, + UID: test.ownerBackup.UID, + APIVersion: test.ownerBackup.APIVersion, + } + } + + test.exposeWaitParam.NodeClient = fakeClient + + result, err := exposer.GetExposed(context.Background(), ownerObject, test.Timeout, &test.exposeWaitParam) + if test.err == "" { + assert.NoError(t, err) + + if test.expectedResult == nil { + assert.Nil(t, result) + } else { + assert.NoError(t, err) + assert.Equal(t, test.expectedResult.ByPod.VolumeName, result.ByPod.VolumeName) + assert.Equal(t, test.expectedResult.ByPod.HostingPod.Name, result.ByPod.HostingPod.Name) + } + } else { + assert.EqualError(t, err, test.err) + } + }) + } +}