Skip to content

raft: fix flaky leader index in waitLeader function#130084

Merged
craig[bot] merged 1 commit into
cockroachdb:masterfrom
nvb:nvanbenschoten/deflake127413
Sep 6, 2024
Merged

raft: fix flaky leader index in waitLeader function#130084
craig[bot] merged 1 commit into
cockroachdb:masterfrom
nvb:nvanbenschoten/deflake127413

Conversation

@nvb

@nvb nvb commented Sep 4, 2024

Copy link
Copy Markdown
Contributor

Fixes #127413.

This commit bypasses the larger rebase in #122133 to pick up the test flake fix in etcd-io/raft#188. There was some discussion in etcd-io/raft#181 about alternatives for fixing this test. For now, we stick with a direct cherry-pick.

Release note: None

@nvb nvb added backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x labels Sep 4, 2024
@nvb nvb requested a review from pav-kv September 4, 2024 15:17
@nvb nvb requested a review from a team as a code owner September 4, 2024 15:17
@cockroach-teamcity

Copy link
Copy Markdown
Member

This change is Reviewable

@pav-kv pav-kv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for picking this up.

@pav-kv

pav-kv commented Sep 6, 2024

Copy link
Copy Markdown
Collaborator

Could you remove the TODO / skip in the beginning of TestRestart?

NB: Back then I thought that this test was inherently flaky and the fix was incomplete. There was some analysis in etcd-io/raft#195 (comment) and other comments there. We can revisit if this flake comes up again after your PR.

Comment thread pkg/raft/rafttest/node_test.go Outdated
func waitLeader(ns []*node) int {
var l map[pb.PeerID]struct{}
var lindex int
var lindex = -1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this be reset to -1 on each iteration? Might be better to move the definition inside the loop then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. I also moved to clearing the map on each iteration, instead of re-allocating it.

@pav-kv

pav-kv commented Sep 6, 2024

Copy link
Copy Markdown
Collaborator

NB: Back then I thought that this test was inherently flaky and the fix was incomplete.

I stressed this now locally 20k times with the fix applied (one with the lindex := -1 moved inside the loop, as commented), and it did not flake. Before the fix it flakes pretty eagerly. So this flake will be much less / not at all as noisy now.

@nvb

nvb commented Sep 6, 2024

Copy link
Copy Markdown
Contributor Author

Could you remove the TODO / skip in the beginning of TestRestart?

Done.

Fixes cockroachdb#127413.

This commit bypasses the larger rebase in cockroachdb#122133 to pick up the test
flake fix in etcd-io/raft#188. There was some
discussion in etcd-io/raft#181 about alternatives
for fixing this test. For now, we stick with a direct cherry-pick.

Release note: None
@nvb nvb force-pushed the nvanbenschoten/deflake127413 branch from c429d4e to bd9edcb Compare September 6, 2024 16:01
@nvb

nvb commented Sep 6, 2024

Copy link
Copy Markdown
Contributor Author

I stressed this now locally 20k times with the fix applied (one with the lindex := -1 moved inside the loop, as commented), and it did not flake. Before the fix it flakes pretty eagerly. So this flake will be much less / not at all as noisy now.

I stressed again for 20k iterations and still see that it is stable.

bors r=pav-kv

@craig

craig Bot commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

@craig craig Bot merged commit ea50475 into cockroachdb:master Sep 6, 2024
@blathers-crl

blathers-crl Bot commented Sep 6, 2024

Copy link
Copy Markdown

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.


Issue #127413: branch-release-24.1, branch-release-24.2.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl

blathers-crl Bot commented Sep 6, 2024

Copy link
Copy Markdown

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from bd9edcb to blathers/backport-release-24.1-130084: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.1.x failed. See errors above.


error creating merge commit from bd9edcb to blathers/backport-release-24.2-130084: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-24.1.x Flags PRs that need to be backported to 24.1.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

raft/rafttest: TestRestart failed

3 participants