diff --git a/cmd/backup-manager/app/backup/manager.go b/cmd/backup-manager/app/backup/manager.go index c4888f076a..c9e4d6073e 100644 --- a/cmd/backup-manager/app/backup/manager.go +++ b/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) diff --git a/cmd/backup-manager/app/clean/clean.go b/cmd/backup-manager/app/clean/clean.go index f12a18972b..495c7c4a6f 100644 --- a/cmd/backup-manager/app/clean/clean.go +++ b/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) diff --git a/cmd/backup-manager/app/export/manager.go b/cmd/backup-manager/app/export/manager.go index 4e2e323b41..2c1dc613a1 100644 --- a/cmd/backup-manager/app/export/manager.go +++ b/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 diff --git a/docs/api-references/docs.md b/docs/api-references/docs.md index 1b1f731a73..1cb235af5b 100644 --- a/docs/api-references/docs.md +++ b/docs/api-references/docs.md @@ -267,13 +267,15 @@ string -cleanData
+cleanPolicy
-bool + +CleanPolicyType + -

CleanData denotes whether to clean backup data before the object is deleted from the cluster

+

CleanPolicy denotes whether to clean backup data when the object is deleted from the cluster, if not set, the backup data will be retained

@@ -2580,13 +2582,15 @@ string -cleanData
+cleanPolicy
-bool + +CleanPolicyType + -

CleanData denotes whether to clean backup data before the object is deleted from the cluster

+

CleanPolicy denotes whether to clean backup data when the object is deleted from the cluster, if not set, the backup data will be retained

@@ -2965,6 +2969,14 @@ Optional: Defaults to range

+

CleanPolicyType

+

+(Appears on: +BackupSpec) +

+

+

CleanPolicyType represents the clean policy of backup data in remote storage

+

CommonConfig

(Appears on: diff --git a/manifests/backup/backup-aws-s3-br.yaml b/manifests/backup/backup-aws-s3-br.yaml index 00fc18a14f..dfafde4449 100644 --- a/manifests/backup/backup-aws-s3-br.yaml +++ b/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: diff --git a/manifests/backup/backup-gcs-br.yaml b/manifests/backup/backup-gcs-br.yaml index 740b762267..44759f807f 100644 --- a/manifests/backup/backup-gcs-br.yaml +++ b/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 diff --git a/manifests/backup/backup-gcs.yaml b/manifests/backup/backup-gcs.yaml index 0fe01cdb1d..63027772b6 100644 --- a/manifests/backup/backup-gcs.yaml +++ b/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 diff --git a/manifests/backup/backup-s3-br.yaml b/manifests/backup/backup-s3-br.yaml index ea87021b34..2f267ce635 100644 --- a/manifests/backup/backup-s3-br.yaml +++ b/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 diff --git a/manifests/backup/backup-s3.yaml b/manifests/backup/backup-s3.yaml index 1042f86772..1a261ce8ee 100644 --- a/manifests/backup/backup-s3.yaml +++ b/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 diff --git a/manifests/backup/backup-schedule-aws-s3-br.yaml b/manifests/backup/backup-schedule-aws-s3-br.yaml index 69a7ab78a6..7fe1466bac 100644 --- a/manifests/backup/backup-schedule-aws-s3-br.yaml +++ b/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 diff --git a/manifests/backup/backup-schedule-gcs-br.yaml b/manifests/backup/backup-schedule-gcs-br.yaml index 99e877e1d5..0d7023601e 100644 --- a/manifests/backup/backup-schedule-gcs-br.yaml +++ b/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 diff --git a/manifests/backup/backup-schedule-gcs.yaml b/manifests/backup/backup-schedule-gcs.yaml index 8f4b8f7ceb..e8dfac0e14 100644 --- a/manifests/backup/backup-schedule-gcs.yaml +++ b/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 diff --git a/manifests/backup/backup-schedule-s3-br.yaml b/manifests/backup/backup-schedule-s3-br.yaml index acd071a817..23947e694e 100644 --- a/manifests/backup/backup-schedule-s3-br.yaml +++ b/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 diff --git a/manifests/backup/backup-schedule-s3.yaml b/manifests/backup/backup-schedule-s3.yaml index f781eeb214..f3f46fda73 100644 --- a/manifests/backup/backup-schedule-s3.yaml +++ b/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 diff --git a/manifests/crd.yaml b/manifests/crd.yaml index 7a70ffcb5b..c768c6c490 100644 --- a/manifests/crd.yaml +++ b/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: diff --git a/pkg/apis/pingcap/v1alpha1/backup.go b/pkg/apis/pingcap/v1alpha1/backup.go index cb3f4eb48b..ac8da4d24e 100644 --- a/pkg/apis/pingcap/v1alpha1/backup.go +++ b/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) +} diff --git a/pkg/apis/pingcap/v1alpha1/openapi_generated.go b/pkg/apis/pingcap/v1alpha1/openapi_generated.go index 5765c7fe50..62d48fedba 100644 --- a/pkg/apis/pingcap/v1alpha1/openapi_generated.go +++ b/pkg/apis/pingcap/v1alpha1/openapi_generated.go @@ -856,10 +856,10 @@ func schema_pkg_apis_pingcap_v1alpha1_BackupSpec(ref common.ReferenceCallback) c Format: "", }, }, - "cleanData": { + "cleanPolicy": { SchemaProps: spec.SchemaProps{ - Description: "CleanData denotes whether to clean backup data before the object is deleted from the cluster", - Type: []string{"boolean"}, + Description: "CleanPolicy denotes whether to clean backup data when the object is deleted from the cluster, if not set, the backup data will be retained", + Type: []string{"string"}, Format: "", }, }, diff --git a/pkg/apis/pingcap/v1alpha1/types.go b/pkg/apis/pingcap/v1alpha1/types.go index 4618ec513c..6b132d6bbd 100644 --- a/pkg/apis/pingcap/v1alpha1/types.go +++ b/pkg/apis/pingcap/v1alpha1/types.go @@ -1111,6 +1111,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 { @@ -1148,8 +1161,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 diff --git a/pkg/backup/backup/backup_cleaner.go b/pkg/backup/backup/backup_cleaner.go index 1df273cd8e..a872c09e31 100644 --- a/pkg/backup/backup/backup_cleaner.go +++ b/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 { diff --git a/pkg/controller/backup/backup_control.go b/pkg/controller/backup/backup_control.go index 98fd870d24..d97b050fda 100644 --- a/pkg/controller/backup/backup_control.go +++ b/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 { diff --git a/pkg/controller/backup/backup_controller_test.go b/pkg/controller/backup/backup_controller_test.go index ea30be73dc..27959668d1 100644 --- a/pkg/controller/backup/backup_controller_test.go +++ b/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, }, } } diff --git a/tests/pkg/fixture/fixture.go b/tests/pkg/fixture/fixture.go index c25e258a78..e0f7338734 100644 --- a/tests/pkg/fixture/fixture.go +++ b/tests/pkg/fixture/fixture.go @@ -468,7 +468,7 @@ func GetBackupCRDWithS3(tc *v1alpha1.TidbCluster, fromSecretName, brType string, ClusterNamespace: tc.GetNamespace(), SendCredToTikv: &sendCredToTikv, }, - CleanData: true, + CleanPolicy: v1alpha1.CleanPolicyTypeDelete, }, } if brType == DumperType {