diff --git a/changelogs/unreleased/7973-Lyndon-Li b/changelogs/unreleased/7973-Lyndon-Li new file mode 100644 index 000000000..da6bb1d17 --- /dev/null +++ b/changelogs/unreleased/7973-Lyndon-Li @@ -0,0 +1 @@ +Fix issue #7972, sync the backupPVC deletion in expose clean up \ No newline at end of file diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 7a37b794c..26cf66ade 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -170,7 +170,7 @@ 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.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVC.Name, backupPVC.Namespace, curLog) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVC.Name, backupPVC.Namespace, 0, curLog) } }() @@ -264,13 +264,15 @@ func (e *csiSnapshotExposer) PeekExposed(ctx context.Context, ownerObject corev1 return nil } +const cleanUpTimeout = time.Minute + func (e *csiSnapshotExposer) CleanUp(ctx context.Context, ownerObject corev1.ObjectReference, vsName string, sourceNamespace string) { backupPodName := ownerObject.Name backupPVCName := ownerObject.Name backupVSName := ownerObject.Name kube.DeletePodIfAny(ctx, e.kubeClient.CoreV1(), backupPodName, ownerObject.Namespace, e.log) - kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVCName, ownerObject.Namespace, e.log) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVCName, ownerObject.Namespace, cleanUpTimeout, e.log) csi.DeleteVolumeSnapshotIfAny(ctx, e.csiSnapshotClient, backupVSName, ownerObject.Namespace, e.log) csi.DeleteVolumeSnapshotIfAny(ctx, e.csiSnapshotClient, vsName, sourceNamespace, e.log) diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index a1ebb7245..70dfb537b 100644 --- a/pkg/exposer/generic_restore.go +++ b/pkg/exposer/generic_restore.go @@ -109,7 +109,7 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1.O defer func() { if err != nil { - kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, curLog) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, 0, curLog) } }() @@ -194,7 +194,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.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVCName, ownerObject.Namespace, e.log) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVCName, ownerObject.Namespace, 0, 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 12daa0e3e..d44150968 100644 --- a/pkg/util/kube/pvc_pv.go +++ b/pkg/util/kube/pvc_pv.go @@ -41,9 +41,10 @@ const ( waitInternal = 2 * time.Second ) -// 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) { +// DeletePVAndPVCIfAny deletes PVC and delete the bound PV if it exists and log an error when the deletion fails. +// It first sets the reclaim policy of the PV to Delete, then PV will be deleted automatically when PVC is deleted. +// If ensureTimeout is not 0, it waits until the PVC is deleted or timeout. +func DeletePVAndPVCIfAny(ctx context.Context, client corev1client.CoreV1Interface, pvcName, pvcNamespace string, ensureTimeout time.Duration, log logrus.FieldLogger) { pvcObj, err := client.PersistentVolumeClaims(pvcNamespace).Get(ctx, pvcName, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { @@ -69,7 +70,7 @@ func DeletePVAndPVCIfAny(ctx context.Context, client corev1client.CoreV1Interfac } } - if err := client.PersistentVolumeClaims(pvcNamespace).Delete(ctx, pvcName, metav1.DeleteOptions{}); err != nil { + if err := EnsureDeletePVC(ctx, client, pvcName, pvcNamespace, ensureTimeout); err != nil { log.Warnf("failed to delete pvc %s/%s with err %v", pvcNamespace, pvcName, err) } } @@ -118,12 +119,17 @@ func DeletePVIfAny(ctx context.Context, pvGetter corev1client.CoreV1Interface, p } // EnsureDeletePVC asserts the existence of a PVC by name, deletes it and waits for its disappearance and returns errors on any failure +// If timeout is 0, it doesn't wait and return nil func EnsureDeletePVC(ctx context.Context, pvcGetter corev1client.CoreV1Interface, pvc string, namespace string, timeout time.Duration) error { err := pvcGetter.PersistentVolumeClaims(namespace).Delete(ctx, pvc, metav1.DeleteOptions{}) if err != nil { return errors.Wrapf(err, "error to delete pvc %s", pvc) } + if timeout == 0 { + return nil + } + err = wait.PollUntilContextTimeout(ctx, waitInternal, timeout, true, func(ctx context.Context) (bool, error) { _, err := pvcGetter.PersistentVolumeClaims(namespace).Get(ctx, pvc, metav1.GetOptions{}) if err != nil { @@ -138,7 +144,7 @@ func EnsureDeletePVC(ctx context.Context, pvcGetter corev1client.CoreV1Interface }) if err != nil { - return errors.Wrapf(err, "error to retrieve pvc info for %s", pvc) + return errors.Wrapf(err, "error to ensure pvc deleted for %s", pvc) } return nil diff --git a/pkg/util/kube/pvc_pv_test.go b/pkg/util/kube/pvc_pv_test.go index 113c28e23..d17423857 100644 --- a/pkg/util/kube/pvc_pv_test.go +++ b/pkg/util/kube/pvc_pv_test.go @@ -333,6 +333,7 @@ func TestDeletePVCIfAny(t *testing.T) { logMessage string logLevel string logError string + ensureTimeout time.Duration }{ { name: "pvc not found", @@ -362,22 +363,6 @@ func TestDeletePVCIfAny(t *testing.T) { 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, @@ -391,13 +376,6 @@ func TestDeletePVCIfAny(t *testing.T) { 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", @@ -407,10 +385,10 @@ func TestDeletePVCIfAny(t *testing.T) { }, }, kubeClientObj: []runtime.Object{ - pvcObject, + pvcWithVolume, pvObject, }, - logMessage: "failed to delete pvc fake-namespace/fake-pvc with err fake-delete-error", + logMessage: "failed to delete pvc fake-namespace/fake-pvc with err error to delete pvc fake-pvc: fake-delete-error", logLevel: "level=warning", }, { @@ -426,13 +404,6 @@ func TestDeletePVCIfAny(t *testing.T) { 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, @@ -451,20 +422,6 @@ func TestDeletePVCIfAny(t *testing.T) { 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", @@ -486,28 +443,39 @@ func TestDeletePVCIfAny(t *testing.T) { pvcWithVolume, pvObject, }, + }, + { + name: "delete pv pvc success but wait fail", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeClientObj: []runtime.Object{ + pvcWithVolume, + pvObject, + }, kubeReactors: []reactor{ { - verb: "get", + verb: "delete", 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 - }, - }, + }, + ensureTimeout: time.Second, + logMessage: "failed to delete pvc fake-namespace/fake-pvc with err error to ensure pvc deleted for fake-pvc: context deadline exceeded", + logLevel: "level=warning", + }, + { + name: "delete pv pvc success, wait won't succeed but ensureTimeout is 0", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeClientObj: []runtime.Object{ + pvcWithVolume, + pvObject, + }, + kubeReactors: []reactor{ { verb: "delete", resource: "persistentvolumeclaims", @@ -530,7 +498,7 @@ func TestDeletePVCIfAny(t *testing.T) { var kubeClient kubernetes.Interface = fakeKubeClient logMessage := "" - DeletePVAndPVCIfAny(context.Background(), kubeClient.CoreV1(), test.pvcName, test.pvcNamespace, velerotest.NewSingleLogger(&logMessage)) + DeletePVAndPVCIfAny(context.Background(), kubeClient.CoreV1(), test.pvcName, test.pvcNamespace, test.ensureTimeout, velerotest.NewSingleLogger(&logMessage)) if len(test.logMessage) > 0 { assert.Contains(t, logMessage, test.logMessage) @@ -623,6 +591,7 @@ func TestEnsureDeletePVC(t *testing.T) { pvcName string namespace string reactors []reactor + timeout time.Duration err string }{ { @@ -631,11 +600,27 @@ func TestEnsureDeletePVC(t *testing.T) { namespace: "fake-ns", err: "error to delete pvc fake-pvc: persistentvolumeclaims \"fake-pvc\" not found", }, + { + name: "0 timeout", + pvcName: "fake-pvc", + namespace: "fake-ns", + clientObj: []runtime.Object{pvcObject}, + reactors: []reactor{ + { + verb: "delete", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcObject, nil + }, + }, + }, + }, { name: "wait fail", pvcName: "fake-pvc", namespace: "fake-ns", clientObj: []runtime.Object{pvcObject}, + timeout: time.Millisecond, reactors: []reactor{ { verb: "get", @@ -645,7 +630,24 @@ func TestEnsureDeletePVC(t *testing.T) { }, }, }, - err: "error to retrieve pvc info for fake-pvc: error to get pvc fake-pvc: fake-get-error", + err: "error to ensure pvc deleted for fake-pvc: error to get pvc fake-pvc: fake-get-error", + }, + { + name: "wait timeout", + pvcName: "fake-pvc", + namespace: "fake-ns", + clientObj: []runtime.Object{pvcObject}, + timeout: time.Millisecond, + reactors: []reactor{ + { + verb: "delete", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcObject, nil + }, + }, + }, + err: "error to ensure pvc deleted for fake-pvc: context deadline exceeded", }, } @@ -659,7 +661,7 @@ func TestEnsureDeletePVC(t *testing.T) { var kubeClient kubernetes.Interface = fakeKubeClient - err := EnsureDeletePVC(context.Background(), kubeClient.CoreV1(), test.pvcName, test.namespace, time.Millisecond) + err := EnsureDeletePVC(context.Background(), kubeClient.CoreV1(), test.pvcName, test.namespace, test.timeout) if err != nil { assert.EqualError(t, err, test.err) } else {