migrate experimental-snapshot-catchup-entries flag to snapshot-catchup-entries #19352

ajaysundark · 2025-02-06T06:53:58Z

Migrate experimental-snapshot-catchup-entries flag to snapshot-catchup-entries

Sub task from #19141

Note:

This PR also handles a fix to an unintended bug from this feature PR, where the field-name was set as experimental-snapshot-catch-up-entries when cmd-line flag experimental-snapshot-catchup-entries was used.
YAML field experimental-snapshot-catch-up-entries will set flag experimental-snapshot-catchup-entries correctly at FlagsExplicitlySet. This is required to mark / identify the field if used from 'config-file' as a deprecated flag usage.
experimental-snapshot-catch-up-entries YAML field usage is maintained the same field-name for backward compability.

k8s-ci-robot · 2025-02-06T06:54:07Z

Hi @ajaysundark. 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.

ajaysundark · 2025-02-06T06:54:16Z

/cc @siyuanfoundation @ahrtr

codecov · 2025-02-06T07:11:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.90%. Comparing base (f197f44) to head (5802231).
Report is 2 commits behind head on main.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/embed/config.go	`80.11% <100.00%> (+0.17%)`	⬆️
server/etcdmain/config.go	`76.08% <100.00%> (+0.26%)`	⬆️

... and 22 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19352      +/-   ##
==========================================
- Coverage   68.93%   68.90%   -0.04%     
==========================================
  Files         420      420              
  Lines       35756    35759       +3     
==========================================
- Hits        24649    24638      -11     
- Misses       9685     9700      +15     
+ Partials     1422     1421       -1

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 f197f44...5802231. Read the comment docs.

ahrtr · 2025-02-06T09:32:34Z

/ok-to-test

server/embed/config.go

siyuanfoundation · 2025-02-06T17:24:11Z

Need to change in tests/framework/e2e/cluster.go

func WithSnapshotCatchUpEntries(count uint64) EPClusterOption {
--	return func(c *EtcdProcessClusterConfig) { c.ServerConfig.SnapshotCatchUpEntries = count }
++     return func(c *EtcdProcessClusterConfig) { c.ServerConfig.ExperimentalSnapshotCatchUpEntries = count }
}

to keep using the --experimental-snapshot-catchup-entries in e2e and robustness test.

We do not want to use the non-experimental flag yet in those tests because of
Opened kubernetes/kubernetes#125031

siyuanfoundation · 2025-02-06T23:40:16Z

/retest

ajaysundark · 2025-02-07T06:34:20Z

This is blocked by #19354 as we have test framework dependency on the ExperimentalSnapshotCatchupEntries.

For example:

TestMixVersionsSnapshotByMockingPartition would need ExperimentalSnapshotCatchupEntries to be set here: framework/e2e/cluster.go#L232 whereas TestRecoverSnapshotBackend require this to be SnapshotCatchupEntries.

server/embed/config.go

ahrtr · 2025-02-07T09:55:17Z

/retest

ahrtr · 2025-02-07T10:31:48Z

/test pull-etcd-e2e-amd64

ahrtr · 2025-02-07T11:15:13Z

/test pull-etcd-e2e-amd64

ahrtr · 2025-02-07T12:55:39Z

tests/framework/e2e/cluster.go

@@ -229,7 +229,7 @@ func WithSnapshotCount(count uint64) EPClusterOption {
 }

 func WithSnapshotCatchUpEntries(count uint64) EPClusterOption {
-	return func(c *EtcdProcessClusterConfig) { c.ServerConfig.SnapshotCatchUpEntries = count }
+	return func(c *EtcdProcessClusterConfig) { c.ServerConfig.ExperimentalSnapshotCatchUpEntries = count }


The e2e test failure of TestRecoverSnapshotBackend is caused by this change. You need to update the line below to use the ExperimentalSnapshotCatchUpEntries

etcd/tests/robustness/failpoint/network.go

Line 147 in 3db3468

return config.ServerConfig.SnapshotCount + config.ServerConfig.SnapshotCatchUpEntries

Or it might be even better to set both SnapshotCatchUpEntries and ExperimentalSnapshotCatchUpEntries in this function WithSnapshotCatchUpEntries; then no need to update the line below,

etcd/tests/robustness/failpoint/network.go

Line 147 in 3db3468

return config.ServerConfig.SnapshotCount + config.ServerConfig.SnapshotCatchUpEntries

Thanks for your review, @ahrtr. I considered setting both the flags at test framework, but we explicitly disallow doing so (we validate for both flags being set), as that could allow misconfigurations in real world use-cases.

I am aware of that setting both flags are disallowed, but what we are talking about here is just test code.

Also note that the robustness test cases test both main and release branches, while ExperimentalSnapshotCatchUpEntries doesn't exist in stable release branches, so setting both fields here might be best choice.

Or we can revert this change, and keep using c.ServerConfig.SnapshotCatchUpEntries = count in WithSnapshotCatchUpEntries.

Note this is a special case, the field ExperimentalSnapshotCatchUpEntries is a new field being added in this PR. While SnapshotCatchUpEntries is the existing field.

cc @ajaysundark @siyuanfoundation

Sorry for the delay in response. The issue is that if we set both SnapshotCatchupEntries and ExperimentalSnapshotCatchupEntries at function WithSnapshotCatchupEntries:

the tests fail with both the flags are set.

{"level":"warn","ts":"2025-02-11T18:35:13.533795Z","caller":"etcdmain/etcd.go:66","msg":"failed to verify flags","error":"cannot set --experimental-snapshot-catchup-entries and --snapshot-catchup-entries at the same time, please use --snapshot-catchup-entries only"}

Another possible option I tried is setting one of the two flags based on the version (if version < 3.6), say at InitEtcdProcessCluster here:

etcd/tests/framework/e2e/cluster.go

Line 437 in 3db3468

func InitEtcdProcessCluster(t testing.TB, cfg *EtcdProcessClusterConfig) (*EtcdProcessCluster, error) {

like:

if cfg.Version != CurrentVersion && UsesExperimentalSnapshotCatchupEntriesFlag(BinPath.EtcdLastRelease) { cfg.ServerConfig.ExperimentalSnapshotCatchUpEntries = cfg.ServerConfig.SnapshotCatchUpEntries cfg.ServerConfig.SnapshotCatchUpEntries = etcdserver.DefaultSnapshotCatchUpEntries }

but for some reason it's failing TestMixVersionsSnapshotByMockingPartition: timing out the waiting for Verify logs to check leader has saved snapshot. Any thoughts? cc @ahrtr / @siyuanfoundation

increasing the timeout to more than 30s helped with the test run:
PASSES="e2e" TIMEOUT=5m TESTCASE=TestMixVersionsSnapshotByMockingPartition ./scripts/test.sh

ahrtr · 2025-02-07T17:24:14Z

Note that I will cut branch release-3.6 about 2 hours later. If the comments can be resolved before that time, then it should be able to be merged before cutting branch; otherwise, you will have to resubmit the PR against the new release-3.6.

ahrtr · 2025-02-07T17:30:14Z

otherwise, you will have to resubmit the PR against the new release-3.6.

This might not be correct. We can still merge this to main, then backport to 3.6. Eventually we will cleanup all experimental flags in main.

ahrtr · 2025-02-11T16:32:23Z

@ajaysundark are you still working on this PR? It's almost ready to be merged with just a couple of comments. If you don't have time, let us know. I am targeting to get it done by tomorrow.

…p-entries Signed-off-by: Ajay Sundar Karuppasamy <ajaysundar@google.com>

ajaysundark · 2025-02-11T19:19:25Z

/retest-required

ahrtr · 2025-02-11T19:40:05Z

tests/framework/e2e/etcd_process.go

+	if err != nil {
+		return false
+	}
+	v3_6 := semver.Version{Major: 3, Minor: 6}


let's reuse

etcd/api/version/version.go

Line 44 in f197f44

V3_6 = semver.Version{Major: 3, Minor: 6}

ahrtr · 2025-02-11T20:03:09Z

tests/framework/e2e/cluster.go

+		if cfg.Version != CurrentVersion && UsesExperimentalSnapshotCatchupEntriesFlag(BinPath.EtcdLastRelease) {
+			cfg.ServerConfig.ExperimentalSnapshotCatchUpEntries = cfg.ServerConfig.SnapshotCatchUpEntries
+			cfg.ServerConfig.SnapshotCatchUpEntries = etcdserver.DefaultSnapshotCatchUpEntries
+		}


This might not be correct. Let me do a minor refactorig.

Refer to #19388

Or please feel free to cherry pick my PR into this PR. @ajaysundark

Thanks for your review and follow-up changes. I'll take a look at #19388 to get more context.
Also, looks like the cherry-pick is not necessary as we handled merging this PR and following up with required changes in yours. Please clarify if my understanding is correct.

ahrtr · 2025-02-11T20:12:22Z

@fuweid @ivanvc let's just merge this PR. I will do a quick followup PR to resolve the comments.

ahrtr · 2025-02-11T20:15:00Z

cc @siyuanfoundation

fuweid

let's just merge this PR. I will do a quick followup PR to resolve the comments.

OK. The alternative is to carry this commit in your pull request and close this one.

k8s-ci-robot · 2025-02-11T20:15:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ajaysundark, fuweid, siyuanfoundation
Once this PR has been reviewed and has the lgtm label, please assign serathius for approval. For more information see the 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

ahrtr · 2025-02-11T20:16:18Z

OK. The alternative is to carry this commit in your pull request and close this one.

Not every people is comfortable with this approach :(

ahrtr · 2025-02-11T20:17:22Z

/cherry-pick release-3.6

k8s-infra-cherrypick-robot · 2025-02-11T20:18:00Z

@ahrtr: new pull request created: #19389

In response to this:

/cherry-pick release-3.6

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.

Also resolved the review comment in etcd-io#19352 Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

k8s-ci-robot added needs-ok-to-test size/L labels Feb 6, 2025

k8s-ci-robot requested review from ahrtr and siyuanfoundation February 6, 2025 06:54

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 6, 2025

siyuanfoundation approved these changes Feb 6, 2025

View reviewed changes

server/embed/config.go Show resolved Hide resolved

siyuanfoundation self-requested a review February 6, 2025 17:25

siyuanfoundation mentioned this pull request Feb 6, 2025

Migrate non-boolean experimental flags in 3.6 #19141

Closed

12 tasks

ajaysundark force-pushed the migrate-experimental-snapshot-catch-up-entries branch from da2d4d8 to 4605e40 Compare February 6, 2025 19:06

k8s-ci-robot added area/testing needs-rebase labels Feb 6, 2025

ajaysundark force-pushed the migrate-experimental-snapshot-catch-up-entries branch 2 times, most recently from d39df53 to 09c560c Compare February 6, 2025 19:39

k8s-ci-robot added needs-rebase and removed needs-rebase labels Feb 6, 2025

ajaysundark force-pushed the migrate-experimental-snapshot-catch-up-entries branch from d11c45d to 30f96c0 Compare February 6, 2025 19:57

k8s-ci-robot removed the needs-rebase label Feb 6, 2025

ahrtr reviewed Feb 7, 2025

View reviewed changes

server/embed/config.go Show resolved Hide resolved

server/embed/config.go Outdated Show resolved Hide resolved

ahrtr reviewed Feb 7, 2025

View reviewed changes

ivanvc mentioned this pull request Feb 7, 2025

Plan to release etcd v3.6.0-rc.0 #19334

Open

k8s-ci-robot added the needs-rebase label Feb 7, 2025

ajaysundark force-pushed the migrate-experimental-snapshot-catch-up-entries branch 2 times, most recently from eb85033 to 189127a Compare February 11, 2025 19:05

migrate experimental-snapshot-catchup-entries flag to snapshot-catchu…

5802231

…p-entries Signed-off-by: Ajay Sundar Karuppasamy <ajaysundar@google.com>

ajaysundark force-pushed the migrate-experimental-snapshot-catch-up-entries branch from 189127a to 5802231 Compare February 11, 2025 19:17

k8s-ci-robot removed the needs-rebase label Feb 11, 2025

ahrtr reviewed Feb 11, 2025

View reviewed changes

ahrtr mentioned this pull request Feb 11, 2025

Minor refacotr on the e2e test framework #19388

Merged

fuweid approved these changes Feb 11, 2025

View reviewed changes

ahrtr merged commit 53d44f0 into etcd-io:main Feb 11, 2025
37 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Feb 11, 2025

[release-3.6] migrate experimental-snapshot-catchup-entries flag to snapshot-catchup-entries #19389

Merged

ahrtr added a commit to ahrtr/etcd that referenced this pull request Feb 11, 2025

Minor refacotr on the e2e test framework

e969f58

Also resolved the review comment in etcd-io#19352 Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

ahrtr added a commit to ahrtr/etcd that referenced this pull request Feb 11, 2025

Minor refacotr on the e2e test framework

5703e6b

Also resolved the review comment in etcd-io#19352 Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

k8s-infra-cherrypick-robot pushed a commit to k8s-infra-cherrypick-robot/etcd that referenced this pull request Feb 11, 2025

Minor refacotr on the e2e test framework

06092f8

Also resolved the review comment in etcd-io#19352 Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate experimental-snapshot-catchup-entries flag to snapshot-catchup-entries #19352

migrate experimental-snapshot-catchup-entries flag to snapshot-catchup-entries #19352

ajaysundark commented Feb 6, 2025

k8s-ci-robot commented Feb 6, 2025

ajaysundark commented Feb 6, 2025

codecov bot commented Feb 6, 2025 •

edited

Loading

ahrtr commented Feb 6, 2025

siyuanfoundation commented Feb 6, 2025 •

edited

Loading

siyuanfoundation commented Feb 6, 2025

ajaysundark commented Feb 7, 2025

ahrtr commented Feb 7, 2025

ahrtr commented Feb 7, 2025

ahrtr commented Feb 7, 2025

ahrtr Feb 7, 2025

ahrtr Feb 7, 2025

ajaysundark Feb 8, 2025

ahrtr Feb 8, 2025 •

edited

Loading

ahrtr Feb 11, 2025

ajaysundark Feb 11, 2025 •

edited

Loading

ajaysundark Feb 11, 2025

ahrtr commented Feb 7, 2025 •

edited

Loading

ahrtr commented Feb 7, 2025

ahrtr commented Feb 11, 2025

ajaysundark commented Feb 11, 2025

ahrtr Feb 11, 2025

ahrtr Feb 11, 2025

ahrtr Feb 11, 2025

ahrtr Feb 11, 2025

ajaysundark Feb 11, 2025

ahrtr commented Feb 11, 2025

ahrtr commented Feb 11, 2025

fuweid left a comment

k8s-ci-robot commented Feb 11, 2025

ahrtr commented Feb 11, 2025

ahrtr commented Feb 11, 2025

k8s-infra-cherrypick-robot commented Feb 11, 2025

migrate experimental-snapshot-catchup-entries flag to snapshot-catchup-entries #19352

migrate experimental-snapshot-catchup-entries flag to snapshot-catchup-entries #19352

Conversation

ajaysundark commented Feb 6, 2025

k8s-ci-robot commented Feb 6, 2025

ajaysundark commented Feb 6, 2025

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

ahrtr commented Feb 6, 2025

siyuanfoundation commented Feb 6, 2025 • edited Loading

siyuanfoundation commented Feb 6, 2025

ajaysundark commented Feb 7, 2025

ahrtr commented Feb 7, 2025

ahrtr commented Feb 7, 2025

ahrtr commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaysundark Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Feb 7, 2025 • edited Loading

ahrtr commented Feb 7, 2025

ahrtr commented Feb 11, 2025

ajaysundark commented Feb 11, 2025

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

ahrtr commented Feb 11, 2025

ahrtr commented Feb 11, 2025

fuweid left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 11, 2025

ahrtr commented Feb 11, 2025

ahrtr commented Feb 11, 2025

k8s-infra-cherrypick-robot commented Feb 11, 2025

codecov bot commented Feb 6, 2025 •

edited

Loading

siyuanfoundation commented Feb 6, 2025 •

edited

Loading

ahrtr Feb 8, 2025 •

edited

Loading

ajaysundark Feb 11, 2025 •

edited

Loading

ahrtr commented Feb 7, 2025 •

edited

Loading