diff --git a/changelogs/unreleased/7114-Lyndon-Li b/changelogs/unreleased/7114-Lyndon-Li new file mode 100644 index 000000000..4b5b81ffd --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 8b501e648..496119245 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/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") diff --git a/pkg/util/csi/volume_snapshot.go b/pkg/util/csi/volume_snapshot.go index 99845d78f..a8ade7acd 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 @@ -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 +} diff --git a/pkg/util/csi/volume_snapshot_test.go b/pkg/util/csi/volume_snapshot_test.go index 43ed9a316..4fbac34f5 100644 --- a/pkg/util/csi/volume_snapshot_test.go +++ b/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) + } + }) + } +}