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.

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

clientv3: add test for snapshot status #10148

Merged

Conversation

jingyih
Copy link
Contributor

@jingyih jingyih commented Oct 3, 2018

Add unit test to check if we can correctly identify a corrupted snapshot backup file.

This is a follow up to #10109, where backup file integrity check was added to snapshot status.

Also adding an example snapshot backup file, where its underlying database freelist is corrupted. We saw this issue in our production cluster, this file is an example test file that is reproduced to have the same type of issue.

/cc @gyuho @xiang90 @hexfusion @jpbetz

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.

awesome. Thanks for test case! LGTM.

@xiang90
Copy link
Contributor

xiang90 commented Oct 3, 2018

lgtm

sp := NewV3(zap.NewExample())
_, err := sp.Status(dbPath)
expectedErrString :=
"snapshot file integrity check failed. 2 errors found.\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a little bit too strict. i would rather just to make sure the error message include some key words... or if sp.Status's return message changes just a little bit, this unit test breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. At least I should remove the 2nd and 3rd lines, which are the error messages from bbolt. Any changes to those error messages in bbolt would break this. Thanks for pointing out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed. PTAL.

@jingyih
Copy link
Contributor Author

jingyih commented Oct 3, 2018

@gyuho
There is a goword checking failure introduced by a comment line in previously merged PR. Can you help take a look?

@jingyih
Copy link
Contributor Author

jingyih commented Oct 3, 2018

@gyuho
I will send a PR to fix the goword checking failure in clientv3/config.go

@jingyih jingyih force-pushed the add_unit_test_for_snapshot_file_integrity branch from e2b27a5 to 647f6f1 Compare October 3, 2018 06:48
@jpbetz
Copy link
Contributor

jpbetz commented Oct 3, 2018

lgtm

goword failure is odd, is it really including comma as a token in the string is spell checking? Or am I misunderstanding?

@jingyih
Copy link
Contributor Author

jingyih commented Oct 3, 2018

@jpbetz I have the same understanding. I already sent a PR to change the wording so that goword does not complain.

@jpbetz
Copy link
Contributor

jpbetz commented Oct 3, 2018

Triggering rebuild to pick up goword fix in other PR.

@gyuho
Copy link
Contributor

gyuho commented Oct 3, 2018

Yeah, for now we can whitelist here https://github.com/etcd-io/etcd/blob/master/.words.

@jingyih jingyih force-pushed the add_unit_test_for_snapshot_file_integrity branch from 647f6f1 to bdb00b9 Compare October 3, 2018 22:18
Add unit test to check if we can correctly identify a corrupted snapshot
backup file.
@jingyih jingyih force-pushed the add_unit_test_for_snapshot_file_integrity branch from bdb00b9 to 87beb83 Compare October 4, 2018 01:17
@jingyih jingyih merged commit 6976819 into etcd-io:master Oct 4, 2018
@jingyih jingyih deleted the add_unit_test_for_snapshot_file_integrity branch September 7, 2019 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants