fix: backup failed for pgbasebackup and mongodump (#5534)

apecloud · Oct 19, 2023 · 5a1d86b · 5a1d86b
1 parent ebb2c9f
commit 5a1d86b
Show file tree

Hide file tree

Showing 9 changed files with 95 additions and 46 deletions.
diff --git a/apis/dataprotection/v1alpha1/actionset_types.go b/apis/dataprotection/v1alpha1/actionset_types.go
@@ -174,7 +174,8 @@ type JobActionSpec struct {
 
 	// runOnTargetPodNode specifies whether to run the job workload on the
 	// target pod node. If backup container should mount the target pod's
-	// volume, this field should be set to true.
+	// volumes, this field should be set to true. otherwise the target pod's
+	// volumes will be ignored.
 	// +optional
 	// +kubebuilder:default=false
 	RunOnTargetPodNode *bool `json:"runOnTargetPodNode,omitempty"`

diff --git a/config/crd/bases/dataprotection.kubeblocks.io_actionsets.yaml b/config/crd/bases/dataprotection.kubeblocks.io_actionsets.yaml
@@ -76,8 +76,9 @@ spec:
                         default: false
                         description: runOnTargetPodNode specifies whether to run the
                           job workload on the target pod node. If backup container
-                          should mount the target pod's volume, this field should
-                          be set to true.
+                          should mount the target pod's volumes, this field should
+                          be set to true. otherwise the target pod's volumes will
+                          be ignored.
                         type: boolean
                       syncProgress:
                         description: syncProgress specifies whether to sync the backup
@@ -163,8 +164,9 @@ spec:
                               default: false
                               description: runOnTargetPodNode specifies whether to
                                 run the job workload on the target pod node. If backup
-                                container should mount the target pod's volume, this
-                                field should be set to true.
+                                container should mount the target pod's volumes, this
+                                field should be set to true. otherwise the target
+                                pod's volumes will be ignored.
                               type: boolean
                           required:
                           - command
@@ -236,8 +238,9 @@ spec:
                               default: false
                               description: runOnTargetPodNode specifies whether to
                                 run the job workload on the target pod node. If backup
-                                container should mount the target pod's volume, this
-                                field should be set to true.
+                                container should mount the target pod's volumes, this
+                                field should be set to true. otherwise the target
+                                pod's volumes will be ignored.
                               type: boolean
                           required:
                           - command
@@ -485,8 +488,9 @@ spec:
                               default: false
                               description: runOnTargetPodNode specifies whether to
                                 run the job workload on the target pod node. If backup
-                                container should mount the target pod's volume, this
-                                field should be set to true.
+                                container should mount the target pod's volumes, this
+                                field should be set to true. otherwise the target
+                                pod's volumes will be ignored.
                               type: boolean
                           required:
                           - command
@@ -518,8 +522,9 @@ spec:
                         default: false
                         description: runOnTargetPodNode specifies whether to run the
                           job workload on the target pod node. If backup container
-                          should mount the target pod's volume, this field should
-                          be set to true.
+                          should mount the target pod's volumes, this field should
+                          be set to true. otherwise the target pod's volumes will
+                          be ignored.
                         type: boolean
                     required:
                     - command

diff --git a/deploy/helm/crds/dataprotection.kubeblocks.io_actionsets.yaml b/deploy/helm/crds/dataprotection.kubeblocks.io_actionsets.yaml
@@ -76,8 +76,9 @@ spec:
                         default: false
                         description: runOnTargetPodNode specifies whether to run the
                           job workload on the target pod node. If backup container
-                          should mount the target pod's volume, this field should
-                          be set to true.
+                          should mount the target pod's volumes, this field should
+                          be set to true. otherwise the target pod's volumes will
+                          be ignored.
                         type: boolean
                       syncProgress:
                         description: syncProgress specifies whether to sync the backup
@@ -163,8 +164,9 @@ spec:
                               default: false
                               description: runOnTargetPodNode specifies whether to
                                 run the job workload on the target pod node. If backup
-                                container should mount the target pod's volume, this
-                                field should be set to true.
+                                container should mount the target pod's volumes, this
+                                field should be set to true. otherwise the target
+                                pod's volumes will be ignored.
                               type: boolean
                           required:
                           - command
@@ -236,8 +238,9 @@ spec:
                               default: false
                               description: runOnTargetPodNode specifies whether to
                                 run the job workload on the target pod node. If backup
-                                container should mount the target pod's volume, this
-                                field should be set to true.
+                                container should mount the target pod's volumes, this
+                                field should be set to true. otherwise the target
+                                pod's volumes will be ignored.
                               type: boolean
                           required:
                           - command
@@ -485,8 +488,9 @@ spec:
                               default: false
                               description: runOnTargetPodNode specifies whether to
                                 run the job workload on the target pod node. If backup
-                                container should mount the target pod's volume, this
-                                field should be set to true.
+                                container should mount the target pod's volumes, this
+                                field should be set to true. otherwise the target
+                                pod's volumes will be ignored.
                               type: boolean
                           required:
                           - command
@@ -518,8 +522,9 @@ spec:
                         default: false
                         description: runOnTargetPodNode specifies whether to run the
                           job workload on the target pod node. If backup container
-                          should mount the target pod's volume, this field should
-                          be set to true.
+                          should mount the target pod's volumes, this field should
+                          be set to true. otherwise the target pod's volumes will
+                          be ignored.
                         type: boolean
                     required:
                     - command

diff --git a/deploy/mongodb/templates/backuppolicytemplate.yaml b/deploy/mongodb/templates/backuppolicytemplate.yaml
@@ -11,7 +11,7 @@ spec:
   - componentDefRef: mongodb
     retentionPeriod: 7d
     target:
-      role: follower
+      role: secondary
     backupMethods:
     - name: datafile
       snapshotVolumes: false

diff --git a/pkg/dataprotection/backup/request.go b/pkg/dataprotection/backup/request.go
@@ -302,27 +302,33 @@ func (r *Request) buildJobActionPodSpec(name string, job *dpv1alpha1.JobActionSp
 	}
 
 	buildVolumes := func() []corev1.Volume {
-		return append(
-			[]corev1.Volume{
-				{
-					Name: syncProgressSharedVolumeName,
-					VolumeSource: corev1.VolumeSource{
-						EmptyDir: &corev1.EmptyDirVolumeSource{},
-					},
+		volumes := []corev1.Volume{
+			{
+				Name: syncProgressSharedVolumeName,
+				VolumeSource: corev1.VolumeSource{
+					EmptyDir: &corev1.EmptyDirVolumeSource{},
 				},
 			},
-			getVolumesByVolumeInfo(targetPod, r.BackupMethod.TargetVolumes)...)
+		}
+		// only mount the volumes when the backup pod is running on the target pod node.
+		if boolptr.IsSetToTrue(job.RunOnTargetPodNode) {
+			volumes = append(volumes, getVolumesByVolumeInfo(targetPod, r.BackupMethod.TargetVolumes)...)
+		}
+		return volumes
 	}
 
 	buildVolumeMounts := func() []corev1.VolumeMount {
-		return append(
-			[]corev1.VolumeMount{
-				{
-					Name:      syncProgressSharedVolumeName,
-					MountPath: syncProgressSharedMountPath,
-				},
+		volumesMount := []corev1.VolumeMount{
+			{
+				Name:      syncProgressSharedVolumeName,
+				MountPath: syncProgressSharedMountPath,
 			},
-			getVolumeMountsByVolumeInfo(targetPod, r.BackupMethod.TargetVolumes)...)
+		}
+		// only mount the volumes when the backup pod is running on the target pod node.
+		if boolptr.IsSetToTrue(job.RunOnTargetPodNode) {
+			volumesMount = append(volumesMount, getVolumeMountsByVolumeInfo(targetPod, r.BackupMethod.TargetVolumes)...)
+		}
+		return volumesMount
 	}
 
 	runAsUser := int64(0)

diff --git a/pkg/dataprotection/restore/manager.go b/pkg/dataprotection/restore/manager.go
@@ -407,16 +407,17 @@ func (r *RestoreManager) BuildPostReadyActionJobs(reqCtx intctrlutil.RequestCtx,
 			return nil, err
 		}
 		targetPod := targetPodList[0]
-		for _, volumeMount := range jobAction.Target.VolumeMounts {
-			for _, volume := range targetPod.Spec.Volumes {
-				if volume.Name != volumeMount.Name {
-					continue
-				}
-				jobBuilder.addToSpecificVolumesAndMounts(&volume, &volumeMount)
-			}
-		}
 		if boolptr.IsSetToTrue(actionSpec.Job.RunOnTargetPodNode) {
 			jobBuilder.setNodeNameToNodeSelector(targetPod.Spec.NodeName)
+			// mount the targe pod's volumes when RunOnTargetPodNode is true
+			for _, volumeMount := range jobAction.Target.VolumeMounts {
+				for _, volume := range targetPod.Spec.Volumes {
+					if volume.Name != volumeMount.Name {
+						continue
+					}
+					jobBuilder.addToSpecificVolumesAndMounts(&volume, &volumeMount)
+				}
+			}
 		}
 		job := jobBuilder.setImage(actionSpec.Job.Image).
 			setJobName(buildJobName(0)).

diff --git a/pkg/dataprotection/restore/manager_test.go b/pkg/dataprotection/restore/manager_test.go
@@ -163,6 +163,17 @@ var _ = Describe("Backup Deleter Test", func() {
 			}
 		}
 
+		checkVolumes := func(job *batchv1.Job, volumeName string, exist bool) {
+			var volumeExist bool
+			for _, v := range job.Spec.Template.Spec.Volumes {
+				if v.Name == volumeName {
+					volumeExist = true
+					break
+				}
+			}
+			Expect(volumeExist).Should(Equal(exist))
+		}
+
 		It("test with RestorePVCFromSnapshot function", func() {
 			reqCtx := getReqCtx()
 			startingIndex := 0
@@ -269,7 +280,7 @@ var _ = Describe("Backup Deleter Test", func() {
 			Expect(job).ShouldNot(BeNil())
 		})
 
-		It("test with BuildPostReadyActionJobs function", func() {
+		testPostReady := func(existVolume bool) {
 			reqCtx := getReqCtx()
 			matchLabels := map[string]string{
 				constant.AppInstanceLabelKey: testdp.ClusterName,
@@ -294,8 +305,21 @@ var _ = Describe("Backup Deleter Test", func() {
 			Expect(err).ShouldNot(HaveOccurred())
 			// count of job should equal to 1
 			Expect(len(jobs)).Should(Equal(1))
+
+			checkVolumes(jobs[0], testdp.DataVolumeName, existVolume)
+		}
+
+		It("test with BuildPostReadyActionJobs function and run target pod node", func() {
+			testPostReady(true)
 		})
 
+		It("test with BuildPostReadyActionJobs function and no need to run target pod node", func() {
+			Expect(testapps.ChangeObj(&testCtx, actionSet, func(set *dpv1alpha1.ActionSet) {
+				runTargetPodNode := false
+				actionSet.Spec.Restore.PostReady[1].Job.RunOnTargetPodNode = &runTargetPodNode
+			})).Should(Succeed())
+			testPostReady(false)
+		})
 	})
 
 })
diff --git a/pkg/testutil/dataprotection/restore_factory.go b/pkg/testutil/dataprotection/restore_factory.go
@@ -162,6 +162,12 @@ func (f *MockRestoreFactory) SetJobActionConfig(matchLabels map[string]string) *
 			PodSelector: metav1.LabelSelector{
 				MatchLabels: matchLabels,
 			},
+			VolumeMounts: []corev1.VolumeMount{
+				{
+					Name:      DataVolumeName,
+					MountPath: DataVolumeMountPath,
+				},
+			},
 		},
 	}
 	return f

diff --git a/test/testdata/backup/actionset.yaml b/test/testdata/backup/actionset.yaml
@@ -45,6 +45,7 @@ spec:
         container: postgres
     - job:
         image: infracreate-registry.cn-zhangjiakou.cr.aliyuncs.com/apecloud/percona-xtrabackup:$(IMAGE_TAG)
+        runOnTargetPodNode: true
         command:
         - sh
         - -c