Add downgrade cancellation e2e tests #19244

henrybear327 · 2025-01-21T15:11:47Z

Part 1 of the e2e test - downgrade cancellation is called before any of the node is actually downgraded

A README fix was also associated with this PR, as downgrade command doesn't seem to take version as a parameter.

Reference: #17976

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

codecov · 2025-01-21T15:29:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.87%. Comparing base (532c601) to head (82f30d1).

Additional details and impacted files

see 20 files with indirect coverage changes

@@           Coverage Diff           @@
##             main   #19244   +/-   ##
=======================================
  Coverage   68.87%   68.87%           
=======================================
  Files         420      420           
  Lines       35680    35680           
=======================================
  Hits        24573    24573           
- Misses       9685     9686    +1     
+ 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 532c601...82f30d1. Read the comment docs.

etcdctl/README.md

tests/e2e/cluster_downgrade_test.go

tests/framework/e2e/downgrade.go

tests/e2e/cluster_downgrade_test.go

henrybear327 · 2025-01-21T21:30:03Z

tests/e2e/cluster_downgrade_test.go

 	t.Logf("Starting downgrade process to %q", lastVersionStr)
-	e2e.DowngradeUpgradeMembers(t, nil, epc, len(epc.Procs), currentVersion, lastClusterVersion)
+	err = e2e.DowngradeUpgradeMembers(t, nil, epc, len(epc.Procs), currentVersion, lastClusterVersion)


@siyuanfoundation is there a reason that the return value of DowngradeUpgradeMembers is ignored? :)

Ah, I missed when merging the function with robustness test. Thanks for fixing it!

henrybear327 · 2025-01-21T22:14:31Z

/retest

ahrtr

LGTM

cc @siyuanfoundation @serathius

k8s-ci-robot · 2025-01-22T10:19:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, henrybear327

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr]

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

serathius · 2025-01-22T10:40:50Z

tests/e2e/cluster_downgrade_test.go

+	if triggerCancellation == cancelRightBeforeEnable {
+		t.Logf("Cancelling downgrade before enabling")
+		e2e.DowngradeCancel(t, epc, generateIdenticalVersions(clusterSize, currentVersionStr))
+		return // No need to perform downgrading, end the test here


I think the test should have some validation, like check if cluster version of all members. For cancellation before and right after enable we the cluster version should stay the same. Also would be good to confirm the state of downgrade.

Maybe I am missing something.

The generateIdenticalVersions function generates the versions for the nodes that should be checked against already, no?

Oh, I didn't noticed that e2e.DowngradeCancel is also doing validation. Any reason to combine it? I would recommend to either split it or make it clear from function name that you are validating.

Like

epc.Etcdutl().DowngradeCancel(context.TODO()) e2e.ValidateMemberVersions(epc, currentVersionStr)

Like

epc.Etcdutl().DowngradeCancel(context.TODO()) e2e.ValidateMemberVersions(epc, currentVersionStr)

+1. It's OK to do it in a separate PR.

+1 to add a util function of ValidateMemberVersions to verify versions based on the binary path of each process.

tests/e2e/cluster_downgrade_test.go

tests/framework/e2e/downgrade.go

ahrtr · 2025-01-23T16:23:22Z

cc @siyuanfoundation @serathius

siyuanfoundation · 2025-01-23T17:57:52Z

tests/framework/e2e/downgrade.go

+		ValidateVersion(t, epc.Cfg, epc.Procs[i], version.Versions{
+			Cluster: versions[i].Cluster,
+			Server:  versions[i].Server,
+			Storage: versions[i].Storage,
+		})


ValidateVersion(t, epc.Cfg, epc.Procs[i], versions[i]) ?

siyuanfoundation · 2025-01-23T17:58:48Z

tests/e2e/cluster_downgrade_test.go

+	if triggerCancellation == cancelRightBeforeEnable {
+		t.Logf("Cancelling downgrade before enabling")
+		e2e.DowngradeCancel(t, epc, generateIdenticalVersions(clusterSize, currentVersionStr))
+		return // No need to perform downgrading, end the test here


+1 to add a util function of ValidateMemberVersions to verify versions based on the binary path of each process.

siyuanfoundation · 2025-01-23T17:59:30Z

tests/e2e/cluster_downgrade_test.go

 	t.Logf("Starting downgrade process to %q", lastVersionStr)
-	e2e.DowngradeUpgradeMembers(t, nil, epc, len(epc.Procs), currentVersion, lastClusterVersion)
+	err = e2e.DowngradeUpgradeMembers(t, nil, epc, len(epc.Procs), currentVersion, lastClusterVersion)


Ah, I missed when merging the function with robustness test. Thanks for fixing it!

ahrtr · 2025-01-26T13:58:33Z

pls fix the DCO failure.

Signed-off-by: Chun-Hung Tseng <[email protected]> Co-authored-by: Benjamin Wang <[email protected]>

Signed-off-by: Chun-Hung Tseng <[email protected]>

k8s-ci-robot · 2025-01-26T15:49:49Z

@henrybear327: The following tests 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-e2e-arm64	`82f30d1`	link	true	`/test pull-etcd-e2e-arm64`
pull-etcd-e2e-386	`82f30d1`	link	true	`/test pull-etcd-e2e-386`
pull-etcd-e2e-amd64	`82f30d1`	link	true	`/test pull-etcd-e2e-amd64`

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.

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.

ahrtr · 2025-01-27T18:28:08Z

Please rebase & squash the commits and resolve the DCO failure

k8s-ci-robot added area/etcdctl area/testing size/M labels Jan 21, 2025

henrybear327 mentioned this pull request Jan 21, 2025

Add downgrade cancellation e2e tests #17976

Open

henrybear327 requested a review from ahrtr January 21, 2025 15:14

henrybear327 force-pushed the e2e/downgrade_cancellation_e2e branch from 547c87e to e1cbb11 Compare January 21, 2025 15:28

henrybear327 self-assigned this Jan 21, 2025

siyuanfoundation reviewed Jan 21, 2025

View reviewed changes

etcdctl/README.md Show resolved Hide resolved

tests/e2e/cluster_downgrade_test.go Outdated Show resolved Hide resolved

ahrtr reviewed Jan 21, 2025

View reviewed changes

tests/framework/e2e/downgrade.go Outdated Show resolved Hide resolved

henrybear327 force-pushed the e2e/downgrade_cancellation_e2e branch from e1cbb11 to f1ff53e Compare January 21, 2025 21:06

henrybear327 requested review from ahrtr and siyuanfoundation January 21, 2025 21:08

ahrtr reviewed Jan 21, 2025

View reviewed changes

tests/e2e/cluster_downgrade_test.go Outdated Show resolved Hide resolved

henrybear327 force-pushed the e2e/downgrade_cancellation_e2e branch 2 times, most recently from d5d064b to 4b25f0f Compare January 21, 2025 21:13

henrybear327 commented Jan 21, 2025

View reviewed changes

henrybear327 force-pushed the e2e/downgrade_cancellation_e2e branch from bb4669c to b795e73 Compare January 21, 2025 21:43

henrybear327 requested a review from ahrtr January 21, 2025 21:43

henrybear327 mentioned this pull request Jan 21, 2025

Add downgrade cancellation e2e tests #19252

Draft

ahrtr approved these changes Jan 22, 2025

View reviewed changes

k8s-ci-robot added the approved label Jan 22, 2025

serathius reviewed Jan 22, 2025

View reviewed changes

tests/e2e/cluster_downgrade_test.go Show resolved Hide resolved

serathius reviewed Jan 22, 2025

View reviewed changes

tests/framework/e2e/downgrade.go Show resolved Hide resolved

siyuanfoundation reviewed Jan 23, 2025

View reviewed changes

henrybear327 force-pushed the e2e/downgrade_cancellation_e2e branch from b795e73 to 2df6942 Compare January 24, 2025 19:04

henrybear327 and others added 2 commits January 26, 2025 16:16

Add downgrade cancellation e2e tests

6414dcc

Signed-off-by: Chun-Hung Tseng <[email protected]> Co-authored-by: Benjamin Wang <[email protected]>

Add missing error checking after calling DowngradeUpgradeMembers

82f30d1

Signed-off-by: Chun-Hung Tseng <[email protected]>

henrybear327 force-pushed the e2e/downgrade_cancellation_e2e branch from 2df6942 to 82f30d1 Compare January 26, 2025 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add downgrade cancellation e2e tests #19244

Add downgrade cancellation e2e tests #19244

henrybear327 commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

henrybear327 Jan 21, 2025 •

edited

Loading

siyuanfoundation Jan 23, 2025

henrybear327 commented Jan 21, 2025

ahrtr left a comment

k8s-ci-robot commented Jan 22, 2025

serathius Jan 22, 2025 •

edited

Loading

henrybear327 Jan 22, 2025 •

edited

Loading

serathius Jan 22, 2025

ahrtr Jan 23, 2025

siyuanfoundation Jan 23, 2025

ahrtr commented Jan 23, 2025

siyuanfoundation Jan 23, 2025

siyuanfoundation Jan 23, 2025

siyuanfoundation Jan 23, 2025

ahrtr commented Jan 26, 2025

k8s-ci-robot commented Jan 26, 2025

ahrtr commented Jan 27, 2025

Add downgrade cancellation e2e tests #19244

Are you sure you want to change the base?

Add downgrade cancellation e2e tests #19244

Conversation

henrybear327 commented Jan 21, 2025 • edited Loading

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

henrybear327 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

siyuanfoundation Jan 23, 2025

Choose a reason for hiding this comment

henrybear327 commented Jan 21, 2025

ahrtr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 22, 2025

serathius Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

henrybear327 Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

serathius Jan 22, 2025

Choose a reason for hiding this comment

ahrtr Jan 23, 2025

Choose a reason for hiding this comment

siyuanfoundation Jan 23, 2025

Choose a reason for hiding this comment

ahrtr commented Jan 23, 2025

siyuanfoundation Jan 23, 2025

Choose a reason for hiding this comment

siyuanfoundation Jan 23, 2025

Choose a reason for hiding this comment

siyuanfoundation Jan 23, 2025

Choose a reason for hiding this comment

ahrtr commented Jan 26, 2025

k8s-ci-robot commented Jan 26, 2025

ahrtr commented Jan 27, 2025

henrybear327 commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

henrybear327 Jan 21, 2025 •

edited

Loading

serathius Jan 22, 2025 •

edited

Loading

henrybear327 Jan 22, 2025 •

edited

Loading