address comments

docs/api-references/docs.md

@@ -275,7 +275,7 @@ CleanPolicyType
 </em>
 </td>
 <td>
-<p>CleanPolicy denotes whether to clean backup data when 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>
@@ -2590,7 +2590,7 @@ CleanPolicyType
 </em>
 </td>
 <td>
-<p>CleanPolicy denotes whether to clean backup data when 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>

pkg/apis/pingcap/v1alpha1/backup.go

@@ -122,12 +122,17 @@ func IsBackupClean(backup *Backup) bool {
 	return condition != nil && condition.Status == corev1.ConditionTrue
 }
 
-// ShouldCleanData returns true if a Backup should be cleaned up according to cleanPolicy
-func ShouldCleanData(backup *Backup) bool {
+// 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

@@ -1161,7 +1161,7 @@ type BackupSpec struct {
 	UseKMS bool `json:"useKMS,omitempty"`
 	// Specify service account of backup
 	ServiceAccount string `json:"serviceAccount,omitempty"`
-	// CleanPolicy denotes whether to clean backup data when 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
 	CleanPolicy CleanPolicyType `json:"cleanPolicy,omitempty"`
 }
 

pkg/backup/backup/backup_cleaner.go

@@ -57,8 +57,7 @@ func NewBackupCleaner(
 }
 
 func (bc *backupCleaner) Clean(backup *v1alpha1.Backup) error {
-	if backup.DeletionTimestamp == nil || !v1alpha1.ShouldCleanData(backup) ||
-		backup.Spec.CleanPolicy == v1alpha1.CleanPolicyTypeOnFailure && !v1alpha1.IsBackupFailed(backup) {
+	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
 	}

pkg/controller/backup/backup_control.go

@@ -86,8 +86,7 @@ func (bc *defaultBackupControl) removeProtectionFinalizer(backup *v1alpha1.Backu
 	ns := backup.GetNamespace()
 	name := backup.GetName()
 
-	if v1alpha1.ShouldCleanData(backup) && isDeletionCandidate(backup) &&
-		(v1alpha1.IsBackupClean(backup) || (backup.Spec.CleanPolicy == v1alpha1.CleanPolicyTypeOnFailure && !v1alpha1.IsBackupFailed(backup))) {
+	if needToRemoveFinalizer(backup) {
 		backup.Finalizers = slice.RemoveString(backup.Finalizers, label.BackupProtectionFinalizer, nil)
 		_, err := bc.cli.PingcapV1alpha1().Backups(ns).Update(backup)
 		if err != nil {
@@ -99,7 +98,12 @@ func (bc *defaultBackupControl) removeProtectionFinalizer(backup *v1alpha1.Backu
 }
 
 func needToAddFinalizer(backup *v1alpha1.Backup) bool {
-	return backup.DeletionTimestamp == nil && v1alpha1.ShouldCleanData(backup) && !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 {