Merge pull request #5518 from blackpiglet/release-1.9

Fix v1.9.3 CSI VolumeSnapshot status duplicate issue.

changelogs/unreleased/5518-blackpiglet

@@ -0,0 +1 @@
+Fix v1.9.3 CSI VolumeSnapshot status duplicate issue.

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

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
+}

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
+}

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

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)
+			}
+		})
+	}
+}

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...)
 }