fix: create snapshot failed if storageClass's provisioner not equals …

…to driver name
apecloud · Oct 25, 2023 · 9fdbfa7 · 9fdbfa7
1 parent aec1be5
commit 9fdbfa7
Show file tree

Hide file tree

Showing 15 changed files with 61 additions and 37 deletions.
diff --git a/controllers/apps/components/hscale_volume_populator.go b/controllers/apps/components/hscale_volume_populator.go
@@ -454,11 +454,8 @@ func isVolumeSnapshotEnabled(ctx context.Context, cli client.Client,
 	if err := cli.Get(ctx, pvcKey, &pvc); err != nil {
 		return false, client.IgnoreNotFound(err)
 	}
-	if pvc.Spec.StorageClassName == nil {
-		return false, nil
-	}
 
-	return intctrlutil.IsVolumeSnapshotEnabled(ctx, cli, *pvc.Spec.StorageClassName)
+	return intctrlutil.IsVolumeSnapshotEnabled(ctx, cli, pvc.Spec.VolumeName)
 }
 
 func getBackupMethods(backupPolicy *dpv1alpha1.BackupPolicy, useVolumeSnapshot bool) []string {

diff --git a/controllers/dataprotection/backup_controller.go b/controllers/dataprotection/backup_controller.go
@@ -283,8 +283,11 @@ func (r *BackupReconciler) prepareBackupRequest(
 	}
 
 	request.BackupPolicy = backupPolicy
-	if err = r.handleBackupRepo(request); err != nil {
-		return nil, err
+	if !snapshotVolumes {
+		// if use volume snapshot, ignore backup repo
+		if err = r.handleBackupRepo(request); err != nil {
+			return nil, err
+		}
 	}
 
 	request.BackupMethod = backupMethod
@@ -349,11 +352,12 @@ func (r *BackupReconciler) patchBackupStatus(
 	request.Status.Path = dpbackup.BuildBackupPath(request.Backup, request.BackupPolicy.Spec.PathPrefix)
 	request.Status.Target = request.BackupPolicy.Spec.Target
 	request.Status.BackupMethod = request.BackupMethod
-	request.Status.BackupRepoName = request.BackupRepo.Name
+	if request.BackupRepo != nil {
+		request.Status.BackupRepoName = request.BackupRepo.Name
+	}
 	if request.BackupRepoPVC != nil {
 		request.Status.PersistentVolumeClaimName = request.BackupRepoPVC.Name
 	}
-
 	// init action status
 	actions, err := request.BuildActions()
 	if err != nil {
@@ -398,16 +402,17 @@ func (r *BackupReconciler) patchBackupObjectMeta(
 		request.Labels[v] = targetPod.Labels[v]
 	}
 
-	request.Labels[dataProtectionBackupRepoKey] = request.BackupRepo.Name
 	request.Labels[constant.AppManagedByLabelKey] = constant.AppName
 	request.Labels[dptypes.BackupTypeLabelKey] = request.GetBackupType()
-
 	// wait for the backup repo controller to prepare the essential resource.
 	wait := false
-	if (request.BackupRepo.AccessByMount() && request.BackupRepoPVC == nil) ||
-		(request.BackupRepo.AccessByTool() && request.ToolConfigSecret == nil) {
-		request.Labels[dataProtectionWaitRepoPreparationKey] = trueVal
-		wait = true
+	if request.BackupRepo != nil {
+		request.Labels[dataProtectionBackupRepoKey] = request.BackupRepo.Name
+		if (request.BackupRepo.AccessByMount() && request.BackupRepoPVC == nil) ||
+			(request.BackupRepo.AccessByTool() && request.ToolConfigSecret == nil) {
+			request.Labels[dataProtectionWaitRepoPreparationKey] = trueVal
+			wait = true
+		}
 	}
 
 	// set annotations

diff --git a/controllers/dataprotection/backup_controller_test.go b/controllers/dataprotection/backup_controller_test.go
@@ -74,6 +74,7 @@ var _ = Describe("Backup Controller test", func() {
 		// non-namespaced
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.ActionSetSignature, true, ml)
 		testapps.ClearResources(&testCtx, generics.StorageClassSignature, ml)
+		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.PersistentVolumeSignature, true, ml)
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.BackupRepoSignature, true, ml)
 		testapps.ClearResources(&testCtx, generics.StorageProviderSignature, ml)
 		testapps.ClearResources(&testCtx, generics.VolumeSnapshotClassSignature, ml)
@@ -328,7 +329,7 @@ var _ = Describe("Backup Controller test", func() {
 
 			BeforeEach(func() {
 				// mock VolumeSnapshotClass for volume snapshot
-				testk8s.CreateVolumeSnapshotClass(&testCtx, testutil.DefaultStorageProvisoner)
+				testk8s.CreateVolumeSnapshotClass(&testCtx, testutil.DefaultCSIDriver)
 
 				By("create a backup from backupPolicy " + testdp.BackupPolicyName)
 				backup = testdp.NewFakeBackup(&testCtx, func(backup *dpv1alpha1.Backup) {

diff --git a/controllers/dataprotection/backupschedule_controller_test.go b/controllers/dataprotection/backupschedule_controller_test.go
@@ -67,6 +67,7 @@ var _ = Describe("Backup Schedule Controller", func() {
 		// non-namespaced
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.ActionSetSignature, true, ml)
 		testapps.ClearResources(&testCtx, generics.StorageClassSignature, ml)
+		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.PersistentVolumeSignature, true, ml)
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.BackupRepoSignature, true, ml)
 		testapps.ClearResources(&testCtx, generics.StorageProviderSignature, ml)
 	}

diff --git a/controllers/dataprotection/gc_controller_test.go b/controllers/dataprotection/gc_controller_test.go
@@ -66,6 +66,7 @@ var _ = Describe("Data Protection Garbage Collection Controller", func() {
 		// non-namespaced
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.ActionSetSignature, true, ml)
 		testapps.ClearResources(&testCtx, generics.StorageClassSignature, ml)
+		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.PersistentVolumeSignature, true, ml)
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.BackupRepoSignature, true, ml)
 		testapps.ClearResources(&testCtx, generics.StorageProviderSignature, ml)
 	}

diff --git a/controllers/dataprotection/restore_controller_test.go b/controllers/dataprotection/restore_controller_test.go
@@ -72,6 +72,7 @@ var _ = Describe("Restore Controller test", func() {
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.StorageClassSignature, true, ml)
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.BackupRepoSignature, true, ml)
 		testapps.ClearResources(&testCtx, generics.StorageProviderSignature, ml)
+		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.PersistentVolumeSignature, true, ml)
 	}
 
 	BeforeEach(func() {

diff --git a/pkg/controllerutil/volumesnapshot.go b/pkg/controllerutil/volumesnapshot.go
@@ -25,7 +25,7 @@ import (
 
 	snapshotv1beta1 "github.com/kubernetes-csi/external-snapshotter/client/v3/apis/volumesnapshot/v1beta1"
 	snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
-	storagev1 "k8s.io/api/storage/v1"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -165,12 +165,17 @@ func typeofV1Beta1(v any) any {
 }
 
 // IsVolumeSnapshotEnabled checks if the CSI supports the volume snapshot.
-func IsVolumeSnapshotEnabled(ctx context.Context, cli client.Client, storageClassName string) (bool, error) {
-	storageClass := storagev1.StorageClass{}
-	if err := cli.Get(ctx, types.NamespacedName{Name: storageClassName}, &storageClass); err != nil {
-		return false, client.IgnoreNotFound(err)
+func IsVolumeSnapshotEnabled(ctx context.Context, cli client.Client, pvName string) (bool, error) {
+	if len(pvName) == 0 {
+		return false, nil
+	}
+	pv := &corev1.PersistentVolume{}
+	if err := cli.Get(ctx, types.NamespacedName{Name: pvName}, pv); err != nil {
+		return false, err
+	}
+	if pv.Spec.CSI == nil {
+		return false, nil
 	}
-
 	vsCli := VolumeSnapshotCompatClient{
 		Client: cli,
 		Ctx:    ctx,
@@ -180,7 +185,7 @@ func IsVolumeSnapshotEnabled(ctx context.Context, cli client.Client, storageClas
 		return false, err
 	}
 	for _, vsc := range vscList.Items {
-		if vsc.Driver == storageClass.Provisioner {
+		if vsc.Driver == pv.Spec.CSI.Driver {
 			return true, nil
 		}
 	}

diff --git a/pkg/dataprotection/action/action_create_vs.go b/pkg/dataprotection/action/action_create_vs.go
@@ -186,7 +186,7 @@ func (c *CreateVolumeSnapshotAction) createVolumeSnapshotIfNotExist(ctx Context,
 
 	msg := fmt.Sprintf("creating volume snapshot %s/%s", snap.Namespace, snap.Name)
 	ctx.Recorder.Event(c.Owner, corev1.EventTypeNormal, "CreatingVolumeSnapshot", msg)
-	if err = ctx.Client.Create(ctx.Ctx, snap); err != nil {
+	if err = vsCli.Create(snap); err != nil {
 		return err
 	}
 	return nil

diff --git a/pkg/dataprotection/backup/request_test.go b/pkg/dataprotection/backup/request_test.go
@@ -78,6 +78,7 @@ var _ = Describe("Request Test", func() {
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.BackupRepoSignature, true, ml)
 		testapps.ClearResources(&testCtx, generics.StorageProviderSignature, ml)
 		testapps.ClearResources(&testCtx, generics.VolumeSnapshotClassSignature, ml)
+		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.PersistentVolumeSignature, true, ml)
 	}
 
 	var clusterInfo *testdp.BackupClusterInfo

diff --git a/pkg/dataprotection/restore/manager_test.go b/pkg/dataprotection/restore/manager_test.go
@@ -66,6 +66,7 @@ var _ = Describe("Backup Deleter Test", func() {
 		// non-namespaced
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.ActionSetSignature, true, ml)
 		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.StorageClassSignature, true, ml)
+		testapps.ClearResourcesWithRemoveFinalizerOption(&testCtx, generics.PersistentVolumeSignature, true, ml)
 	}
 
 	BeforeEach(func() {

diff --git a/pkg/dataprotection/utils/utils.go b/pkg/dataprotection/utils/utils.go
@@ -163,24 +163,17 @@ func VolumeSnapshotEnabled(ctx context.Context, cli client.Client, pod *corev1.P
 		return false, fmt.Errorf(`can not find any volume by targetVolumes %v on pod "%s"`, volumes, pod.Name)
 	}
 	// get the storageClass by pvc
-	storageClassMap := map[string]string{}
 	for i := range pvcNames {
 		pvc := &corev1.PersistentVolumeClaim{}
 		if err := cli.Get(ctx, types.NamespacedName{Name: pvcNames[i], Namespace: pod.Namespace}, pvc); err != nil {
 			return false, nil
 		}
-		if pvc.Spec.StorageClassName == nil {
-			return false, nil
-		}
-		storageClassMap[*pvc.Spec.StorageClassName] = pvcNames[i]
-	}
-	for k := range storageClassMap {
-		enabled, err := intctrlutil.IsVolumeSnapshotEnabled(ctx, cli, k)
+		enabled, err := intctrlutil.IsVolumeSnapshotEnabled(ctx, cli, pvc.Spec.VolumeName)
 		if err != nil {
 			return false, err
 		}
 		if !enabled {
-			return false, fmt.Errorf(`cannot find any VolumeSnapshotClass of persistentVolumeClaim "%s" to do volume snapshot on pod "%s"`, storageClassMap[k], pod.Name)
+			return false, fmt.Errorf(`cannot find any VolumeSnapshotClass of persistentVolumeClaim "%s" to do volume snapshot on pod "%s"`, pvc.Name, pod.Name)
 		}
 	}
 	return true, nil

diff --git a/pkg/testutil/apps/native_object_util.go b/pkg/testutil/apps/native_object_util.go
@@ -89,7 +89,7 @@ func CreateVolumeSnapshotClass(testCtx *testutil.TestContext) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "default-vs",
 		},
-		Driver:         testutil.DefaultStorageProvisoner,
+		Driver:         testutil.DefaultCSIDriver,
 		DeletionPolicy: vsv1.VolumeSnapshotContentDelete,
 	}
 	CreateK8sResource(testCtx, volumeSnapshotClass)

diff --git a/pkg/testutil/apps/pv_factoy.go b/pkg/testutil/apps/pv_factoy.go
@@ -76,3 +76,13 @@ func (f *MockPersistentVolumeFactory) SetClaimRef(obj client.Object) *MockPersis
 	}
 	return f
 }
+
+func (f *MockPersistentVolumeFactory) SetCSIDriver(driverName string) *MockPersistentVolumeFactory {
+	f.Get().Spec.CSI = &corev1.CSIPersistentVolumeSource{
+		Driver:       driverName,
+		VolumeHandle: f.Get().Name,
+	}
+	// clear default persistentVolumeSource
+	f.Get().Spec.PersistentVolumeSource.HostPath = nil
+	return f
+}
diff --git a/pkg/testutil/dataprotection/backup_utils.go b/pkg/testutil/dataprotection/backup_utils.go
@@ -124,12 +124,19 @@ func NewFakeBackup(testCtx *testutil.TestContext,
 }
 
 func NewFakeCluster(testCtx *testutil.TestContext) *BackupClusterInfo {
-	createPVC := func(name string) *corev1.PersistentVolumeClaim {
-		return testapps.NewPersistentVolumeClaimFactory(
+	createPVCAndPV := func(name string) *corev1.PersistentVolumeClaim {
+		pvName := "pv-" + name
+		pvc := testapps.NewPersistentVolumeClaimFactory(
 			testCtx.DefaultNamespace, name, ClusterName, ComponentName, "data").
+			SetVolumeName(pvName).
 			SetStorage("1Gi").
 			SetStorageClass(StorageClassName).
 			Create(testCtx).GetObject()
+
+		testapps.NewPersistentVolumeFactory(testCtx.DefaultNamespace, pvName, name).
+			SetStorage("1Gi").
+			SetClaimRef(pvc).SetCSIDriver(testutil.DefaultCSIDriver).Create(testCtx)
+		return pvc
 	}
 
 	podFactory := func(name string) *testapps.MockPodFactory {
@@ -148,10 +155,10 @@ func NewFakeCluster(testCtx *testutil.TestContext) *BackupClusterInfo {
 	_ = testapps.CreateStorageClass(testCtx, StorageClassName, true)
 
 	By("mocking a pvc belonging to the pod 0")
-	pvc := createPVC("data-" + podName + "-0")
+	pvc := createPVCAndPV("data-" + podName + "-0")
 
 	By("mocking a pvc belonging to the pod 1")
-	pvc1 := createPVC("data-" + podName + "-1")
+	pvc1 := createPVCAndPV("data-" + podName + "-1")
 
 	By("mocking pod 0 belonging to the statefulset")
 	volume := corev1.Volume{Name: DataVolumeName, VolumeSource: corev1.VolumeSource{

diff --git a/pkg/testutil/type.go b/pkg/testutil/type.go
@@ -60,6 +60,7 @@ const (
 	envExistingClusterType   = "EXISTING_CLUSTER_TYPE"
 	envUseExistingCluster    = "USE_EXISTING_CLUSTER"
 	DefaultStorageProvisoner = "kubernetes.io/no-provisioner"
+	DefaultCSIDriver         = "hostpath.csi.k8s.io"
 )
 
 func init() {