diff --git a/changelogs/unreleased/5518-blackpiglet b/changelogs/unreleased/5518-blackpiglet new file mode 100644 index 000000000..77fe28eee --- /dev/null +++ b/changelogs/unreleased/5518-blackpiglet @@ -0,0 +1 @@ +Fix v1.9.3 CSI VolumeSnapshot status duplicate issue. \ No newline at end of file diff --git a/pkg/backup/request.go b/pkg/backup/request.go index 38cd49917..69dbeca70 100644 --- a/pkg/backup/request.go +++ b/pkg/backup/request.go @@ -50,7 +50,7 @@ type Request struct { VolumeSnapshots []*volume.Snapshot PodVolumeBackups []*velerov1api.PodVolumeBackup BackedUpItems map[itemKey]struct{} - CSISnapshots []*snapshotv1api.VolumeSnapshot + CSISnapshots []snapshotv1api.VolumeSnapshot } // BackupResourceList returns the list of backed up resources grouped by the API diff --git a/pkg/builder/volume_snapshot_builder.go b/pkg/builder/volume_snapshot_builder.go new file mode 100644 index 000000000..8ee32219c --- /dev/null +++ b/pkg/builder/volume_snapshot_builder.go @@ -0,0 +1,66 @@ +/* +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 builder + +import ( + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// VolumeSnapshotBuilder builds VolumeSnapshot objects. +type VolumeSnapshotBuilder struct { + object *snapshotv1api.VolumeSnapshot +} + +// ForVolumeSnapshot is the constructor for VolumeSnapshotBuilder. +func ForVolumeSnapshot(ns, name string) *VolumeSnapshotBuilder { + return &VolumeSnapshotBuilder{ + object: &snapshotv1api.VolumeSnapshot{ + TypeMeta: metav1.TypeMeta{ + APIVersion: snapshotv1api.SchemeGroupVersion.String(), + Kind: "VolumeSnapshot", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + }, + }, + } +} + +// ObjectMeta applies functional options to the VolumeSnapshot's ObjectMeta. +func (v *VolumeSnapshotBuilder) ObjectMeta(opts ...ObjectMetaOpt) *VolumeSnapshotBuilder { + for _, opt := range opts { + opt(v.object) + } + + return v +} + +// Result return the built VolumeSnapshot. +func (v *VolumeSnapshotBuilder) Result() *snapshotv1api.VolumeSnapshot { + return v.object +} + +// Status init the built VolumeSnapshot's status. +func (v *VolumeSnapshotBuilder) Status() *VolumeSnapshotBuilder { + v.object.Status = &snapshotv1api.VolumeSnapshotStatus{} + return v +} + +// BoundVolumeSnapshotContentName set built VolumeSnapshot's status BoundVolumeSnapshotContentName field. +func (v *VolumeSnapshotBuilder) BoundVolumeSnapshotContentName(vscName string) *VolumeSnapshotBuilder { + v.object.Status.BoundVolumeSnapshotContentName = &vscName + return v +} diff --git a/pkg/builder/volume_snapshot_content_builder.go b/pkg/builder/volume_snapshot_content_builder.go new file mode 100644 index 000000000..4488f4009 --- /dev/null +++ b/pkg/builder/volume_snapshot_content_builder.go @@ -0,0 +1,67 @@ +/* +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 builder + +import ( + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// VolumeSnapshotContentBuilder builds VolumeSnapshotContent object. +type VolumeSnapshotContentBuilder struct { + object *snapshotv1api.VolumeSnapshotContent +} + +// ForVolumeSnapshotContent is the constructor of VolumeSnapshotContentBuilder. +func ForVolumeSnapshotContent(name string) *VolumeSnapshotContentBuilder { + return &VolumeSnapshotContentBuilder{ + object: &snapshotv1api.VolumeSnapshotContent{ + TypeMeta: metav1.TypeMeta{ + APIVersion: snapshotv1api.SchemeGroupVersion.String(), + Kind: "VolumeSnapshotContent", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + }, + } +} + +// Result returns the built VolumeSnapshotContent. +func (v *VolumeSnapshotContentBuilder) Result() *snapshotv1api.VolumeSnapshotContent { + return v.object +} + +// Status initiates VolumeSnapshotContent's status. +func (v *VolumeSnapshotContentBuilder) Status() *VolumeSnapshotContentBuilder { + v.object.Status = &snapshotv1api.VolumeSnapshotContentStatus{} + return v +} + +// DeletionPolicy sets built VolumeSnapshotContent's spec.DeletionPolicy value. +func (v *VolumeSnapshotContentBuilder) DeletionPolicy(policy snapshotv1api.DeletionPolicy) *VolumeSnapshotContentBuilder { + v.object.Spec.DeletionPolicy = policy + return v +} + +func (v *VolumeSnapshotContentBuilder) VolumeSnapshotRef(namespace, name string) *VolumeSnapshotContentBuilder { + v.object.Spec.VolumeSnapshotRef = v1.ObjectReference{ + APIVersion: "snapshot.storage.k8s.io/v1", + Kind: "VolumeSnapshot", + Namespace: namespace, + Name: name, + } + return v +} diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 6bdace8a3..86b118602 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -93,7 +93,7 @@ type backupController struct { backupStoreGetter persistence.ObjectBackupStoreGetter formatFlag logging.Format volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister - volumeSnapshotClient *snapshotterClientSet.Clientset + volumeSnapshotClient snapshotterClientSet.Interface volumeSnapshotContentLister snapshotv1listers.VolumeSnapshotContentLister volumeSnapshotClassLister snapshotv1listers.VolumeSnapshotClassLister } @@ -117,7 +117,7 @@ func NewBackupController( metrics *metrics.ServerMetrics, formatFlag logging.Format, volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister, - volumeSnapshotClient *snapshotterClientSet.Clientset, + volumeSnapshotClient snapshotterClientSet.Interface, volumeSnapshotContentLister snapshotv1listers.VolumeSnapshotContentLister, volumesnapshotClassLister snapshotv1listers.VolumeSnapshotClassLister, backupStoreGetter persistence.ObjectBackupStoreGetter, @@ -631,17 +631,14 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { // Empty slices here so that they can be passed in to the persistBackup call later, regardless of whether or not CSI's enabled. // This way, we only make the Lister call if the feature flag's on. - var volumeSnapshots []*snapshotv1api.VolumeSnapshot + var volumeSnapshots []snapshotv1api.VolumeSnapshot var volumeSnapshotContents []*snapshotv1api.VolumeSnapshotContent var volumeSnapshotClasses []*snapshotv1api.VolumeSnapshotClass if features.IsEnabled(velerov1api.CSIFeatureFlag) { - tmpVSArray, err := c.waitVolumeSnapshotReadyToUse(context.Background(), backup.Spec.CSISnapshotTimeout.Duration, backup.Name) + volumeSnapshots, err := c.waitVolumeSnapshotReadyToUse(context.Background(), backup.Spec.CSISnapshotTimeout.Duration, backup.Name) if err != nil { backupLog.Errorf("fail to wait VolumeSnapshot change to Ready: %s", err.Error()) } - for _, vs := range tmpVSArray { - volumeSnapshots = append(volumeSnapshots, &vs) - } backup.CSISnapshots = volumeSnapshots @@ -671,7 +668,7 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { } // Delete the VolumeSnapshots created in the backup, when CSI feature is enabled. - c.deleteVolumeSnapshot(volumeSnapshots, volumeSnapshotContents, *backup, backupLog) + c.deleteVolumeSnapshot(volumeSnapshots, volumeSnapshotContents, backupLog) } @@ -768,7 +765,7 @@ func persistBackup(backup *pkgbackup.Request, backupContents, backupLog *os.File, backupStore persistence.BackupStore, log logrus.FieldLogger, - csiVolumeSnapshots []*snapshotv1api.VolumeSnapshot, + csiVolumeSnapshots []snapshotv1api.VolumeSnapshot, csiVolumeSnapshotContents []*snapshotv1api.VolumeSnapshotContent, csiVolumesnapshotClasses []*snapshotv1api.VolumeSnapshotClass, ) []error { @@ -943,9 +940,9 @@ func (c *backupController) waitVolumeSnapshotReadyToUse(ctx context.Context, // which will cause snapshot deletion on cloud provider, then backup cannot restore the PV. // If DeletionPolicy is Retain, just delete it. If DeletionPolicy is Delete, need to // change DeletionPolicy to Retain before deleting VS, then change DeletionPolicy back to Delete. -func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []*snapshotv1api.VolumeSnapshot, +func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api.VolumeSnapshot, volumeSnapshotContents []*snapshotv1api.VolumeSnapshotContent, - backup pkgbackup.Request, logger logrus.FieldLogger) { + logger logrus.FieldLogger) { var wg sync.WaitGroup vscMap := make(map[string]*snapshotv1api.VolumeSnapshotContent) for _, vsc := range volumeSnapshotContents { @@ -954,7 +951,7 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []*snapshotv1api for _, vs := range volumeSnapshots { wg.Add(1) - go func(vs *snapshotv1api.VolumeSnapshot) { + go func(vs snapshotv1api.VolumeSnapshot) { defer wg.Done() var vsc *snapshotv1api.VolumeSnapshotContent modifyVSCFlag := false diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 29e7ecc9c..e2508e800 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -27,6 +27,9 @@ import ( "testing" "time" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + snapshotfake "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake" + snapshotinformers "github.com/kubernetes-csi/external-snapshotter/client/v4/informers/externalversions" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -1163,3 +1166,101 @@ func Test_getLastSuccessBySchedule(t *testing.T) { }) } } + +func TestDeleteVolumeSnapshot(t *testing.T) { + tests := []struct { + name string + vsArray []snapshotv1api.VolumeSnapshot + vscArray []*snapshotv1api.VolumeSnapshotContent + expectedVSArray []snapshotv1api.VolumeSnapshot + expectedVSCArray []snapshotv1api.VolumeSnapshotContent + }{ + { + name: "VS is ReadyToUse, and VS has corresponding VSC. VS should be deleted.", + vsArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").ObjectMeta(builder.WithLabels("testing-vs", "vs1")).Status().BoundVolumeSnapshotContentName("vsc1").Result(), + }, + vscArray: []*snapshotv1api.VolumeSnapshotContent{ + builder.ForVolumeSnapshotContent("vsc1").DeletionPolicy(snapshotv1api.VolumeSnapshotContentDelete).Status().Result(), + }, + expectedVSArray: []snapshotv1api.VolumeSnapshot{}, + expectedVSCArray: []snapshotv1api.VolumeSnapshotContent{ + *builder.ForVolumeSnapshotContent("vsc1").DeletionPolicy(snapshotv1api.VolumeSnapshotContentRetain).VolumeSnapshotRef("ns-", "name-").Status().Result(), + }, + }, + { + name: "Corresponding VSC not found for VS. VS is not deleted.", + vsArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").ObjectMeta(builder.WithLabels("testing-vs", "vs1")).Status().BoundVolumeSnapshotContentName("vsc1").Result(), + }, + vscArray: []*snapshotv1api.VolumeSnapshotContent{}, + expectedVSArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").Status().BoundVolumeSnapshotContentName("vsc1").Result(), + }, + expectedVSCArray: []snapshotv1api.VolumeSnapshotContent{}, + }, + { + name: "VS status is nil. VSC should not be modified.", + vsArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").ObjectMeta(builder.WithLabels("testing-vs", "vs1")).Result(), + }, + vscArray: []*snapshotv1api.VolumeSnapshotContent{ + builder.ForVolumeSnapshotContent("vsc1").DeletionPolicy(snapshotv1api.VolumeSnapshotContentDelete).Status().Result(), + }, + expectedVSArray: []snapshotv1api.VolumeSnapshot{}, + expectedVSCArray: []snapshotv1api.VolumeSnapshotContent{ + *builder.ForVolumeSnapshotContent("vsc1").DeletionPolicy(snapshotv1api.VolumeSnapshotContentDelete).Status().Result(), + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fakeClient := velerotest.NewFakeControllerRuntimeClientBuilder(t).WithLists().Build() + + vsClient := snapshotfake.NewSimpleClientset() + for _, vsc := range tc.vscArray { + _, err := vsClient.SnapshotV1().VolumeSnapshotContents().Create(context.TODO(), vsc, metav1.CreateOptions{}) + require.NoError(t, err) + err = fakeClient.Create(context.TODO(), vsc, &kbclient.CreateOptions{}) + require.NoError(t, err) + } + for _, vs := range tc.vsArray { + _, err := vsClient.SnapshotV1().VolumeSnapshots("velero").Create(context.TODO(), &vs, metav1.CreateOptions{}) + require.NoError(t, err) + err = fakeClient.Create(context.TODO(), &vs, &kbclient.CreateOptions{}) + require.NoError(t, err) + } + + sharedInformers := snapshotinformers.NewSharedInformerFactory(vsClient, 0) + + for _, vs := range tc.vsArray { + sharedInformers.Snapshot().V1().VolumeSnapshots().Informer().GetStore().Add(vs) + } + + logger := logging.DefaultLogger(logrus.DebugLevel, logging.FormatText) + c := &backupController{ + kbClient: fakeClient, + volumeSnapshotClient: vsClient, + volumeSnapshotLister: sharedInformers.Snapshot().V1().VolumeSnapshots().Lister(), + } + + c.deleteVolumeSnapshot(tc.vsArray, tc.vscArray, logger) + + vsList, err := c.volumeSnapshotClient.SnapshotV1().VolumeSnapshots("velero").List(context.TODO(), metav1.ListOptions{}) + require.NoError(t, err) + assert.Equal(t, len(tc.expectedVSArray), len(vsList.Items)) + for index := range tc.expectedVSArray { + assert.Equal(t, tc.expectedVSArray[index].Status, vsList.Items[index].Status) + assert.Equal(t, tc.expectedVSArray[index].Spec, vsList.Items[index].Spec) + } + + vscList, err := c.volumeSnapshotClient.SnapshotV1().VolumeSnapshotContents().List(context.TODO(), metav1.ListOptions{}) + require.NoError(t, err) + assert.Equal(t, len(tc.expectedVSCArray), len(vscList.Items)) + for index := range tc.expectedVSCArray { + assert.Equal(t, tc.expectedVSCArray[index].Spec, vscList.Items[index].Spec) + } + }) + } +} diff --git a/pkg/test/fake_controller_runtime_client.go b/pkg/test/fake_controller_runtime_client.go index d1c1b6106..0be391bd9 100644 --- a/pkg/test/fake_controller_runtime_client.go +++ b/pkg/test/fake_controller_runtime_client.go @@ -19,6 +19,7 @@ package test import ( "testing" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/stretchr/testify/require" corev1api "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -34,6 +35,8 @@ func NewFakeControllerRuntimeClientBuilder(t *testing.T) *k8sfake.ClientBuilder require.NoError(t, err) err = corev1api.AddToScheme(scheme) require.NoError(t, err) + err = snapshotv1api.AddToScheme(scheme) + require.NoError(t, err) return k8sfake.NewClientBuilder().WithScheme(scheme) } @@ -43,5 +46,7 @@ func NewFakeControllerRuntimeClient(t *testing.T, initObjs ...runtime.Object) cl require.NoError(t, err) err = corev1api.AddToScheme(scheme) require.NoError(t, err) + err = snapshotv1api.AddToScheme(scheme) + require.NoError(t, err) return k8sfake.NewFakeClientWithScheme(scheme, initObjs...) }