VReplication: Migrate intra-keyspace materialize workflows when Resharding the keyspace

[mattlord]

Description

Intra-keyspace materializations were not properly migrated during Reshards. This PR corrects that so that they are properly migrated during Reshard (from the old shards to the new, support switching traffic in both directions).

This is likely to be more important going forward as more users leverage sharded keyspaces, which at some point need to be resharded, along with materializations and reference tables within the sharded keyspace to support data locality for some their queries which don't align with the primary sharding scheme (vindexes).

This is ONLY being fixed in the vtctldclient implementation, as we want to keep the old vtctlclient implementation "on ice" as a fallback mechanism until we can remove it. There are also no plans to backport this fix since it's a "beginning of time" bug and could have unintended side effects that we catch during the v20 development cycle.

Related Issue(s)

• Fixes: Reshard fails to update source shards of Materialize workflows #15464

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

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

Attention: Patch coverage is 26.31579% with 70 lines in your changes are missing coverage. Please review.

Project coverage is 68.38%. Comparing base (791ca02) to head (d9af228).

Files	Patch %	Lines
go/vt/vtctl/workflow/stream_migrator.go	17.80%	60 Missing ⚠️
go/vt/vtctl/workflow/server.go	0.00%	8 Missing ⚠️
...blet/tabletmanager/vreplication/controller_plan.go	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15536      +/-   ##
==========================================
- Coverage   68.39%   68.38%   -0.01%     
==========================================
  Files        1556     1556              
  Lines      195283   195356      +73     
==========================================
+ Hits       133563   133596      +33     
- Misses      61720    61760      +40     

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

[rohit-nayak-ps]

Great work fixing this issue.

A couple of comments:

1. For the case where the materialize is intra-shard and the vindex of the materialized table matches that of the source table, we can avoid creating <num_shards> source streams. We already do something similar for MoveTables today. This is an optimization so we can do it in a followup PR at a future date.

2. Should we run VDiffs for these workflows in the e2e tests to confirm that the workflow streams are being generated correctly. This will also catch issues like the bug we had noticed in the past of materialized streams not starting on the target after a Reshard.

[mattlord]

1. For the case where the materialize is intra-shard and the vindex of the materialized table matches that of the source table, we can avoid creating <num_shards> source streams. We already do something similar for MoveTables today. This is an optimization so we can do it in a followup PR at a future date.

Yes, we only do that for MoveTables today:

vitess/go/vt/vtctl/workflow/materializer.go

Lines 554 to 562 in 9e40015

	// filterSourceShards filters out source shards that do not overlap with the
	// provided target shard. This is an optimization to avoid copying unnecessary
	// data between the shards. This optimization is only applied for MoveTables
	// when the source and target shard have the same primary vindexes.
	func (mz *materializer) filterSourceShards(targetShard *topo.ShardInfo) []*topo.ShardInfo {
	if mz.primaryVindexesDiffer || mz.ms.MaterializationIntent != vtctldatapb.MaterializationIntent_MOVETABLES {
	// Use all source shards.
	return mz.sourceShards
	}

I agree that we should expand that for other workflow types (including Materialize).

2. Should we run VDiffs for these workflows in the e2e tests to confirm that the workflow streams are being generated correctly. This will also catch issues like the bug we had noticed in the past of materialized streams not starting on the target after a Reshard.

We currently compare the row counts at the end in the source table with both materialized tables. I'll look into doing a VDiff instead / in addition.

Thanks!