Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

br/restore: add version check for backup schema_version #929

Merged
merged 18 commits into from
May 10, 2021

Conversation

YuJuncen
Copy link
Collaborator

@YuJuncen YuJuncen commented Mar 25, 2021

What problem does this PR solve?

a special fix for v4.0 versions of #790

Because #685 cannot be cherry-picked to release-4.0, 4.0.x BR would corrupt 4.0 clusters without even a warn message when the backup archive come from a table with non-int clustered primary key. This PR would add a check to the backup archive version, and intercept restoring from incompatible backup archives.

What is changed and how it works?

CheckClusterVersion was changed to check version in different way.
And added a check to cluster version stored in backupmeta during restore. (after reading backup meta)

Currently, this check only checks major versions (because of clustered index).

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Code changes

  • Has exported function/method change
    signature change for CheckClusterVersion.
- CheckClusterVersion(ctx context.Context, client pd.Client) error
+ CheckClusterVersion(ctx context.Context, client pd.Client, checker VerChecker) error

Release Note

  • BR would check cluster version of backup now.

@YuJuncen
Copy link
Collaborator Author

/run-all-tests

@YuJuncen
Copy link
Collaborator Author

/build

@YuJuncen YuJuncen added this to the v4.0.13 milestone Apr 1, 2021
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

pkg/version/version.go Outdated Show resolved Hide resolved
pkg/version/version.go Outdated Show resolved Hide resolved
@@ -199,3 +224,20 @@ func CheckTiDBVersion(versionStr string, requiredMinVersion, requiredMaxVersion
}
return CheckVersion("TiDB", *version, requiredMinVersion, requiredMaxVersion)
}

// NormalizeBackupVersion normalizes the version string from backupmeta.
func NormalizeBackupVersion(version string) *semver.Version {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem version.removeVAndHash can be replaced by this funcation.

Copy link
Collaborator Author

@YuJuncen YuJuncen Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean merge those two functions? Seems removeVAndHash focus on the version string made by the make script, and NormalizeBackupVersion focus on the version string returned from PD. Maybe it wouldn't work if we just replace the former with the latter. 🤔

@YuJuncen
Copy link
Collaborator Author

/run-integration-tests

port in use

pkg/version/version.go Outdated Show resolved Hide resolved
@YuJuncen YuJuncen requested a review from overvenus April 28, 2021 06:27
@YuJuncen YuJuncen requested a review from 3pointer April 28, 2021 06:27
@YuJuncen
Copy link
Collaborator Author

listen tcp 0.0.0.0:10080: bind: address already in use

/run-integration-tests

@overvenus
Copy link
Member

/lgtm

Copy link
Collaborator

@3pointer 3pointer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • 3pointer
  • overvenus

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 LGTM2 and removed status/LGT1 LGTM1 labels May 8, 2021
@3pointer
Copy link
Collaborator

3pointer commented May 8, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: c568f98

@YuJuncen
Copy link
Collaborator Author

YuJuncen commented May 9, 2021

[2021-05-08T11:29:42.533Z] [2021/05/08 19:29:40.505 +08:00] [INFO] [restore_raw.go:110] ["all files are filtered out from the backup archive, nothing to restore"]

/run-integration-tests

@3pointer
Copy link
Collaborator

/run-integration-tests

@ti-chi-bot ti-chi-bot merged commit 70b0a35 into pingcap:master May 10, 2021
ti-chi-bot pushed a commit to ti-chi-bot/br that referenced this pull request May 10, 2021
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #1090.

@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #1091.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants