Migrate Workflow: Scope vindex names correctly when target and source keyspace have different names #16769

rohit-nayak-ps · 2024-09-12T14:49:42Z

Description

We use the target keyspace’s vindex for filtering out rows from the source intended for each target shard. The vindex is qualified by the target keyspace name. When the source cluster has a different name for the keyspace, it fails while trying to stream from the source cluster, because it cannot locate the vindex.

This PR sets the vindex scope to that of the source keyspace if the keyspace names differ. Note that it is assumed that the same vindexes are used on both keyspaces during a Migrate

Related Issue(s)

#16777

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

…names are different, we need to scope the vindex used in the vreplication filter to the local name Signed-off-by: Rohit Nayak <rohit@planetscale.com>

vitess-bot · 2024-09-12T14:49:46Z

codecov · 2024-09-12T15:09:55Z

Codecov Report

Attention: Patch coverage is 54.54545% with 10 lines in your changes missing coverage. Please review.

Project coverage is 69.53%. Comparing base (1db282a) to head (e6a3989).
Report is 86 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtctl/workflow/materializer.go	50.00%	4 Missing ⚠️
go/vt/wrangler/materializer.go	50.00%	4 Missing ⚠️
go/vt/vtctl/workflow/server.go	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16769      +/-   ##
==========================================
+ Coverage   68.98%   69.53%   +0.54%     
==========================================
  Files        1562     1568       +6     
  Lines      200697   202442    +1745     
==========================================
+ Hits       138459   140764    +2305     
+ Misses      62238    61678     -560

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

mattlord

In general it LGTM. I had some minor nits and suggestions, the only thing that I think we really should do is address the it is expected that the source and target keyspaces have the same vindex name and data type part and confirm that ourselves. Unless there's some reason(s) that we cannot?

mattlord · 2024-09-18T00:49:52Z

go/test/endtoend/vreplication/migrate_test.go

+// TestShardedMigrate adds a test for a sharded cluster to validate a fix for a bug where the target keyspace name
+// doesn't match that of the source cluster. The test migrates from a cluster with keyspace customer to an "external"
+// cluster with keyspace rating.
+func TestMigrateSharded(t *testing.T) {


I don't see where this test is executed in the CI. Am I missing it? I can see that this test fails on main with:

=== NAME TestMigrateSharded migrate_test.go:351: Migrate command failed with exit status 1 : E0917 21:04:21.580700 89860 main.go:56] rpc error: code = Unknown desc = node doesn't exist: /vitess/global/keyspaces/customer/shards/-80/Shard

mattlord · 2024-09-18T00:50:36Z

go/test/endtoend/vreplication/migrate_test.go

@@ -165,7 +168,7 @@ func TestVtctlMigrate(t *testing.T) {
 // However now we need to create an external Vitess cluster. For this we need a different VTDATAROOT and
 // hence the VTDATAROOT env variable gets overwritten.
 // Each time we need to create vt processes in the "other" cluster we need to set the appropriate VTDATAROOT
-func TestVtctldMigrate(t *testing.T) {
+func TestVtctldMigrateUnsharded(t *testing.T) {


IMO, better to update this here as well:

test/config.json: "Args": ["vitess.io/vitess/go/test/endtoend/vreplication", "-run", "TestVtctldMigrate", "-timeout", "30m"],

It should still get run because the value is a regex but I'm not sure that we've intentionally relied on that in the CI.

Changed name of second test to use TestVtctldMigrate prefix. go test is run here, so all tests with that prefix will be run. It keeps the config.json smaller, esp since we are running both tests on same shard.

mattlord · 2024-09-18T00:55:20Z

go/vt/wrangler/materializer.go

IMO we should not change the wrangler anymore on main.

We have users from older Vitess versions who are using this and it will make it easier to backport to those versions.

mattlord · 2024-09-18T00:57:07Z

go/test/endtoend/vreplication/migrate_test.go

+		"--target-keyspace", "rating", "--workflow", "e1",
+		"create", "--source-keyspace", "customer", "--mount-name", "external", "--all-tables", "--cells=zone1",
+		"--tablet-types=primary,replica"); err != nil {
+		t.Fatalf("Migrate command failed with %+v : %s\n", err, output)


IMO we shouldn't use t.Fatal for new tests but instead require.Fail*

mattlord · 2024-09-18T01:00:39Z

go/vt/vtctl/workflow/materializer.go

+				if mz.workflowType == binlogdatapb.VReplicationWorkflowType_Migrate {
+					// For a Migrate, if the TargetKeyspace name is different from the SourceKeyspace name, we need to use the
+					// SourceKeyspace name to determine the vindex since the TargetKeyspace name is not known to the source.
+					// Note: it is expected that the source and target keyspaces have the same vindex name and data type.


I think that we should check and confirm this expectation and error out if it's not true.

It is assumed that the vschemas and tables etc are identical on source and target for Migrate. We can/should add all vaidations for this and all other workflow types in another PR. I don't want to add that to this PR.

mattlord · 2024-09-18T01:11:15Z

go/test/endtoend/vreplication/migrate_test.go

+// cluster with keyspace rating.
+func TestMigrateSharded(t *testing.T) {
+	setSidecarDBName("_vt")
+	defaultRdonly = 1


Why set this? I don't see where we use it.

This is that global variable that we have not refactored yet. It is used in creating future clusters in the test. Added code to keep previous values and reset in a defer.

mattlord · 2024-09-18T01:11:55Z

go/test/endtoend/vreplication/migrate_test.go

+}
+
+func setupExtKeyspace(t *testing.T, vc *VitessCluster, ksName, cellName string) {
+	rdonly := 0


Why not set replicas here as well and leave the global default for both alone? Then below:

if replicas > 0 { require.NoError(t, vtgate.WaitForStatusOfTabletInShard(fmt.Sprintf("%s.%s.replica", ksName, shard), defaultReplicas, waitTimeout)) }

Leaving globals alone when we can and making each test deterministic seems best.

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

deepthi

Nice work!

When Migrate workflow is run on sharded keyspaces where the keyspace …

6a55dcd

…names are different, we need to scope the vindex used in the vreplication filter to the local name Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps added Type: Bug Component: VReplication and removed NeedsWebsiteDocsUpdate What it says labels Sep 12, 2024

github-actions bot added this to the v21.0.0 milestone Sep 12, 2024

Self-review

04684af

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps marked this pull request as ready for review September 13, 2024 12:12

rohit-nayak-ps requested review from GuptaManan100, frouioui, deepthi, mattlord, harshit-gangal and ajm188 as code owners September 13, 2024 12:12

This was referenced Sep 11, 2024

Release of v20.0.2 #16741

Closed

Release of v19.0.6 #16742

Closed

mattlord reviewed Sep 18, 2024

View reviewed changes

Address Review Comments

e6a3989

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

deepthi approved these changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Workflow: Scope vindex names correctly when target and source keyspace have different names #16769

Migrate Workflow: Scope vindex names correctly when target and source keyspace have different names #16769

rohit-nayak-ps commented Sep 12, 2024 •

edited

Loading

vitess-bot bot commented Sep 12, 2024

codecov bot commented Sep 12, 2024 •

edited

Loading

mattlord left a comment •

edited

Loading

mattlord Sep 18, 2024

rohit-nayak-ps Sep 20, 2024

mattlord Sep 18, 2024 •

edited

Loading

rohit-nayak-ps Sep 20, 2024

mattlord Sep 18, 2024

rohit-nayak-ps Sep 20, 2024

mattlord Sep 18, 2024

mattlord Sep 18, 2024

rohit-nayak-ps Sep 20, 2024

mattlord Sep 18, 2024

rohit-nayak-ps Sep 20, 2024

mattlord Sep 18, 2024

deepthi left a comment

Migrate Workflow: Scope vindex names correctly when target and source keyspace have different names #16769

Are you sure you want to change the base?

Migrate Workflow: Scope vindex names correctly when target and source keyspace have different names #16769

Conversation

rohit-nayak-ps commented Sep 12, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Sep 12, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

mattlord left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattlord Sep 18, 2024 • edited Loading

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

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

deepthi left a comment

Choose a reason for hiding this comment

rohit-nayak-ps commented Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

mattlord left a comment •

edited

Loading

mattlord Sep 18, 2024 •

edited

Loading