From 92fdf407c78bb38c98603cee3d1d9953dbcf318d Mon Sep 17 00:00:00 2001 From: Ming Qiu Date: Wed, 3 Jan 2024 05:54:04 +0000 Subject: [PATCH] Fix intermediate pv delete for data mover Signed-off-by: Ming Qiu --- pkg/exposer/csi_snapshot.go | 12 +- pkg/exposer/generic_restore.go | 4 +- pkg/util/kube/pvc_pv.go | 31 ++++- pkg/util/kube/pvc_pv_test.go | 207 +++++++++++++++++++++++++++++++-- 4 files changed, 234 insertions(+), 20 deletions(-) diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 9979c5fdd..7113f2fe8 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -183,10 +183,9 @@ func (e *csiSnapshotExposer) Expose(ctx context.Context, ownerObject corev1.Obje } curLog.WithField("pvc name", backupPVC.Name).Info("Backup PVC is created") - defer func() { if err != nil { - kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVC.Name, backupPVC.Namespace, curLog) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVC.Name, backupPVC.Namespace, curLog) } }() @@ -262,7 +261,8 @@ func (e *csiSnapshotExposer) CleanUp(ctx context.Context, ownerObject corev1.Obj backupVSName := ownerObject.Name kube.DeletePodIfAny(ctx, e.kubeClient.CoreV1(), backupPodName, ownerObject.Namespace, e.log) - kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVCName, ownerObject.Namespace, e.log) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVCName, ownerObject.Namespace, e.log) + csi.DeleteVolumeSnapshotIfAny(ctx, e.csiSnapshotClient, backupVSName, ownerObject.Namespace, e.log) csi.DeleteVolumeSnapshotIfAny(ctx, e.csiSnapshotClient, vsName, sourceNamespace, e.log) } @@ -329,8 +329,8 @@ func (e *csiSnapshotExposer) createBackupVSC(ctx context.Context, ownerObject co return e.csiSnapshotClient.VolumeSnapshotContents().Create(ctx, vsc, metav1.CreateOptions{}) } -func (e *csiSnapshotExposer) createBackupPVC(ctx context.Context, ownerObject corev1.ObjectReference, backupVS string, storageClass string, accessMode string, resource resource.Quantity) (*corev1.PersistentVolumeClaim, error) { - backupVCName := ownerObject.Name +func (e *csiSnapshotExposer) createBackupPVC(ctx context.Context, ownerObject corev1.ObjectReference, backupVS, storageClass, accessMode string, resource resource.Quantity) (*corev1.PersistentVolumeClaim, error) { + backupPVCName := ownerObject.Name volumeMode, err := getVolumeModeByAccessMode(accessMode) if err != nil { @@ -346,7 +346,7 @@ func (e *csiSnapshotExposer) createBackupPVC(ctx context.Context, ownerObject co pvc := &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Namespace: ownerObject.Namespace, - Name: backupVCName, + Name: backupPVCName, OwnerReferences: []metav1.OwnerReference{ { APIVersion: ownerObject.APIVersion, diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index de628533c..c73e4a4c3 100644 --- a/pkg/exposer/generic_restore.go +++ b/pkg/exposer/generic_restore.go @@ -104,7 +104,7 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1.O defer func() { if err != nil { - kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, curLog) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, curLog) } }() @@ -165,7 +165,7 @@ func (e *genericRestoreExposer) CleanUp(ctx context.Context, ownerObject corev1. restorePVCName := ownerObject.Name kube.DeletePodIfAny(ctx, e.kubeClient.CoreV1(), restorePodName, ownerObject.Namespace, e.log) - kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVCName, ownerObject.Namespace, e.log) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVCName, ownerObject.Namespace, e.log) } func (e *genericRestoreExposer) RebindVolume(ctx context.Context, ownerObject corev1.ObjectReference, targetPVCName string, sourceNamespace string, timeout time.Duration) error { diff --git a/pkg/util/kube/pvc_pv.go b/pkg/util/kube/pvc_pv.go index 1a393b3ab..6954db769 100644 --- a/pkg/util/kube/pvc_pv.go +++ b/pkg/util/kube/pvc_pv.go @@ -41,16 +41,37 @@ const ( waitInternal = 2 * time.Second ) -// DeletePVCIfAny deletes a PVC by name if it exists, and log an error when the deletion fails -func DeletePVCIfAny(ctx context.Context, pvcGetter corev1client.CoreV1Interface, pvcName string, pvcNamespace string, log logrus.FieldLogger) { - err := pvcGetter.PersistentVolumeClaims(pvcNamespace).Delete(ctx, pvcName, metav1.DeleteOptions{}) +// DeletePVAndPVCIfAny deletes PVC and delete the bound PV if it exists and log an error when the deletion fails +// We first set the reclaim policy of the PV to Delete, then PV will be deleted automatically when PVC is deleted. +func DeletePVAndPVCIfAny(ctx context.Context, client corev1client.CoreV1Interface, pvcName, pvcNamespace string, log logrus.FieldLogger) { + pvcObj, err := client.PersistentVolumeClaims(pvcNamespace).Get(ctx, pvcName, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { - log.WithError(err).Debugf("Abort deleting PVC, it doesn't exist, %s/%s", pvcNamespace, pvcName) + log.WithError(err).Debugf("Abort deleting PV and PVC, for related PVC doesn't exist, %s/%s", pvcNamespace, pvcName) + return } else { - log.WithError(err).Errorf("Failed to delete pvc %s/%s", pvcNamespace, pvcName) + log.Warnf("failed to get pvc %s/%s with err %v", pvcNamespace, pvcName, err) + return } } + + if pvcObj.Spec.VolumeName == "" { + log.Warnf("failed to delete PV, for related PVC %s/%s has no bind volume name", pvcNamespace, pvcName) + } else { + pvObj, err := client.PersistentVolumes().Get(ctx, pvcObj.Spec.VolumeName, metav1.GetOptions{}) + if err != nil { + log.Warnf("failed to delete PV %s with err %v", pvcObj.Spec.VolumeName, err) + } else { + _, err = SetPVReclaimPolicy(ctx, client, pvObj, corev1api.PersistentVolumeReclaimDelete) + if err != nil { + log.Warnf("failed to set reclaim policy of PV %s to delete with err %v", pvObj.Name, err) + } + } + } + + if err := client.PersistentVolumeClaims(pvcNamespace).Delete(ctx, pvcName, metav1.DeleteOptions{}); err != nil { + log.Warnf("failed to delete pvc %s/%s with err %v", pvcNamespace, pvcName, err) + } } // WaitPVCBound wait for binding of a PVC specified by name and returns the bound PV object diff --git a/pkg/util/kube/pvc_pv_test.go b/pkg/util/kube/pvc_pv_test.go index 67edca6ee..f6950af0a 100644 --- a/pkg/util/kube/pvc_pv_test.go +++ b/pkg/util/kube/pvc_pv_test.go @@ -289,10 +289,44 @@ func TestWaitPVCConsumed(t *testing.T) { } func TestDeletePVCIfAny(t *testing.T) { + pvObject := &corev1api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-pv", + Annotations: map[string]string{ + KubeAnnBoundByController: "true", + }, + }, + Spec: corev1api.PersistentVolumeSpec{ + ClaimRef: &corev1api.ObjectReference{ + Kind: "fake-kind", + Namespace: "fake-ns", + Name: "fake-pvc", + }, + }, + } + + pvcObject := &corev1api.PersistentVolumeClaim{ + TypeMeta: metav1.TypeMeta{ + Kind: "fake-kind-1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "fake-namespace", + Name: "fake-pvc", + Annotations: map[string]string{ + KubeAnnBindCompleted: "true", + KubeAnnBoundByController: "true", + }, + }, + } + + pvcWithVolume := pvcObject.DeepCopy() + pvcWithVolume.Spec.VolumeName = "fake-pv" + tests := []struct { name string pvcName string pvcNamespace string + pvName string kubeClientObj []runtime.Object kubeReactors []reactor logMessage string @@ -300,17 +334,69 @@ func TestDeletePVCIfAny(t *testing.T) { logError string }{ { - name: "get fail", + name: "pvc not found", pvcName: "fake-pvc", pvcNamespace: "fake-namespace", - logMessage: "Abort deleting PVC, it doesn't exist, fake-namespace/fake-pvc", + logMessage: "Abort deleting PV and PVC, for related PVC doesn't exist, fake-namespace/fake-pvc", logLevel: "level=debug", }, { - name: "delete fail", + name: "failed to get pvc", pvcName: "fake-pvc", pvcNamespace: "fake-namespace", kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake-get-error") + }, + }, + }, + logMessage: "failed to get pvc fake-namespace/fake-pvc with err fake-get-error", + logLevel: "level=warning", + }, + { + name: "pvc has no volume name", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcObject, nil + }, + }, + { + verb: "delete", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, nil + }, + }, + }, + kubeClientObj: []runtime.Object{ + pvcObject, + pvObject, + }, + logMessage: "failed to delete PV, for related PVC fake-namespace/fake-pvc has no bind volume name", + logLevel: "level=warning", + }, + { + name: "failed to delete pvc", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcObject, nil + }, + }, { verb: "delete", resource: "persistentvolumeclaims", @@ -319,9 +405,116 @@ func TestDeletePVCIfAny(t *testing.T) { }, }, }, - logMessage: "Failed to delete pvc fake-namespace/fake-pvc", - logLevel: "level=error", - logError: "error=fake-delete-error", + kubeClientObj: []runtime.Object{ + pvcObject, + pvObject, + }, + logMessage: "failed to delete pvc fake-namespace/fake-pvc with err fake-delete-error", + logLevel: "level=warning", + }, + { + name: "failed to get pv", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake-get-error") + }, + }, + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcWithVolume, nil + }, + }, + }, + kubeClientObj: []runtime.Object{ + pvcWithVolume, + pvObject, + }, + logMessage: "failed to delete PV fake-pv with err fake-get-error", + logLevel: "level=warning", + }, + { + name: "set reclaim policy fail", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeClientObj: []runtime.Object{ + pvcWithVolume, + pvObject, + }, + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcWithVolume, nil + }, + }, + { + verb: "get", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvObject, nil + }, + }, + { + verb: "patch", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvObject, errors.New("fake-patch-error") + }, + }, + }, + logMessage: "failed to set reclaim policy of PV fake-pv to delete with err error patching PV: fake-patch-error", + logLevel: "level=warning", + logError: "fake-patch-error", + }, + { + name: "delete pv pvc success", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeClientObj: []runtime.Object{ + pvcWithVolume, + pvObject, + }, + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcWithVolume, nil + }, + }, + { + verb: "get", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvObject, nil + }, + }, + { + verb: "patch", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvObject, nil + }, + }, + { + verb: "delete", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcWithVolume, nil + }, + }, + }, }, } @@ -336,7 +529,7 @@ func TestDeletePVCIfAny(t *testing.T) { var kubeClient kubernetes.Interface = fakeKubeClient logMessage := "" - DeletePVCIfAny(context.Background(), kubeClient.CoreV1(), test.pvcName, test.pvcNamespace, velerotest.NewSingleLogger(&logMessage)) + DeletePVAndPVCIfAny(context.Background(), kubeClient.CoreV1(), test.pvcName, test.pvcNamespace, velerotest.NewSingleLogger(&logMessage)) if len(test.logMessage) > 0 { assert.Contains(t, logMessage, test.logMessage)