etcdserver: creates a non-empty raft log snapshot on server startup #18494

clement2026 · 2024-08-25T18:59:10Z

#18459 requires that a non-empty raft log snapshot is always available. This PR creates a non-empty raft log snapshot on server startup.

Part of #17098

Key changes

Alter shouldSnapshot function
Added integration tests in tests/integration/raft_log_snapshot_test.go
Fixed failing tests

Blocked by

k8s-ci-robot · 2024-08-25T18:59:19Z

Hi @clement2026. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

codecov-commenter · 2024-08-25T19:40:23Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 22.50000% with 31 lines in your changes missing coverage. Please review.

Project coverage is 68.79%. Comparing base (2c53be7) to head (8ab968e).
Report is 14 commits behind head on main.

❗ Current head 8ab968e differs from pull request most recent head ccbec07

Please upload reports for the commit ccbec07 to get more accurate results.

Files with missing lines	Patch %	Lines
client/v3/kubernetes/client.go	0.00%	27 Missing ⚠️
server/etcdserver/server.go	69.23%	3 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/etcdserver/server.go	`81.13% <69.23%> (-0.14%)`	⬇️
client/v3/kubernetes/client.go	`0.00% <0.00%> (ø)`

... and 22 files with indirect coverage changes

@@           Coverage Diff           @@
##             main   #18494   +/-   ##
=======================================
  Coverage   68.79%   68.79%           
=======================================
  Files         420      420           
  Lines       35489    35471   -18     
=======================================
- Hits        24413    24404    -9     
+ Misses       9646     9633   -13     
- Partials     1430     1434    +4

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 2c53be7...ccbec07. Read the comment docs.

k8s-ci-robot · 2024-08-25T19:54:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: clement2026
Once this PR has been reviewed and has the lgtm label, please assign ahrtr for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Signed-off-by: Clement <[email protected]>

…itial snap index Signed-off-by: Clement <[email protected]>

Signed-off-by: Clement <[email protected]>

clement2026 · 2024-08-31T18:51:17Z

server/etcdserver/server.go

@@ -2181,24 +2184,24 @@ func (s *EtcdServer) snapshot(snapi uint64, confState raftpb.ConfState) {
 	}

 	// keep some in memory log entries for slow followers.
-	compacti := uint64(1)


This could cause a panic if the applied index is also 1

clement2026 · 2024-08-31T18:55:09Z

server/etcdserver/server_test.go

@@ -99,8 +99,12 @@ func TestApplyRepeat(t *testing.T) {
 		SyncTicker:   &time.Ticker{},
 		consistIndex: cindex.NewFakeConsistentIndex(0),
 		uberApply:    uberApplierMock{},
+		kv:           mvcc.New(zaptest.NewLogger(t), be, &lease.FakeLessor{}, mvcc.StoreConfig{}),


Set kv to avoid a nil pointer panic:

etcd/server/etcdserver/server.go

Lines 2132 to 2149 in 4bb9392

func (s *EtcdServer) snapshot(snapi uint64, confState raftpb.ConfState) {

d := GetMembershipInfoInV2Format(s.Logger(), s.cluster)

// commit kv to write metadata (for example: consistent index) to disk.

//

// This guarantees that Backend's consistent_index is >= index of last snapshot.

//

// KV().commit() updates the consistent index in backend.

// All operations that update consistent index must be called sequentially

// from applyAll function.

// So KV().Commit() cannot run in parallel with toApply. It has to be called outside

// the go routine created below.

s.KV().Commit()

lg := s.Logger()

// For backward compatibility, generate v2 snapshot from v3 state.

snap, err := s.r.raftStorage.CreateSnapshot(snapi, &confState, d)

if err != nil {

clement2026 · 2024-08-31T19:05:07Z

server/etcdserver/server_test.go

 	}
 	s.start()
+
+	n.readyc <- newDummyPutReqReady()
+


The test case wasn’t appending raft log entries correctly when the applied index increases, leading to an “slice bounds out of range” panic during snapshot creation.

Can you move it to a separate PR as proposed above?

clement2026 · 2024-08-31T19:15:35Z

tests/framework/integration/cluster.go

+		}
+		listeners = append(listeners, l)
+	}
+	m.PeerListeners = listeners


The test case added a learner member with peer URL http://127.0.0.1:1234 to the cluster, but the learner member didn't listen to the host and couldn’t receive a snapshot from the leader.

Please move to a separate PR so we can merge this faster.

Signed-off-by: Clement <[email protected]>

k8s-ci-robot · 2024-08-31T19:56:07Z

@clement2026: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Signed-off-by: Clement <[email protected]>

ivanvc · 2024-09-04T20:34:00Z

/ok-to-test

clement2026 · 2024-09-06T00:43:33Z

Hey @serathius, it would be great if you could check this out when you have a moment. Thanks!

serathius · 2024-09-09T08:20:32Z

server/etcdserver/server_test.go

@@ -99,8 +99,12 @@ func TestApplyRepeat(t *testing.T) {
 		SyncTicker:   &time.Ticker{},
 		consistIndex: cindex.NewFakeConsistentIndex(0),
 		uberApply:    uberApplierMock{},
+		kv:           mvcc.New(zaptest.NewLogger(t), be, &lease.FakeLessor{}, mvcc.StoreConfig{}),


I see that we might need to fix couple of things in tests before we can merge the logic change. How about creating a separate PR to fix the tests first? That would give us confidence that there is no interdependence.

If those tests fixes are really beneficial, we can merge them immediately so detailed review needed for raft logic change.

It’s good practice to avoid interdependence. Thanks for the insight! I’ll create new PRs to fix the tests whenever they can be separated.

serathius · 2024-09-09T08:22:27Z

tests/e2e/v2store_deprecation_test.go

@@ -138,7 +139,32 @@ func TestV2DeprecationSnapshotMatches(t *testing.T) {
 	members2 := addAndRemoveKeysAndMembers(ctx, t, cc2, snapshotCount)
 	assert.NoError(t, epc.Close())

-	assertSnapshotsMatch(t, oldMemberDataDir, newMemberDataDir, func(data []byte) []byte {
+	lastVer, err := e2e.GetVersionFromBinary(e2e.BinPath.EtcdLastRelease)


Not sure why this is needed, please all test changes to separate PR.

ahrtr · 2024-09-13T13:18:02Z

#18459 requires that a non-empty raft log snapshot is always available.

Can you explain this? Why a non-empty snapshot is always required? We don't have such restriction before.

clement2026 · 2024-09-13T13:23:04Z

Can you explain this? Why a non-empty snapshot is always required? We don't have such restriction before.

@ahrtr sure, check out this comment for more details #18459 (comment)

k8s-ci-robot · 2024-09-20T17:31:41Z

PR needs rebase.

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.

clement2026 · 2024-09-23T13:24:38Z

After chatting with serathius, I've realized this PR isn’t needed anymore. I'll keep working on #17098 in a new PR.
Closing this.

k8s-ci-robot added do-not-merge/work-in-progress area/testing labels Aug 25, 2024

k8s-ci-robot added needs-ok-to-test size/M size/XS and removed size/M labels Aug 25, 2024

clement2026 force-pushed the non-empty-raft-log-snapshot-always-available branch from 86dc7a7 to a0c0639 Compare August 25, 2024 19:54

k8s-ci-robot added size/M and removed size/XS labels Aug 25, 2024

clement2026 mentioned this pull request Aug 26, 2024

etcdserver: separate raft log compact from snapshot #18459

Closed

1 task

k8s-ci-robot added size/L and removed size/M labels Aug 28, 2024

clement2026 changed the title ~~etcdserver: a non-empty raft log snapshot should always be available~~ WIP: etcdserver: a non-empty raft log snapshot should always be available Aug 28, 2024

clement2026 added 3 commits August 29, 2024 03:43

etcdserver: a non-empty raft log snapshot should always be available

fa0fb6f

Signed-off-by: Clement <[email protected]>

fix: TestMemberPromote should override PeerURLS

87faf55

Signed-off-by: Clement <[email protected]>

fix: TestGrpcProxyAutoSync should wait a bit longer at CloseProc

103398a

Signed-off-by: Clement <[email protected]>

clement2026 force-pushed the non-empty-raft-log-snapshot-always-available branch from 0a3fb00 to 103398a Compare August 28, 2024 19:51

clement2026 added 5 commits August 29, 2024 20:18

fix: compacti must be less than appliedi

926df1a

Signed-off-by: Clement <[email protected]>

fix: adjust TestV3WatchRestoreSnapshotUnsync to handle a different in…

7f42b01

…itial snap index Signed-off-by: Clement <[email protected]>

fix: TestV2SetMemberAttributes should have a kv

5e8a2f3

Signed-off-by: Clement <[email protected]>

fix: TestApplyRepeat

654aee8

Signed-off-by: Clement <[email protected]>

fix: unit tests

49c5a42

Signed-off-by: Clement <[email protected]>

clement2026 changed the title ~~WIP: etcdserver: a non-empty raft log snapshot should always be available~~ etcdserver: a non-empty raft log snapshot should always be available Aug 30, 2024

clement2026 marked this pull request as ready for review August 30, 2024 10:29

k8s-ci-robot removed the do-not-merge/work-in-progress label Aug 30, 2024

clement2026 changed the title ~~etcdserver: a non-empty raft log snapshot should always be available~~ [needs-ok-to-test] etcdserver: a non-empty raft log snapshot should always be available Aug 30, 2024

fix: TestV2DeprecationSnapshotMatches

3c14a8f

Signed-off-by: Clement <[email protected]>

fix: improve readability

6136355

Signed-off-by: Clement <[email protected]>

clement2026 changed the title ~~[needs-ok-to-test] etcdserver: a non-empty raft log snapshot should always be available~~ [WIP] etcdserver: a non-empty raft log snapshot should always be available Aug 31, 2024

k8s-ci-robot added the do-not-merge/work-in-progress label Aug 31, 2024

test: rewrite tests for raft log snapshot

1955f1e

Signed-off-by: Clement <[email protected]>

clement2026 commented Aug 31, 2024

View reviewed changes

fix: improve readability

e40267f

Signed-off-by: Clement <[email protected]>

clement2026 changed the title ~~[WIP] etcdserver: a non-empty raft log snapshot should always be available~~ etcdserver: a non-empty raft log snapshot should always be available Aug 31, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress label Aug 31, 2024

fix: improve readability

ccbec07

Signed-off-by: Clement <[email protected]>

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Sep 4, 2024

serathius reviewed Sep 9, 2024

View reviewed changes

clement2026 changed the title ~~etcdserver: a non-empty raft log snapshot should always be available~~ etcdserver: creates a non-empty raft log snapshot on server startup Sep 13, 2024

This was referenced Sep 13, 2024

tests: fix MustNewMember #18584

Closed

tests: fix TestV2DeprecationSnapshotMatches #18583

Closed

tests: fix server_test.go #18582

Closed

k8s-ci-robot added the needs-rebase label Sep 20, 2024

clement2026 closed this Sep 23, 2024

clement2026 deleted the non-empty-raft-log-snapshot-always-available branch September 24, 2024 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcdserver: creates a non-empty raft log snapshot on server startup #18494

etcdserver: creates a non-empty raft log snapshot on server startup #18494

clement2026 commented Aug 25, 2024 •

edited

Loading

k8s-ci-robot commented Aug 25, 2024

codecov-commenter commented Aug 25, 2024 •

edited

Loading

k8s-ci-robot commented Aug 25, 2024

clement2026 Aug 31, 2024

clement2026 Aug 31, 2024

clement2026 Aug 31, 2024

serathius Sep 9, 2024

clement2026 Aug 31, 2024

serathius Sep 9, 2024

k8s-ci-robot commented Aug 31, 2024

ivanvc commented Sep 4, 2024

clement2026 commented Sep 6, 2024

serathius Sep 9, 2024 •

edited

Loading

clement2026 Sep 9, 2024

serathius Sep 9, 2024

ahrtr commented Sep 13, 2024

clement2026 commented Sep 13, 2024

k8s-ci-robot commented Sep 20, 2024

clement2026 commented Sep 23, 2024

	func (s *EtcdServer) snapshot(snapi uint64, confState raftpb.ConfState) {
	d := GetMembershipInfoInV2Format(s.Logger(), s.cluster)
	// commit kv to write metadata (for example: consistent index) to disk.
	//
	// This guarantees that Backend's consistent_index is >= index of last snapshot.
	//
	// KV().commit() updates the consistent index in backend.
	// All operations that update consistent index must be called sequentially
	// from applyAll function.
	// So KV().Commit() cannot run in parallel with toApply. It has to be called outside
	// the go routine created below.
	s.KV().Commit()

	lg := s.Logger()

	// For backward compatibility, generate v2 snapshot from v3 state.
	snap, err := s.r.raftStorage.CreateSnapshot(snapi, &confState, d)
	if err != nil {

etcdserver: creates a non-empty raft log snapshot on server startup #18494

etcdserver: creates a non-empty raft log snapshot on server startup #18494

Conversation

clement2026 commented Aug 25, 2024 • edited Loading

Key changes

k8s-ci-robot commented Aug 25, 2024

codecov-commenter commented Aug 25, 2024 • edited Loading

Codecov Report

k8s-ci-robot commented Aug 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 31, 2024

ivanvc commented Sep 4, 2024

clement2026 commented Sep 6, 2024

serathius Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Sep 13, 2024

clement2026 commented Sep 13, 2024

k8s-ci-robot commented Sep 20, 2024

clement2026 commented Sep 23, 2024

clement2026 commented Aug 25, 2024 •

edited

Loading

codecov-commenter commented Aug 25, 2024 •

edited

Loading

serathius Sep 9, 2024 •

edited

Loading