Towards a VReplication/OnlineDDL testing suite #8181

shlomi-noach · 2021-05-25T07:52:06Z

Setting up a VReplication testing suite.

Initial commit: support for ddl_strategy '-vreplication-test-suite', which renames both tables on cut-over to names inaccessible to the app

e.g. if table name is mytable, then we end up with mytable_before and mytable_after. The app (and the scripts executed by the test suite) knows nothing about those tables and therefore cannot make changes to those tabes. This means both tables are frozen in time at the moment of cut-over, and this means they can be compared without interference. And that's how the suite validates that the migration is correct: by comparing table data.

Related Issue(s)

Tracking: import gh-ost test suite to Vitess/VReplication #8179

Checklist

Tests were added or are not required
Documentation was added or is not required

Deployment Notes

…th tables on cut-over to names inaccessible to the app, paving the way for a reliable test suite Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…ast 5 seconds Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…e.g. via an internal EVENT), we need to cut-over using two-step, to hard-hide the original table Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2021-05-26T08:02:51Z

Ready for review

Suite is now working: https://github.com/vitessio/vitess/pull/8181/checks?check_run_id=2672882181

Main file is https://github.com/vitessio/vitess/pull/8181/files#diff-5b2f7e26e6a031b4098461528dca98e5ff630e70e1986f35efaab59f3fa5bd76, the endtoend test that runs the suite.
It is a translatoin of the test shell script https://github.com/vitessio/vitess/pull/8181/files#diff-fc1d6f6f07384c7c6b53fba52d16e64f5d3943d0ca1fcc8b4b4e4dd7df7982fb imported from gh-ost (which is kept for a while; I'll delete it later).

All tests files are under testdata: https://github.com/planetscale/vitess/tree/online-ddl-vreplication-test-suite/go/test/endtoend/onlineddl/vrepl_suite/testdata. Right now I only added two tests: one trivial that has to pass, and one that is expected to fail.

The rest of the tests are temporarily in untestdata: https://github.com/planetscale/vitess/tree/online-ddl-vreplication-test-suite/go/test/endtoend/onlineddl/vrepl_suite/untestdata. There's dozens of them, and I expect many to fail, if tested.

The objective is:

Approve and merge this PR with proof of concept of two working tests
Later, iterate the tests in untestdata and slowly move them to testdata. Anything that passes, great! Anything that fails: fix VReplication/OnlineDDL to make it pass...

vmg · 2021-05-26T09:04:47Z

go/test/endtoend/onlineddl/vtgate_util.go

@@ -106,6 +106,12 @@ func CheckCancelAllMigrations(t *testing.T, vtParams *mysql.ConnParams, expectCo
 	assert.Equal(t, expectCount, int(r.RowsAffected))
 }

+// ReadMigrations reads migration entries
+func ReadMigrations(t *testing.T, vtParams *mysql.ConnParams, like string) *sqltypes.Result {
+	showQuery := fmt.Sprintf("show vitess_migrations like '%s'", like)


Can I talk you into using sqlparser.TrackedBuffer for this? I don't think anybody is gonna do SQL injection in our integration test suite buuuut it's a good consistency habit to pick up :p

I'm looking at Vitess code and I don't see sqlparser.TrackedBuffer widely used for encapsulating values; I see construction using sqlparser.NewStrLiteral, like in:

vitess/go/vt/vtgate/planbuilder/show.go

Lines 217 to 226 in bb15f60

sql := "SELECT * FROM _vt.schema_migrations"

if show.Filter != nil {

if show.Filter.Filter != nil {

sql += fmt.Sprintf(" where %s", sqlparser.String(show.Filter.Filter))

} else if show.Filter.Like != "" {

lit := sqlparser.String(sqlparser.NewStrLiteral(show.Filter.Like))

sql += fmt.Sprintf(" where migration_uuid LIKE %s OR migration_context LIKE %s OR migration_status LIKE %s", lit, lit, lit)

}

}

Any thoughts on how to use TrackedBuffer to avoid SQL injection?

Rewritten to use sqlparser.ParseAndBind()

That's the one! Sorry I didn't make it explicit.

Good good, thank you!

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2021-05-31T08:20:05Z

Request for review 🙏 I have more to come on this topic but don't want to overwhelm with a giant PR

rohit-nayak-ps · 2021-05-31T10:52:25Z

go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go

@@ -0,0 +1,376 @@
+/*
+Copyright 2019 The Vitess Authors.


rohit-nayak-ps

some nits, otherwise lgtm

rohit-nayak-ps · 2021-05-31T10:55:06Z

go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go

+
+	exitcode, err := func() (int, error) {
+		clusterInstance = cluster.NewCluster(cell, hostname)
+		schemaChangeDirectory = path.Join("/tmp", fmt.Sprintf("schema_change_dir_%d", clusterInstance.GetAndReserveTabletUID()))


ClusterInstance has a TmpDirectory parameter (based off of VTDATAROOT). Maybe using that here might be more consistent with the other tests. I have found that helps while debugging: if you comment out the teardown temporarily all transient files are then in the same root dir.

Actually, I inherited this schemaChangeDirectory variable, and am unsure how Vitess even uses it. Having said that, there's a problem with using suggested TmpDirectory:

TmpDirectory is only set in StartTopo()

StartTopo() requires that VtctldExtraArgs are set

VtctldExtraArgs rely on schemaChangeDirectory

so if we introduce schemaChangeDirectory = cluster.TmpDirectory there's a dependency loop.

I have no strong opinions (and really no opinions at all) on how this should be resolved. Even after reviewing the code I don't understand how SchemaChangeDirName is being used: it's used by LocalController -- I don't really understand what LocalController is?

I'm gonna merge this PR with the nit still outstanding, and resolve TmpDirectory in a future PR 🙏

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added 2 commits May 25, 2021 10:47

support for ddl_strategy '-vreplication-test-suite', which renames bo…

705a8d0

…th tables on cut-over to names inaccessible to the app, paving the way for a reliable test suite Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

new endtoend test/shard: onlineddl_vrepl_suite

a7f7f84

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Component: Build/CI Component: Query Serving Component: VReplication Type: CI/Build labels May 25, 2021

shlomi-noach added 6 commits May 25, 2021 12:12

ensure vreplication -vreplication-test-suite migration runs for at le…

0dc8008

…ast 5 seconds Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

adding vrepl_suite. For now it's just a copy of vrepl_stress

bfd7923

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename vrepl_suite test file

582e092

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename vrepl_suite test file

f1ff000

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

because the test suite can run queries directly on the mysql server (…

cc85895

…e.g. via an internal EVENT), we need to cut-over using two-step, to hard-hide the original table Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

working suite; most tests exluded right now

c03fdea

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach marked this pull request as ready for review May 26, 2021 08:03

shlomi-noach requested review from rohit-nayak-ps, deepthi and a team May 26, 2021 08:06

shlomi-noach mentioned this pull request May 26, 2021

Tracking: import gh-ost test suite to Vitess/VReplication #8179

Closed

vmg reviewed May 26, 2021

View reviewed changes

shlomi-noach added 4 commits May 26, 2021 12:44

ParseAndBind

fe7cc1a

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

more safety, using ParseAndBind

fa1f65b

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

support ignore_versions flag file

61ae510

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

fixed ordering of expected/got params

23f5438

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rohit-nayak-ps reviewed May 31, 2021

View reviewed changes

rohit-nayak-ps approved these changes May 31, 2021

View reviewed changes

copyright year

d32b6df

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach merged commit f3bcf98 into vitessio:master May 31, 2021

shlomi-noach deleted the online-ddl-vreplication-test-suite branch May 31, 2021 11:45

shlomi-noach mentioned this pull request May 31, 2021

Online DDL Vreplication test suite: adding tests #8213

Merged

2 tasks

shlomi-noach mentioned this pull request Jun 13, 2021

Online DDL/VReplication: support non-UTF8 character sets #8322

Merged

2 tasks

shlomi-noach mentioned this pull request Jul 13, 2021

Cleanup: import of gh-ost test suite is complete #8459

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Towards a VReplication/OnlineDDL testing suite #8181

Towards a VReplication/OnlineDDL testing suite #8181

shlomi-noach commented May 25, 2021 •

edited

Loading

shlomi-noach commented May 26, 2021 •

edited

Loading

vmg May 26, 2021

shlomi-noach May 26, 2021

shlomi-noach May 26, 2021

shlomi-noach May 26, 2021

vmg May 26, 2021

shlomi-noach May 26, 2021

shlomi-noach commented May 31, 2021

rohit-nayak-ps May 31, 2021

shlomi-noach May 31, 2021

rohit-nayak-ps left a comment

rohit-nayak-ps May 31, 2021

shlomi-noach May 31, 2021

shlomi-noach May 31, 2021

	sql := "SELECT * FROM _vt.schema_migrations"

	if show.Filter != nil {
	if show.Filter.Filter != nil {
	sql += fmt.Sprintf(" where %s", sqlparser.String(show.Filter.Filter))
	} else if show.Filter.Like != "" {
	lit := sqlparser.String(sqlparser.NewStrLiteral(show.Filter.Like))
	sql += fmt.Sprintf(" where migration_uuid LIKE %s OR migration_context LIKE %s OR migration_status LIKE %s", lit, lit, lit)
	}
	}

Towards a VReplication/OnlineDDL testing suite #8181

Towards a VReplication/OnlineDDL testing suite #8181

Conversation

shlomi-noach commented May 25, 2021 • edited Loading

Related Issue(s)

Checklist

Deployment Notes

shlomi-noach commented May 26, 2021 • edited Loading

Ready for review

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

Choose a reason for hiding this comment

shlomi-noach commented May 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohit-nayak-ps left a 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

shlomi-noach commented May 25, 2021 •

edited

Loading

shlomi-noach commented May 26, 2021 •

edited

Loading