Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run a separate in memory snapshot to reduce number of entries stored in raft memory storage #18825

Merged
merged 6 commits into from
Nov 29, 2024

Conversation

serathius
Copy link
Member

@serathius serathius commented Nov 2, 2024

Part of #17098

Alternative to #18635

Goal: Reduce number of raft entries stored in memory

Context:

  • Etcd maintains 2 different independently updates storages. V2 - stored as snapshots every X entries (100`000 by default), V3 - memory mapped bbolt storage, flushed every 5 seconds to disk
  • Etcd maintains guarantee for the data stored on disk. V3 storage needs to be always fresher than V2 snapshot. Meaning V3 consistent index is >= index of last snapshot.
  • This guarantee requires that before writing any V2 snapshots etcd needs to flush V3 storage which is costly.
  • In etcd v3.6 no longer uses v2 storage as source of truth, however it maintains periodic snapshot generation for backward compatibility.
  • Raft in memory store needs to store last snapshot and all entries since. However, this snapshot doesn't need to be persisted to disk.

Proposal:

  • Separate disk and memory snapshotting.
  • Disk snapshotting will be used for backward compatibility. Done every --snapshot-count.
  • Memory snapshots would no longer be bound by disk overhead.

Benchmark results ./bin/tools/benchmark put --total=15000 --val-size=100000

Scenario Snapshot count Memory snapshot count CPU[%] Memory[MB] PUTs per second
Base 10000 n/a 49.4 1954.5 1029.2
Snapshot in memory 10000 1 51.1 1330.4 1006.1
Snapshot in memory 10000 10 48.9 1264.2 1046.3
Snapshot in memory 10000 100 48.1 1210.4 1051.8
Snapshot in memory 10000 1000 48.2 1360.0 1047.2
Snapshot in memory 10000 10000 49.4 2040.3 1027.4

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 68.73%. Comparing base (8f91eb1) to head (4989834).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
server/etcdserver/server.go 87.23% 4 Missing and 2 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
server/etcdserver/api/membership/cluster.go 88.53% <100.00%> (ø)
server/etcdserver/server.go 82.49% <87.23%> (+0.25%) ⬆️

... and 13 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18825      +/-   ##
==========================================
- Coverage   68.80%   68.73%   -0.08%     
==========================================
  Files         420      420              
  Lines       35575    35583       +8     
==========================================
- Hits        24479    24458      -21     
- Misses       9670     9696      +26     
- Partials     1426     1429       +3     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f91eb1...4989834. Read the comment docs.

@serathius serathius force-pushed the snapshot-separate branch 3 times, most recently from d0cff01 to 2dd72a2 Compare November 2, 2024 12:43
@serathius serathius force-pushed the snapshot-separate branch 2 times, most recently from 2a560ea to df6ba3f Compare November 2, 2024 20:37
@serathius serathius marked this pull request as ready for review November 2, 2024 20:37
server/etcdserver/server.go Outdated Show resolved Hide resolved
server/etcdserver/api/membership/cluster.go Outdated Show resolved Hide resolved
server/etcdserver/server.go Outdated Show resolved Hide resolved
server/etcdserver/server.go Outdated Show resolved Hide resolved
@serathius
Copy link
Member Author

ping @ahrtr

server/etcdserver/api/membership/cluster.go Outdated Show resolved Hide resolved
server/etcdserver/server.go Outdated Show resolved Hide resolved
server/etcdserver/server.go Show resolved Hide resolved
server/etcdserver/server.go Outdated Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented Nov 28, 2024

Overall looks good to me.

We don't keep up to 100K (--snapshot-count) + 5K (catch-up-entries) raft log entries any more, instead, we only keep at most 100 (--compact-raft-log-interval-indices) + 5K.

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@serathius serathius force-pushed the snapshot-separate branch 3 times, most recently from c2f9cb8 to e4d3691 Compare November 29, 2024 13:54
@serathius
Copy link
Member Author

/retest

server/etcdserver/server.go Show resolved Hide resolved
server/etcdserver/server.go Show resolved Hide resolved
server/etcdserver/server.go Show resolved Hide resolved
server/etcdserver/server_test.go Outdated Show resolved Hide resolved
server/etcdserver/server_test.go Outdated Show resolved Hide resolved
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
…CompactRaftLog

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
…in raft memory storage

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, serathius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@serathius
Copy link
Member Author

image
Could care less about coverage going down by 0.08%. @jmhbnz can we disable this?

@serathius serathius merged commit 4068189 into etcd-io:main Nov 29, 2024
33 of 34 checks passed
@jmhbnz
Copy link
Member

jmhbnz commented Nov 30, 2024

Could care less about coverage going down by 0.08%. @jmhbnz can we disable this?

We can tune it yeah, opened #18964

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants