Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

clientv3: add db integrity check in snapshot status #10109

Merged
merged 1 commit into from
Sep 21, 2018
Merged

clientv3: add db integrity check in snapshot status #10109

merged 1 commit into from
Sep 21, 2018

Conversation

jingyih
Copy link
Contributor

@jingyih jingyih commented Sep 20, 2018 •
edited

Add snapshot file integrity verification in snapshot status. If the file is corrupted, user should be notified.

fix: #10108

Example output:

etcdctl snapshot status corrupt_db/backup.db

Error: snapshot file integrity check failed. 14 errors found.
page 961: already freed
page 962: already freed
page 1347: already freed
page 1442: already freed
page 1760: already freed
page 2023: already freed
page 2033: already freed
page 969: unreachable unfreed
page 970: unreachable unfreed
page 1355: unreachable unfreed
page 1450: unreachable unfreed
page 1768: unreachable unfreed
page 2031: unreachable unfreed
page 2041: unreachable unfreed

@jingyih jingyih changed the title clientv3: add db integrity check to snapshot status clientv3: add db integrity check in snapshot status Sep 20, 2018
@gyuho
Copy link
Contributor

gyuho commented Sep 20, 2018

@jingyih Can we add a test? Thanks! /cc @jpbetz

@codecov-io
Copy link

codecov-io commented Sep 20, 2018 •
edited

Codecov Report

Merging #10109 into master will increase coverage by 0.27%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10109      +/-   ##
==========================================
+ Coverage   71.52%    71.8%   +0.27%     
==========================================
  Files         390      390              
  Lines       36264    36269       +5     
==========================================
+ Hits        25939    26044     +105     
+ Misses       8513     8425      -88     
+ Partials     1812     1800      -12
Impacted Files Coverage Δ
clientv3/snapshot/v3_snapshot.go 63.85% <20%> (-0.9%) ⬇️
proxy/grpcproxy/watcher.go 89.79% <0%> (-4.09%) ⬇️
etcdserver/api/rafthttp/msgappv2_codec.go 69.56% <0%> (-1.74%) ⬇️
etcdserver/api/rafthttp/peer.go 78.77% <0%> (-1.12%) ⬇️
etcdserver/api/v3rpc/watch.go 82.35% <0%> (ø) ⬆️
clientv3/balancer/grpc1.7-health.go 59.3% <0%> (+0.29%) ⬆️
etcdserver/api/rafthttp/stream.go 79.82% <0%> (+0.42%) ⬆️
pkg/transport/listener_tls.go 66.22% <0%> (+0.66%) ⬆️
clientv3/lease.go 92.59% <0%> (+0.74%) ⬆️
pkg/netutil/netutil.go 70.49% <0%> (+0.81%) ⬆️
... and 15 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 1a3be73...422f867. Read the comment docs.

@xiang90
Copy link
Contributor

xiang90 commented Sep 20, 2018

lgtm

gyuho
gyuho approved these changes Sep 20, 2018
View reviewed changes
Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm after error message updates as @xiang90 suggested.

@jingyih jingyih force-pushed the add_db_integrity_verification_to_snapshot_status branch from a8fa39e to d6984c0 Compare September 20, 2018 17:26
jpbetz
jpbetz reviewed Sep 20, 2018
View reviewed changes
Copy link
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

Looks great, just one minor wording suggestion.

clientv3/snapshot/v3_snapshot.go Outdated
count++
}
if count > 0 {
return fmt.Errorf("database corrupted! %d errors found.\n"+strings.Join(db_err_strings, "\n"), count)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "snapshot file integrity check failed." ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Thanks!

@jingyih
clientv3: add integrity check in snapshot status
422f867 
Add snapshot file integrity check in snapshot status. If the file is
corrupted, return with error message.
@jingyih jingyih force-pushed the add_db_integrity_verification_to_snapshot_status branch from d6984c0 to 422f867 Compare September 20, 2018 20:02
@xiang90 xiang90 merged commit d987cae into etcd-io:master Sep 21, 2018
@xiang90
Copy link
Contributor

xiang90 commented Sep 21, 2018

@jingyih thanks!

jingyih added a commit to jingyih/etcd that referenced this pull request Sep 25, 2018
@jingyih
etcdctl: cherry pick of etcd-io#10109 to release-3.3
60d546e 
Add snapshot file integrity verification in snapshot status.
@jingyih jingyih mentioned this pull request Sep 26, 2018
Merged
jingyih added a commit to jingyih/etcd that referenced this pull request Sep 26, 2018
@jingyih
etcdctl: cherry pick of etcd-io#10109 to release-3.2
affd468 
Add snapshot file integrity verification when querying snapshot status.
gyuho added a commit that referenced this pull request Sep 26, 2018
@gyuho
Merge pull request #10122 from jingyih/cherry-pick-of-#10109-origin-r…
40ed18a 
…elease-3.3

etcdctl: cherry pick of #10109 to release-3.3
@jingyih jingyih mentioned this pull request Sep 26, 2018
Merged
jingyih added a commit to jingyih/etcd that referenced this pull request Sep 26, 2018
@jingyih
etcdctl: cherry pick of etcd-io#10109 to release-3.1
87b4e08 
Add snapshot file integrity verification when querying snapshot status.
@jingyih jingyih mentioned this pull request Sep 26, 2018
Merged
gyuho added a commit that referenced this pull request Sep 26, 2018
@gyuho
Merge pull request #10123 from jingyih/cherry-pick-of-#10109-origin-r…
a4a8d07 
…elease-3.2

etcdctl: cherry pick of #10109 to release-3.2
gyuho added a commit that referenced this pull request Sep 26, 2018
@gyuho
Merge pull request #10124 from jingyih/cherry-pick-of-#10109-origin-r…
65fff06 
…elease-3.1

etcdctl: cherry pick of #10109 to release-3.1
jingyih added a commit to jingyih/etcd that referenced this pull request Sep 28, 2018
@jingyih
CHANGELOG-3.4: update from etcd-io#10109
11fc2ce
@jingyih jingyih mentioned this pull request Sep 28, 2018
Merged
jingyih added a commit to jingyih/etcd that referenced this pull request Sep 29, 2018
@jingyih
CHANGELOG-3.3: updata from etcd-io#10109
0c20457
jingyih added a commit to jingyih/etcd that referenced this pull request Sep 29, 2018
@jingyih
CHANGELOG-3.3: update from etcd-io#10109
fd816d0
@jingyih jingyih mentioned this pull request Sep 29, 2018
Merged
gyuho added a commit that referenced this pull request Sep 29, 2018
@gyuho
Merge pull request #10135 from jingyih/update_changelog_3p3_from_pr10109
ab47035 
CHANGELOG-3.3: update from #10109
gyuho added a commit that referenced this pull request Sep 29, 2018
@gyuho
Merge pull request #10134 from jingyih/update_changelog_3p4_from_pr10109
fa495d5 
CHANGELOG-3.4: update from #10109
jingyih added a commit to jingyih/etcd that referenced this pull request Sep 29, 2018
@jingyih
CHANGELOG-3.2: update from etcd-io#10109
a249c19
@jingyih jingyih mentioned this pull request Sep 29, 2018
Merged
jingyih added a commit to jingyih/etcd that referenced this pull request Sep 29, 2018
@jingyih
CHANGELOG-3.1: update from etcd-io#10109
4a3f7c6
@jingyih jingyih mentioned this pull request Sep 29, 2018
Merged
jingyih added a commit that referenced this pull request Sep 29, 2018
@jingyih
Merge pull request #10136 from jingyih/update_changelog_3p2_from_pr10109
31e2c08 
CHANGELOG-3.2: update from #10109
jingyih added a commit that referenced this pull request Sep 29, 2018
@jingyih
Merge pull request #10137 from jingyih/update_changelog_3p1_from_pr10109
91fe8cb 
CHANGELOG-3.1: update from #10109
@jingyih jingyih mentioned this pull request Oct 3, 2018
Merged
@jpbetz jpbetz mentioned this pull request Oct 16, 2018
Merged
@hexfusion hexfusion mentioned this pull request Apr 6, 2019
Closed
@jingyih jingyih deleted the add_db_integrity_verification_to_snapshot_status branch September 7, 2019 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpbetz jpbetz jpbetz left review comments

@hexfusion hexfusion hexfusion left review comments

@xiang90 xiang90 xiang90 left review comments

@gyuho gyuho gyuho approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clientv3: snapshot status does not check database integrity
6 participants
@jingyih @gyuho @codecov-io @xiang90 @jpbetz @hexfusion