Skip to content

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

Merged
serathius merged 6 commits intoetcd-io:mainfrom
serathius:snapshot-separate
Nov 29, 2024
Merged

Run a separate in memory snapshot to reduce number of entries stored in raft memory storage#18825
serathius merged 6 commits intoetcd-io:mainfrom
serathius:snapshot-separate

Conversation

@serathius
Copy link
Copy Markdown
Member

@serathius serathius commented Nov 2, 2024 •
edited
Loading

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
Copy Markdown

k8s-ci-robot commented Nov 2, 2024

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
Copy Markdown

codecov-commenter commented Nov 2, 2024 •
edited by codecov bot
Loading

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
ahrtr
ahrtr reviewed Nov 3, 2024
View reviewed changes
Comment thread server/etcdserver/server.go Outdated
Comment thread server/etcdserver/api/membership/cluster.go
Comment thread server/etcdserver/server.go
Comment thread server/etcdserver/server.go
@serathius
Copy link
Copy Markdown
Member Author

serathius commented Nov 28, 2024

ping @ahrtr

ahrtr
ahrtr reviewed Nov 28, 2024
View reviewed changes
Comment thread server/etcdserver/api/membership/cluster.go
Comment thread server/etcdserver/server.go
Comment thread server/etcdserver/server.go
Comment thread server/etcdserver/server.go Outdated
@ahrtr
Copy link
Copy Markdown
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.

@serathius
Rename snapshot variables to explicitly mention using disk
1529734 
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
Copy Markdown
Member Author

serathius commented Nov 29, 2024

/retest

ahrtr
ahrtr reviewed Nov 29, 2024
View reviewed changes
Comment thread server/etcdserver/server.go
Comment thread server/etcdserver/server.go
Comment thread server/etcdserver/server.go
Comment thread server/etcdserver/server_test.go Outdated
Comment thread server/etcdserver/server_test.go Outdated
@serathius
Move updating snapshot index just after we safe snapshot to disk
9d55044 
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@serathius
Separate memory and disk snapshot indexes
d907ecd 
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@serathius
Cleanup triggerSnapshot function and rename it to snapshotIfNeededAnd…
cfe958d 
…CompactRaftLog

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@serathius
Run a separate in memory snapshot to reduce number of entries stored …
6cebf97 
…in raft memory storage

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@serathius
Use assert in TestSnapshot tests
4989834 
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Nov 29, 2024

[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

Details 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
Copy Markdown
Member Author

serathius commented Nov 29, 2024

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
@jmhbnz
Copy link
Copy Markdown
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

@ahrtr ahrtr mentioned this pull request Dec 3, 2024
Closed
@serathius serathius mentioned this pull request Dec 12, 2025
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahrtr ahrtr ahrtr approved these changes

+1 more reviewer

@mmorel-35 mmorel-35 mmorel-35 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved area/testing size/L

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@serathius @k8s-ci-robot @codecov-commenter @ahrtr @jmhbnz @mmorel-35