support cleanPolicy for backup CR #3002

lichunzhu · 2020-07-22T10:11:41Z

What problem does this PR solve?

fix #2989

What is changed and how does it work?

Change cleanData to cleanPolicy
Change rclone judgement for remote file not exist error.

Check List

Tests

Unit test
E2E test
Stability test
Manual test (add detailed scripts or steps below)
- Test on failure test for br and works successfully.

Code changes

Has Go code change

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Does this PR introduce a user-facing change?:

support cleanPolicy for backup CR

…o removeCloudTempData

codecov-commenter · 2020-07-22T10:19:17Z

Codecov Report

Merging #3002 into master will decrease coverage by 0.05%.
The diff coverage is 28.40%.

@@            Coverage Diff             @@
##           master    #3002      +/-   ##
==========================================
- Coverage   42.00%   41.94%   -0.06%     
==========================================
  Files         155      156       +1     
  Lines       16744    16773      +29     
==========================================
+ Hits         7033     7036       +3     
- Misses       9144     9170      +26     
  Partials      567      567

Flag	Coverage Δ
#unittest	`41.94% <28.40%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

DanielZhangQD · 2020-07-22T12:21:50Z

pkg/apis/pingcap/v1alpha1/types.go

+	// CleanPolicyTypeRetain represents the clean policy is retain
+	CleanPolicyTypeRetain CleanPolicyType = "Retain"
+	// CleanPolicyTypeOnFailure represents the clean policy is on failure
+	CleanPolicyTypeOnFailure CleanPolicyType = "OnFailure"
+	// CleanPolicyTypeIfFailed represents the clean policy is delete
+	CleanPolicyTypeDelete CleanPolicyType = "Delete"


Please describe the behavior clearly for each policy.

DanielZhangQD · 2020-07-22T12:22:42Z

pkg/apis/pingcap/v1alpha1/types.go

@@ -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 before the object is deleted from the cluster


Suggested change

// CleanPolicy 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

What's the default behavior?

DanielZhangQD · 2020-07-22T12:39:35Z

pkg/controller/backup/backup_control.go

+	case v1alpha1.CleanPolicyTypeRetain:
+		return false


This can be removed?

DanielZhangQD · 2020-07-22T12:49:34Z

pkg/backup/backup/backup_cleaner.go

-	if backup.DeletionTimestamp == nil || !backup.Spec.CleanData {
-		// The backup object has not been deleted，do nothing
+	if backup.DeletionTimestamp == nil ||
+		backup.Spec.CleanPolicy == v1alpha1.CleanPolicyTypeRetain || backup.Spec.CleanPolicy == "" {


!shouldCleanData()?

DanielZhangQD · 2020-07-23T02:25:22Z

pkg/controller/backup/backup_control.go

@@ -86,7 +86,8 @@ func (bc *defaultBackupControl) removeProtectionFinalizer(backup *v1alpha1.Backu
 	ns := backup.GetNamespace()
 	name := backup.GetName()

-	if backup.Spec.CleanData && isDeletionCandidate(backup) && v1alpha1.IsBackupClean(backup) {
+	if backup.Spec.CleanPolicy != v1alpha1.CleanPolicyTypeRetain && backup.Spec.CleanPolicy != "" &&


v1alpha1.ShouldCleanData(backup)

DanielZhangQD · 2020-07-23T02:30:16Z

pkg/backup/backup/backup_cleaner.go

@@ -80,6 +80,15 @@ func (bc *backupCleaner) Clean(backup *v1alpha1.Backup) error {
 			Status: corev1.ConditionTrue,
 		})
 	}
+
+	if backup.Spec.CleanPolicy == v1alpha1.CleanPolicyTypeOnFailure && !v1alpha1.IsBackupFailed(backup) {


This logic should be in L60 and we should not set BackupClean condition for this case as the data is not cleaned but retained for the successful backup.

The finalizer handling logic should also be updated accordingly.

DanielZhangQD · 2020-07-23T02:51:08Z

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 specific delete cloud data policy


Suggested change

// CleanPolicyType represents the specific delete cloud data policy

// CleanPolicyType represents the clean policy of backup data in remote storage

DanielZhangQD · 2020-07-23T02:52:21Z

pkg/apis/pingcap/v1alpha1/types.go

+type CleanPolicyType string
+
+const (
+	// CleanPolicyTypeRetain represents the clean policy is to retain S3 backup files at any time


Suggested change

// CleanPolicyTypeRetain represents the clean policy is to retain S3 backup files at any time

// CleanPolicyTypeRetain represents that the backup data in remote storage will be retained when the Backup CR is deleted

DanielZhangQD · 2020-07-23T02:54:29Z

pkg/apis/pingcap/v1alpha1/types.go

+const (
+	// CleanPolicyTypeRetain represents the clean policy is to retain S3 backup files at any time
+	CleanPolicyTypeRetain CleanPolicyType = "Retain"
+	// CleanPolicyTypeOnFailure represents the clean policy is to clean S3 backup files on failure


Suggested change

// CleanPolicyTypeOnFailure represents the clean policy is to clean S3 backup files on failure

// CleanPolicyTypeOnFailure represents that the backup data in remote storage will be cleaned only for the failed backups when the Backup CR is deleted

DanielZhangQD · 2020-07-23T02:55:01Z

pkg/apis/pingcap/v1alpha1/types.go

+	CleanPolicyTypeRetain CleanPolicyType = "Retain"
+	// CleanPolicyTypeOnFailure represents the clean policy is to clean S3 backup files on failure
+	CleanPolicyTypeOnFailure CleanPolicyType = "OnFailure"
+	// CleanPolicyTypeIfFailed represents the clean policy is to clean S3 backup files at any time


Suggested change

// CleanPolicyTypeIfFailed represents the clean policy is to clean S3 backup files at any time

// CleanPolicyTypeIfFailed represents that the backup data in remote storage will be cleaned when the Backup CR is deleted

…db-operator into removeCloudTempData

lichunzhu · 2020-07-24T03:56:30Z

/test pull-e2e-kind

DanielZhangQD · 2020-07-24T04:13:36Z

pkg/backup/backup/backup_cleaner.go

-	if backup.DeletionTimestamp == nil || !backup.Spec.CleanData {
-		// The backup object has not been deleted，do nothing
+	if backup.DeletionTimestamp == nil || !v1alpha1.ShouldCleanData(backup) ||
+		backup.Spec.CleanPolicy == v1alpha1.CleanPolicyTypeOnFailure && !v1alpha1.IsBackupFailed(backup) {


Suggested change

backup.Spec.CleanPolicy == v1alpha1.CleanPolicyTypeOnFailure && !v1alpha1.IsBackupFailed(backup) {

(backup.Spec.CleanPolicy == v1alpha1.CleanPolicyTypeOnFailure && !v1alpha1.IsBackupFailed(backup)) {

DanielZhangQD · 2020-07-24T04:14:18Z

pkg/apis/pingcap/v1alpha1/types.go

@@ -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


Suggested change

// 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

DanielZhangQD · 2020-07-24T04:16:07Z

pkg/controller/backup/backup_control.go

@@ -86,7 +86,8 @@ func (bc *defaultBackupControl) removeProtectionFinalizer(backup *v1alpha1.Backu
 	ns := backup.GetNamespace()
 	name := backup.GetName()

-	if backup.Spec.CleanData && isDeletionCandidate(backup) && v1alpha1.IsBackupClean(backup) {
+	if v1alpha1.ShouldCleanData(backup) && isDeletionCandidate(backup) &&


We can define a function for these conditions.

DanielZhangQD

LGTM

lichunzhu · 2020-07-24T08:23:03Z

@cofyc PTAL

cofyc · 2020-07-24T08:32:22Z

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

-		}
-		return fmt.Errorf("cluster %s, execute rclone deletefile command failed, output: %s, err: %v", bo, string(output), err)
+	if err != nil && !strings.Contains(string(output), "doesn't exist") {
+		return fmt.Errorf("cluster %s, execute rclone deletefile command to delete archive failed, output: %s, err: %v", bo, string(output), err)


why changing this back?

Because according to test, this error code is not accurate. When rclone returns dir doesn't exist error the exit code is 1. May the last time I forgot to use hack/local-up-operator.sh to update operator.

maybe rclone delete is a better command in this scenario

https://rclone.org/commands/rclone_delete/

in my testing, it succeeds if no file found at the specified path

Yes. It won't even return error. Should we use this command?
C.C. @DanielZhangQD

It's OK if the file can be removed if exist.

Revise to rclone delete in 4e1d38f

cofyc · 2020-07-24T08:33:17Z

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,


can we add tests for other values?

will add tests later

cofyc · 2020-07-24T09:15:28Z

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

+		return fmt.Errorf("cluster %s, execute rclone deletefile command to delete archive failed, output: %s, err: %v", bo, string(output), err)
+	}
+
+	args = util.ConstructArgs(constants.RcloneConfigArg, opts, "deletefile", fmt.Sprintf("%s.tmp", destBucket), "")


when will this *.tmp file be created?

After file is archived and uploaded to S3.

upload archive file to *.tgz.tmp

move *.tgz.tmp to *.tgz

I see. Thanks!

And this is for backup with dumpling only.

DanielZhangQD · 2020-07-24T09:45:30Z

/test pull-e2e-kind

…db-operator into removeCloudTempData

lichunzhu · 2020-07-24T10:13:50Z

@cofyc addressed, PTAL again

DanielZhangQD

LGTM

cofyc · 2020-07-24T11:41:50Z

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

-				return nil
-			}
-		}
+		return fmt.Errorf("cluster %s, execute rclone deletefile command failed, output: %s, err: %v", bo, string(output), err)


s/deletefile/delete

cofyc · 2020-07-24T11:42:08Z

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

+
+	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 deletefile command failed, output: %s, err: %v", bo, string(output), err)


s/deletefile/delete

cofyc · 2020-07-24T11:45:29Z

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

+			Status:  corev1.ConditionTrue,
+			Reason:  "UpdateBackupPathFailed",
+			Message: err.Error(),
+		})


why setting this condition if StatusUpdater.Update( fails?

copyed from https://github.com/pingcap/tidb-operator/blob/master/cmd/backup-manager/app/backup/manager.go#L159
It sames that it's better to return error directly?

IMO it's meaningless to call StatusUpdater.Update again if the previous StatusUpdater.Update fails as the second call can fail too

our current architecture relies on our backup job to update Backup CR's status, the situation that the status may not be reported to the apiserver is unavoidable

does it cause problems when the StatusUpdater.Update call fails?

copyed from https://github.com/pingcap/tidb-operator/blob/master/cmd/backup-manager/app/backup/manager.go#L159
It sames that it's better to return error directly?

yes

If we omit this error, we might be unable to clean files on the Cloud when we delete this CR because BackupPath is not recorded.

but the bm.backupDataToRemote will not be run if reporting the backup path to the apiserver fails

(if this is not true, I think we should fix it. one more API call does not help)

So should we omit this error? What about BR?

we didn't omit the error, the job fails

lichunzhu · 2020-07-24T12:55:26Z

@DanielZhangQD PTAL again

DanielZhangQD

LGTM

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

ti-srebot · 2020-07-24T14:51:06Z

cherry pick to release-1.1 in PR #3018

Signed-off-by: ti-srebot <[email protected]> Co-authored-by: Chunzhu Li <[email protected]>

lichunzhu added 3 commits July 22, 2020 10:42

revise CleanData to CleanPolicy

26b1eb8

Merge branch 'master' of https://github.com/pingcap/tidb-operator int…

0bae349

…o removeCloudTempData

refine code

aedf29b

lichunzhu requested a review from cofyc July 22, 2020 10:11

lichunzhu added the status/PTAL PR needs to be reviewed label Jul 22, 2020

lichunzhu requested a review from DanielZhangQD July 22, 2020 10:11

lichunzhu added the needs-cherry-pick-1.1 label Jul 22, 2020

Merge branch 'master' into removeCloudTempData

f2d7cc7

generate code

69d2cad

DanielZhangQD reviewed Jul 22, 2020

View reviewed changes

lichunzhu added 2 commits July 22, 2020 20:56

update status for dumpling before upload

31217f9

address comments

7d896d9

DanielZhangQD reviewed Jul 23, 2020

View reviewed changes

lichunzhu added 3 commits July 24, 2020 10:39

Merge branch 'master' into removeCloudTempData

e484693

verify code

a6c7b80

Merge branch 'removeCloudTempData' of https://github.com/lichunzhu/ti…

fe9c627

…db-operator into removeCloudTempData

DanielZhangQD reviewed Jul 24, 2020

View reviewed changes

address comments

d177a55

DanielZhangQD previously approved these changes Jul 24, 2020

View reviewed changes

ti-srebot added the status/LGT1 label Jul 24, 2020

cofyc reviewed Jul 24, 2020

View reviewed changes

Merge branch 'master' into removeCloudTempData

32d164f

cofyc reviewed Jul 24, 2020

View reviewed changes

lichunzhu added 2 commits July 24, 2020 18:10

address comments

4e1d38f

Merge branch 'removeCloudTempData' of https://github.com/lichunzhu/ti…

c5e77dc

…db-operator into removeCloudTempData

lichunzhu dismissed DanielZhangQD’s stale review via c5e77dc July 24, 2020 10:11

DanielZhangQD previously approved these changes Jul 24, 2020

View reviewed changes

lichunzhu mentioned this pull request Jul 24, 2020

zh: revise cleanData to cleanPolicy pingcap/docs-tidb-operator#591

Merged

3 tasks

Merge branch 'master' into removeCloudTempData

6341abc

cofyc reviewed Jul 24, 2020

View reviewed changes

address comments

337934b

lichunzhu dismissed DanielZhangQD’s stale review via 337934b July 24, 2020 12:46

cofyc approved these changes Jul 24, 2020

View reviewed changes

ti-srebot removed the status/LGT1 label Jul 24, 2020

ti-srebot approved these changes Jul 24, 2020

View reviewed changes

ti-srebot added the status/LGT2 label Jul 24, 2020

DanielZhangQD approved these changes Jul 24, 2020

View reviewed changes

DanielZhangQD merged commit fe8bf9f into pingcap:master Jul 24, 2020

ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request Jul 24, 2020

cherry pick pingcap#3002 to release-1.1

6635d61

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

ti-srebot mentioned this pull request Jul 24, 2020

support cleanPolicy for backup CR (#3002) #3018

Merged

DanielZhangQD mentioned this pull request Jul 27, 2020

When deleting a failed backup CR, the backup data on s3 is not cleaned up。 #2989

Closed

DanielZhangQD pushed a commit that referenced this pull request Jul 27, 2020

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

5e9ddb1

Signed-off-by: ti-srebot <[email protected]> Co-authored-by: Chunzhu Li <[email protected]>

lichunzhu mentioned this pull request Jul 27, 2020

v1.1.3 release notes #3022

Merged

ti-srebot mentioned this pull request Jul 27, 2020

revise cleanData to cleanPolicy (#591) pingcap/docs-tidb-operator#593

Merged

3 tasks

lichunzhu deleted the removeCloudTempData branch September 2, 2020 07:11

	// CleanPolicy 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

	// CleanPolicyType represents the specific delete cloud data policy
	// CleanPolicyType represents the clean policy of backup data in remote storage

	// CleanPolicyTypeRetain represents the clean policy is to retain S3 backup files at any time
	// CleanPolicyTypeRetain represents that the backup data in remote storage will be retained when the Backup CR is deleted

	// CleanPolicyTypeOnFailure represents the clean policy is to clean S3 backup files on failure
	// CleanPolicyTypeOnFailure represents that the backup data in remote storage will be cleaned only for the failed backups when the Backup CR is deleted

	// CleanPolicyTypeIfFailed represents the clean policy is to clean S3 backup files at any time
	// CleanPolicyTypeIfFailed represents that the backup data in remote storage will be cleaned when the Backup CR is deleted

	backup.Spec.CleanPolicy == v1alpha1.CleanPolicyTypeOnFailure && !v1alpha1.IsBackupFailed(backup) {
	(backup.Spec.CleanPolicy == v1alpha1.CleanPolicyTypeOnFailure && !v1alpha1.IsBackupFailed(backup)) {

support cleanPolicy for backup CR #3002

support cleanPolicy for backup CR #3002

Conversation

lichunzhu commented Jul 22, 2020 • edited by DanielZhangQD Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

codecov-commenter commented Jul 22, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielZhangQD left a comment

Choose a reason for hiding this comment

lichunzhu commented Jul 24, 2020

Choose a reason for hiding this comment

lichunzhu Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

cofyc Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielZhangQD commented Jul 24, 2020

lichunzhu commented Jul 24, 2020

DanielZhangQD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu commented Jul 24, 2020

DanielZhangQD left a comment

Choose a reason for hiding this comment

ti-srebot commented Jul 24, 2020

lichunzhu commented Jul 22, 2020 •

edited by DanielZhangQD

Loading

codecov-commenter commented Jul 22, 2020 •

edited

Loading

lichunzhu Jul 24, 2020 •

edited

Loading

cofyc Jul 24, 2020 •

edited

Loading

lichunzhu Jul 24, 2020 •

edited

Loading

cofyc Jul 24, 2020 •

edited

Loading