WIP: etcdserver: separate "raft log compact" from snapshot

[clement2026]

I'm looking into #17098 @serathius and made changes to separate the compact logic from the snapshot logic.

I ran rw-benchmark.sh several times to observe performance changes. I expected this patch to slow down etcd, but it seems to have improved rw-benchmark.sh results. I’m not sure how. Similar results were observed on both cloud VMs (8vCPU, 16GB Memory, NVMe) and a bare-metal machine (8CPU, 32GB Memory, SSD).

Since the rw-benchmark.sh script was taking forever to finish, I adjusted the parameters to complete the benchmark within a few hours.

Below are the script and the results from running it on a Vultr bare-metal machine (8 CPUs, 32GB Memory, SSD, Ubuntu 24.04 LTS x64).

export RATIO_LIST="4/1"
export REPEAT_COUNT=3
export RUN_COUNT=50000
./rw-benchmark.sh

main.csv
issue-17098-patch.csv

[k8s-ci-robot]

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.

[serathius]

/ok-to-test

[serathius]

/retest

[serathius]

Why change from 1 to 0?

[clement2026]

https://github.com/etcd-io/raft/blob/ee19bbe7f9bb16fd7d874a6c08dfac319596d2ad/storage.go#L258-L260

	if compactIndex > ms.lastIndex() {
		getLogger().Panicf("compact %d is out of bound lastindex(%d)", compactIndex, ms.lastIndex())
	}

A panic occurred at this line during local test runs. It appears the initial value of ms.lastIndex() is 0, so compacti should default to zero.

In the main branch, a panic doesn't occur because ms always contains more than one element when compaction starts, which isn't the case in this patch.

[serathius]

Great work, the change look good, but as any change in the etcd apply loop we need to be careful here.

I expected this patch to slow down etcd, but it seems to have improved rw-benchmark.sh results. I’m not sure how.

Would be good to investigate, would you be able to collect CPU&Memory profiles?

[serathius]

Found by robustness tests:

panic: need non-empty snapshot

goroutine 210 [running]:
go.etcd.io/raft/v3.(*raft).maybeSendAppend(0xc0002be180, 0xeabdbb777cf498cb, 0x1)
	go.etcd.io/raft/v3@v3.6.0-alpha.0/raft.go:640 +0x838
go.etcd.io/raft/v3.(*raft).sendAppend(...)
	go.etcd.io/raft/v3@v3.6.0-alpha.0/raft.go:592
go.etcd.io/raft/v3.stepLeader(0xc0002be180, {0x9, 0xbf19ae4419db00dc, 0xeabdbb777cf498cb, 0x3, 0x0, 0x0, {0x0, 0x0, 0x0}, ...})
	go.etcd.io/raft/v3@v3.6.0-alpha.0/raft.go:1545 +0x99d
go.etcd.io/raft/v3.(*raft).Step(0xc0002be180, {0x9, 0xbf19ae4419db00dc, 0xeabdbb777cf498cb, 0x3, 0x0, 0x0, {0x0, 0x0, 0x0}, ...})
	go.etcd.io/raft/v3@v3.6.0-alpha.0/raft.go:1215 +0xe3c
go.etcd.io/raft/v3.(*node).run(0xc000018240)
	go.etcd.io/raft/v3@v3.6.0-alpha.0/node.go:399 +0x825
created by go.etcd.io/raft/v3.StartNode in goroutine 1
	go.etcd.io/raft/v3@v3.6.0-alpha.0/node.go:273 +0x59

[clement2026]

Great work, the change look good, but as any change in the etcd apply loop we need to be careful here.

I expected this patch to slow down etcd, but it seems to have improved rw-benchmark.sh results. I’m not sure how.

Would be good to investigate, would you be able to collect CPU&Memory profiles?

Thanks! I understand the importance of the apply loop. I'll investigate it and provide CPU & Memory profiles after resolving the bug and test failures.

[clement2026]

/retest

[serathius]

Thanks for fixing test, please note this PR is small, but might have big impact on etcd behavior, it's review might take a while. Would be great to repeat the tests and see the profile difference to understand performance difference.

cc @ahrtr

[clement2026]

Thanks for fixing test, please note this PR is small, but might have big impact on etcd behavior, it's review might take a while. Would be great to repeat the tests and see the profile difference to understand performance difference.

cc @ahrtr

Thanks for checking in. I temporarily fixed the test to test my ideas, but it might cause a serious memory leak. Currently, I'm developing an optimized version to properly fixed the test. I’ll update you once it's done and provide the CPU and memory profiles.

[clement2026]

/retest

[clement2026]

/retest

[clement2026]

/retest

[clement2026]

/retest

[clement2026]

/retest