Raft async storage writes - #22223
Conversation
Replace the overloaded notify channel with explicit completion signals for snapshot persistence, Raft storage updates, and state machine application. Signed-off-by: Michael Shen <mishen@umich.edu>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mjlshen The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @mjlshen. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| // Bound the number of append messages accepted ahead of stable storage. | ||
| // The append worker coalesces compatible messages into a single WAL sync. | ||
| // This is a performance tuning value, not a Raft protocol limit. | ||
| asyncAppendQueueSize = 64 |
There was a problem hiding this comment.
TODO: This value was selected based on local performance experiments and is not otherwise fixed. Once there's an agreed upon way to benchmark this change, this parameter should be re-swept before this PR should be merged.
| // RaftAsyncStorageWrites enables Raft log persistence and state machine application | ||
| // through Raft's asynchronous storage write message interface. | ||
| // owner: @mjlshen | ||
| // issue: https://github.com/etcd-io/etcd/issues/16291 | ||
| // alpha: v3.8 | ||
| RaftAsyncStorageWrites featuregate.Feature = "RaftAsyncStorageWrites" |
There was a problem hiding this comment.
We may or may not want a feature flag (discussion), but it appears that we can't run antithesis tests on code that adds a feature flag and wants to use it immediately. If we choose to keep the feature flag, then I can make a separate MR so that:
- The changes to this file are in the MR and merge in
- The rest of this PRs changes, importantly
tests/antithesis/config/*which try to enable theRaftAsyncStorageWritesfeature flag immediately.
|
/ok-to-test Looks promising but the most important test is the robustness one to confirm correctness of implementation. |
| SetMemberLocalAddr: {Default: false, PreRelease: featuregate.Alpha}, | ||
| FastLeaseKeepAlive: {Default: true, PreRelease: featuregate.Beta}, | ||
| PriorityRequest: {Default: false, PreRelease: featuregate.Alpha}, | ||
| RaftAsyncStorageWrites: {Default: false, PreRelease: featuregate.Alpha}, |
There was a problem hiding this comment.
For test sake, could you flip the flag to true so as we review PR we know that all tests are passing on the new implementation?
There was a problem hiding this comment.
Sure thing! I was thinking of opening an issue so that Antithesis can handle this general workflow of adding a featuregate that defaults to false and validating it in the same PR as well if that sounds good to you.
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 19 files with indirect coverage changes @@ Coverage Diff @@
## main #22223 +/- ##
==========================================
+ Coverage 69.70% 69.80% +0.09%
==========================================
Files 449 449
Lines 38207 38421 +214
==========================================
+ Hits 26631 26818 +187
- Misses 10148 10165 +17
- Partials 1428 1438 +10 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
f7e8c91 to
a1def08
Compare
Add an alpha feature gate, reliable append and apply lanes, safe WAL batching, and focused synchronization tests for Raft asynchronous storage writes. Signed-off-by: Michael Shen <mishen@umich.edu>
Cover snapshot catch-up and failpoint-driven robustness with Raft asynchronous storage writes enabled. Signed-off-by: Michael Shen <mishen@umich.edu>
Exercise the alpha Raft asynchronous storage-write path in the one- and three-node Antithesis configurations. Signed-off-by: Michael Shen <mishen@umich.edu>
Signed-off-by: Michael Shen <mishen@umich.edu>
a1def08 to
3a64fae
Compare
|
Seeing a lot of |
|
@mjlshen: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This PR was written in part with the assistance of generative AI, specifically the tests and designing the preliminary performance experiment whose results are in the description below. I am open to any other guidance around how best to take a benchmark before/after this PR's changes to provide what's expected.
Fixes #16291
What this changes
Integrates Raft asynchronous storage writes into
etcdserverbehind the alphaRaftAsyncStorageWritesfeature gate, disabled by default.When enabled, etcd:
Preliminary performance
I ran five repetitions before/after the changes using a three-member Docker Desktop cluster on my MacBook
Results:
The preliminary results show an improvement! But emphasizing that I am open to any other guidance around how best to take a benchmark before/after this PR's changes to provide potentially more accurate numbers.