From 82faa554bda815fef10bdad4e6d7004f53653ce9 Mon Sep 17 00:00:00 2001 From: Anshul Ahuja Date: Fri, 19 Jul 2024 04:32:39 +0000 Subject: [PATCH 1/5] Fail Delete Backup if BSL is not available Signed-off-by: Anshul Ahuja --- pkg/controller/backup_deletion_controller.go | 6 +++++- pkg/controller/backup_deletion_controller_test.go | 9 +++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index a37f1f9e82..86612f871d 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -248,7 +248,11 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque backupStore, err := r.backupStoreGetter.Get(location, pluginManager, log) if err != nil { - return ctrl.Result{}, errors.Wrap(err, "error getting the backup store") + _, patchErr := r.patchDeleteBackupRequest(ctx, dbr, func(r *velerov1api.DeleteBackupRequest) { + r.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed + r.Status.Errors = append(r.Status.Errors, fmt.Sprintf("cannot delete backup because backup storage location %s is currently unavailable, error: %s", location.Name, err.Error())) + }) + return ctrl.Result{}, patchErr } actions, err := pluginManager.GetDeleteItemActions() diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index c82a90bca1..6b4bb7818c 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -125,8 +125,13 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { td := setupBackupDeletionControllerTest(t, defaultTestDbr(), location, backup) td.controller.backupStoreGetter = &fakeErrorBackupStoreGetter{} _, err := td.controller.Reconcile(ctx, td.req) - assert.Error(t, err) - assert.True(t, strings.HasPrefix(err.Error(), "error getting the backup store")) + assert.Nil(t, err) + res := &velerov1api.DeleteBackupRequest{} + err = td.fakeClient.Get(ctx, td.req.NamespacedName, res) + require.NoError(t, err) + assert.Equal(t, "Processed", string(res.Status.Phase)) + assert.Equal(t, 1, len(res.Status.Errors)) + assert.True(t, strings.HasPrefix(res.Status.Errors[0], fmt.Sprintf("cannot delete backup because backup storage location %s is currently unavailable", location.Name))) }) t.Run("missing spec.backupName", func(t *testing.T) { From 2aecd45285a9a56fc0d10bbdc372ff7032a215fe Mon Sep 17 00:00:00 2001 From: Anshul Ahuja Date: Fri, 19 Jul 2024 09:12:06 +0000 Subject: [PATCH 2/5] linter Signed-off-by: Anshul Ahuja --- pkg/controller/backup_deletion_controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index 6b4bb7818c..e36876f87a 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -125,12 +125,12 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { td := setupBackupDeletionControllerTest(t, defaultTestDbr(), location, backup) td.controller.backupStoreGetter = &fakeErrorBackupStoreGetter{} _, err := td.controller.Reconcile(ctx, td.req) - assert.Nil(t, err) + require.NoError(t, err) res := &velerov1api.DeleteBackupRequest{} err = td.fakeClient.Get(ctx, td.req.NamespacedName, res) require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) - assert.Equal(t, 1, len(res.Status.Errors)) + assert.Len(t, res.Status.Errors, 1) assert.True(t, strings.HasPrefix(res.Status.Errors[0], fmt.Sprintf("cannot delete backup because backup storage location %s is currently unavailable", location.Name))) }) From 8be8fc6671ff0b9cfce7fe4ba94185a85b262efe Mon Sep 17 00:00:00 2001 From: Anshul Ahuja Date: Mon, 2 Sep 2024 04:50:16 +0000 Subject: [PATCH 3/5] Add check for PV claimref nil Signed-off-by: Anshul Ahuja --- pkg/restore/restore.go | 4 ++-- pkg/restore/restore_test.go | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 63c26538ba..742ffb5928 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1952,8 +1952,8 @@ func hasCSIVolumeSnapshot(ctx *restoreContext, unstructuredPV *unstructured.Unst } for _, vs := range ctx.csiVolumeSnapshots { - // In some error cases, the VSs' source PVC could be nil. Skip them. - if vs.Spec.Source.PersistentVolumeClaimName == nil { + // In some error cases, the VSs' source PVC could be nil or PVs ClaimRef could be nil. Skip them. + if vs.Spec.Source.PersistentVolumeClaimName == nil || pv.Spec.ClaimRef == nil { continue } diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 8bf24d8457..4b25d295db 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -3963,11 +3963,33 @@ func TestHasCSIVolumeSnapshot(t *testing.T) { "namespace": "default", "name": "test", }, + "spec": map[string]interface{}{ + "claimRef": map[string]interface{}{ + "namespace": "velero", + "name": "test", + }, + }, }, }, vs: builder.ForVolumeSnapshot("velero", "test").Result(), expectedResult: false, }, + { + name: "PVs claimref is nil, expect false.", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "velero", + "name": "test", + }, + }, + }, + vs: builder.ForVolumeSnapshot("velero", "test").SourcePVC("test").Result(), + expectedResult: false, + }, + { name: "Find VS, expect true.", obj: &unstructured.Unstructured{ From 434bd2f3aee55c299d0baa94dd154dcb85e75980 Mon Sep 17 00:00:00 2001 From: Anshul Ahuja Date: Mon, 2 Sep 2024 05:08:59 +0000 Subject: [PATCH 4/5] linter Signed-off-by: Anshul Ahuja --- pkg/restore/restore.go | 2 +- pkg/restore/restore_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 742ffb5928..b78eb307e4 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1953,7 +1953,7 @@ func hasCSIVolumeSnapshot(ctx *restoreContext, unstructuredPV *unstructured.Unst for _, vs := range ctx.csiVolumeSnapshots { // In some error cases, the VSs' source PVC could be nil or PVs ClaimRef could be nil. Skip them. - if vs.Spec.Source.PersistentVolumeClaimName == nil || pv.Spec.ClaimRef == nil { + if vs.Spec.Source.PersistentVolumeClaimName == nil || pv.Spec.ClaimRef == nil { continue } diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index dfb7c0cc85..e7aa4fddb7 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -3989,7 +3989,7 @@ func TestHasCSIVolumeSnapshot(t *testing.T) { vs: builder.ForVolumeSnapshot("velero", "test").SourcePVC("test").Result(), expectedResult: false, }, - + { name: "Find VS, expect true.", obj: &unstructured.Unstructured{ From 2d3521a56cca299693bf8a2d7f76e113758aa68a Mon Sep 17 00:00:00 2001 From: Anshul Ahuja Date: Mon, 2 Sep 2024 09:18:44 +0000 Subject: [PATCH 5/5] fix Signed-off-by: Anshul Ahuja --- pkg/restore/restore.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index b78eb307e4..471f38119a 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1950,10 +1950,14 @@ func hasCSIVolumeSnapshot(ctx *restoreContext, unstructuredPV *unstructured.Unst ctx.log.WithError(err).Warnf("Unable to convert PV from unstructured to structured") return false } + // ignoring static PV cases where there is no claimRef + if pv.Spec.ClaimRef == nil { + return false + } for _, vs := range ctx.csiVolumeSnapshots { - // In some error cases, the VSs' source PVC could be nil or PVs ClaimRef could be nil. Skip them. - if vs.Spec.Source.PersistentVolumeClaimName == nil || pv.Spec.ClaimRef == nil { + // In some error cases, the VSs' source PVC could be nil. Skip them. + if vs.Spec.Source.PersistentVolumeClaimName == nil { continue }