Add commit ts to restore status #2899

lichunzhu · 2020-07-10T07:57:32Z

What problem does this PR solve?

What is changed and how does it work?

Reuse the getCommitTs function for BR and lightning. Save commitTS in restoreStatus.

Check List

Tests

Unit test
E2E test
Manual test (add detailed scripts or steps below)
The manual checks for lightning and br restore are both okay.

  ...
    secretName: restore-demo-basic-secret
    user: root
status:
  commitTs: "417953765864243201"
  conditions:
  ...

Related changes

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

Does this PR introduce a user-facing change?:

Support show commitTS in restore status.

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

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

@@ -58,18 +58,18 @@ type gcsQuery struct {
 }

 // NewRemoteStorage creates new remote storage
-func NewRemoteStorage(backup *v1alpha1.Backup) (*blob.Bucket, error) {
-	st := util.GetStorageType(backup.Spec.StorageProvider)
+func NewRemoteStorage(provider v1alpha1.StorageProvider, fakeRegion bool) (*blob.Bucket, error) {


Why add fakeRegion to this function?

…nzhu/tidb-operator into addCommitTsToRestoreStatus

lichunzhu · 2020-07-13T06:22:51Z

@DanielZhangQD @cofyc PTAL again

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

DanielZhangQD · 2020-07-10T12:36:21Z

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

+			Message: err.Error(),
+		})
+		errs = append(errs, uerr)
+		return errorutils.NewAggregate(errs)


Suggest not to return here.

I think it's ok. I don't think it's necessary to continue the restore operation with the backup from which we can't even get the commitTs information.

btw, should we move this before the restore operation like we do in import/manager.go file?

OK, then i think we can move this block before the GC time setting code block.

cofyc · 2020-07-14T01:40:14Z

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

@@ -186,6 +201,7 @@ func (rm *RestoreManager) performRestore(restore *v1alpha1.Restore) error {

 	restore.Status.TimeStarted = metav1.Time{Time: started}
 	restore.Status.TimeCompleted = metav1.Time{Time: finish}
+	restore.Status.CommitTs = commitTs


~~always update commitTs when we get it successfully~~ (edit: ignore)

sorry, maybe it's better not to update it when the restore fails as the commitTs indicates the commit timestamp or position of the restored backup

which means current implementation is correct?

changes in this file LGTM

cofyc · 2020-07-14T01:41:51Z

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

 	finish := time.Now()
 	restore.Status.TimeStarted = metav1.Time{Time: started}
 	restore.Status.TimeCompleted = metav1.Time{Time: finish}
+	restore.Status.CommitTs = fmt.Sprintf("%d", commitTs)


nit: use strconv.FormatInt(commitTs, 10)

cofyc · 2020-07-14T02:11:00Z

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

+
+		Finished dump at: 2019-06-13 10:00:04
+*/
+func GetCommitTsFromMetadata(backupPath string) (string, error) {


Can you add a unit test function for this function?

added in b6e3120

cofyc · 2020-07-14T02:11:23Z

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

+}
+
+// GetCommitTsFromBRMetaData get backup position from `EndVersion` in BR backup meta
+func GetCommitTsFromBRMetaData(provider v1alpha1.StorageProvider) (uint64, error) {


ditto, better to have a unit test for this function

this function get TS from S3 and ts is decoded by proto.Unmarshal. Maybe later we can mock it in another PR. This PR won't add this.

…nzhu/tidb-operator into addCommitTsToRestoreStatus

DanielZhangQD · 2020-07-14T03:20:34Z

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

@@ -299,7 +299,7 @@ func (bm *BackupManager) performBackup(backup *v1alpha1.Backup, db *sql.DB) erro
 	}
 	klog.Infof("get cluster %s archived backup file %s size %d success", bm, archiveBackupPath, size)

-	commitTs, err := getCommitTsFromMetadata(backupFullPath)
+	commitTs, err := util.GetCommitTsFromMetadata(backupFullPath)


Please wait for #2897 which removes the temp data after the compression.

DanielZhangQD · 2020-07-14T03:22:23Z

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

@@ -182,10 +182,26 @@ func (rm *RestoreManager) performRestore(restore *v1alpha1.Restore) error {
 	}
 	klog.Infof("restore cluster %s from backup %s success", rm, rm.BackupPath)

+	commitTs, err := util.GetCommitTsFromMetadata(unarchiveDataPath)


As per discussion, please move this block after the uncompression code block.

lichunzhu · 2020-07-14T05:37:39Z

After manual test, br & export & import can record commitTs correctly. PTAL again @cofyc @DanielZhangQD

lichunzhu · 2020-07-14T06:22:30Z

/test pull-e2e-kind-serial

DanielZhangQD

LGTM

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

ti-srebot · 2020-07-14T10:05:33Z

cherry pick to release-1.1 in PR #2925

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

lichunzhu added 3 commits July 10, 2020 14:29

tmp

769e3cf

add commit ts in restore status

4a967ae

fix bug

ae25bc3

lichunzhu requested review from cofyc and DanielZhangQD July 10, 2020 07:57

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

lichunzhu added 2 commits July 10, 2020 16:06

add doc

55fcd61

Merge branch 'master' into addCommitTsToRestoreStatus

889aac5

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

Merge branch 'master' into addCommitTsToRestoreStatus

6c089c1

DanielZhangQD reviewed Jul 10, 2020

View reviewed changes

lichunzhu added 4 commits July 10, 2020 20:49

remove fakeRegion argument

3140409

Merge branch 'addCommitTsToRestoreStatus' of https://github.com/lichu…

42d2926

…nzhu/tidb-operator into addCommitTsToRestoreStatus

resolve conflicts

3d6d55f

Merge branch 'master' into addCommitTsToRestoreStatus

48d0d02

lichunzhu requested a review from DanielZhangQD July 13, 2020 06:22

DanielZhangQD reviewed Jul 13, 2020

View reviewed changes

lichunzhu and others added 2 commits July 13, 2020 15:05

address comment

a42470c

Merge branch 'master' into addCommitTsToRestoreStatus

9ab33cd

cofyc reviewed Jul 14, 2020

View reviewed changes

lichunzhu added 4 commits July 14, 2020 10:14

Merge branch 'master' into addCommitTsToRestoreStatus

139f983

Merge branch 'master' into addCommitTsToRestoreStatus

c9c8c6d

add UT

b6e3120

Merge branch 'addCommitTsToRestoreStatus' of https://github.com/lichu…

3924503

…nzhu/tidb-operator into addCommitTsToRestoreStatus

DanielZhangQD reviewed Jul 14, 2020

View reviewed changes

lichunzhu added 3 commits July 14, 2020 13:23

Merge branch 'master' into addCommitTsToRestoreStatus

eb958e9

move get committs to top part

6f88e7b

resolve conflicts

1ee7648

cofyc approved these changes Jul 14, 2020

View reviewed changes

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

DanielZhangQD approved these changes Jul 14, 2020

View reviewed changes

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

ti-srebot approved these changes Jul 14, 2020

View reviewed changes

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

Merge branch 'master' into addCommitTsToRestoreStatus

dc29e84

DanielZhangQD merged commit 48eb1cd into pingcap:master Jul 14, 2020

lichunzhu deleted the addCommitTsToRestoreStatus branch July 14, 2020 10:05

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

cherry pick pingcap#2899 to release-1.1

bbdf742

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

ti-srebot mentioned this pull request Jul 14, 2020

Add commit ts to restore status (#2899) #2925

Merged

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

Add commit ts to restore status (#2899) (#2925)

3a7ac10

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

lichunzhu mentioned this pull request Jul 14, 2020

add commitTS in column for restore #2926

Merged

ti-srebot mentioned this pull request Jul 17, 2020

add commitTS in column for restore (#2926) #2970

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commit ts to restore status #2899

Add commit ts to restore status #2899

lichunzhu commented Jul 10, 2020 •

edited

Loading

DanielZhangQD Jul 10, 2020

lichunzhu commented Jul 13, 2020

DanielZhangQD Jul 10, 2020

cofyc Jul 14, 2020 •

edited

Loading

DanielZhangQD Jul 14, 2020

cofyc Jul 14, 2020 •

edited

Loading

cofyc Jul 14, 2020 •

edited

Loading

lichunzhu Jul 14, 2020

cofyc Jul 14, 2020

lichunzhu Jul 14, 2020

cofyc Jul 14, 2020

lichunzhu Jul 14, 2020

cofyc Jul 14, 2020

lichunzhu Jul 14, 2020

cofyc Jul 14, 2020

lichunzhu Jul 14, 2020

DanielZhangQD Jul 14, 2020

DanielZhangQD Jul 14, 2020

lichunzhu commented Jul 14, 2020

lichunzhu commented Jul 14, 2020

DanielZhangQD left a comment

ti-srebot commented Jul 14, 2020

Add commit ts to restore status #2899

Add commit ts to restore status #2899

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?:

Choose a reason for hiding this comment

lichunzhu commented Jul 13, 2020

Choose a reason for hiding this comment

cofyc Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

cofyc Jul 14, 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

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 14, 2020

lichunzhu commented Jul 14, 2020

DanielZhangQD left a comment

Choose a reason for hiding this comment

ti-srebot commented Jul 14, 2020

lichunzhu commented Jul 10, 2020 •

edited

Loading

cofyc Jul 14, 2020 •

edited

Loading

cofyc Jul 14, 2020 •

edited

Loading

cofyc Jul 14, 2020 •

edited

Loading