diff --git a/internal/delete/actions/csi/volumesnapshot_action.go b/internal/delete/actions/csi/volumesnapshot_action.go deleted file mode 100644 index 73f60b06e..000000000 --- a/internal/delete/actions/csi/volumesnapshot_action.go +++ /dev/null @@ -1,120 +0,0 @@ -/* -Copyright the Velero contributors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package csi - -import ( - "context" - "fmt" - - snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" - "github.com/pkg/errors" - "github.com/sirupsen/logrus" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" - crclient "sigs.k8s.io/controller-runtime/pkg/client" - - "github.com/vmware-tanzu/velero/pkg/client" - plugincommon "github.com/vmware-tanzu/velero/pkg/plugin/framework/common" - "github.com/vmware-tanzu/velero/pkg/plugin/velero" - "github.com/vmware-tanzu/velero/pkg/util/csi" - kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" -) - -// volumeSnapshotDeleteItemAction is a backup item action plugin for Velero. -type volumeSnapshotDeleteItemAction struct { - log logrus.FieldLogger - crClient crclient.Client -} - -// AppliesTo returns information indicating that the -// VolumeSnapshotBackupItemAction should be invoked to backup -// VolumeSnapshots. -func (p *volumeSnapshotDeleteItemAction) AppliesTo() (velero.ResourceSelector, error) { - p.log.Debug("VolumeSnapshotBackupItemAction AppliesTo") - - return velero.ResourceSelector{ - IncludedResources: []string{"volumesnapshots.snapshot.storage.k8s.io"}, - }, nil -} - -func (p *volumeSnapshotDeleteItemAction) Execute( - input *velero.DeleteItemActionExecuteInput, -) error { - p.log.Info("Starting VolumeSnapshotDeleteItemAction for volumeSnapshot") - - var vs snapshotv1api.VolumeSnapshot - - if err := runtime.DefaultUnstructuredConverter.FromUnstructured( - input.Item.UnstructuredContent(), - &vs, - ); err != nil { - return errors.Wrapf(err, "failed to convert input.Item from unstructured") - } - - // We don't want this DeleteItemAction plugin to delete VolumeSnapshot - // taken outside of Velero. So skip deleting VolumeSnapshot objects - // that were not created in the process of creating the Velero - // backup being deleted. - if !kubeutil.HasBackupLabel(&vs.ObjectMeta, input.Backup.Name) { - p.log.Info( - "VolumeSnapshot %s/%s was not taken by backup %s, skipping deletion", - vs.Namespace, vs.Name, input.Backup.Name, - ) - return nil - } - - p.log.Infof("Deleting VolumeSnapshot %s/%s", vs.Namespace, vs.Name) - if vs.Status != nil && vs.Status.BoundVolumeSnapshotContentName != nil { - // we patch the DeletionPolicy of the VolumeSnapshotContent - // to set it to Delete. This ensures that the volume snapshot - // in the storage provider is also deleted. - err := csi.SetVolumeSnapshotContentDeletionPolicy( - *vs.Status.BoundVolumeSnapshotContentName, - p.crClient, - ) - if err != nil && !apierrors.IsNotFound(err) { - return errors.Wrapf( - err, - fmt.Sprintf("failed to patch DeletionPolicy of volume snapshot %s/%s", - vs.Namespace, vs.Name), - ) - } - - if apierrors.IsNotFound(err) { - return nil - } - } - err := p.crClient.Delete(context.TODO(), &vs) - if err != nil && !apierrors.IsNotFound(err) { - return err - } - return nil -} - -func NewVolumeSnapshotDeleteItemAction(f client.Factory) plugincommon.HandlerInitializer { - return func(logger logrus.FieldLogger) (any, error) { - crClient, err := f.KubebuilderClient() - if err != nil { - return nil, errors.WithStack(err) - } - - return &volumeSnapshotDeleteItemAction{ - log: logger, - crClient: crClient, - }, nil - } -} diff --git a/internal/delete/actions/csi/volumesnapshot_action_test.go b/internal/delete/actions/csi/volumesnapshot_action_test.go deleted file mode 100644 index 9aa8bb1ae..000000000 --- a/internal/delete/actions/csi/volumesnapshot_action_test.go +++ /dev/null @@ -1,151 +0,0 @@ -/* -Copyright the Velero contributors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package csi - -import ( - "context" - "fmt" - "testing" - - snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" - "github.com/sirupsen/logrus" - "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - - velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" - "github.com/vmware-tanzu/velero/pkg/builder" - factorymocks "github.com/vmware-tanzu/velero/pkg/client/mocks" - "github.com/vmware-tanzu/velero/pkg/plugin/velero" - velerotest "github.com/vmware-tanzu/velero/pkg/test" -) - -func TestVSExecute(t *testing.T) { - tests := []struct { - name string - item runtime.Unstructured - vs *snapshotv1api.VolumeSnapshot - backup *velerov1api.Backup - createVS bool - expectErr bool - }{ - { - name: "VolumeSnapshot doesn't have backup label", - item: velerotest.UnstructuredOrDie( - ` - { - "apiVersion": "snapshot.storage.k8s.io/v1", - "kind": "VolumeSnapshot", - "metadata": { - "namespace": "ns", - "name": "foo" - } - } - `, - ), - backup: builder.ForBackup("velero", "backup").Result(), - expectErr: false, - }, - { - name: "VolumeSnapshot doesn't exist in the cluster", - vs: builder.ForVolumeSnapshot("foo", "bar"). - ObjectMeta(builder.WithLabelsMap( - map[string]string{velerov1api.BackupNameLabel: "backup"}, - )).Status(). - BoundVolumeSnapshotContentName("vsc"). - Result(), - backup: builder.ForBackup("velero", "backup").Result(), - expectErr: true, - }, - { - name: "Normal case, VolumeSnapshot should be deleted", - vs: builder.ForVolumeSnapshot("foo", "bar"). - ObjectMeta(builder.WithLabelsMap( - map[string]string{velerov1api.BackupNameLabel: "backup"}, - )).Status(). - BoundVolumeSnapshotContentName("vsc"). - Result(), - backup: builder.ForBackup("velero", "backup").Result(), - expectErr: false, - createVS: true, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - crClient := velerotest.NewFakeControllerRuntimeClient(t) - logger := logrus.StandardLogger() - - p := volumeSnapshotDeleteItemAction{log: logger, crClient: crClient} - - if test.vs != nil { - vsMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(test.vs) - require.NoError(t, err) - test.item = &unstructured.Unstructured{Object: vsMap} - } - - if test.createVS { - require.NoError(t, crClient.Create(context.TODO(), test.vs)) - } - - err := p.Execute( - &velero.DeleteItemActionExecuteInput{ - Item: test.item, - Backup: test.backup, - }, - ) - - if test.expectErr == false { - require.NoError(t, err) - } - }) - } -} - -func TestVSAppliesTo(t *testing.T) { - p := volumeSnapshotDeleteItemAction{ - log: logrus.StandardLogger(), - } - selector, err := p.AppliesTo() - - require.NoError(t, err) - - require.Equal( - t, - velero.ResourceSelector{ - IncludedResources: []string{"volumesnapshots.snapshot.storage.k8s.io"}, - }, - selector, - ) -} - -func TestNewVolumeSnapshotDeleteItemAction(t *testing.T) { - logger := logrus.StandardLogger() - crClient := velerotest.NewFakeControllerRuntimeClient(t) - - f := &factorymocks.Factory{} - f.On("KubebuilderClient").Return(nil, fmt.Errorf("")) - plugin := NewVolumeSnapshotDeleteItemAction(f) - _, err := plugin(logger) - require.Error(t, err) - - f1 := &factorymocks.Factory{} - f1.On("KubebuilderClient").Return(crClient, nil) - plugin1 := NewVolumeSnapshotDeleteItemAction(f1) - _, err1 := plugin1(logger) - require.NoError(t, err1) -} diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action.go b/internal/delete/actions/csi/volumesnapshotcontent_action.go index a3e76437a..641513eef 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action.go @@ -18,19 +18,22 @@ package csi import ( "context" - "fmt" + "time" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" + corev1api "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/wait" crclient "sigs.k8s.io/controller-runtime/pkg/client" + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/client" plugincommon "github.com/vmware-tanzu/velero/pkg/plugin/framework/common" "github.com/vmware-tanzu/velero/pkg/plugin/velero" - "github.com/vmware-tanzu/velero/pkg/util/csi" + "github.com/vmware-tanzu/velero/pkg/util/boolptr" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" ) @@ -77,25 +80,48 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute( p.log.Infof("Deleting VolumeSnapshotContent %s", snapCont.Name) - if err := csi.SetVolumeSnapshotContentDeletionPolicy( - snapCont.Name, - p.crClient, + snapCont.Spec.DeletionPolicy = snapshotv1api.VolumeSnapshotContentDelete + + snapCont.Spec.Source = snapshotv1api.VolumeSnapshotContentSource{ + SnapshotHandle: snapCont.Status.SnapshotHandle, + } + + snapCont.Spec.VolumeSnapshotRef = corev1api.ObjectReference{ + APIVersion: snapshotv1api.SchemeGroupVersion.String(), + Kind: "VolumeSnapshot", + Namespace: "ns-" + string(snapCont.UID), + Name: "name-" + string(snapCont.UID), + } + + snapCont.ResourceVersion = "" + + if err := p.crClient.Create(context.TODO(), &snapCont); err != nil { + return errors.Wrapf(err, "fail to create VolumeSnapshotContent %s", snapCont.Name) + } + + // Read resource timeout from backup annotation, if not set, use default value. + timeout, err := time.ParseDuration( + input.Backup.Annotations[velerov1api.ResourceTimeoutAnnotation]) + if err != nil { + p.log.Warnf("fail to parse resource timeout annotation %s: %s", + input.Backup.Annotations[velerov1api.ResourceTimeoutAnnotation], err.Error()) + timeout = 10 * time.Minute + } + p.log.Debugf("resource timeout is set to %s", timeout.String()) + + interval := 5 * time.Second + + // Wait until VSC created and ReadyToUse is true. + if err := wait.PollUntilContextTimeout( + context.Background(), + interval, + timeout, + true, + func(ctx context.Context) (bool, error) { + return checkVSCReadiness(ctx, &snapCont, p.crClient) + }, ); err != nil { - // #4764: Leave a warning when VolumeSnapshotContent cannot be found for deletion. - // Manual deleting VolumeSnapshotContent can cause this. - // It's tricky for Velero to handle this inconsistency. - // Even if Velero restores the VolumeSnapshotContent, CSI snapshot controller - // may not delete it correctly due to the snapshot represented by VolumeSnapshotContent - // already deleted on cloud provider. - if apierrors.IsNotFound(err) { - p.log.Warnf( - "VolumeSnapshotContent %s of backup %s cannot be found. May leave orphan snapshot %s on cloud provider.", - snapCont.Name, input.Backup.Name, *snapCont.Status.SnapshotHandle) - return nil - } - return errors.Wrapf(err, fmt.Sprintf( - "failed to set DeletionPolicy on volumesnapshotcontent %s. Skipping deletion", - snapCont.Name)) + return errors.Wrapf(err, "fail to wait VolumeSnapshotContent %s becomes ready.", snapCont.Name) } if err := p.crClient.Delete( @@ -109,6 +135,25 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute( return nil } +var checkVSCReadiness = func( + ctx context.Context, + vsc *snapshotv1api.VolumeSnapshotContent, + client crclient.Client, +) (bool, error) { + tmpVSC := new(snapshotv1api.VolumeSnapshotContent) + if err := client.Get(ctx, crclient.ObjectKeyFromObject(vsc), tmpVSC); err != nil { + return false, errors.Wrapf( + err, "failed to get VolumeSnapshotContent %s", vsc.Name, + ) + } + + if tmpVSC.Status != nil && boolptr.IsSetToTrue(tmpVSC.Status.ReadyToUse) { + return true, nil + } + + return false, nil +} + func NewVolumeSnapshotContentDeleteItemAction( f client.Factory, ) plugincommon.HandlerInitializer { diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go index 5c1f6dcb9..805c1b82e 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go @@ -22,10 +22,12 @@ import ( "testing" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" + "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + crclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" @@ -37,11 +39,15 @@ import ( func TestVSCExecute(t *testing.T) { snapshotHandleStr := "test" tests := []struct { - name string - item runtime.Unstructured - vsc *snapshotv1api.VolumeSnapshotContent - backup *velerov1api.Backup - createVSC bool + name string + item runtime.Unstructured + vsc *snapshotv1api.VolumeSnapshotContent + backup *velerov1api.Backup + function func( + ctx context.Context, + vsc *snapshotv1api.VolumeSnapshotContent, + client crclient.Client, + ) (bool, error) expectErr bool }{ { @@ -62,17 +68,30 @@ func TestVSCExecute(t *testing.T) { expectErr: false, }, { - name: "VolumeSnapshotContent doesn't exist in the cluster, no error", + name: "Normal case, VolumeSnapshot should be deleted", vsc: builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(), - backup: builder.ForBackup("velero", "backup").Result(), + backup: builder.ForBackup("velero", "backup").ObjectMeta(builder.WithAnnotationsMap(map[string]string{velerov1api.ResourceTimeoutAnnotation: "5s"})).Result(), expectErr: false, + function: func( + ctx context.Context, + vsc *snapshotv1api.VolumeSnapshotContent, + client crclient.Client, + ) (bool, error) { + return true, nil + }, }, { name: "Normal case, VolumeSnapshot should be deleted", vsc: builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(), - backup: builder.ForBackup("velero", "backup").Result(), - expectErr: false, - createVSC: true, + backup: builder.ForBackup("velero", "backup").ObjectMeta(builder.WithAnnotationsMap(map[string]string{velerov1api.ResourceTimeoutAnnotation: "5s"})).Result(), + expectErr: true, + function: func( + ctx context.Context, + vsc *snapshotv1api.VolumeSnapshotContent, + client crclient.Client, + ) (bool, error) { + return false, errors.Errorf("test error case") + }, }, } @@ -80,6 +99,7 @@ func TestVSCExecute(t *testing.T) { t.Run(test.name, func(t *testing.T) { crClient := velerotest.NewFakeControllerRuntimeClient(t) logger := logrus.StandardLogger() + checkVSCReadiness = test.function p := volumeSnapshotContentDeleteItemAction{log: logger, crClient: crClient} @@ -89,10 +109,6 @@ func TestVSCExecute(t *testing.T) { test.item = &unstructured.Unstructured{Object: vscMap} } - if test.createVSC { - require.NoError(t, crClient.Create(context.TODO(), test.vsc)) - } - err := p.Execute( &velero.DeleteItemActionExecuteInput{ Item: test.item, diff --git a/pkg/cmd/server/plugin/plugin.go b/pkg/cmd/server/plugin/plugin.go index f85431b36..1c0f0bc7d 100644 --- a/pkg/cmd/server/plugin/plugin.go +++ b/pkg/cmd/server/plugin/plugin.go @@ -138,10 +138,6 @@ func NewCommand(f client.Factory) *cobra.Command { "velero.io/dataupload-delete", newDateUploadDeleteItemAction(f), ). - RegisterDeleteItemAction( - "velero.io/csi-volumesnapshot-delete", - newVolumeSnapshotDeleteItemAction(f), - ). RegisterDeleteItemAction( "velero.io/csi-volumesnapshotcontent-delete", newVolumeSnapshotContentDeleteItemAction(f), @@ -432,10 +428,6 @@ func newVolumeSnapshotClassBackupItemAction(logger logrus.FieldLogger) (any, err // DeleteItemAction plugins -func newVolumeSnapshotDeleteItemAction(f client.Factory) plugincommon.HandlerInitializer { - return dia.NewVolumeSnapshotDeleteItemAction(f) -} - func newVolumeSnapshotContentDeleteItemAction(f client.Factory) plugincommon.HandlerInitializer { return dia.NewVolumeSnapshotContentDeleteItemAction(f) }