From cb651d0436d95c8bad165eee417390487c40dd73 Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Mon, 13 Nov 2023 17:37:18 +0800 Subject: [PATCH] issue 7068: add a finalizer to protect retained VSC Signed-off-by: Lyndon-Li --- changelogs/unreleased/7095-Lyndon-Li | 1 + pkg/exposer/csi_snapshot.go | 4 +- pkg/util/csi/volume_snapshot.go | 78 +++++++++++++++++----------- pkg/util/csi/volume_snapshot_test.go | 41 ++++++++++++--- 4 files changed, 85 insertions(+), 39 deletions(-) create mode 100644 changelogs/unreleased/7095-Lyndon-Li diff --git a/changelogs/unreleased/7095-Lyndon-Li b/changelogs/unreleased/7095-Lyndon-Li new file mode 100644 index 0000000000..e3a11801dd --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index f54d7632da..9fb25d70d7 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/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") } diff --git a/pkg/util/csi/volume_snapshot.go b/pkg/util/csi/volume_snapshot.go index 99845d78f8..1440ab6e11 100644 --- a/pkg/util/csi/volume_snapshot.go +++ b/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 +} diff --git a/pkg/util/csi/volume_snapshot_test.go b/pkg/util/csi/volume_snapshot_test.go index 43ed9a3162..9e4ecec36d 100644 --- a/pkg/util/csi/volume_snapshot_test.go +++ b/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,