Add more nil pointer check for CSI related code in backup controller. (#5388)

Add some corner cases checking for CSI snapshot in backup controller.

Signed-off-by: Xun Jiang <blackpiglet@gmail.com>

changelogs/unreleased/5388-blackpiglet

@@ -0,0 +1 @@
+Add some corner cases checking for CSI snapshot in backup controller.

pkg/builder/volume_snapshot_builder.go

@@ -0,0 +1,69 @@
+/*
+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,70 @@
+/*
+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

@@ -95,7 +95,7 @@ type backupController struct {
 	backupStoreGetter         persistence.ObjectBackupStoreGetter
 	formatFlag                logging.Format
 	volumeSnapshotLister      snapshotv1listers.VolumeSnapshotLister
-	volumeSnapshotClient      *snapshotterClientSet.Clientset
+	volumeSnapshotClient      snapshotterClientSet.Interface
 	credentialFileStore       credentials.FileStore
 }
 
@@ -119,7 +119,7 @@ func NewBackupController(
 	backupStoreGetter persistence.ObjectBackupStoreGetter,
 	formatFlag logging.Format,
 	volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister,
-	volumeSnapshotClient *snapshotterClientSet.Clientset,
+	volumeSnapshotClient snapshotterClientSet.Interface,
 	credentialStore credentials.FileStore,
 ) Interface {
 	c := &backupController{
@@ -674,10 +674,11 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error {
 			}
 		}
 
-		err = c.checkVolumeSnapshotReadyToUse(context.Background(), volumeSnapshots, backup.Spec.CSISnapshotTimeout.Duration)
+		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())
 		}
+
 		backup.CSISnapshots = volumeSnapshots
 
 		err = c.kbClient.List(context.Background(), vscList, &kbclient.ListOptions{LabelSelector: selector})
@@ -911,18 +912,35 @@ func encodeToJSONGzip(data interface{}, desc string) (*bytes.Buffer, []error) {
 	return buf, nil
 }
 
-// Waiting for VolumeSnapshot ReadyTosue to true is time consuming. Try to make the process parallel by
+// waitVolumeSnapshotReadyToUse is used to wait VolumeSnapshot turned to ReadyToUse.
+// Waiting for VolumeSnapshot ReadyToUse to true is time consuming. Try to make the process parallel by
 // using goroutine here instead of waiting in CSI plugin, because it's not easy to make BackupItemAction
 // parallel by now. After BackupItemAction parallel is implemented, this logic should be moved to CSI plugin
 // as https://github.com/vmware-tanzu/velero-plugin-for-csi/pull/100
-func (c *backupController) checkVolumeSnapshotReadyToUse(ctx context.Context, volumesnapshots []snapshotv1api.VolumeSnapshot,
-	csiSnapshotTimeout time.Duration) error {
+func (c *backupController) waitVolumeSnapshotReadyToUse(ctx context.Context,
+	csiSnapshotTimeout time.Duration, backupName string) ([]snapshotv1api.VolumeSnapshot, error) {
 	eg, _ := errgroup.WithContext(ctx)
 	timeout := csiSnapshotTimeout
 	interval := 5 * time.Second
+	volumeSnapshots := make([]snapshotv1api.VolumeSnapshot, 0)
 
-	for _, vs := range volumesnapshots {
-		volumeSnapshot := vs
+	if c.volumeSnapshotLister != nil {
+		tmpVSs, err := c.volumeSnapshotLister.List(label.NewSelectorForBackup(backupName))
+		if err != nil {
+			c.logger.Error(err)
+			return volumeSnapshots, err
+		}
+
+		for _, vs := range tmpVSs {
+			volumeSnapshots = append(volumeSnapshots, *vs)
+		}
+	}
+
+	vsChannel := make(chan snapshotv1api.VolumeSnapshot, len(volumeSnapshots))
+	defer close(vsChannel)
+
+	for index := range volumeSnapshots {
+		volumeSnapshot := volumeSnapshots[index]
 		eg.Go(func() error {
 			err := wait.PollImmediate(interval, timeout, func() (bool, error) {
 				tmpVS, err := c.volumeSnapshotClient.SnapshotV1().VolumeSnapshots(volumeSnapshot.Namespace).Get(ctx, volumeSnapshot.Name, metav1.GetOptions{})
@@ -934,6 +952,9 @@ func (c *backupController) checkVolumeSnapshotReadyToUse(ctx context.Context, vo
 					return false, nil
 				}
 
+				c.logger.Debugf("VolumeSnapshot %s/%s turned into ReadyToUse.", volumeSnapshot.Namespace, volumeSnapshot.Name)
+				// Put the ReadyToUse VolumeSnapshot element in the result channel.
+				vsChannel <- *tmpVS
 				return true, nil
 			})
 			if err == wait.ErrWaitTimeout {
@@ -942,7 +963,16 @@ func (c *backupController) checkVolumeSnapshotReadyToUse(ctx context.Context, vo
 			return err
 		})
 	}
-	return eg.Wait()
+
+	err := eg.Wait()
+
+	result := make([]snapshotv1api.VolumeSnapshot, 0)
+	length := len(vsChannel)
+	for index := 0; index < length; index++ {
+		result = append(result, <-vsChannel)
+	}
+
+	return result, err
 }
 
 // deleteVolumeSnapshot delete VolumeSnapshot created during backup.
@@ -965,7 +995,8 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api.
 			defer wg.Done()
 			var vsc snapshotv1api.VolumeSnapshotContent
 			modifyVSCFlag := false
-			if vs.Status.BoundVolumeSnapshotContentName != nil &&
+			if vs.Status != nil &&
+				vs.Status.BoundVolumeSnapshotContentName != nil &&
 				len(*vs.Status.BoundVolumeSnapshotContentName) > 0 {
 				var found bool
 				if vsc, found = vscMap[*vs.Status.BoundVolumeSnapshotContentName]; !found {
@@ -976,6 +1007,8 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api.
 				if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentDelete {
 					modifyVSCFlag = true
 				}
+			} else {
+				logger.Errorf("VolumeSnapshot %s/%s is not ready. This is not expected.", vs.Namespace, vs.Name)
 			}
 
 			// Change VolumeSnapshotContent's DeletionPolicy to Retain before deleting VolumeSnapshot,
@@ -1001,7 +1034,7 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api.
 			}
 
 			// Delete VolumeSnapshot from cluster
-			logger.Debugf("Deleting VolumeSnapshotContent %s", vsc.Name)
+			logger.Debugf("Deleting VolumeSnapshot %s/%s", vs.Namespace, vs.Name)
 			err := c.volumeSnapshotClient.SnapshotV1().VolumeSnapshots(vs.Namespace).Delete(context.TODO(), vs.Name, metav1.DeleteOptions{})
 			if err != nil {
 				logger.Errorf("fail to delete VolumeSnapshot %s/%s: %s", vs.Namespace, vs.Name, err.Error())

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"
@@ -1393,3 +1396,90 @@ 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(
+				&snapshotv1api.VolumeSnapshotContentList{Items: tc.vscArray},
+			).Build()
+
+			vsClient := snapshotfake.NewSimpleClientset(&tc.vsArray[0])
+			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 := &snapshotv1api.VolumeSnapshotContentList{}
+			require.NoError(t, c.kbClient.List(context.Background(), vscList))
+			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...)
 }