Skip to content

Commit

Permalink
Merge pull request #7973 from Lyndon-Li/issue-fix-7972
Browse files Browse the repository at this point in the history 
Issue 7972: sync the backupPVC deletion in expose clean up
  • Loading branch information
@ywk253100
ywk253100 authored Jul 5, 2024
2 parents 920396d + 7408dbd commit 824bebb
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 73 deletions.
1 change: 1 addition & 0 deletions changelogs/unreleased/7973-Lyndon-Li
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix issue #7972, sync the backupPVC deletion in expose clean up
6 changes: 4 additions & 2 deletions pkg/exposer/csi_snapshot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}()

Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions pkg/exposer/generic_restore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}()

Expand Down Expand Up @@ -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 {
Expand Down
16 changes: 11 additions & 5 deletions pkg/util/kube/pvc_pv.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down
130 changes: 66 additions & 64 deletions pkg/util/kube/pvc_pv_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ func TestDeletePVCIfAny(t *testing.T) {
logMessage string
logLevel string
logError string
ensureTimeout time.Duration
}{
{
name: "pvc not found",
Expand Down Expand Up @@ -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,
Expand All @@ -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",
Expand All @@ -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",
},
{
Expand All @@ -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,
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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)
Expand Down Expand Up @@ -623,6 +591,7 @@ func TestEnsureDeletePVC(t *testing.T) {
pvcName string
namespace string
reactors []reactor
timeout time.Duration
err string
}{
{
Expand All @@ -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",
Expand All @@ -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",
},
}

Expand All @@ -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 {
Expand Down

0 comments on commit 824bebb

Please sign in to comment.