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

Verification of persisted data #12901

Merged
merged 4 commits into from
Apr 28, 2021
Merged

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Apr 27, 2021

Introducing a package that contains encoded invariants about etcd data model.
This package is used by tests and (opt-in) in business logic.

The purpose is:

  • to have consistent set of checks used across many tests.
  • to have (encoded) documentation of storage invariants

For now its seeded with 2 checks:

  • WAL logs consistency
  • Backend.consisistent_index need to be <= last(HardState.commit)

Verification framework is integrated with:

  • integration tests (by default)
  • ETCD_VERIFY=all etcdctl snapshot restore command
  • etcd shutdown when running with ETCD_VERIFY=all env.

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 correct. Commented on a couple minor things.

server/datadir/doc.go Outdated Show resolved Hide resolved
server/verify/verify.go Outdated Show resolved Hide resolved
if index > hardstate.Commit {
return fmt.Errorf("backend.ConsistentIndex (%v) must be < WAL.HardState.commit (%v)", index, hardstate.Commit)
}
if cfg.ExactIndex && index != hardstate.Commit {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this check above if index > hardstate.Commit check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (but not sure about the motivation)

Logger *zap.Logger
}

func Verify(cfg Config) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to document any non-obvious expectations this function has? E.g. Since it opens a backend, is it required that the backend not already be open?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

LGTM

For now verifies whete Backend.cindex is consistent with WAL log,
but should get expanded to cover memberships & revisions.
Verification framework is integrated with:
  - integration tests (by default)
  - `ETCD_VERIFY=all etcdctl snapshot restore` command
  - etcd shutdown when running with `ETCD_VERIFY=all` env.
@ptabor
Copy link
Contributor Author

ptabor commented Apr 28, 2021

Thank you for review.

@ptabor ptabor merged commit ed4a87d into etcd-io:master Apr 28, 2021
@ptabor ptabor deleted the 20210427-verification branch April 28, 2021 06:45
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.

3 participants