Issue 7068: add a finalizer to protect retained VSC

changelogs/unreleased/7114-Lyndon-Li

@@ -0,0 +1 @@
+Fix issue #7068, due to a 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

@@ -128,6 +128,13 @@ 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.RemoveVSCProtect(ctx, e.csiSnapshotClient, vsc.Name, csiExposeParam.Timeout)
+	if err != nil {
+		return errors.Wrap(err, "error to remove protect from volume snapshot content")
+	}
+
+	curLog.WithField("vsc name", vsc.Name).Infof("Removed protect from VSC")
+
 	err = csi.EnsureDeleteVSC(ctx, e.csiSnapshotClient, vsc.Name, csiExposeParam.Timeout)
 	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
@@ -169,11 +152,35 @@ func EnsureDeleteVS(ctx context.Context, snapshotClient snapshotter.SnapshotV1In
 	return nil
 }
 
+func RemoveVSCProtect(ctx context.Context, snapshotClient snapshotter.SnapshotV1Interface, vscName string, timeout time.Duration) error {
+	err := wait.PollImmediate(waitInternal, timeout, func() (bool, error) {
+		vsc, err := snapshotClient.VolumeSnapshotContents().Get(ctx, vscName, metav1.GetOptions{})
+		if err != nil {
+			return false, errors.Wrapf(err, "error to get VolumeSnapshotContent %s", vscName)
+		}
+
+		vsc.Finalizers = stringslice.Except(vsc.Finalizers, volumeSnapshotContentProtectFinalizer)
+
+		_, err = snapshotClient.VolumeSnapshotContents().Update(ctx, vsc, metav1.UpdateOptions{})
+		if err == nil {
+			return true, nil
+		}
+
+		if !apierrors.IsConflict(err) {
+			return false, errors.Wrapf(err, "error to update VolumeSnapshotContent %s", vscName)
+		}
+
+		return false, nil
+	})
+
+	return err
+}
+
 // 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{})
-	if err != nil {
+	if err != nil && !apierrors.IsNotFound(err) {
 		return errors.Wrap(err, "error to delete volume snapshot content")
 	}
 
@@ -208,3 +215,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

@@ -34,6 +34,8 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/util/stringptr"
 
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 )
 
 type reactor struct {
@@ -364,9 +366,23 @@ func TestEnsureDeleteVSC(t *testing.T) {
 		err       string
 	}{
 		{
-			name:    "delete fail",
+			name:    "delete fail on VSC not found",
 			vscName: "fake-vsc",
-			err:     "error to delete volume snapshot content: volumesnapshotcontents.snapshot.storage.k8s.io \"fake-vsc\" not found",
+		},
+		{
+			name:      "delete fail on others",
+			vscName:   "fake-vsc",
+			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",
@@ -399,7 +415,7 @@ func TestEnsureDeleteVSC(t *testing.T) {
 			}
 
 			err := EnsureDeleteVSC(context.Background(), fakeSnapshotClient.SnapshotV1(), test.vscName, time.Millisecond)
-			if err != nil {
+			if test.err != "" {
 				assert.EqualError(t, err, test.err)
 			} else {
 				assert.NoError(t, err)
@@ -601,7 +617,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,
@@ -634,3 +651,98 @@ func TestRetainVSC(t *testing.T) {
 		})
 	}
 }
+
+func TestRemoveVSCProtect(t *testing.T) {
+	vscObj := &snapshotv1api.VolumeSnapshotContent{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "fake-vsc",
+			Finalizers: []string{volumeSnapshotContentProtectFinalizer},
+		},
+	}
+
+	tests := []struct {
+		name      string
+		clientObj []runtime.Object
+		reactors  []reactor
+		vsc       string
+		updated   *snapshotv1api.VolumeSnapshotContent
+		timeout   time.Duration
+		err       string
+	}{
+		{
+			name: "get vsc error",
+			vsc:  "fake-vsc",
+			err:  "error to get VolumeSnapshotContent fake-vsc: volumesnapshotcontents.snapshot.storage.k8s.io \"fake-vsc\" not found",
+		},
+		{
+			name:      "update vsc fail",
+			vsc:       "fake-vsc",
+			clientObj: []runtime.Object{vscObj},
+			reactors: []reactor{
+				{
+					verb:     "update",
+					resource: "volumesnapshotcontents",
+					reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) {
+						return true, nil, errors.New("fake-update-error")
+					},
+				},
+			},
+			err: "error to update VolumeSnapshotContent fake-vsc: fake-update-error",
+		},
+		{
+			name:      "update vsc timeout",
+			vsc:       "fake-vsc",
+			clientObj: []runtime.Object{vscObj},
+			reactors: []reactor{
+				{
+					verb:     "update",
+					resource: "volumesnapshotcontents",
+					reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) {
+						return true, nil, &apierrors.StatusError{ErrStatus: metav1.Status{
+							Reason: metav1.StatusReasonConflict,
+						}}
+					},
+				},
+			},
+			timeout: time.Second,
+			err:     "timed out waiting for the condition",
+		},
+		{
+			name:      "succeed",
+			vsc:       "fake-vsc",
+			clientObj: []runtime.Object{vscObj},
+			timeout:   time.Second,
+			updated: &snapshotv1api.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "fake-vsc",
+					Finalizers: []string{},
+				},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			fakeSnapshotClient := snapshotFake.NewSimpleClientset(test.clientObj...)
+
+			for _, reactor := range test.reactors {
+				fakeSnapshotClient.Fake.PrependReactor(reactor.verb, reactor.resource, reactor.reactorFunc)
+			}
+
+			err := RemoveVSCProtect(context.Background(), fakeSnapshotClient.SnapshotV1(), test.vsc, test.timeout)
+
+			if len(test.err) == 0 {
+				assert.NoError(t, err)
+			} else {
+				assert.EqualError(t, err, test.err)
+			}
+
+			if test.updated != nil {
+				updated, err := fakeSnapshotClient.SnapshotV1().VolumeSnapshotContents().Get(context.Background(), test.vsc, metav1.GetOptions{})
+				assert.NoError(t, err)
+
+				assert.Equal(t, test.updated.Finalizers, updated.Finalizers)
+			}
+		})
+	}
+}