cherry pick #3002 to release-1.1 (#3018)

Signed-off-by: ti-srebot <[email protected]>

Co-authored-by: Chunzhu Li <[email protected]>

cmd/backup-manager/app/backup/manager.go

@@ -157,15 +157,7 @@ func (bm *Manager) performBackup(backup *v1alpha1.Backup, db *sql.DB) error {
 		Status: corev1.ConditionTrue,
 	})
 	if err != nil {
-		errs = append(errs, err)
-		uerr := bm.StatusUpdater.Update(backup, &v1alpha1.BackupCondition{
-			Type:    v1alpha1.BackupFailed,
-			Status:  corev1.ConditionTrue,
-			Reason:  "UpdatePrepareBackupFailed",
-			Message: err.Error(),
-		})
-		errs = append(errs, uerr)
-		return errorutils.NewAggregate(errs)
+		return err
 	}
 
 	oldTikvGCTime, err := bm.GetTikvGCLifeTime(db)

cmd/backup-manager/app/clean/clean.go

@@ -65,16 +65,16 @@ func (bo *Options) cleanBRRemoteBackupData(backup *v1alpha1.Backup) error {
 
 func (bo *Options) cleanRemoteBackupData(bucket string, opts []string) error {
 	destBucket := util.NormalizeBucketURI(bucket)
-	args := util.ConstructArgs(constants.RcloneConfigArg, opts, "deletefile", destBucket, "")
+	args := util.ConstructArgs(constants.RcloneConfigArg, opts, "delete", destBucket, "")
 	output, err := exec.Command("rclone", args...).CombinedOutput()
 	if err != nil {
-		if exitError, ok := err.(*exec.ExitError); ok {
-			if code := exitError.ExitCode(); code == 3 || code == 4 {
-				klog.Infof("cluster %s backup %s has already been deleted before", bo, bucket)
-				return nil
-			}
-		}
-		return fmt.Errorf("cluster %s, execute rclone deletefile command failed, output: %s, err: %v", bo, string(output), err)
+		return fmt.Errorf("cluster %s, execute rclone delete command failed, output: %s, err: %v", bo, string(output), err)
+	}
+
+	args = util.ConstructArgs(constants.RcloneConfigArg, opts, "delete", fmt.Sprintf("%s.tmp", destBucket), "")
+	output, err = exec.Command("rclone", args...).CombinedOutput()
+	if err != nil {
+		return fmt.Errorf("cluster %s, execute rclone delete command failed, output: %s, err: %v", bo, string(output), err)
 	}
 
 	klog.Infof("cluster %s backup %s was deleted successfully", bo, bucket)

cmd/backup-manager/app/export/manager.go

@@ -323,6 +323,15 @@ func (bm *BackupManager) performBackup(backup *v1alpha1.Backup, db *sql.DB) erro
 
 	remotePath := strings.TrimPrefix(archiveBackupPath, constants.BackupRootPath+"/")
 	bucketURI := bm.getDestBucketURI(remotePath)
+	backup.Status.BackupPath = bucketURI
+	err = bm.StatusUpdater.Update(backup, &v1alpha1.BackupCondition{
+		Type:   v1alpha1.BackupPrepare,
+		Status: corev1.ConditionTrue,
+	})
+	if err != nil {
+		return err
+	}
+
 	err = bm.backupDataToRemote(archiveBackupPath, bucketURI, opts)
 	if err != nil {
 		errs = append(errs, err)
@@ -342,7 +351,6 @@ func (bm *BackupManager) performBackup(backup *v1alpha1.Backup, db *sql.DB) erro
 
 	finish := time.Now()
 
-	backup.Status.BackupPath = bucketURI
 	backup.Status.TimeStarted = metav1.Time{Time: started}
 	backup.Status.TimeCompleted = metav1.Time{Time: finish}
 	backup.Status.BackupSize = size

docs/api-references/docs.md

@@ -263,13 +263,15 @@ string
 </tr>
 <tr>
 <td>
-<code>cleanData</code></br>
+<code>cleanPolicy</code></br>
 <em>
-bool
+<a href="#cleanpolicytype">
+CleanPolicyType
+</a>
 </em>
 </td>
 <td>
-<p>CleanData denotes whether to clean backup data before the object is deleted from the cluster</p>
+<p>CleanPolicy denotes whether to clean backup data when the object is deleted from the cluster, if not set, the backup data will be retained</p>
 </td>
 </tr>
 </table>
@@ -2374,13 +2376,15 @@ string
 </tr>
 <tr>
 <td>
-<code>cleanData</code></br>
+<code>cleanPolicy</code></br>
 <em>
-bool
+<a href="#cleanpolicytype">
+CleanPolicyType
+</a>
 </em>
 </td>
 <td>
-<p>CleanData denotes whether to clean backup data before the object is deleted from the cluster</p>
+<p>CleanPolicy denotes whether to clean backup data when the object is deleted from the cluster, if not set, the backup data will be retained</p>
 </td>
 </tr>
 </tbody>
@@ -2759,6 +2763,14 @@ Optional: Defaults to range</p>
 </tr>
 </tbody>
 </table>
+<h3 id="cleanpolicytype">CleanPolicyType</h3>
+<p>
+(<em>Appears on:</em>
+<a href="#backupspec">BackupSpec</a>)
+</p>
+<p>
+<p>CleanPolicyType represents the clean policy of backup data in remote storage</p>
+</p>
 <h3 id="commonconfig">CommonConfig</h3>
 <p>
 (<em>Appears on:</em>

manifests/backup/backup-aws-s3-br.yaml

@@ -10,7 +10,7 @@ spec:
   # backupType: full
   # useKMS: false
   # serviceAccount: myServiceAccount
-  # cleanData: true
+  # cleanPolicy: OnFailure
   br:
     cluster: myCluster
     # clusterNamespce: <backup-namespace>

manifests/backup/backup-gcs-br.yaml

@@ -7,7 +7,7 @@ metadata:
 spec:
   # backupType: full
   # serviceAccount: myServiceAccount
-  # cleanData: true
+  # cleanPolicy: OnFailure
   br:
     cluster: mycluster
     sendCredToTikv: true

manifests/backup/backup-gcs.yaml

@@ -5,7 +5,7 @@ metadata:
   name: demo1-backup-gcs
   namespace: test1
 spec:
-  # cleanData: true
+  # cleanPolicy: OnFailure
   # resources:
   #   limits:
   #     cpu: 300m

manifests/backup/backup-s3-br.yaml

@@ -10,7 +10,7 @@ spec:
   # backupType: full
   # useKMS: false
   # serviceAccount: myServiceAccount
-  # cleanData: true
+  # cleanPolicy: OnFailure
   # resources:
   #   limits:
   #     cpu: 300m

manifests/backup/backup-s3.yaml

@@ -5,7 +5,7 @@ metadata:
   name: demo1-backup-s3
   namespace: test1
 spec:
-  # cleanData: true
+  # cleanPolicy: OnFailure
   from:
     host: 10.233.10.242
     port: 4000

manifests/backup/backup-schedule-aws-s3-br.yaml

@@ -15,7 +15,7 @@ spec:
     #backupType: full
     # useKMS: false
     # serviceAccount: myServiceAccount
-    # cleanData: true
+    # cleanPolicy: OnFailure
     br:
       cluster: myCluster
       # clusterNamespce: backupNamespace

manifests/backup/backup-schedule-gcs-br.yaml

@@ -12,7 +12,7 @@ spec:
   backupTemplate:
     #backupType: full
     # serviceAccount: myServiceAccount
-    # cleanData: true
+    # cleanPolicy: OnFailure
     br:
       cluster: myCluster
       sendCredToTikv: true

manifests/backup/backup-schedule-gcs.yaml

@@ -12,7 +12,7 @@ spec:
   storageSize: 10Gi
   schedule: "*/2 * * * *"
   backupTemplate:
-    # cleanData: true
+    # cleanPolicy: OnFailure
     from:
       host: 10.0.0.1
       port: 4000

manifests/backup/backup-schedule-s3-br.yaml

@@ -15,7 +15,7 @@ spec:
     #backupType: full
     # useKMS: false
     # serviceAccount: myServiceAccount
-    # cleanData: true
+    # cleanPolicy: OnFailure
     br:
       cluster: myCluster
       # clusterNamespce: backupNamespace

manifests/backup/backup-schedule-s3.yaml

@@ -12,7 +12,7 @@ spec:
   storageSize: 10Gi
   schedule: "*/2 * * * *"
   backupTemplate:
-    # cleanData: true
+    # cleanPolicy: OnFailure
     from:
       host: 10.0.0.1
       port: 4000

manifests/crd.yaml

@@ -11616,8 +11616,8 @@ spec:
               required:
               - cluster
               type: object
-            cleanData:
-              type: boolean
+            cleanPolicy:
+              type: string
             dumpling:
               properties:
                 options:
@@ -12517,8 +12517,8 @@ spec:
                   required:
                   - cluster
                   type: object
-                cleanData:
-                  type: boolean
+                cleanPolicy:
+                  type: string
                 dumpling:
                   properties:
                     options:

pkg/apis/pingcap/v1alpha1/backup.go

@@ -121,3 +121,18 @@ func IsBackupClean(backup *Backup) bool {
 	_, condition := GetBackupCondition(&backup.Status, BackupClean)
 	return condition != nil && condition.Status == corev1.ConditionTrue
 }
+
+// IsCleanCandidate returns true if a Backup should be added to clean candidate according to cleanPolicy
+func IsCleanCandidate(backup *Backup) bool {
+	switch backup.Spec.CleanPolicy {
+	case CleanPolicyTypeDelete, CleanPolicyTypeOnFailure:
+		return true
+	default:
+		return false
+	}
+}
+
+// NeedNotClean returns true if a Backup need not to be cleaned up according to cleanPolicy
+func NeedNotClean(backup *Backup) bool {
+	return backup.Spec.CleanPolicy == CleanPolicyTypeOnFailure && !IsBackupFailed(backup)
+}

pkg/apis/pingcap/v1alpha1/types.go

@@ -1103,6 +1103,19 @@ type TiDBAccessConfig struct {
 	TLSClientSecretName *string `json:"tlsClientSecretName,omitempty"`
 }
 
+// +k8s:openapi-gen=true
+// CleanPolicyType represents the clean policy of backup data in remote storage
+type CleanPolicyType string
+
+const (
+	// CleanPolicyTypeRetain represents that the backup data in remote storage will be retained when the Backup CR is deleted
+	CleanPolicyTypeRetain CleanPolicyType = "Retain"
+	// CleanPolicyTypeOnFailure represents that the backup data in remote storage will be cleaned only for the failed backups when the Backup CR is deleted
+	CleanPolicyTypeOnFailure CleanPolicyType = "OnFailure"
+	// CleanPolicyTypeIfFailed represents that the backup data in remote storage will be cleaned when the Backup CR is deleted
+	CleanPolicyTypeDelete CleanPolicyType = "Delete"
+)
+
 // +k8s:openapi-gen=true
 // BackupSpec contains the backup specification for a tidb cluster.
 type BackupSpec struct {
@@ -1140,8 +1153,8 @@ type BackupSpec struct {
 	UseKMS bool `json:"useKMS,omitempty"`
 	// Specify service account of backup
 	ServiceAccount string `json:"serviceAccount,omitempty"`
-	// CleanData denotes whether to clean backup data before the object is deleted from the cluster
-	CleanData bool `json:"cleanData,omitempty"`
+	// CleanPolicy denotes whether to clean backup data when the object is deleted from the cluster, if not set, the backup data will be retained
+	CleanPolicy CleanPolicyType `json:"cleanPolicy,omitempty"`
 }
 
 // +k8s:openapi-gen=true

pkg/backup/backup/backup_cleaner.go

@@ -57,8 +57,8 @@ func NewBackupCleaner(
 }
 
 func (bc *backupCleaner) Clean(backup *v1alpha1.Backup) error {
-	if backup.DeletionTimestamp == nil || !backup.Spec.CleanData {
-		// The backup object has not been deleted，do nothing
+	if backup.DeletionTimestamp == nil || !v1alpha1.IsCleanCandidate(backup) || v1alpha1.NeedNotClean(backup) {
+		// The backup object has not been deleted or we need to retain backup data，do nothing
 		return nil
 	}
 	ns := backup.GetNamespace()
@@ -80,6 +80,7 @@ func (bc *backupCleaner) Clean(backup *v1alpha1.Backup) error {
 			Status: corev1.ConditionTrue,
 		})
 	}
+
 	// not found clean job, create it
 	job, reason, err := bc.makeCleanJob(backup)
 	if err != nil {

pkg/controller/backup/backup_control.go

@@ -86,7 +86,7 @@ func (bc *defaultBackupControl) removeProtectionFinalizer(backup *v1alpha1.Backu
 	ns := backup.GetNamespace()
 	name := backup.GetName()
 
-	if backup.Spec.CleanData && isDeletionCandidate(backup) && v1alpha1.IsBackupClean(backup) {
+	if needToRemoveFinalizer(backup) {
 		backup.Finalizers = slice.RemoveString(backup.Finalizers, label.BackupProtectionFinalizer, nil)
 		_, err := bc.cli.PingcapV1alpha1().Backups(ns).Update(backup)
 		if err != nil {
@@ -98,7 +98,12 @@ func (bc *defaultBackupControl) removeProtectionFinalizer(backup *v1alpha1.Backu
 }
 
 func needToAddFinalizer(backup *v1alpha1.Backup) bool {
-	return backup.DeletionTimestamp == nil && backup.Spec.CleanData && !slice.ContainsString(backup.Finalizers, label.BackupProtectionFinalizer, nil)
+	return backup.DeletionTimestamp == nil && v1alpha1.IsCleanCandidate(backup) && !slice.ContainsString(backup.Finalizers, label.BackupProtectionFinalizer, nil)
+}
+
+func needToRemoveFinalizer(backup *v1alpha1.Backup) bool {
+	return v1alpha1.IsCleanCandidate(backup) && isDeletionCandidate(backup) &&
+		(v1alpha1.IsBackupClean(backup) || v1alpha1.NeedNotClean(backup))
 }
 
 func isDeletionCandidate(backup *v1alpha1.Backup) bool {

pkg/controller/backup/backup_controller_test.go

@@ -288,7 +288,7 @@ func newBackup() *v1alpha1.Backup {
 			},
 			StorageClassName: pointer.StringPtr("local-storage"),
 			StorageSize:      "1Gi",
-			CleanData:        true,
+			CleanPolicy:      v1alpha1.CleanPolicyTypeDelete,
 		},
 	}
 }

tests/pkg/fixture/fixture.go

@@ -437,7 +437,7 @@ func GetBackupCRDWithS3(tc *v1alpha1.TidbCluster, fromSecretName, brType string,
 				ClusterNamespace: tc.GetNamespace(),
 				SendCredToTikv:   &sendCredToTikv,
 			},
-			CleanData: true,
+			CleanPolicy: v1alpha1.CleanPolicyTypeDelete,
 		},
 	}
 	if brType == DumperType {