Merge pull request #7095 from Lyndon-Li/issue-fix-7068

Issue 7068: add a finalizer to protect retained VSC

changelogs/unreleased/7095-Lyndon-Li

@@ -0,0 +1 @@
+Fix issue #7068, due to an behavior of CSI external snapshotter, manipulations of VS and VSC may not be handled in the same order inside external snapshotter as the API is called. So add a protection finalizer to ensure the order

pkg/exposer/csi_snapshot.go

@@ -119,6 +119,8 @@ func (e *csiSnapshotExposer) Expose(ctx context.Context, ownerObject corev1.Obje
 		return errors.Wrap(err, "error to retain volume snapshot content")
 	}
 
+	vsc = retained
+
 	curLog.WithField("vsc name", vsc.Name).WithField("retained", (retained != nil)).Info("Finished to retain VSC")
 
 	defer func() {
@@ -134,7 +136,7 @@ func (e *csiSnapshotExposer) Expose(ctx context.Context, ownerObject corev1.Obje
 
 	curLog.WithField("vs name", volumeSnapshot.Name).Infof("VS is deleted in namespace %s", volumeSnapshot.Namespace)
 
-	err = csi.EnsureDeleteVSC(ctx, e.csiSnapshotClient, vsc.Name, csiExposeParam.OperationTimeout)
+	err = csi.EnsureDeleteVSC(ctx, e.csiSnapshotClient, vsc, csiExposeParam.OperationTimeout)
 	if err != nil {
 		return errors.Wrap(err, "error to delete volume snapshot content")
 	}

pkg/util/csi/volume_snapshot.go

@@ -31,6 +31,7 @@ import (
 
 	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	"github.com/vmware-tanzu/velero/pkg/util/stringptr"
+	"github.com/vmware-tanzu/velero/pkg/util/stringslice"
 
 	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
 	snapshotter "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/typed/volumesnapshot/v1"
@@ -41,7 +42,8 @@ import (
 )
 
 const (
-	waitInternal = 2 * time.Second
+	waitInternal                          = 2 * time.Second
+	volumeSnapshotContentProtectFinalizer = "velero.io/volume-snapshot-content-protect-finalizer"
 )
 
 // WaitVolumeSnapshotReady waits a VS to become ready to use until the timeout reaches
@@ -97,36 +99,17 @@ func GetVolumeSnapshotContentForVolumeSnapshot(volSnap *snapshotv1api.VolumeSnap
 	return vsc, nil
 }
 
-// RetainVSC updates the VSC's deletion policy to Retain and return the update VSC
+// RetainVSC updates the VSC's deletion policy to Retain and add a finalier and then return the update VSC
 func RetainVSC(ctx context.Context, snapshotClient snapshotter.SnapshotV1Interface,
 	vsc *snapshotv1api.VolumeSnapshotContent) (*snapshotv1api.VolumeSnapshotContent, error) {
 	if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentRetain {
 		return vsc, nil
 	}
-	origBytes, err := json.Marshal(vsc)
-	if err != nil {
-		return nil, errors.Wrap(err, "error marshaling original VSC")
-	}
-
-	updated := vsc.DeepCopy()
-	updated.Spec.DeletionPolicy = snapshotv1api.VolumeSnapshotContentRetain
-
-	updatedBytes, err := json.Marshal(updated)
-	if err != nil {
-		return nil, errors.Wrap(err, "error marshaling updated VSC")
-	}
-
-	patchBytes, err := jsonpatch.CreateMergePatch(origBytes, updatedBytes)
-	if err != nil {
-		return nil, errors.Wrap(err, "error creating json merge patch for VSC")
-	}
-
-	retained, err := snapshotClient.VolumeSnapshotContents().Patch(ctx, vsc.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{})
-	if err != nil {
-		return nil, errors.Wrap(err, "error patching VSC")
-	}
 
-	return retained, nil
+	return patchVSC(ctx, snapshotClient, vsc, func(updated *snapshotv1api.VolumeSnapshotContent) {
+		updated.Spec.DeletionPolicy = snapshotv1api.VolumeSnapshotContentRetain
+		updated.Finalizers = append(updated.Finalizers, volumeSnapshotContentProtectFinalizer)
+	})
 }
 
 // DeleteVolumeSnapshotContentIfAny deletes a VSC by name if it exists, and log an error when the deletion fails
@@ -171,27 +154,34 @@ func EnsureDeleteVS(ctx context.Context, snapshotClient snapshotter.SnapshotV1In
 
 // EnsureDeleteVSC asserts the existence of a VSC by name, deletes it and waits for its disappearance and returns errors on any failure
 func EnsureDeleteVSC(ctx context.Context, snapshotClient snapshotter.SnapshotV1Interface,
-	vscName string, timeout time.Duration) error {
-	err := snapshotClient.VolumeSnapshotContents().Delete(ctx, vscName, metav1.DeleteOptions{})
+	vsc *snapshotv1api.VolumeSnapshotContent, timeout time.Duration) error {
+	_, err := patchVSC(ctx, snapshotClient, vsc, func(updated *snapshotv1api.VolumeSnapshotContent) {
+		updated.Finalizers = stringslice.Except(updated.Finalizers, volumeSnapshotContentProtectFinalizer)
+	})
 	if err != nil {
+		return errors.Wrapf(err, "error to remove protect finalizer from vsc %s", vsc.Name)
+	}
+
+	err = snapshotClient.VolumeSnapshotContents().Delete(ctx, vsc.Name, metav1.DeleteOptions{})
+	if err != nil && !apierrors.IsNotFound(err) {
 		return errors.Wrap(err, "error to delete volume snapshot content")
 	}
 
 	err = wait.PollImmediate(waitInternal, timeout, func() (bool, error) {
-		_, err := snapshotClient.VolumeSnapshotContents().Get(ctx, vscName, metav1.GetOptions{})
+		_, err := snapshotClient.VolumeSnapshotContents().Get(ctx, vsc.Name, metav1.GetOptions{})
 		if err != nil {
 			if apierrors.IsNotFound(err) {
 				return true, nil
 			}
 
-			return false, errors.Wrapf(err, fmt.Sprintf("error to get VolumeSnapshotContent %s", vscName))
+			return false, errors.Wrapf(err, fmt.Sprintf("error to get VolumeSnapshotContent %s", vsc.Name))
 		}
 
 		return false, nil
 	})
 
 	if err != nil {
-		return errors.Wrapf(err, "error to assure VolumeSnapshotContent is deleted, %s", vscName)
+		return errors.Wrapf(err, "error to assure VolumeSnapshotContent is deleted, %s", vsc.Name)
 	}
 
 	return nil
@@ -208,3 +198,31 @@ func DeleteVolumeSnapshotIfAny(ctx context.Context, snapshotClient snapshotter.S
 		}
 	}
 }
+
+func patchVSC(ctx context.Context, snapshotClient snapshotter.SnapshotV1Interface,
+	vsc *snapshotv1api.VolumeSnapshotContent, updateFunc func(*snapshotv1api.VolumeSnapshotContent)) (*snapshotv1api.VolumeSnapshotContent, error) {
+	origBytes, err := json.Marshal(vsc)
+	if err != nil {
+		return nil, errors.Wrap(err, "error marshaling original VSC")
+	}
+
+	updated := vsc.DeepCopy()
+	updateFunc(updated)
+
+	updatedBytes, err := json.Marshal(updated)
+	if err != nil {
+		return nil, errors.Wrap(err, "error marshaling updated VSC")
+	}
+
+	patchBytes, err := jsonpatch.CreateMergePatch(origBytes, updatedBytes)
+	if err != nil {
+		return nil, errors.Wrap(err, "error creating json merge patch for VSC")
+	}
+
+	patched, err := snapshotClient.VolumeSnapshotContents().Patch(ctx, vsc.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{})
+	if err != nil {
+		return nil, errors.Wrap(err, "error patching VSC")
+	}
+
+	return patched, nil
+}

pkg/util/csi/volume_snapshot_test.go

@@ -360,17 +360,41 @@ func TestEnsureDeleteVSC(t *testing.T) {
 		name      string
 		clientObj []runtime.Object
 		reactors  []reactor
-		vscName   string
+		vscObj    *snapshotv1api.VolumeSnapshotContent
 		err       string
 	}{
 		{
-			name:    "delete fail",
-			vscName: "fake-vsc",
-			err:     "error to delete volume snapshot content: volumesnapshotcontents.snapshot.storage.k8s.io \"fake-vsc\" not found",
+			name:   "remove finalizer fail",
+			vscObj: vscObj,
+			reactors: []reactor{
+				{
+					verb:     "patch",
+					resource: "volumesnapshotcontents",
+					reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) {
+						return true, nil, errors.New("fake-patch-error")
+					},
+				},
+			},
+			err: "error to remove protect finalizer from vsc fake-vsc: error patching VSC: fake-patch-error",
+		},
+		{
+			name:      "delete fail",
+			vscObj:    vscObj,
+			clientObj: []runtime.Object{vscObj},
+			reactors: []reactor{
+				{
+					verb:     "delete",
+					resource: "volumesnapshotcontents",
+					reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) {
+						return true, nil, errors.New("fake-delete-error")
+					},
+				},
+			},
+			err: "error to delete volume snapshot content: fake-delete-error",
 		},
 		{
 			name:      "wait fail",
-			vscName:   "fake-vsc",
+			vscObj:    vscObj,
 			clientObj: []runtime.Object{vscObj},
 			reactors: []reactor{
 				{
@@ -385,7 +409,7 @@ func TestEnsureDeleteVSC(t *testing.T) {
 		},
 		{
 			name:      "success",
-			vscName:   "fake-vsc",
+			vscObj:    vscObj,
 			clientObj: []runtime.Object{vscObj},
 		},
 	}
@@ -398,7 +422,7 @@ func TestEnsureDeleteVSC(t *testing.T) {
 				fakeSnapshotClient.Fake.PrependReactor(reactor.verb, reactor.resource, reactor.reactorFunc)
 			}
 
-			err := EnsureDeleteVSC(context.Background(), fakeSnapshotClient.SnapshotV1(), test.vscName, time.Millisecond)
+			err := EnsureDeleteVSC(context.Background(), fakeSnapshotClient.SnapshotV1(), test.vscObj, time.Millisecond)
 			if err != nil {
 				assert.EqualError(t, err, test.err)
 			} else {
@@ -601,7 +625,8 @@ func TestRetainVSC(t *testing.T) {
 			clientObj: []runtime.Object{vscObj},
 			updated: &snapshotv1api.VolumeSnapshotContent{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "fake-vsc",
+					Name:       "fake-vsc",
+					Finalizers: []string{volumeSnapshotContentProtectFinalizer},
 				},
 				Spec: snapshotv1api.VolumeSnapshotContentSpec{
 					DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain,