VReplication based traffic migrater by sougou · Pull Request #4981 · vitessio/vitess

sougou · 2019-07-08T05:16:49Z

This is the new VReplication based migrater that will eventually replace all existing MigrateServed (From and Types).

At a high level, the input for the migration is a list of vreplication streams. The migrate command will migrate the traffic for all the requested streams. This means that the higher level MigrateServed functions can be changed to reuse the new implementation instead. The main idea is that the resharding metadata that was previously crucial for traffic migration will become non-critical. We instead rely on the vreplication streams to be the source of truth.

The functionality is exposed as vtctl commands MigrateReads and MigrateWrites. One of the input arguments is a workflow name which can be used to identify a group of vreplication streams across all shards.

The new code has the following properties:

Readable: The steps of the migration are clearly readable with well defined code paths about when something can be canceled, and when it cannot.
Unified: A unified code path is used for both table and shard migration.
Versatile: The new code supports many kinds of migrations. For tables, it can support 1-1, 1-many, many-1, and many-many. For shards, it can support simultaneous splits and merges.
Safe: A well defined set of steps are executed to cancel migration if there are failures before the point of no return. All operations after the point of no return are idempotent.
Journaled: A _vt.resharding_journal table has been created to keep track of the important parameters and binlog positions of a migration. This can be used to reverse vreplication, and also for redirecting other vreplication streams.
Unit tested: There are now local unit tests that exercise all critical code paths, and that simulate all important failure conditions.

rafael · 2019-07-10T01:14:58Z

Oh this is very cool. I'll take time to review this as I'm familiarized with the legacy MigrateServed.

deepthi

Very nice! clean and well decomposed code 🥇

go/vt/vtctl/vtctl.go

deepthi · 2019-07-16T21:23:17Z

go/vt/vttablet/tabletmanager/vreplication/engine.go

-	// If it's a bad table or db, it could be because _vt.vreplication wasn't created.
-	// In that case we can try creating it again.
+	// If it's a bad table or db, it could be because the vreplication tables weren't created.
+	// In that case we can try creating then again.


typo then -> them

deepthi · 2019-07-18T19:08:20Z

go/vt/wrangler/fake_tablet_test.go

+// This file was copied from testlib. All tests from testlib should be moved
+// to the current directory. In order to move tests from there, we have to
+// remove the circular dependency it causes (through vtctl dependence).
+// The tests in this diectory call wrangler functions directory. So, there's


Suggested change

// The tests in this diectory call wrangler functions directory. So, there's

// The tests in this directory call wrangler functions directly. So, there's

deepthi · 2019-07-18T21:15:08Z

go/vt/wrangler/migrater.go

+		return err
+	}
+
+	// For reads, locking the source keysppace is sufficient.


Suggested change

// For reads, locking the source keysppace is sufficient.

// For reads, locking the source keyspace is sufficient.

deepthi · 2019-07-18T21:17:12Z

go/vt/wrangler/migrater.go

+	}
+	defer unlock(&err)
+
+	if mi.migrationType == binlogdatapb.MigrationType_TABLES {


When would you use TABLES vs SHARDS?

deepthi · 2019-07-18T21:34:34Z

go/vt/wrangler/migrater.go

+	return targets, nil
+}
+
+// hashStreams produces a reproduceable hash based on the input parameters.


Suggested change

// hashStreams produces a reproduceable hash based on the input parameters.

// hashStreams produces a reproducible hash based on the input parameters.

deepthi · 2019-07-18T23:08:03Z

go/vt/wrangler/migrater_test.go

+
+	//-------------------------------------------------------------------------------------------------------------------
+	// Single cell RDONLY migration.
+	err := tme.wr.MigrateReads(ctx, tme.targetKeyspace, "test", topodatapb.TabletType_RDONLY, []string{"cell1"}, DirectionForward)


put a checkCellRouting before this to show that there are only 4 entries for cell1 before we MigrateReads, so that it is obvious to anyone reading the test what changes happened because of the call to MigrateReads.

deepthi · 2019-07-18T23:09:43Z

go/vt/wrangler/migrater_test.go

+	// Other cell REPLICA migration.
+	// The global routing already contains redirections for rdonly.
+	// So, adding routes for replica and deploying to cell2 will also cause
+	// cell2 to migrat rdonly. This is a quirk that can be fixed later if necessary.


Suggested change

// cell2 to migrat rdonly. This is a quirk that can be fixed later if necessary.

// cell2 to migrate rdonly. This is a quirk that can be fixed later if necessary.

deepthi · 2019-07-18T23:10:13Z

go/vt/wrangler/migrater_test.go

+	// Other cell REPLICA migration.
+	// The global routing already contains redirections for rdonly.
+	// So, adding routes for replica and deploying to cell2 will also cause
+	// cell2 to migrat rdonly. This is a quirk that can be fixed later if necessary.


Add a TODO for this?

deepthi · 2019-07-18T23:11:46Z

go/vt/wrangler/migrater_test.go

+		"ks1.t2@replica": {"ks2.t2"},
+	})
+	verifyQueries(t, tme.allDBClients)
+


will be nice to add one more backward migration here - of REPLICA

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Since we don't support multiple source or target keyspaces, the uid parameters can be simplified to only have shards as keys, and there will be a separate targetKeyspace parameter that applies to all shards. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Specifying shards and uids wasn't user-friendly. Specifying workflow names should better. However, it will be the user's responsibilty to keep them unique. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

rafael

I didn't review the tests in detail. I spent most of my time in migrater.go

This looks really good and way more readable that we have today. I added some comments.

Next steps would be to refactor the low level functions. I need to remember what was the plan we had for that.

rafael · 2019-08-04T17:39:52Z

go/vt/vtctl/vtctl.go

 }

+func commandMigrateReads(ctx context.Context, wr *wrangler.Wrangler, subFlags *flag.FlagSet, args []string) error {
+	reverse := subFlags.Bool("reverse", false, "Moves the served tablet type backward instead of forward. Use in case of trouble")


Maybe: Replace use in case of trouble. Use in case a rollback of a migration is needed.

I removed that entire statement (including places where I copied from). It doesn't add any value.

go/vt/vtctl/vtctl.go

rafael · 2019-08-04T17:49:09Z

go/vt/vttablet/tabletmanager/vreplication/engine.go

 	}

-	log.Info("Looks like _vt.vreplication table may not exist. Trying to recreate... ")
+	log.Info("Looks like the vreplcation tables may not exist. Trying to recreate... ")


I forgot if this is this log, but there is one that as is similar to this one that in practice becomes noisy and confusing. I think we should remove and only log if there are errors.

I removed the confusing comment a few weeks ago. It was in tablet manager. This should not spam and looks useful, because it will let us know when the tables got created.

rafael · 2019-08-04T18:19:56Z

go/vt/wrangler/migrater.go

+	if servedType != topodatapb.TabletType_REPLICA && servedType != topodatapb.TabletType_RDONLY {
+		return fmt.Errorf("tablet type must be REPLICA or RDONLY: %v", servedType)
+	}
+	mi, err := wr.buildMigrater(ctx, targetKeyspace, workflow)


Wouldn't be better to pass servedType and direction to the migrator? That way this whole method could become way more succinct because:

The validation above could be push down to the mi.validate.

The logic to decide if it's migrateTableReads or migrateShardReads could also be pushed down there. So this whole method could be something like:

mi, err = buildReadMigrater(...) ... if err := mi.validate(ctx); err != nil { return err } ctx, unlock, lockErr := wr.ts.LockKeyspace(ctx, mi.sourceKeyspace, "MigrateReads") if lockErr != nil { return lockErr } return mi.migrate(ctx)

This implies that you have a migrator for reads and a migrator for writes. But I'm thinking that this is actually better as right now the paths are already different for the validations and we are passing a flag to distinguish this.

I tried changing. It made some things better and some things worse. So, let's leave it the way it is for now until we add more functionality.

go/vt/wrangler/migrater.go

rafael · 2019-08-04T18:43:34Z

go/vt/wrangler/migrater.go

+		if err != nil {
+			return nil, err
+		}
+		if len(p3qr.Rows) < 1 {


I think the comment is still not very clear. Maybe adding a concrete example will help visualize.

rafael · 2019-08-04T18:48:01Z

go/vt/wrangler/migrater.go

+	if err := mi.wr.saveRoutingRules(ctx, rules); err != nil {
+		return err
+	}
+	return mi.wr.ts.RebuildSrvVSchema(ctx, cells)


Why do we need this for the tables case? This is not clear to me.

RebuildSrvVSchema also propagates routing rules to the cells. Routing rules are logically part of the vschema, except that they're not associated to a specific keyspace.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

rafael

LGTM

sougou requested review from deepthi and rafael July 8, 2019 05:16

deepthi reviewed Jul 18, 2019

View reviewed changes

sougou force-pushed the ss-migrater branch from 5761272 to 6b8b09d Compare July 20, 2019 17:34

sougou added 16 commits August 3, 2019 19:52

migrater: initial cut

3fb2944

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: migrate reads and validate

f1d0958

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: test framework and initial test

bcf5787

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: quick cleanup

82cfaf1

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: more tests

fa295de

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: more tests and cleanup

0614231

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: refactor tests, and add more

12d92d8

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: unit tests done

e359f3e

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: make table routing rules symmetrical

da43f73

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: add more fields to Journal

19f23cc

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: add logs and comments

61af9f7

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: use workflow as input

40ef5ff

Specifying shards and uids wasn't user-friendly. Specifying workflow names should better. However, it will be the user's responsibilty to keep them unique. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: auto-detect migration type

0257cc0

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: expose separate vtctl commands

3b44876

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

migrater: address review comments

4b86104

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou force-pushed the ss-migrater branch from 6b8b09d to 4b86104 Compare August 4, 2019 02:52

rafael reviewed Aug 4, 2019

View reviewed changes

migrater: adddress more review comments

dfda284

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

rafael approved these changes Aug 4, 2019

View reviewed changes

sougou merged commit 9682ee0 into vitessio:master Aug 4, 2019

sougou deleted the ss-migrater branch August 4, 2019 22:43

arka-g mentioned this pull request Sep 11, 2019

Slack vitess 2019.08.26.r0 tinyspeck/vitess#137

Merged

	// The tests in this diectory call wrangler functions directory. So, there's
	// The tests in this directory call wrangler functions directly. So, there's

	// For reads, locking the source keysppace is sufficient.
	// For reads, locking the source keyspace is sufficient.

	// hashStreams produces a reproduceable hash based on the input parameters.
	// hashStreams produces a reproducible hash based on the input parameters.

	// cell2 to migrat rdonly. This is a quirk that can be fixed later if necessary.
	// cell2 to migrate rdonly. This is a quirk that can be fixed later if necessary.

Conversation

sougou commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafael commented Jul 10, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafael 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafael left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sougou commented Jul 8, 2019 •

edited

Loading

deepthi left a comment •

edited

Loading

rafael left a comment •

edited

Loading