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.

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

e2e test: add schema version verification in mix_version_test. #17881

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

siyuanfoundation
Copy link
Contributor

@siyuanfoundation siyuanfoundation commented Apr 25, 2024

#17361
#17752

Verify storage version is consistent with expected at the end of tests.
Since storageVersion is the only field added in 3.6, there is no need to check other buckets.

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

@siyuanfoundation
Copy link
Contributor Author

/retest

tests/framework/e2e/etcd_process.go Outdated Show resolved Hide resolved
tests/framework/e2e/etcd_process.go Outdated Show resolved Hide resolved
tests/framework/e2e/etcd_process.go Outdated Show resolved Hide resolved
@siyuanfoundation
Copy link
Contributor Author

/retest

@siyuanfoundation
Copy link
Contributor Author

cc @ahrtr

@siyuanfoundation
Copy link
Contributor Author

/retest

tests/framework/e2e/etcd_process.go Outdated Show resolved Hide resolved
tests/framework/e2e/etcd_process.go Outdated Show resolved Hide resolved
@siyuanfoundation siyuanfoundation force-pushed the downgrade-snap branch 4 times, most recently from b7f6a39 to bad484f Compare April 29, 2024 23:37
tests/framework/e2e/etcd_process.go Outdated Show resolved Hide resolved
tests/framework/e2e/etcd_process.go Outdated Show resolved Hide resolved
tests/framework/e2e/etcd_process.go Outdated Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented Apr 30, 2024

The existing schemaChanges isn't flexible. Propose to add the following changes, and we can construct the the schemaChanges based on the new schemaChangeMappings.


func GetNewFieldsInVersion(ver semver.Version) []NewField {
	if newFields, found := schemaChangeMappings[ver]; found {
		return newFields
	}

	return nil
}

type NewField struct {
	bucket       backend.Bucket
	field        []byte
	defaultValue []byte
}

var (
	schemaChangeMappings = map[semver.Version][]NewField{
		version.V3_6: {
			{Meta, MetaStorageVersionName, emptyStorageVersion},
		},
	}

	......
)

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

@ahrtr
Copy link
Member

ahrtr commented Apr 30, 2024

cc @serathius to take a look

Signed-off-by: Siyuan Zhang <sizhang@google.com>
Signed-off-by: Siyuan Zhang <sizhang@google.com>
if newFields, found := newFieldsMapping[v]; found {
return newFields
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Should panic/error if user calls with invalid version.

},
}
// schemaChanges list changes that were introduced in a particular version.
// schema was introduced in v3.6 as so its changes were not tracked before.
schemaChanges = newFieldMappingsToSchemaChanges(newFieldsMapping)
Copy link
Member

Choose a reason for hiding this comment

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

Don't like this change, it limits the possible schema changes to just adding new empty fields.

}
lg.Info("verify no new storage schema field is present in the db file of last release process")
nextEtcdVer := semver.Version{Major: currentEtcdVer.Major, Minor: currentEtcdVer.Minor + 1}
newFields := schema.NewFieldsForVersion(nextEtcdVer)
Copy link
Member

Choose a reason for hiding this comment

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

Validating consistency of schema should be a feature of schema package. Same as bbolt consistency check, schema should be able to confirm that fields presents and their values are consistent with storage version value.

@k8s-ci-robot
Copy link

PR needs rebase.

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.

@k8s-ci-robot
Copy link

@siyuanfoundation: 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-unit-test-amd64 56fab21 link true /test pull-etcd-unit-test-amd64
pull-etcd-unit-test-arm64 56fab21 link true /test pull-etcd-unit-test-arm64

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants