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

kvserver: fix and unskip TestCheckConsistencyInconsistent #99114

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

pav-kv
Copy link
Collaborator

@pav-kv pav-kv commented Mar 21, 2023

This PR unskips TestCheckConsistencyInconsistent which was skipped for a reason that no longer holds.

It also fixes the race possible in TestCheckConsistencyInconsistent:

  • Node 2 is corrupted.
  • The second phase of runConsistency check times out on node 1, and returns early when only nodes 2 and 3 have created the storage checkpoint.
  • Node 1 haven't created the checkpoint, but has started doing so.
  • Node 2 "fatals" (mocked out in the test) shortly after the check is complete.
  • Node 1 is still creating its checkpoint, but has probably created the directory by now.
  • Hence, the test assumes that the checkpoint has been created, and proceeds to open it and compute the checksum of the range.

The test now waits for the moment when all the checkpoint are known to be fully populated.

Fixes #81819
Epic: none
Release note: none

@pav-kv pav-kv requested review from tbg and a team March 21, 2023 13:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pav-kv pav-kv requested a review from erikgrinaker March 21, 2023 13:42
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@pav-kv
Copy link
Collaborator Author

pav-kv commented Mar 21, 2023

bors r=erikgrinaker

@tbg tbg removed their request for review March 21, 2023 15:40
@craig
Copy link
Contributor

craig bot commented Mar 21, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 21, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 21, 2023

Merge conflict.

pav-kv added 2 commits March 22, 2023 09:34
This commit fixes the race possible in TestCheckConsistencyInconsistent:
- Node 2 is corrupted.
- The second phase of runConsistency check times out on node 1, and returns
  early when only nodes 2 and 3 have created the storage checkpoint.
- Node 1 haven't created the checkpoint, but has started doing so.
- Node 2 "fatals" (mocked out in the test) shortly after the check is complete.
- Node 1 is still creating its checkpoint, but has probably created the
  directory by now.
- Hence, the test assumes that the checkpoint has been created, and proceeds to
  open it and compute the checksum of the range.

This commit makes the test wait for the moment when all the checkpoint are
known to be fully populated.

Part of cockroachdb#81819
Epic: none
Release note: none
The test was skipped for a reason that no longer holds.

Epic: none
Release note: none
@pav-kv pav-kv force-pushed the fix-and-unskip-consistency-test branch from a5bbd5c to 6372b6e Compare March 22, 2023 09:35
@pav-kv pav-kv added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 22, 2023
@pav-kv
Copy link
Collaborator Author

pav-kv commented Mar 22, 2023

bors r=erikgrinaker

@craig
Copy link
Contributor

craig bot commented Mar 22, 2023

Build failed:

@pav-kv
Copy link
Collaborator Author

pav-kv commented Mar 22, 2023

bors retry

@craig
Copy link
Contributor

craig bot commented Mar 22, 2023

Build failed:

@pav-kv
Copy link
Collaborator Author

pav-kv commented Mar 22, 2023

bors retry

@craig
Copy link
Contributor

craig bot commented Mar 22, 2023

Build failed:

@pav-kv
Copy link
Collaborator Author

pav-kv commented Mar 22, 2023

bors retry

@craig
Copy link
Contributor

craig bot commented Mar 22, 2023

Build succeeded:

@craig craig bot merged commit f592975 into cockroachdb:master Mar 22, 2023
@pav-kv pav-kv deleted the fix-and-unskip-consistency-test branch March 22, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kvserver: TestCheckConsistencyInconsistent failed, data race in TestingSetRedactable
3 participants