From 50f8af48070ed3e43ab466fe7321071ca02d77a8 Mon Sep 17 00:00:00 2001 From: wangyelei Date: Wed, 25 Oct 2023 18:07:38 +0800 Subject: [PATCH] fix: create snapshot failed if storageClass's provisioner not equals to driver name (#5620) --- .../components/hscale_volume_populator.go | 5 +--- .../dataprotection/backup_controller.go | 25 +++++++++++-------- .../dataprotection/backup_controller_test.go | 3 ++- .../backupschedule_controller_test.go | 1 + .../dataprotection/gc_controller_test.go | 1 + .../dataprotection/restore_controller_test.go | 1 + pkg/controllerutil/volumesnapshot.go | 19 ++++++++------ pkg/dataprotection/action/action_create_vs.go | 2 +- pkg/dataprotection/backup/request_test.go | 1 + pkg/dataprotection/restore/manager_test.go | 1 + pkg/dataprotection/utils/utils.go | 11 ++------ pkg/testutil/apps/native_object_util.go | 2 +- pkg/testutil/apps/pv_factoy.go | 10 ++++++++ pkg/testutil/dataprotection/backup_utils.go | 15 ++++++++--- pkg/testutil/type.go | 1 + 15 files changed, 61 insertions(+), 37 deletions(-) diff --git a/controllers/apps/components/hscale_volume_populator.go b/controllers/apps/components/hscale_volume_populator.go index c5da849b834..131b0d2d74d 100644 --- 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 index 0fdfd6cd15b..a243af1d3fd 100644 --- 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 index bc7305ecb35..a7fa1d497e3 100644 --- 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 index 11519dcd6ba..a3a54893860 100644 --- 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 index 7073b85ca52..c2571af0dc1 100644 --- 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 index cd8edeb48f3..49aa2944ac4 100644 --- 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 index bfb17334e44..6425eddda8b 100644 --- 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 index 318bc0bbc24..ccd763f741d 100644 --- 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 index 02213b477b5..bf7d7cc98eb 100644 --- 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 index ee3b8bdeeeb..5228c15edb4 100644 --- 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 index d26471e37c6..5f067420065 100644 --- 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 index 68ff0523945..11d6aa16d01 100644 --- 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 index 345063e4cd1..a2d5b0018c8 100644 --- 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 index 40da7660bb2..59b488187e7 100644 --- 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 index 00c8e3bc056..5c2914c3a8b 100644 --- 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() {