Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add downgrade cancellation e2e tests #19244

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+84 −8
Open

Add downgrade cancellation e2e tests #19244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6414dcc
Add downgrade cancellation e2e tests
henrybear327 Jan 21, 2025
82f30d1
Add missing error checking after calling DowngradeUpgradeMembers
Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion etcdctl/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1119,7 +1119,7 @@ DOWNGRADE ENABLE starts a downgrade action to cluster.
Downgrade enable success, cluster version 3.6
```

### DOWNGRADE CANCEL \<TARGET_VERSION\>
### DOWNGRADE CANCEL
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved

DOWNGRADE CANCEL cancels the ongoing downgrade action to cluster.

Expand Down
65 changes: 58 additions & 7 deletions tests/e2e/cluster_downgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,47 @@ import (
"go.etcd.io/etcd/tests/v3/framework/e2e"
)

type CancellationState int

const (
noCancellation CancellationState = iota
cancelRightBeforeEnable
cancelRightAfterEnable
)

func TestDowngradeUpgradeClusterOf1(t *testing.T) {
testDowngradeUpgrade(t, 1, false)
testDowngradeUpgrade(t, 1, false, noCancellation)
}

func TestDowngradeUpgradeClusterOf3(t *testing.T) {
testDowngradeUpgrade(t, 3, false)
testDowngradeUpgrade(t, 3, false, noCancellation)
}

func TestDowngradeUpgradeClusterOf1WithSnapshot(t *testing.T) {
testDowngradeUpgrade(t, 1, true)
testDowngradeUpgrade(t, 1, true, noCancellation)
}

func TestDowngradeUpgradeClusterOf3WithSnapshot(t *testing.T) {
testDowngradeUpgrade(t, 3, true)
testDowngradeUpgrade(t, 3, true, noCancellation)
}

func TestDowngradeCancellationWithoutEnablingClusterOf1(t *testing.T) {
testDowngradeUpgrade(t, 1, false, cancelRightBeforeEnable)
}

func TestDowngradeCancellationRightAfterEnablingClusterOf1(t *testing.T) {
testDowngradeUpgrade(t, 1, false, cancelRightAfterEnable)
}

func TestDowngradeCancellationWithoutEnablingClusterOf3(t *testing.T) {
testDowngradeUpgrade(t, 3, false, cancelRightBeforeEnable)
}

func TestDowngradeCancellationRightAfterEnablingClusterOf3(t *testing.T) {
testDowngradeUpgrade(t, 3, false, cancelRightAfterEnable)
}

func testDowngradeUpgrade(t *testing.T, clusterSize int, triggerSnapshot bool) {
func testDowngradeUpgrade(t *testing.T, clusterSize int, triggerSnapshot bool, triggerCancellation CancellationState) {
currentEtcdBinary := e2e.BinPath.Etcd
lastReleaseBinary := e2e.BinPath.EtcdLastRelease
if !fileutil.Exist(lastReleaseBinary) {
Expand Down Expand Up @@ -107,9 +131,21 @@ func testDowngradeUpgrade(t *testing.T, clusterSize int, triggerSnapshot bool) {
require.NoError(t, err)
beforeMembers, beforeKV := getMembersAndKeys(t, cc)

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
Copy link
Member

@serathius serathius Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@henrybear327 henrybear327 Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am missing something.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
e2e.DowngradeEnable(t, epc, lastVersion)
if triggerCancellation == cancelRightAfterEnable {
t.Logf("Cancelling downgrade right after enabling (no node is downgraded yet)")
e2e.DowngradeCancel(t, epc, generateIdenticalVersions(clusterSize, currentVersionStr))
return // No need to perform downgrading, end the test here
}

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)
Copy link
Contributor Author

@henrybear327 henrybear327 Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

require.NoError(t, err)
e2e.AssertProcessLogs(t, leader(t, epc), "the cluster has been downgraded")

t.Log("Downgrade complete")
Expand All @@ -135,7 +171,8 @@ func testDowngradeUpgrade(t *testing.T, clusterSize int, triggerSnapshot bool) {
beforeMembers, beforeKV = getMembersAndKeys(t, cc)

t.Logf("Starting upgrade process to %q", currentVersionStr)
e2e.DowngradeUpgradeMembers(t, nil, epc, len(epc.Procs), lastClusterVersion, currentVersion)
err = e2e.DowngradeUpgradeMembers(t, nil, epc, len(epc.Procs), lastClusterVersion, currentVersion)
serathius marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(t, err)
t.Log("Upgrade complete")

afterMembers, afterKV = getMembersAndKeys(t, cc)
Expand Down Expand Up @@ -233,3 +270,17 @@ func getMembersAndKeys(t *testing.T, cc *e2e.EtcdctlV3) (*clientv3.MemberListRes

return members, kvs
}

func generateIdenticalVersions(clusterSize int, currentVersion string) []*version.Versions {
ret := make([]*version.Versions, clusterSize)

for i := range clusterSize {
ret[i] = &version.Versions{
Cluster: currentVersion,
Server: currentVersion,
Storage: currentVersion,
}
}

return ret
}
20 changes: 20 additions & 0 deletions tests/framework/e2e/downgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,26 @@ func DowngradeEnable(t *testing.T, epc *EtcdProcessCluster, ver *semver.Version)
t.Log("Cluster is ready for downgrade")
}

func DowngradeCancel(t *testing.T, epc *EtcdProcessCluster, versions []*version.Versions) {
t.Logf("etcdctl downgrade cancel")
c := epc.Etcdctl()
testutils.ExecuteWithTimeout(t, 20*time.Second, func() {
ahrtr marked this conversation as resolved.
Show resolved Hide resolved
err := c.DowngradeCancel(context.TODO())
require.NoError(t, err)
})

t.Log("Downgrade cancelled, validating if cluster is in the right state")
for i := 0; i < len(epc.Procs); i++ {
ValidateVersion(t, epc.Cfg, epc.Procs[i], version.Versions{
Cluster: versions[i].Cluster,
Server: versions[i].Server,
Storage: versions[i].Storage,
})
Comment on lines +66 to +70
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

t.Log("Cluster downgrade cancellation is completed")
}

func DowngradeUpgradeMembers(t *testing.T, lg *zap.Logger, clus *EtcdProcessCluster, numberOfMembersToChange int, currentVersion, targetVersion *semver.Version) error {
if lg == nil {
lg = clus.lg
Expand Down
5 changes: 5 additions & 0 deletions tests/framework/e2e/etcdctl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@ func (ctl *EtcdctlV3) DowngradeEnable(ctx context.Context, version string) error
return err
}

func (ctl *EtcdctlV3) DowngradeCancel(ctx context.Context) error {
_, err := SpawnWithExpectLines(ctx, ctl.cmdArgs("downgrade", "cancel"), nil, expect.ExpectedResponse{Value: "Downgrade cancel success"})
return err
}

func (ctl *EtcdctlV3) Get(ctx context.Context, key string, o config.GetOptions) (*clientv3.GetResponse, error) {
resp := clientv3.GetResponse{}
var args []string
Expand Down
Loading