search

jgroups-extras / jgroups-raft

Implementation of the RAFT consensus protocol in JGroups
https://jgroups-extras.github.io/jgroups-raft/
Apache License 2.0
266 stars 84 forks source link

CounterTest.testIgnoreReturnValue test failure #222

Closed jabolina closed 11 months ago

jabolina commented 1 year ago

CI has identified a failure on CounterTest.testIgnoreReturnValue, which seems to be flaky.

https://github.com/jgroups-extras/jgroups-raft/actions/runs/6944281023/job/18891074026#step:4:1730

jabolina commented 1 year ago

Some other failures also appeared when running locally using taskset -c 0, where most seem fixed with waitUntil or derivates. One thing I want to do in the future is to abstract some of the boilerplate in the tests, something like an AbstractClusteredTest.

jabolina commented 11 months ago

I'll take this as a chance to do the above.

I'll create abstract classes to separate the test setup and combine the assertions and utilities. I'll update to include the Assertj, too. Although a little more verbose than using assert directly, it is very nice to write test assertions. I'll add everything in different commits to not mix the history too much.

jabolina commented 11 months ago

I added the last commit (9a05186b77c2472dc254182a15e3f6332a28a565) with everything discussed above. The commit creates a base structure for tests and updates the existing tests. All the assertions are shared, so there is no need to fix them across multiple files, and we could relax some of them. For example, we only wait for a majority to have a leader instead of all nodes in the cluster.

While working on the tests, I found some corner cases to fix. Since I didn't open dedicated issues to track the problems, I'll describe them here.

  1. Sending commits immediately after commit. It only affects it relying solely upon sending the entries after commit, but I believe everyone uses the task with the resend-interval configured. The issue happens with a message reordering. Since the entry is committed on a majority, when sending after a commit, one of the nodes might not know the entry to commit yet. This would cause the node to never commit since the commit message arrives before the actual entry message. I highly doubt this affects anyone because it needs to have the resend interval "disabled".
  2. Chaining the membership CF with the .thenCompose only. If the future ever fails exceptionally, the user is unable to issue membership changes after because the .thenCompose is not invoked.
  3. Completing the pending requests on the RequestTable when stepping down. Instead of letting requests time out, we complete them exceptionally. Previously, if a request doesn't have a timeout configured and the leader steps down, the request is never complete.

There is more future work, but I wanted to push what I had. We can add later a few utilities to help in some tests, something similar to the CheckPoint class we have in ISPN to test and stop at specific events. Include a StateMachine implementation relying on the CheckPoint to more easily test failure cases.

Some of the new tests to add include checking the commands complete when the leader steps down (exceptionally only for now). Assert leadership changes while applying operations, and verify redirecting during leader changes.

jabolina commented 11 months ago

And to add a little more context. I am happy with the final result of the test suite. When we started, running with taskset and single core was frequently failing. After applying all the changes, I let it run in a while loop with while taskset -c 0 mvn clean test; do :; done for around 2hrs, with each execution taking ~50s. It ran all this time without failures.

belaban commented 11 months ago

Perfect!