Ensure leader always commits when replicated to quorum #8357

deepthidevaki · 2021-12-10T14:01:36Z

Description

Added a randomized liveness test similar to consistency test that already exists. This test was able to reproduce the bug in Raft leader sometimes does not update commitIndex even if the records are replicated to a quorum #8324 The new test verifies that in the presence of no message lost or delay raft can make progress - that means a leader will be elected, all entries are replicated to all followers, and all entries are committed.
Leader will now commit entries when receiving response for a heartbeat, thus ensuring that it can make progress.

Related issues

closes #8324

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

The changes are backwards compatibility with previous versions
If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

Testing:

There are unit/integration tests that verify all acceptance criterias of the issue
New tests are written to ensure backwards compatibility with further versions
The behavior is tested manually
The change has been verified by a QA run
The impact of the changes is verified by a benchmark

Documentation:

The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
New content is added to the release announcement

npepinpe · 2021-12-13T13:55:57Z

Code freeze is next Monday - @oleschoenburg or @Zelldon can one of you review and take this over so it can make it into 1.3.0? Thanks! Let me know if you don't have time, Roman or me could also look into it.

Zelldon

Thanks @deepthidevaki for your fix and work 🎖️

I have some comments and questions, @oleschoenburg could you also please review this PR and than we can discuss them maybe you have an answer or different opinion. Afterwards we could adjust the PR and merge it :) What do you think ?

atomix/cluster/src/test/java/io/atomix/raft/ControllableRaftContexts.java

atomix/cluster/src/test/java/io/atomix/raft/protocol/ControllableRaftServerProtocol.java

atomix/cluster/src/test/java/io/atomix/raft/ControllableRaftContexts.java

atomix/cluster/src/test/java/io/atomix/raft/RandomizedRaftTest.java

atomix/cluster/src/main/java/io/atomix/raft/roles/LeaderAppender.java

lenaschoenburg

The fix itself looks great but I think some of the test code could be improved. Nothing blocking from my side though, so I'm approving.

atomix/cluster/src/test/java/io/atomix/raft/protocol/ControllableRaftServerProtocol.java

lenaschoenburg · 2021-12-14T11:50:10Z

atomix/cluster/src/test/java/io/atomix/raft/ControllableRaftContexts.java

+  boolean hasCommittedAllEntries() {
+    return raftServers.values().stream()
+        .noneMatch(
+            s -> {
+              final var lastCommittedEntry = getLastCommittedEntry(s);
+              final var lastUncommittedEntry = getLastUncommittedEntry(s);
+
+              return lastUncommittedEntry != null
+                  && !lastUncommittedEntry.equals(lastCommittedEntry);
+            });
+  }


🔧 I found this method to be a bit difficult to understand, maybe we could simplify?

boolean hasCommittedAllEntries() { return raftServers.values().stream().allMatch(s -> getLastUncommittedEntry(s) == getLastCommittedEntry(s)); }

I think == doesn't work here because each reader creates a different object for the same entry. But I will use allMatch instead of noneMatch.

deepthidevaki · 2021-12-20T07:59:23Z

Thanks @oleschoenburg @Zelldon for your review. I will apply your comments and try to get it merge this before code-freeze.

deepthidevaki · 2021-12-20T08:53:51Z

@Zelldon @oleschoenburg I have updated the PR. Please take a look again.

Zelldon

Thanks @deepthidevaki 🎖️

deepthidevaki · 2021-12-20T09:29:00Z

bors merge

deepthidevaki · 2021-12-20T09:29:11Z

bors r-

ghost · 2021-12-20T09:29:13Z

Canceled.

Raft expects the messaging layer to timeout the requests. In our mock implementation of the messaging layer, we did not implement the timeout correctly. As a result the requests are pending in raft. This will break some of the liveness guarantees. Ideally raft should timeout requests itself. But since that is a bigger change, we will just fix the test setup to mimic the actual implementation.

This is similar to the correctness test that already exists. First we execute a random sequence of operation, which may contain "failures" like messages are lost or delayed. Then we execute a fixed sequence of operations without any "failures". The idea is that if there are no message lost, then eventually a leader should be elected, and eventually all entries are replicated to all replicas, and all entries are committed.

If a previous ack was lost, and the followers have already received the latest logs, then the leader will send only heartbeats. In this case, previously, the leader did not commit because it did not update the commit index when an ack for heartbeat is received. This can cause problems because as long as new entries are not written the existing last few entries will never get committed.

deepthidevaki · 2021-12-20T09:33:21Z

bors merge

ghost · 2021-12-20T10:00:43Z

Build succeeded:

continuous-integration/jenkins/branch

github-actions · 2021-12-20T10:01:41Z

Successfully created backport PR #8434 for stable/1.1.

github-actions · 2021-12-20T10:01:46Z

Successfully created backport PR #8435 for stable/1.2.

8435: [Backport stable/1.2] Ensure leader always commits when replicated to quorum r=deepthidevaki a=github-actions[bot] # Description Backport of #8357 to `stable/1.2`. closes #8324 Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>

8411: Release 1.1.8 r=npepinpe a=npepinpe ## Description This PR includes improvements/fixes to the release pipeline, as well as the release commits preparing for the next patch release. The `.gocompat.json` file is expected to be modified since the ordering changes, but that's a unfortunate side effect of the tool. It's still the same, and is checked by the pipeline (both release and test) beforehand. 8434: [Backport stable/1.1] Ensure leader always commits when replicated to quorum r=deepthidevaki a=github-actions[bot] # Description Backport of #8357 to `stable/1.1`. closes #8324 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com> Co-authored-by: camunda-jenkins <ci@camunda.com> Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>

deepthidevaki requested review from Zelldon and lenaschoenburg December 10, 2021 14:01

deepthidevaki added backport stable/1.2 labels Dec 10, 2021

Zelldon reviewed Dec 14, 2021

View reviewed changes

lenaschoenburg approved these changes Dec 14, 2021

View reviewed changes

deepthidevaki requested review from Zelldon and lenaschoenburg December 20, 2021 08:53

Zelldon approved these changes Dec 20, 2021

View reviewed changes

lenaschoenburg approved these changes Dec 20, 2021

View reviewed changes

deepthidevaki added 3 commits December 20, 2021 10:32

deepthidevaki force-pushed the dd-8324-raft-commit branch from 64f3fab to 3e4556c Compare December 20, 2021 09:33

ghost merged commit fe843ca into develop Dec 20, 2021

ghost deleted the dd-8324-raft-commit branch December 20, 2021 10:00

github-actions bot mentioned this pull request Dec 20, 2021

[Backport stable/1.1] Ensure leader always commits when replicated to quorum #8434

Merged

github-actions bot mentioned this pull request Dec 20, 2021

[Backport stable/1.2] Ensure leader always commits when replicated to quorum #8435

Merged

menski added Release: 1.1.9 labels Dec 22, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure leader always commits when replicated to quorum #8357

Ensure leader always commits when replicated to quorum #8357

deepthidevaki commented Dec 10, 2021

npepinpe commented Dec 13, 2021

Zelldon left a comment

lenaschoenburg left a comment

lenaschoenburg Dec 14, 2021

deepthidevaki Dec 20, 2021

deepthidevaki commented Dec 20, 2021

deepthidevaki commented Dec 20, 2021

Zelldon left a comment

deepthidevaki commented Dec 20, 2021

deepthidevaki commented Dec 20, 2021

ghost commented Dec 20, 2021

deepthidevaki commented Dec 20, 2021

ghost commented Dec 20, 2021

github-actions bot commented Dec 20, 2021

github-actions bot commented Dec 20, 2021

Ensure leader always commits when replicated to quorum #8357

Ensure leader always commits when replicated to quorum #8357

Conversation

deepthidevaki commented Dec 10, 2021

Description

Related issues

Definition of Done

npepinpe commented Dec 13, 2021

Zelldon left a comment

Choose a reason for hiding this comment

lenaschoenburg left a comment

Choose a reason for hiding this comment

lenaschoenburg Dec 14, 2021

Choose a reason for hiding this comment

deepthidevaki Dec 20, 2021

Choose a reason for hiding this comment

deepthidevaki commented Dec 20, 2021

deepthidevaki commented Dec 20, 2021

Zelldon left a comment

Choose a reason for hiding this comment

deepthidevaki commented Dec 20, 2021

deepthidevaki commented Dec 20, 2021

ghost commented Dec 20, 2021

deepthidevaki commented Dec 20, 2021

ghost commented Dec 20, 2021

github-actions bot commented Dec 20, 2021

github-actions bot commented Dec 20, 2021