Make vttestserver compatible with persistent data directories by hkdsun · Pull Request #7718 · vitessio/vitess

hkdsun · 2021-03-19T18:37:09Z

Description

This is an attempt at making the vttestserver container honour a persisted data directory.

There were a few changes required to get here and I'm happy to split into multiple PRs:

Previously, the vttestserver binary did not correctly handle interruption signals. As such, the defer cluster.TearDown() was not run in go/cmd/vttestserver/main.go. I have added a signal handler to correctly run deferred functions.
The mysqlctl binary was previously always run with the init subcommand. We now check if the directory provided in -data_dir argument already exists, and if so, we use the mysqlctl start command instead!
I have added a -keep_data flag to vttestserver binary. This will guard all existing invocations from the new codepath and keeps things backwards compatible.

Related Issue(s)

Fixes #7717

Checklist

Should this PR be backported?
- I'm not sure!
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

deepthi

This is a nice improvement to vttestserver.
We have a unit test and an integration test (vttest_sample) for this package.
vttest_sample runs in docker on CI. Can you add some tests to cover the new functionality?

https://github.com/vitessio/vitess/blob/master/go/test/endtoend/vtcombo/vttest_sample_test.go

Makefile

hkdsun · 2021-03-22T20:24:18Z

I discovered an issue while integrating the new container into our development environment.

The LocalCluster instance actually calls db.loadSchema() in its Setup method.

loadSchema applies sql and vschema migrations respectively for each keyspace in the topology

This becomes problematic across reboots of the test container if the SQL migrations destroy tables – which is the case in the typical output of a Rails application's "schema dump" tool. Something like:

DROP TABLE IF EXISTS `test_table`;
CREATE TABLE `test_table` (....) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 ROW_FORMAT=DYNAMIC;
DROP TABLE IF EXISTS `another_table`;
CREATE TABLE `another_table` (....) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 ROW_FORMAT=DYNAMIC;

What would the Vitess team want this workflow to look like? I see two options:

We short-circuit SQL migrations in the LocalCluster code if running with the -keep_data option (except when Initializeing), or
We avoid using destructive migrations and assume that all .sql files are run every time the container starts

deepthi · 2021-03-22T20:53:22Z

What would the Vitess team want this workflow to look like? I see two options:

We short-circuit SQL migrations in the LocalCluster code if running with the -keep_data option (except when Initializeing), or

We avoid using destructive migrations and assume that all .sql files are run every time the container starts

Which one is easier for you as a user? I suspect it is the first one.

Signed-off-by: Hormoz K <hkheradm@gmail.com>

hkdsun · 2021-03-23T14:31:48Z

Which one is easier for you as a user? I suspect it is the first one.

Correct suspicion indeed 😄

I have addressed all your feedback, tidied up the code, added the migration short-circuit, and wrote some tests. Would you please have a look at the latest commits?

I found the e2e test in go/test/endtoend/vtcombo/vttest_sample_test.go to be redundant compared to the go/cmd/vttestserver/vttestserver_test.go but I may be overlooking context. Please let me know if you were expecting coverage in both.

deepthi · 2021-03-23T18:40:50Z

I'll re-review, but tests are failing

F0323 15:32:34.243946   15971 main.go:252] mkdir /tmp/vttestserver_persistent_mode_510768164/logs: no such file or directory
goroutine 375 [running]:
github.com/golang/glog.stacks(0xc000127500, 0xc0004b8140, 0x7d, 0x96)
	/home/runner/go/pkg/mod/github.com/golang/glog@v0.0.0-20160126235308-23def4e6c14b/glog.go:769 +0xb9
github.com/golang/glog.(*loggingT).output(0x18d1f60, 0xc000000003, 0xc0001291f0, 0x16bdb9e, 0x7, 0xfc, 0x0)
	/home/runner/go/pkg/mod/github.com/golang/glog@v0.0.0-20160126235308-23def4e6c14b/glog.go:720 +0x3b3
github.com/golang/glog.(*loggingT).printDepth(0x18d1f60, 0x3, 0x1, 0xc00012ad90, 0x1, 0x1)
	/home/runner/go/pkg/mod/github.com/golang/glog@v0.0.0-20160126235308-23def4e6c14b/glog.go:646 +0x12d
github.com/golang/glog.(*loggingT).print(...)
	/home/runner/go/pkg/mod/github.com/golang/glog@v0.0.0-20160126235308-23def4e6c14b/glog.go:637
github.com/golang/glog.Fatal(0xc00012ad90, 0x1, 0x1)
	/home/runner/go/pkg/mod/github.com/golang/glog@v0.0.0-20160126235308-23def4e6c14b/glog.go:1128 +0x5c
vitess.io/vitess/go/cmd/vttestserver.runCluster(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/runner/work/vitess/vitess/go/cmd/vttestserver/main.go:252 +0x342
vitess.io/vitess/go/cmd/vttestserver.startCluster(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/runner/work/vitess/vitess/go/cmd/vttestserver/vttestserver_test.go:267 +0x2f8
vitess.io/vitess/go/cmd/vttestserver.TestCanVtGateExecute(0xc000102600)
	/home/runner/work/vitess/vitess/go/cmd/vttestserver/vttestserver_test.go:128 +0xa5

deepthi · 2021-03-23T18:41:36Z

I found the e2e test in go/test/endtoend/vtcombo/vttest_sample_test.go to be redundant compared to the go/cmd/vttestserver/vttestserver_test.go but I may be overlooking context. Please let me know if you were expecting coverage in both.

The endtoend test runs in docker, so it verifies that things work as expected in a docker environment.

go/cmd/vttestserver/vttestserver_test.go

Signed-off-by: Hormoz K <hkheradm@gmail.com>

hkdsun · 2021-03-23T21:52:34Z

@deepthi this should be ready for review I think, I fixed the failing test and added the e2e test.

The failing test is flaky on master as well, it seems

deepthi

LGTM

deepthi · 2021-03-23T21:54:45Z

Makefile


+DOCKER_VTTESTSERVER_SUFFIX = mysql57 mysql80
+DOCKER_VTTESTSERVER_TARGETS = $(addprefix docker_vttestserver_,$(DOCKER_VTTESTSERVER_SUFFIX))
+$(DOCKER_VTTESTSERVER_TARGETS): docker_vttestserver_%:


This is a neat trick

deepthi · 2021-03-23T21:56:42Z

go/cmd/vttestserver/vttestserver_test.go

+func startPersistentCluster(dir string, flags ...string) (vttest.LocalCluster, error) {
+	flags = append(flags, []string{
+		"-persistent_mode",
+		// FIXME: if port is not provided, data_dir is not respected


The FIXME can be removed now?

This is still a bug unfortunately. -data_dir= needs to always be passed along with -port= or it won't be respected 😬

It's not really something that concerns us day-to-day but I may give it an hour or two today to make a PR for it anyway, if it's a simple fix

I propose we move forward without this fix in this PR and tackle this work separately. I can open an issue.

Edit: opened an issue

hkdsun requested a review from deepthi as a code owner March 19, 2021 18:37

hkdsun mentioned this pull request Mar 19, 2021

vttestserver cannot work with a persistent -data_dir= volume #7717

Closed

deepthi reviewed Mar 19, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

GuptaManan100 requested a review from deepthi March 22, 2021 14:30

hkdsun force-pushed the persistent-vttest branch from 31a8237 to 2642859 Compare March 22, 2021 19:27

hkdsun force-pushed the persistent-vttest branch from 2642859 to d2ec4a8 Compare March 23, 2021 14:23

hkdsun added 2 commits March 23, 2021 14:24

persisting vttestserver container

13cd23f

Signed-off-by: Hormoz K <hkheradm@gmail.com>

add mysql57 and mysql80 vttestserver makefile rules

08cdfe1

Signed-off-by: Hormoz K <hkheradm@gmail.com>

hkdsun force-pushed the persistent-vttest branch from d2ec4a8 to 08cdfe1 Compare March 23, 2021 14:24

hkdsun commented Mar 23, 2021

View reviewed changes

go/cmd/vttestserver/vttestserver_test.go Outdated Show resolved Hide resolved

fix test setup and teardown in vttestserver_test.go

fa98b7a

Signed-off-by: Hormoz K <hkheradm@gmail.com>

hkdsun force-pushed the persistent-vttest branch from c51680c to fa98b7a Compare March 23, 2021 20:21

use persistent mode in vttestserver e2e test

0698a01

Signed-off-by: Hormoz K <hkheradm@gmail.com>

deepthi approved these changes Mar 23, 2021

View reviewed changes

deepthi merged commit 92584e9 into vitessio:master Mar 24, 2021

askdba added the Component: Build/CI label Mar 26, 2021

askdba added this to the v10.0 milestone Mar 26, 2021

GuptaManan100 mentioned this pull request Mar 30, 2021

Fixes bug that prevents creation of logs directory #7761

Merged

8 tasks

ajm188 mentioned this pull request Jul 15, 2021

slack vitess v10.pre tinyspeck/vitess#228

Merged

hkdsun mentioned this pull request May 10, 2023

vttestserver: persist vschema changes in --persistent_mode #13065

Merged

4 tasks

deepthi mentioned this pull request Jun 2, 2023

Question: How to persist vitess database on local docker container #13176

Open

Conversation

hkdsun commented Mar 19, 2021

Description

Related Issue(s)

Checklist

Deployment Notes

Impacted Areas in Vitess

Uh oh!

deepthi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hkdsun commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepthi commented Mar 22, 2021

Uh oh!

hkdsun commented Mar 23, 2021

Uh oh!

deepthi commented Mar 23, 2021

Uh oh!

deepthi commented Mar 23, 2021

Uh oh!

Uh oh!

hkdsun commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepthi left a comment

Choose a reason for hiding this comment

Uh oh!

deepthi Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

deepthi Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

hkdsun Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

hkdsun Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

deepthi left a comment •

edited

Loading

hkdsun commented Mar 22, 2021 •

edited

Loading

hkdsun commented Mar 23, 2021 •

edited

Loading

hkdsun Mar 24, 2021 •

edited

Loading