Guard from concurrent elections #21361

bashtanov · 2024-07-12T15:12:51Z

At the moment nothing prevents a raft node from being a candidate in two elections at the same time, two vote_stms actively working. This may lead to a nasty race condition where one of them has succeeded (becoming a leader, last heartbeat goes max), and then the other one fails (role becomes follower, no changes to last heartbeat). As a result, a node never becomes a candidate, as it's not a leader but has max last heartbeat.

This PR is mostly should guard against concurrent elections with the same candidate. Some minor fixes and refactorings included.

Backports Required

Release Notes

none

bashtanov · 2024-07-12T16:25:48Z

/dt

bashtanov · 2024-07-12T22:05:40Z

/dt

vbotbuildovich · 2024-07-13T01:00:57Z

skipped ducktape retry in https://buildkite.com/redpanda/redpanda/builds/51446#0190a954-a6c1-44e8-bbf6-044ccd4e04d1:
pandatriage cache was not found

bashtanov · 2024-07-15T07:31:51Z

DT failure: #21376

vbotbuildovich · 2024-07-15T18:01:45Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51524#0190b743-1966-448c-b79e-880fdf3b39f0

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51591#0190bcb2-9e1d-4673-96da-8dac9e306a68

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51635#0190c012-df11-4b80-9bf3-be0fadc9ac92

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51726#0190c642-c227-4057-b75f-39430f3c9822

bashtanov · 2024-07-16T16:48:06Z

I removed the consensus.cc continuations refactoring, as we decided to do it differently and it doesn't really interfere with my other changes in this PR (I originally did them because I thought of touching this part of the code). Maybe next time.

src/v/raft/consensus.h

bharathv · 2024-07-16T19:34:44Z

src/v/raft/vote_stm.cc

@@ -479,4 +479,13 @@ ss::future<> vote_stm::self_vote() {
    auto m = _replies.find(_ptr->self());
    m->second.set_value(reply);
 }
+
+void vote_stm::lose() {


nit: a more descriptive name would be nicer

Changed to lose_election, do you think it's better? (I'm not quite sure what else we can lose in a class whose purpose is to run an election)

Also if we're after naming things right I'd rename half of the vote to election to clarify whether we arrange an election or merely vote in an election already organised. Say, consensus::vote() vs vote_stm::vote confused me, as the latter runs an election, not just votes really. How conservative are we regarding existing names? Is it okay to rename things if I touch the code around them?

src/v/raft/vote_stm.cc

bashtanov · 2024-07-22T07:41:04Z

failure in previous build unrelated: https://redpandadata.atlassian.net/browse/CORE-5736

vstm has already been moved-from, it makes no sense to capture it here

will be used later to guard from concurrent elections

Use a lock held by critical part of the voting process, released when decision is made

vbotbuildovich · 2024-07-22T13:56:40Z

/backport v24.1.x

vbotbuildovich · 2024-07-22T13:56:41Z

/backport v23.3.x

vbotbuildovich · 2024-07-22T13:56:42Z

/backport v23.2.x

vbotbuildovich · 2024-07-22T13:57:30Z

Oops! Something went wrong.

Workflow run logs.

vbotbuildovich · 2024-07-22T13:57:42Z

Failed to create a backport PR to v23.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-21361-v23.3.x-580 remotes/upstream/v23.3.x
git cherry-pick -x cafb4458f7fe29071ddc2d4e9ffa1a4224b6cfae f8f7561a9e1739916244b98d1de51560a0171ce6 a7f1d75a91477e55522d836b8ae9e8da204992c5 97e52de555701a0ee5ce6129bca8bef82920cceb 8a933e6976fa7bc9ebb93942fcb36d59e3e46b1c

Workflow run logs.

dotnwat · 2024-07-25T19:09:07Z

src/v/raft/consensus.cc

@@ -1046,9 +1046,7 @@ void consensus::dispatch_vote(bool leadership_transfer) {
                          }
                          // background
                          ssx::spawn_with_gate(
-                            _bg,
-                            [vstm = std::move(vstm),


not just dead, but i wonder why clang-tidy didn't warn against what looks like a use-after-move (or at least a move-after-move).

https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html#use:

An exception to this are objects of type std::unique_ptr, std::shared_ptr and std::weak_ptr, which have defined move behavior (objects of these classes are guaranteed to be empty after they have been moved from). Therefore, an object of these classes will only be considered to be used if it is dereferenced, i.e. if operator*, operator-> or operator[] (in the case of std::unique_ptr<T []>) is called on it.

oh interesting TIL

Backport for "Merged Guard from concurrent elections" #21361

bashtanov · 2024-11-20T12:59:44Z

/backport v24.2.x

github-actions bot added the area/redpanda label Jul 12, 2024

bashtanov force-pushed the guard-from-concurrent-elections branch from 006353b to 62aa52f Compare July 12, 2024 16:17

bashtanov force-pushed the guard-from-concurrent-elections branch from 62aa52f to a4e6535 Compare July 15, 2024 14:59

bashtanov marked this pull request as ready for review July 15, 2024 14:59

bashtanov marked this pull request as draft July 15, 2024 15:00

bashtanov requested review from mmaslankaprv, bharathv and ztlpn July 15, 2024 15:11

bashtanov marked this pull request as ready for review July 15, 2024 15:11

bashtanov force-pushed the guard-from-concurrent-elections branch from a4e6535 to deabb87 Compare July 16, 2024 16:45

bharathv reviewed Jul 16, 2024

View reviewed changes

mmaslankaprv reviewed Jul 17, 2024

View reviewed changes

src/v/raft/vote_stm.cc Show resolved Hide resolved

bashtanov force-pushed the guard-from-concurrent-elections branch 2 times, most recently from 81fdfbe to ef89f73 Compare July 18, 2024 13:21

bashtanov requested review from mmaslankaprv and bharathv July 22, 2024 07:41

bashtanov force-pushed the guard-from-concurrent-elections branch from ef89f73 to 5e8643a Compare July 22, 2024 10:42

bashtanov added 5 commits July 22, 2024 11:42

r/consensus: remove dead code

cafb445

vstm has already been moved-from, it makes no sense to capture it here

raft: correct comments

f8f7561

r/consensus: add election lock

a7f1d75

will be used later to guard from concurrent elections

r/vote_stm: guard from concurrent elections where we are the candidate

97e52de

Use a lock held by critical part of the voting process, released when decision is made

r/vote_stm: assert we're not becoming a leader in the background

8a933e6

bashtanov force-pushed the guard-from-concurrent-elections branch from 5e8643a to 8a933e6 Compare July 22, 2024 10:42

mmaslankaprv approved these changes Jul 22, 2024

View reviewed changes

bashtanov merged commit c925d0d into redpanda-data:dev Jul 22, 2024
19 checks passed

vbotbuildovich mentioned this pull request Jul 22, 2024

[v23.2.x] Guard from concurrent elections #21564

Closed

This was referenced Jul 22, 2024

[v24.1.x] Guard from concurrent elections #21565

Merged

[v23.3.x] Guard from concurrent elections #21566

Closed

bashtanov mentioned this pull request Jul 22, 2024

Backport for "Merged Guard from concurrent elections" #21361 #21568

Merged

7 tasks

dotnwat reviewed Jul 25, 2024

View reviewed changes

mmaslankaprv added a commit that referenced this pull request Jul 30, 2024

Merge pull request #21568 from bashtanov/backport-pr-21361-v23.3.x-580

26114c9

Backport for "Merged Guard from concurrent elections" #21361

bashtanov mentioned this pull request Jul 31, 2024

CI Failure (TimeoutError: KgoVerifierConsumerGroupConsumer didn't complete in 599 seconds) in RandomNodeOperationsTest.test_node_operations #21345

Closed

vbotbuildovich mentioned this pull request Nov 20, 2024

[v24.2.x] Guard from concurrent elections #24196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guard from concurrent elections #21361

Guard from concurrent elections #21361

bashtanov commented Jul 12, 2024 •

edited

Loading

bashtanov commented Jul 12, 2024

bashtanov commented Jul 12, 2024

vbotbuildovich commented Jul 13, 2024

bashtanov commented Jul 15, 2024

vbotbuildovich commented Jul 15, 2024 •

edited

Loading

bashtanov commented Jul 16, 2024

bharathv Jul 16, 2024

mmaslankaprv Jul 17, 2024

bashtanov Jul 17, 2024

bashtanov commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

dotnwat Jul 25, 2024

bashtanov Jul 31, 2024

dotnwat Oct 8, 2024

bashtanov commented Nov 20, 2024

Guard from concurrent elections #21361

Guard from concurrent elections #21361

Conversation

bashtanov commented Jul 12, 2024 • edited Loading

Backports Required

Release Notes

bashtanov commented Jul 12, 2024

bashtanov commented Jul 12, 2024

vbotbuildovich commented Jul 13, 2024

bashtanov commented Jul 15, 2024

vbotbuildovich commented Jul 15, 2024 • edited Loading

bashtanov commented Jul 16, 2024

bharathv Jul 16, 2024

Choose a reason for hiding this comment

mmaslankaprv Jul 17, 2024

Choose a reason for hiding this comment

bashtanov Jul 17, 2024

Choose a reason for hiding this comment

bashtanov commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

vbotbuildovich commented Jul 22, 2024

dotnwat Jul 25, 2024

Choose a reason for hiding this comment

bashtanov Jul 31, 2024

Choose a reason for hiding this comment

dotnwat Oct 8, 2024

Choose a reason for hiding this comment

bashtanov commented Nov 20, 2024

bashtanov commented Jul 12, 2024 •

edited

Loading

vbotbuildovich commented Jul 15, 2024 •

edited

Loading