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

Jump to bottom

Update downgrade test to use a snapshot #16041

Merged
merged 1 commit into from
Jun 13, 2023
Merged

Update downgrade test to use a snapshot #16041

merged 1 commit into from
Jun 13, 2023

Conversation

geetasg
Copy link

@geetasg geetasg commented Jun 9, 2023

Related to task - "V2 snapshots are generated from V3 data" - #12913

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

serathius
serathius reviewed Jun 9, 2023
View reviewed changes
tests/e2e/v2store_deprecation_test.go Outdated
Comment on lines 199 to 205
lastVersion, err := e2e.GetVersionFromBinary(e2e.BinPath.EtcdLastRelease)
lastVersionStr := lastVersion.String()
lastClusterVersion := semver.New(lastVersionStr)
lastClusterVersion.Patch = 0
lastClusterVersionStr := lastClusterVersion.String()
t.Logf("etcdctl downgrade enable %s", lastVersionStr)
downgradeEnable(t, epc, lastVersion)
Copy link
Member

Choose a reason for hiding this comment

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

Can you refactor this code to separate function?

Copy link
Author

Choose a reason for hiding this comment

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

updating.

serathius
serathius reviewed Jun 9, 2023
View reviewed changes
tests/e2e/v2store_deprecation_test.go Outdated
@@ -182,6 +184,74 @@ func TestV2DeprecationSnapshotRecover(t *testing.T) {
assert.NoError(t, epc.Close())
}

func TestV2DeprecationSnapshotRecoverOldVersion(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a standard downgrade test. Have you seen the test in

etcd/tests/e2e/cluster_downgrade_test.go

Line 22 in a708bed

"testing"
? Why do we need a separate test for V2 backward compatibility?

Copy link
Author

Choose a reason for hiding this comment

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

etcd/tests/e2e/cluster_downgrade_test.go

Line 22 in a708bed

"testing"
test does not seem to create a snapshot and kvs.
In next changes when we change how the .snap is published by 3.6, we need a test that recovers from .snap and verifies membership match.

Copy link
Author

Choose a reason for hiding this comment

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

please let me know if I should add the test to cluster_downgrade_test.go instead of this file.

Copy link
Member

Choose a reason for hiding this comment

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

test does not seem to create a snapshot and kvs.

This is not unique to v2 store. Please just update existing downgrade tests with additional validation.

@geetasg geetasg changed the title Adding test to verify membership after old version recovery Update downgrade test to generate a snapshot Jun 9, 2023
@geetasg geetasg force-pushed the pr branch 2 times, most recently from e0a0a65 to 41c47d7 Compare June 9, 2023 23:45
@geetasg geetasg changed the title Update downgrade test to generate a snapshot Update downgrade test to use a snapshot Jun 9, 2023
@geetasg geetasg marked this pull request as draft June 10, 2023 00:17
@geetasg geetasg marked this pull request as ready for review June 10, 2023 01:47
ahrtr
ahrtr reviewed Jun 12, 2023
View reviewed changes
tests/e2e/cluster_downgrade_test.go Outdated
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

t.Log("Adding and removing keys")
Copy link
Member

Choose a reason for hiding this comment

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

nit: I only see adding keys, and do not se removing keys:)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I will update the comment.

tests/e2e/cluster_downgrade_test.go Outdated
@@ -67,7 +69,7 @@ func testDowngradeUpgrade(t *testing.T, clusterSize int) {
e2e.BeforeTest(t)

t.Logf("Create cluster with version %s", currentVersionStr)
epc := newCluster(t, clusterSize)
epc := newCluster(t, clusterSize, 10, t.TempDir())
Copy link
Member

Choose a reason for hiding this comment

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

The dataDirPath is t.TempDir by default, FYI.

etcd/tests/framework/e2e/cluster.go

Line 504 in 61736c3

dataDirPath = tb.TempDir()

Suggested change
epc := newCluster(t, clusterSize, 10, t.TempDir())
epc := newCluster(t, clusterSize, 10)

Copy link
Member

Choose a reason for hiding this comment

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

And also the hard coded value 10 is used multiple times, can you define a constant?

Copy link
Author

Choose a reason for hiding this comment

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

updating.

serathius
serathius reviewed Jun 12, 2023
View reviewed changes
tests/e2e/cluster_downgrade_test.go Outdated
@@ -76,6 +78,8 @@ func testDowngradeUpgrade(t *testing.T, clusterSize int) {
})
}
t.Logf("Cluster created")
generateSnapshot(t, 10, epc)
Copy link
Member

Choose a reason for hiding this comment

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

Note, it might be useful to have both downgrade tests with and without snapshot.
TestDowngradeUpgradeClusterOf1 and TestDowngradeUpgradeClusterOf1WithSnapshot

Copy link
Author

Choose a reason for hiding this comment

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

sg. updating.

serathius
serathius reviewed Jun 12, 2023
View reviewed changes
tests/e2e/cluster_downgrade_test.go Outdated
@@ -76,6 +78,8 @@ func testDowngradeUpgrade(t *testing.T, clusterSize int) {
})
}
t.Logf("Cluster created")
generateSnapshot(t, 10, epc)
Copy link
Member

Choose a reason for hiding this comment

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

Please use names const variables instead of magical numbers 10

Copy link
Author

Choose a reason for hiding this comment

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

updating.

ahrtr
ahrtr approved these changes Jun 12, 2023
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @geetasg

serathius
serathius reviewed Jun 13, 2023
View reviewed changes
tests/e2e/cluster_downgrade_test.go
for i = 0; i < snapshotCount*3; i++ {
err := cc.Put(ctx, fmt.Sprintf("%d", i), "1", config.PutOptions{})
assert.NoError(t, err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Followup @geetasg. Could this function confirm that there is a snapshot file on disk present?

Copy link
Author

Choose a reason for hiding this comment

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

updating.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

Update downgrade test to use snapshot
06350ab 
Signed-off-by: Geeta Gharpure <geetagh@amazon.com>
@serathius serathius merged commit 455c0c6 into etcd-io:main Jun 13, 2023
@chaochn47
Copy link
Member

Thanks @geetasg for sharing the doc v2deprecation-3.6 !!

For context to anyone else that has interest in this PR, the added test case is to ensure the following attempt won't regress..

With the v2 removal work for 3.6, the snapshot will continue to look the same but 3.6 will NOT keep a running copy of v2store. It will keep membership info only in the backend and at the time of the snapshot it will export a v2store json to put into the .snap file.

For etcd 3.6, we still want to publish membership information in v2 format in the .snap files so that etcd 3.5 can recover membership from those in case of downgrade.

@geetasg geetasg mentioned this pull request Jun 15, 2023
Open
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serathius serathius serathius approved these changes

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
stage/merge-when-tests-green
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@geetasg @chaochn47 @serathius @ahrtr