Ensure raft storage lock file is update atomically #10683

deepthidevaki · 2022-10-12T11:55:28Z

Description

Previously the file creation and updating the contents were not done atomically. Moreover the content of the files were not flushed immediately. Because of this, if the pod restarts there is a chance the lock file exists but it is empty. As a result, a new lock cannot be acquired and the partition startup fails.

To fix this, we first the write to a temporary file with "SYNC" option and then move the file atomically to the actual lock file.

Existing tests are refactored. No new test is added to verify this, as it is difficult to simulate crashes while acquiring the lock.

Related issues

closes #10681

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/1.3) 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
If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

This is required to prevent an empty lock files during restart, if the system crashed before the lock content is written to the file.

github-actions · 2022-10-12T12:11:44Z

Test Results

  939 files ±    0   939 suites ±0 1h 44m 3s ⏱️ - 5m 35s
7 459 tests - 329 7 453 ✔️ - 329 6 💤 ±0 0 ❌ ±0
7 649 runs - 329 7 641 ✔️ - 329 8 💤 ±0 0 ❌ ±0

Results for commit 8694c15. ± Comparison against base commit fb62d65.

Zelldon

Thanks @deepthidevaki as always for the quick fix! Please consider my comments before merging :)

Zelldon · 2022-10-13T06:43:08Z

atomix/cluster/src/main/java/io/atomix/raft/storage/RaftStorage.java

+            StandardOpenOption.SYNC);
+
+        // If two nodes tries to acquire lock, move will fail with FileAlreadyExistsException
+        FileUtil.moveDurably(


❓ I can remember a discussion that this was only supported on some environments wasn't that something? Like only linux or? Is this an issue?

Do you know what exactly was the problem?

Flushing the parent directory does not work in windows. But according to what is documented in FileUtil, this is ok.

We are using FileUtil#moveDurably in other places as well. So far no problems are reported. So I guess it should be ok. So I will merge this PR. If we see/know any problems later, let's tackle it then.

atomix/cluster/src/main/java/io/atomix/raft/storage/RaftStorage.java

Zelldon · 2022-10-13T06:47:15Z

atomix/cluster/src/test/java/io/atomix/raft/storage/RaftStorageTest.java

+  }
+
+  @Test
+  public void canAcquireLockOnDirectoryLockedBySameNode() {


💭 Just as an idea regarding another test. If we would make the writing to a file injectable (via dependency injection) we could also fail the writing and write a test whether failing write doesn't lock the storage anymore.

🤔 Ya. May be. I will pass it for now. It would be also good if we can inject a mock filesystem in which we can simulate all kinds of failure.

Agree but then we shouldn't longer use Files class :) or at least some wrapper :)

I think @npepinpe had looked into to similar ideas for testing journal.

deepthidevaki · 2022-10-13T07:41:14Z

bors merge

zeebe-bors-camunda · 2022-10-13T07:57:06Z

Build succeeded:

Test summary

backport-action · 2022-10-13T07:58:18Z

Successfully created backport PR #10703 for stable/8.0.

backport-action · 2022-10-13T07:58:27Z

Successfully created backport PR #10704 for stable/8.1.

10704: [Backport stable/8.1] Ensure raft storage lock file is update atomically r=deepthidevaki a=backport-action # Description Backport of #10683 to `stable/8.1`. closes #10681 Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>

10703: [Backport stable/8.0] Ensure raft storage lock file is update atomically r=deepthidevaki a=backport-action # Description Backport of #10683 to `stable/8.0`. closes #10681 Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>

deepthidevaki added 2 commits October 12, 2022 13:37

fix(raft): ensure lock files is created and updated atomically

8322db7

This is required to prevent an empty lock files during restart, if the system crashed before the lock content is written to the file.

test(raft): refactor RaftStorageTest

8694c15

deepthidevaki added backport stable/8.0 labels Oct 12, 2022

deepthidevaki requested a review from Zelldon October 12, 2022 11:55

Zelldon approved these changes Oct 13, 2022

View reviewed changes

zeebe-bors-camunda bot merged commit fe37687 into main Oct 13, 2022

zeebe-bors-camunda bot deleted the dd-10681-raft-lock branch October 13, 2022 07:57

backport-action mentioned this pull request Oct 13, 2022

[Backport stable/8.0] Ensure raft storage lock file is update atomically #10703

Merged

backport-action mentioned this pull request Oct 13, 2022

[Backport stable/8.1] Ensure raft storage lock file is update atomically #10704

Merged

korthout added the version:8.1.1 Marks an issue as being completely or in parts released in 8.1.1 label Oct 13, 2022

korthout added the release/8.0.8 label Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure raft storage lock file is update atomically #10683

Ensure raft storage lock file is update atomically #10683

deepthidevaki commented Oct 12, 2022

github-actions bot commented Oct 12, 2022

Zelldon left a comment

Zelldon Oct 13, 2022

deepthidevaki Oct 13, 2022

deepthidevaki Oct 13, 2022

Zelldon Oct 13, 2022

deepthidevaki Oct 13, 2022

Zelldon Oct 13, 2022

deepthidevaki Oct 13, 2022 •

edited

deepthidevaki commented Oct 13, 2022

zeebe-bors-camunda bot commented Oct 13, 2022

backport-action commented Oct 13, 2022

backport-action commented Oct 13, 2022

Ensure raft storage lock file is update atomically #10683

Ensure raft storage lock file is update atomically #10683

Conversation

deepthidevaki commented Oct 12, 2022

Description

Related issues

Definition of Done

github-actions bot commented Oct 12, 2022

Test Results

Zelldon left a comment

Choose a reason for hiding this comment

Zelldon Oct 13, 2022

Choose a reason for hiding this comment

deepthidevaki Oct 13, 2022

Choose a reason for hiding this comment

deepthidevaki Oct 13, 2022

Choose a reason for hiding this comment

Zelldon Oct 13, 2022

Choose a reason for hiding this comment

deepthidevaki Oct 13, 2022

Choose a reason for hiding this comment

Zelldon Oct 13, 2022

Choose a reason for hiding this comment

deepthidevaki Oct 13, 2022 • edited

Choose a reason for hiding this comment

deepthidevaki commented Oct 13, 2022

zeebe-bors-camunda bot commented Oct 13, 2022

backport-action commented Oct 13, 2022

backport-action commented Oct 13, 2022

deepthidevaki Oct 13, 2022 •

edited