*: add version check before start by 3pointer · Pull Request #311 · pingcap/br

3pointer · 2020-05-26T08:56:03Z

What problem does this PR solve?

Resolve #301

What is changed and how it works?

Add cluster version check when br start

Check List

Tests

Unit test

Related changes

Need to cherry-pick to the release branch

Release Note

Feature: Add version check at beginning in order to avoid version mismatch.

kennytm · 2020-05-26T10:06:26Z

Please include a flag to disable version check (e.g. --check-requirements=false).

pkg/utils/version.go

YuJuncen

Rest LGTM

pkg/utils/version.go

3pointer · 2020-05-26T11:24:30Z

/run-integration-test

Co-authored-by: kennytm <kennytm@gmail.com>

3pointer · 2020-05-26T11:27:57Z

/run-integration-test

3pointer · 2020-05-26T11:31:17Z

/run-integration-test

3pointer · 2020-05-26T11:34:51Z

/run-integration-test

3pointer · 2020-05-26T11:52:16Z

/run-integration-test

3pointer · 2020-05-26T11:53:23Z

/run-integration-test

3pointer · 2020-05-26T12:05:08Z

/run-integration-test

YuJuncen · 2020-05-26T12:51:45Z

[2020-05-26T12:08:45.673Z] Error: None is not in dotted-tri format

Maybe PD returns "None" at Version Field?

codecov · 2020-05-26T13:01:45Z

Codecov Report

Merging #311 into master will decrease coverage by 0.87%.
The diff coverage is 76.59%.

@@            Coverage Diff             @@
##           master     #311      +/-   ##
==========================================
- Coverage   72.44%   71.56%   -0.88%     
==========================================
  Files          48       48              
  Lines        5277     5135     -142     
==========================================
- Hits         3823     3675     -148     
- Misses        976      989      +13     
+ Partials      478      471       -7

Impacted Files	Coverage Δ
pkg/conn/conn.go	`77.82% <50.00%> (-4.87%)`	⬇️
pkg/task/restore.go	`54.31% <50.00%> (ø)`
pkg/task/common.go	`65.54% <71.42%> (-7.50%)`	⬇️
pkg/utils/version.go	`87.50% <80.64%> (-1.79%)`	⬇️
pkg/task/backup.go	`61.74% <100.00%> (ø)`
pkg/task/backup_raw.go	`56.52% <100.00%> (ø)`
pkg/task/restore_raw.go	`59.37% <100.00%> (ø)`
pkg/restore/import.go	`67.95% <0.00%> (-3.87%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1edbe26...6e053e3. Read the comment docs.

3pointer · 2020-05-26T13:06:17Z

[2020-05-26T12:08:45.673Z] Error: None is not in dotted-tri format

I found it's BRReleaseVersion is None, but CI has build BR with ldflags, after a few attempt to adjust CI, It didn't work well, So I decide to add skip to work around.

YuJuncen

Rest LGTM

YuJuncen · 2020-05-27T03:12:16Z

pkg/task/common.go

 	flags.Uint64(flagRateLimitUnit, utils.MB, "The unit of rate limit")
 	_ = flags.MarkHidden(flagRateLimitUnit)

+	flags.Bool(flagCheckRequirement, true,


Boolean flags seems must use the form --some=false to disable(see here), otherwise when it appears, it would be set to true.
Maybe --skip-check-reqirements here would be better? Since we must use --check-requirements=false to skip check requirements if we let the default value be true on this flag.

IMO this is fine since we've got --checksum=false already.

--skip-check-requirements would be a "negative flag".

overvenus · 2020-05-28T02:34:23Z

[2020-05-26T12:08:45.673Z] Error: None is not in dotted-tri format
I found it's BRReleaseVersion is None, but CI has build BR with ldflags, after a few attempt to adjust CI, It didn't work well, So I decide to add skip to work around.

You can add -ldflags '$(LDFLAGS)' to the build_for_integration_test target in Makefile.

kennytm

lGTM

overvenus · 2020-05-28T12:27:37Z

Please remove .DS_Store files https://github.com/pingcap/br/blob/1a825f01f7e61fc0ac6b3d2408de7b75116d89f3/.DS_Store

Rest LGTM

3pointer · 2020-05-29T06:09:56Z

/run-integration-test

kennytm · 2020-05-29T06:20:16Z

/run-cherry-picker

sre-bot · 2020-05-29T06:20:23Z

cherry pick to release-3.1 failed

sre-bot · 2020-05-29T06:20:42Z

cherry pick to release-3.1 failed

sre-bot · 2020-05-29T06:21:07Z

cherry pick to release-4.0 in PR #326

* add version check * Update pkg/utils/version.go Co-authored-by: kennytm <kennytm@gmail.com> * fix test * add flag to control check * address comment * fix ci * remove DS_Store Co-authored-by: luancheng <luancheng@pingcap.com> Co-authored-by: kennytm <kennytm@gmail.com>

* add version check * Update pkg/utils/version.go Co-authored-by: kennytm <kennytm@gmail.com> * fix test * add flag to control check * address comment * fix ci * remove DS_Store Co-authored-by: kennytm <kennytm@gmail.com> Co-authored-by: Neil Shen <overvenus@gmail.com>

add version check

c2945ea

3pointer added type/feature-request New feature or request needs-cherry-pick-release-3.1 needs-cherry-pick-release-4.0 labels May 26, 2020

3pointer requested review from YuJuncen, kennytm and overvenus May 26, 2020 09:23

kennytm reviewed May 26, 2020

View reviewed changes

pkg/utils/version.go Outdated Show resolved Hide resolved

pkg/utils/version.go Outdated Show resolved Hide resolved

YuJuncen reviewed May 26, 2020

View reviewed changes

pkg/utils/version.go Show resolved Hide resolved

Update pkg/utils/version.go

42863df

Co-authored-by: kennytm <kennytm@gmail.com>

fix test

61e1738

add flag to control check

b67be9d

3pointer force-pushed the add_version_check branch from dbe6e2e to b67be9d Compare May 26, 2020 13:14

address comment

dc73ae1

YuJuncen reviewed May 27, 2020

View reviewed changes

fix ci

1a825f0

kennytm reviewed May 28, 2020

View reviewed changes

kennytm added the status/LGT1 LGTM1 label May 28, 2020

overvenus and others added 2 commits May 29, 2020 11:34

Merge branch 'master' into add_version_check

ba8b7af

remove DS_Store

6e053e3

kennytm approved these changes May 29, 2020

View reviewed changes

kennytm merged commit 3e05ea6 into pingcap:master May 29, 2020

sre-bot mentioned this pull request May 29, 2020

*: add version check before start (#311) #326

Merged

3pointer mentioned this pull request Jun 5, 2020

*: add version check before start (#311) #335

Merged

overvenus pushed a commit that referenced this pull request Jun 5, 2020

*: add version check before start (#311) (#335)

1639c24

kennytm mentioned this pull request Sep 10, 2020

Support clustered index in BR #497

Open

Conversation

3pointer commented May 26, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

Release Note

Uh oh!

kennytm commented May 26, 2020

Uh oh!

Uh oh!

Uh oh!

YuJuncen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

3pointer commented May 26, 2020

Uh oh!

3pointer commented May 26, 2020

Uh oh!

3pointer commented May 26, 2020

Uh oh!

3pointer commented May 26, 2020

Uh oh!

3pointer commented May 26, 2020

Uh oh!

3pointer commented May 26, 2020

Uh oh!

3pointer commented May 26, 2020

Uh oh!

YuJuncen commented May 26, 2020

Uh oh!

codecov bot commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

3pointer commented May 26, 2020

Uh oh!

YuJuncen left a comment

Choose a reason for hiding this comment

Uh oh!

YuJuncen May 27, 2020

Choose a reason for hiding this comment

Uh oh!

kennytm May 27, 2020

Choose a reason for hiding this comment

Uh oh!

overvenus commented May 28, 2020

Uh oh!

kennytm left a comment

Choose a reason for hiding this comment

Uh oh!

overvenus commented May 28, 2020

Uh oh!

3pointer commented May 29, 2020

Uh oh!

kennytm commented May 29, 2020

Uh oh!

sre-bot commented May 29, 2020

Uh oh!

sre-bot commented May 29, 2020

Uh oh!

sre-bot commented May 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented May 26, 2020 •

edited

Loading