Do not load v2 snapshot on bootstrap by ahrtr · Pull Request #21107 · etcd-io/etcd

ahrtr · 2026-01-12T10:55:48Z

Link to #20187

In this PR, we don't load v2 snapshot files (*.snap) anymore, instead we just pick the latest snapshot entry from WAL. We don't need the raftpb.Snapshot.Data anymore, so it's correct to just read the latest snapshot entry from WAL.

The end goal is to completely get rid of v2 snapshot, which means we don't read v2 snapshot, nor write it. But in order to be compatible with 3.6, we still need to write v2 snapshot. This PR just stops reading v2 snapshot, so that it's safe to completely remove it in 3.8.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

codecov · 2026-01-12T11:26:55Z

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.46%. Comparing base (f5e6640) to head (e74bb6a).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
etcdutl/etcdutl/common.go	60.00%	2 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
server/etcdserver/bootstrap.go	`65.81% <100.00%> (+0.56%)`	⬆️
etcdutl/etcdutl/common.go	`35.29% <60.00%> (-10.86%)`	⬇️

... and 20 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #21107      +/-   ##
==========================================
+ Coverage   68.39%   68.46%   +0.07%     
==========================================
  Files         429      429              
  Lines       35244    35242       -2     
==========================================
+ Hits        24105    24130      +25     
+ Misses       9745     9728      -17     
+ Partials     1394     1384      -10

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 f5e6640...e74bb6a. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ahrtr · 2026-01-12T16:30:54Z

@fuweid @serathius PTAL

Note We still need to generate v2 snapshot in 3.7, so as to be compatible with 3.6. We don't need to generate v2 snapshot at all in 3.8.

serathius · 2026-01-12T17:17:31Z

+			},
+		}
+		if walSnaps[idx].ConfState != nil {
+			snapshot.Metadata.ConfState = *walSnaps[idx].ConfState


Shouldn't we read ConfState from v3?

This is the next step.

I am trying to keep this PR as small as possible so that it's easier to review. The change in this PR should be independent. The goal is that we don't load v2snapshot at all in this PR, so that it's safe to completely remove v2snapshot in 3.8.

I don't follow how dropping snapshotter logic to carefully select a snapshot and replacing it with just picking the last one based on WAL helps with that.

I think my comment above #21107 (comment) already clarify the goal of this PR.

also #21107 (comment)

No, having small is not a goal, it's a good practice. I think the goal should be avoiding incorrect etcd behavior, even if it's temporary.

Main thing confusing for me is title of PR doesn't match the code. We still load v2 snapshot, we just don't use snapshotter. Don't know if there is much benefit of dropping snapshotter, it's just logic to select which snapshot to open, so we don't really loading v2 snapshot, we just downgraded our logic to select it.

No, having small is not a goal, it's a good practice

As mentioned above multiple times, the end goal is to completely get rid of v2 snapshot, which means we don't read v2 snapshot, nor write it. But in order to be compatible with 3.6, we still need to write v2 snapshot. This PR just stops read v2 snapshot, so that it's safe to completely remove it in 3.8.

I think the goal should be avoiding incorrect etcd behavior,

Do you see any incorrect behaviour?

Main thing confusing for me is title of PR doesn't match the code. We still load v2 snapshot

No, etcd doesn't load v2 snapshot anymore in this PR.

Note that snapshot is a generic concept in raft. raft may bootstrap from a snapshot. etcd also processes snapshots in two ways:

send snapshot to slow followers

generate snapshot every 10K entries. This may not be necessary, but it's a good timing to purge old entries.

There is not undermining the end goal. I'm asking about the goal of this PR.

I missed one thing, this PR replaces snapshotter loading snapshot with just picking last Snapshot entry from WAL. As we no longer need the snapshot file to exist, just picking the last entry is correct. I think it makes sense now, this is a correct next step.

To speed up the review, please consider adding context to the PR. What the change is doing and why this is correct. Just "Do not load v2 snapshot on bootstrap" is not enough, as there is no direct code that you change that touches snapshots.

What the change is doing and why this is correct. Just "Do not load v2 snapshot on bootstrap" is not enough, as there is no direct code that you change that touches snapshots.

Just updated the description. thx

k8s-ci-robot · 2026-01-13T10:18:54Z

[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:

~~OWNERS~~ [ahrtr,serathius]

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

In this commit, we don't load v2 snapshot files (*.snap) anymore, instead we just pick the latest snapshot entry from WAL. We don't need the raftpb.Snapshot.Data anymore, so it's correct to just read the latest snapshot entry from WAL. The end goal is to completely get rid of v2 snapshot, which means we don't read v2 snapshot, nor write it. But in order to be compatible with 3.6, we still need to write v2 snapshot. This PR just stops reading v2 snapshot, so that it's safe to completely remove it in 3.8. Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

k8s-ci-robot · 2026-01-13T11:15:13Z

@ahrtr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-coverage-report	`e74bb6a`	link	true	`/test pull-etcd-coverage-report`

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.

Details

Instructions 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.

fuweid

LGTM

fuweid · 2026-01-13T19:33:43Z

-			"recovered v2 store from snapshot",
+	var snapshot *raftpb.Snapshot
+	if len(walSnaps) > 0 {
+		idx := len(walSnaps) - 1


ETCD commits the snapshot to disk first and then appends the WAL. It should be fine and it won't break test cases.

k8s-ci-robot added do-not-merge/work-in-progress approved size/S labels Jan 12, 2026

ahrtr force-pushed the 20260112_bootstrap_v3_not_read_v2snap branch from d517cd1 to 881a355 Compare January 12, 2026 11:07

k8s-ci-robot added size/M and removed size/S labels Jan 12, 2026

ahrtr force-pushed the 20260112_bootstrap_v3_not_read_v2snap branch from 881a355 to d5f4a0d Compare January 12, 2026 15:50

ahrtr changed the title ~~[WIP] Do not load v2 snapshot on bootstrap~~ Do not load v2 snapshot on bootstrap Jan 12, 2026

k8s-ci-robot removed the do-not-merge/work-in-progress label Jan 12, 2026

ahrtr mentioned this pull request Jan 12, 2026

Bootstrap etcdserver from v3store (consistent_index) #20187

Open

serathius reviewed Jan 12, 2026

View reviewed changes

ahrtr force-pushed the 20260112_bootstrap_v3_not_read_v2snap branch from d5f4a0d to d68f180 Compare January 12, 2026 17:18

k8s-ci-robot added area/etcdutl size/L and removed size/M labels Jan 12, 2026

ahrtr force-pushed the 20260112_bootstrap_v3_not_read_v2snap branch 2 times, most recently from 686f0f9 to f6241ee Compare January 12, 2026 17:32

serathius reviewed Jan 13, 2026

View reviewed changes

Comment thread server/etcdserver/api/snap/snapshotter_test.go

serathius approved these changes Jan 13, 2026

View reviewed changes

ahrtr force-pushed the 20260112_bootstrap_v3_not_read_v2snap branch from f6241ee to e74bb6a Compare January 13, 2026 10:42

k8s-ci-robot added size/M and removed size/L labels Jan 13, 2026

ahrtr merged commit c8a05d6 into etcd-io:main Jan 13, 2026
31 of 32 checks passed

ahrtr deleted the 20260112_bootstrap_v3_not_read_v2snap branch January 13, 2026 11:33

fuweid reviewed Jan 13, 2026

View reviewed changes

Conversation

ahrtr commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ahrtr commented Jan 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serathius Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

k8s-ci-robot commented Jan 13, 2026

Uh oh!

k8s-ci-robot commented Jan 13, 2026

Uh oh!

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ahrtr commented Jan 12, 2026 •

edited

Loading

codecov bot commented Jan 12, 2026 •

edited

Loading

serathius Jan 13, 2026 •

edited

Loading