Add more nil pointer check for CSI related code in backup controller.

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

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,60 @@
+/*
+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,
+			},
+		},
+	}
+}
+
+// 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

@@ -670,6 +670,16 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error {
 		if err != nil {
 			backupLog.Errorf("fail to wait VolumeSnapshot change to Ready: %s", err.Error())
 		}
+
+		// checkVolumeSnapshotReadyToUse could run for a long time. Update VolumeSnapshot array after it finishes.
+		err = c.kbClient.List(context.Background(), vsList, &kbclient.ListOptions{LabelSelector: selector})
+		if err != nil {
+			backupLog.Error(err)
+		}
+		if len(vsList.Items) >= 0 {
+			volumeSnapshots = vsList.Items
+		}
+
 		backup.CSISnapshots = volumeSnapshots
 
 		err = c.kbClient.List(context.Background(), vscList, &kbclient.ListOptions{LabelSelector: selector})
@@ -699,7 +709,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)
 
 	}
 
@@ -944,8 +954,7 @@ func (c *backupController) checkVolumeSnapshotReadyToUse(ctx context.Context, vo
 // 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,
-	volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent,
-	backup pkgbackup.Request, logger logrus.FieldLogger) {
+	volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent, logger logrus.FieldLogger) {
 	var wg sync.WaitGroup
 	vscMap := make(map[string]snapshotv1api.VolumeSnapshotContent)
 	for _, vsc := range volumeSnapshotContents {
@@ -958,7 +967,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 {
@@ -969,6 +979,9 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api.
 				if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentDelete {
 					modifyVSCFlag = true
 				}
+			} else {
+				logger.Errorf("VS %s/%s is not ready. This is not expected.", vs.Namespace, vs.Name)
+				return
 			}
 
 			// Change VolumeSnapshotContent's DeletionPolicy to Retain before deleting VolumeSnapshot,

pkg/controller/backup_controller_test.go

@@ -27,6 +27,7 @@ import (
 	"testing"
 	"time"
 
+	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
@@ -1281,3 +1282,79 @@ 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").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").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. VS should not be modified.",
+			vsArray: []snapshotv1api.VolumeSnapshot{
+				*builder.ForVolumeSnapshot("velero", "vs1").Result(),
+			},
+			vscArray: []snapshotv1api.VolumeSnapshotContent{},
+			expectedVSArray: []snapshotv1api.VolumeSnapshot{
+				*builder.ForVolumeSnapshot("velero", "vs1").Result(),
+			},
+			expectedVSCArray: []snapshotv1api.VolumeSnapshotContent{},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			fakeClient := velerotest.NewFakeControllerRuntimeClientBuilder(t).WithLists(
+				&snapshotv1api.VolumeSnapshotList{Items: tc.vsArray},
+				&snapshotv1api.VolumeSnapshotContentList{Items: tc.vscArray},
+			).Build()
+			logger := logging.DefaultLogger(logrus.DebugLevel, logging.FormatText)
+			c := &backupController{
+				kbClient: fakeClient,
+			}
+
+			c.deleteVolumeSnapshot(tc.vsArray, tc.vscArray, logger)
+
+			vsList := &snapshotv1api.VolumeSnapshotList{}
+			require.NoError(t, c.kbClient.List(context.Background(), vsList))
+			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...)
 }