remove temp data in pv for backup #2897

lichunzhu · 2020-07-10T03:55:34Z

What problem does this PR solve?

Fix #2075

What is changed and how does it work?

Delete temp files in PV (even if an error occurs).

Check List

Tests

Unit test
E2E test
Manual test (add detailed scripts or steps below)
Manual apply import/export job, the data can be deleted.

Related changes

Need to cherry-pick to the release branch

Does this PR introduce a user-facing change?:

Clean temporary files in `Backup` export job to save space

lichunzhu · 2020-07-10T03:57:49Z

Current implementation will delete tempary data even if an error occurs. It's not good for debug but can save more space.
WDYT?

cofyc

oh, maybe it's better to clean data only if the upload succeeds.

This is not only for debugging, it's about data safety. For example, the upload process may fail due to network failures, IMO it's better we don't delete local backup data in this case.

cofyc · 2020-07-10T09:15:03Z

After the upload is completed successfully, it's safe to remove local backup data.

lichunzhu · 2020-07-10T09:27:52Z

After the upload is completed successfully, it's safe to remove local backup data.

What about restore?

lichunzhu · 2020-07-10T09:29:11Z

After the upload is completed successfully, it's safe to remove local backup data.

Maybe we can remove the unpacked datadir? Similiarly can we remove the tar package in lightning restore after it's unpacked?

cofyc · 2020-07-10T09:49:16Z

I think it's ok to clean temporary files in restoring since the original data is persisted.

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

DanielZhangQD · 2020-07-10T12:12:29Z

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

@@ -152,7 +153,9 @@ func (rm *RestoreManager) performRestore(restore *v1alpha1.Restore) error {
 	klog.Infof("download cluster %s backup %s data success", rm, rm.BackupPath)

 	restoreDataDir := filepath.Dir(restoreDataPath)
+	defer os.RemoveAll(restoreDataPath)


Suggest not removing data after restore.

…perator into removePVTmpData

DanielZhangQD · 2020-07-10T13:13:56Z

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

@@ -262,6 +263,8 @@ func (bm *BackupManager) performBackup(backup *v1alpha1.Backup, db *sql.DB) erro
 			Message: backupErr.Error(),
 		})
 		errs = append(errs, uerr)
+		// just delete backupFullPath since it will never be used except for debug


If it's useful for debug, can we keep it just in case?

addressed in df73f02

DanielZhangQD · 2020-07-10T13:17:28Z

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

@@ -166,6 +167,8 @@ func (rm *RestoreManager) performRestore(restore *v1alpha1.Restore) error {
 		return errorutils.NewAggregate(errs)
 	}
 	klog.Infof("unarchive cluster %s backup %s data success", rm, restoreDataPath)
+	// unarchive succeed, origin archive can be deleted now


What about keeping the tar, it does not make much difference since we keep the unarchieved files.

addressed in df73f02

DanielZhangQD

LGTM

ti-srebot · 2020-07-11T03:34:28Z

@DanielZhangQD,Thanks for your review.

cofyc · 2020-07-13T02:37:20Z

release notes should be accurate and concise for the end-users as they are part of our documentation. please always update the release notes to reflect the latest change and follow the conventions used in our documentation.

my suggestion:

Clean temporary files in `Backup` export job to save space

lichunzhu · 2020-07-13T06:23:35Z

release notes should be accurate and concise for the end-users as they are part of our documentation. please always update the release notes to reflect the latest change and follow the conventions used in our documentation.

my suggestion:
Clean temporary files in `Backup` export job to save space

revised

lichunzhu · 2020-07-13T08:58:16Z

/run-all-tests

DanielZhangQD · 2020-07-14T01:30:32Z

/run-all-tests

lichunzhu · 2020-07-14T03:14:09Z

@cofyc PTAL again

DanielZhangQD · 2020-07-14T03:33:50Z

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

@@ -282,6 +283,8 @@ func (bm *BackupManager) performBackup(backup *v1alpha1.Backup, db *sql.DB) erro
 		return errorutils.NewAggregate(errs)
 	}
 	klog.Infof("archive cluster %s backup data %s success", bm, archiveBackupPath)
+	// archive succeed, origin dir can be deleted safely


getCommitTsFromMetadata in L305 will fail.

DanielZhangQD

LGTM

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2020-07-14T05:08:16Z

cherry pick to release-1.1 in PR #2922

Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>

…ment) and pingcap/tidb-operator#2897

* document how to delete backup * document gc behavior during backup * address comments, add description for pingcap/tidb-operator#2888 (comment) and pingcap/tidb-operator#2897 * address comments * address comments * address comments * Apply suggestions from code review Co-authored-by: Ran <huangran@pingcap.com>

add remove temp data in pv

40dbdcd

lichunzhu requested review from DanielZhangQD and cofyc July 10, 2020 03:55

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

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

cofyc previously approved these changes Jul 10, 2020

View reviewed changes

cofyc reviewed Jul 10, 2020

View reviewed changes

Merge branch 'master' into removePVTmpData

2e7a0ca

DanielZhangQD reviewed Jul 10, 2020

View reviewed changes

cmd/backup-manager/app/import/manager.go Outdated Show resolved Hide resolved

DanielZhangQD reviewed Jul 10, 2020

View reviewed changes

lichunzhu added 2 commits July 10, 2020 20:45

address discussion result

91b70ac

Merge branch 'removePVTmpData' of https://github.com/lichunzhu/tidb-o…

904a335

…perator into removePVTmpData

lichunzhu dismissed cofyc’s stale review via 904a335 July 10, 2020 12:46

DanielZhangQD reviewed Jul 10, 2020

View reviewed changes

lichunzhu added 2 commits July 10, 2020 21:50

address comment

df73f02

Merge branch 'master' into removePVTmpData

d44f5c9

DanielZhangQD previously approved these changes Jul 11, 2020

View reviewed changes

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

DanielZhangQD added 2 commits July 11, 2020 11:35

Merge branch 'master' into removePVTmpData

1a50ece

Merge branch 'master' into removePVTmpData

08a6892

Merge branch 'master' into removePVTmpData

9e83ca4

Merge branch 'master' into removePVTmpData

9d5c506

Merge branch 'master' into removePVTmpData

b73460a

cofyc previously approved these changes Jul 14, 2020

View reviewed changes

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

ti-srebot previously approved these changes Jul 14, 2020

View reviewed changes

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

DanielZhangQD mentioned this pull request Jul 14, 2020

Add commit ts to restore status #2899

Merged

DanielZhangQD self-requested a review July 14, 2020 03:32

DanielZhangQD reviewed Jul 14, 2020

View reviewed changes

fix bug

a23fa3b

lichunzhu dismissed stale reviews from ti-srebot, cofyc, and DanielZhangQD via a23fa3b July 14, 2020 03:55

DanielZhangQD approved these changes Jul 14, 2020

View reviewed changes

cofyc approved these changes Jul 14, 2020

View reviewed changes

DanielZhangQD merged commit 02bc61b into pingcap:master Jul 14, 2020

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

cherry pick pingcap#2897 to release-1.1

8e84e33

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Jul 14, 2020

remove temp data in pv for backup (#2897) #2922

Merged

ti-srebot added a commit that referenced this pull request Jul 14, 2020

remove temp data in pv for backup (#2897) (#2922)

5e88e34

Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>

DanielZhangQD mentioned this pull request Jul 14, 2020

zh: document handle backup gc behavior and how to delete backup pingcap/docs-tidb-operator#566

Merged

3 tasks

lichunzhu added a commit to lichunzhu/docs-tidb-operator that referenced this pull request Jul 14, 2020

address comments, add description for pingcap/tidb-operator#2888 (com…

95d6ac1

…ment) and pingcap/tidb-operator#2897

ti-srebot mentioned this pull request Jul 16, 2020

zh: document handle backup gc behavior and how to delete backup (#566) pingcap/docs-tidb-operator#575

Merged

3 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove temp data in pv for backup #2897

remove temp data in pv for backup #2897

lichunzhu commented Jul 10, 2020 •

edited

Loading

lichunzhu commented Jul 10, 2020

cofyc left a comment

cofyc commented Jul 10, 2020

lichunzhu commented Jul 10, 2020

lichunzhu commented Jul 10, 2020

cofyc commented Jul 10, 2020

DanielZhangQD Jul 10, 2020

DanielZhangQD Jul 10, 2020

lichunzhu Jul 10, 2020

DanielZhangQD Jul 10, 2020

lichunzhu Jul 10, 2020

DanielZhangQD left a comment

ti-srebot commented Jul 11, 2020

cofyc commented Jul 13, 2020

lichunzhu commented Jul 13, 2020

lichunzhu commented Jul 13, 2020

DanielZhangQD commented Jul 14, 2020

lichunzhu commented Jul 14, 2020

DanielZhangQD Jul 14, 2020

DanielZhangQD left a comment

ti-srebot commented Jul 14, 2020

remove temp data in pv for backup #2897

remove temp data in pv for backup #2897

Conversation

lichunzhu commented Jul 10, 2020 • edited 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?:

lichunzhu commented Jul 10, 2020

cofyc left a comment

Choose a reason for hiding this comment

cofyc commented Jul 10, 2020

lichunzhu commented Jul 10, 2020

lichunzhu commented Jul 10, 2020

cofyc commented Jul 10, 2020

DanielZhangQD Jul 10, 2020

Choose a reason for hiding this comment

DanielZhangQD Jul 10, 2020

Choose a reason for hiding this comment

lichunzhu Jul 10, 2020

Choose a reason for hiding this comment

DanielZhangQD Jul 10, 2020

Choose a reason for hiding this comment

lichunzhu Jul 10, 2020

Choose a reason for hiding this comment

DanielZhangQD left a comment

Choose a reason for hiding this comment

ti-srebot commented Jul 11, 2020

cofyc commented Jul 13, 2020

lichunzhu commented Jul 13, 2020

lichunzhu commented Jul 13, 2020

DanielZhangQD commented Jul 14, 2020

lichunzhu commented Jul 14, 2020

DanielZhangQD Jul 14, 2020

Choose a reason for hiding this comment

DanielZhangQD left a comment

Choose a reason for hiding this comment

ti-srebot commented Jul 14, 2020

lichunzhu commented Jul 10, 2020 •

edited

Loading