VtctldClient Reshard: add e2e tests to confirm CLI options and fix discovered issues. #15353
Conversation
Signed-off-by: Rohit Nayak <[email protected]>
…es were being ignored; nil response was returned. Signed-off-by: Rohit Nayak <[email protected]>
rohit-nayak-ps
added
Type: Bug
Component: VReplication
Type: Testing
Backport to: release-19.0
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
rohit-nayak-ps
removed
NeedsDescriptionUpdate
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15353 +/- ##
==========================================
- Coverage 67.41% 65.40% -2.02%
==========================================
Files 1560 1561 +1
Lines 192752 193532 +780
==========================================
- Hits 129952 126583 -3369
- Misses 62800 66949 +4149 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Rohit Nayak <[email protected]>
rohit-nayak-ps
requested review from
ajm188,
deepthi and
mattlord
as code owners
| @@ -1672,7 +1672,11 @@ func (s *Server) ReshardCreate(ctx context.Context, req *vtctldatapb.ReshardCrea | |||
| log.Errorf("%w", err2) | |||
| return nil, err | |||
| } | |||
| rs, err := s.buildResharder(ctx, keyspace, req.Workflow, req.SourceShards, req.TargetShards, strings.Join(cells, ","), "") | |||
| tabletTypesStr := topoproto.MakeStringTypeCSV(req.TabletTypes) | |||
| if req.TabletSelectionPreference == tabletmanagerdatapb.TabletSelectionPreference_INORDER { | |||
There was a problem hiding this comment.
What's the default if this is not specified?
There was a problem hiding this comment.
All tablet types
| } | ||
|
|
||
| func (v VtctldReshard) ReverseReadsAndWrites() { | ||
| v.exec("ReverseTraffic") | ||
| } | ||
|
|
||
| func (v VtctldReshard) Show() { | ||
| //TODO implement me | ||
| panic("implement me") | ||
| v.exec("Show") |
There was a problem hiding this comment.
Does Show not take any other flags? Same question for ReverseTraffic above.
There was a problem hiding this comment.
Show does not. Reverse does, but shares the same as MoveTables and they both have the same internal code path, which are separately tested in other e2e tests.
There was a problem hiding this comment.
It does take flags, FWIW:
❯ vtctldclient MoveTables show --help
Show the details for a MoveTables VReplication workflow.
Usage:
vtctldclient MoveTables show
Aliases:
show, Show
Examples:
vtctldclient --server localhost:15999 MoveTables --workflow commerce2customer --target-keyspace customer show
Flags:
-h, --help help for show
--include-logs Include recent logs for the workflow. (default true)
--shards strings (Optional) Specifies a comma-separated list of shards to operate on.
We might want to test --shards at some point here.
There was a problem hiding this comment.
Thanks, @rohit-nayak-ps ! ❤️ I only had minor nits/comments/suggestions that you can address as you feel best.
| moveTablesResponse.Workflows[0].MaxVReplicationTransactionLag = 0 | ||
| moveTablesResponse.Workflows[0].MaxVReplicationLag = 0 |
There was a problem hiding this comment.
Curious why we're doing this here. Worth a comment.
| validateReshardResponse(rs) | ||
| workflowResponse := getWorkflow(keyspace, workflowName) | ||
| reshardShowResponse := getReshardShowResponse(&rs) | ||
| require.EqualValues(t, reshardShowResponse, workflowResponse) |
There was a problem hiding this comment.
Curious why we use CloneVT() in some places since we don't do it everywhere. Probably worth a comment somewhere. 🙂
| reshardShowResponse := getReshardShowResponse(&rs) | ||
| require.EqualValues(t, reshardShowResponse, workflowResponse) | ||
| validateReshardWorkflow(t, workflowResponse.Workflows) | ||
| waitForWorkflowState(t, vc, fmt.Sprintf("%s.%s", keyspace, workflowName), binlogdatapb.VReplicationWorkflowState_Stopped.String()) |
There was a problem hiding this comment.
Nit, but we can use ksWorkflow here too as we do below.
| rs.Start() | ||
| waitForWorkflowState(t, vc, ksWorkflow, binlogdatapb.VReplicationWorkflowState_Stopped.String()) | ||
| for _, tab := range targetTabs { | ||
| alias := fmt.Sprintf("zone1-%d", tab.TabletUID) |
There was a problem hiding this comment.
Nit, but would be nicer to make the cell name a variable if not already. And then use the variable.
| for _, tab := range targetTabs { | ||
| alias := fmt.Sprintf("zone1-%d", tab.TabletUID) | ||
| query := "update _vt.vreplication set source := replace(source, 'stop_after_copy:true', 'stop_after_copy:false') where db_name = 'vt_customer' and workflow = '" + workflowName + "'" | ||
| output, err := vc.VtctlClient.ExecuteCommandWithOutput("ExecuteFetchAsDba", alias, query) |
There was a problem hiding this comment.
Nit, but IMO we should use VtctldClient for new stuff (otherwise we'll just have to change it at some point).
There was a problem hiding this comment.
+1 yes please and thank you :)
| require.NotNil(vc.t, resp) | ||
| require.NotNil(vc.t, resp.ShardStreams) | ||
| require.Equal(vc.t, len(resp.ShardStreams), 2) | ||
| keyspace := "customer" |
There was a problem hiding this comment.
We should use the rs.workflowInfo.targetKeyspace variable here.
| } | ||
| } | ||
|
|
||
| func validateReshardWorkflow(t *testing.T, workflows []*vtctldatapb.Workflow) { |
There was a problem hiding this comment.
IMO we should pass rs *iReshard so it's clear what reshard workflow we're validating and we can replace the string literals in here with variables from rs.
| // helper functions | ||
|
|
||
| func getWorkflow(targetKeyspace, workflow string) *vtctldatapb.GetWorkflowsResponse { | ||
| workflowOutput, err := vc.VtctldClient.ExecuteCommandWithOutput("Workflow", "--keyspace", targetKeyspace, "show", "--workflow", workflow) |
There was a problem hiding this comment.
FWIW, it's more compact if we also pass --compact --include-logs=false. Then we can also test those two flags.
| workflowResponse.Workflows[0].MaxVReplicationTransactionLag = 0 | ||
| workflowResponse.Workflows[0].MaxVReplicationLag = 0 |
There was a problem hiding this comment.
Another spot where this catches my eye. :-)
| } | ||
|
|
||
| func (v VtctldReshard) ReverseReadsAndWrites() { | ||
| v.exec("ReverseTraffic") | ||
| } | ||
|
|
||
| func (v VtctldReshard) Show() { | ||
| //TODO implement me | ||
| panic("implement me") | ||
| v.exec("Show") |
There was a problem hiding this comment.
It does take flags, FWIW:
❯ vtctldclient MoveTables show --help
Show the details for a MoveTables VReplication workflow.
Usage:
vtctldclient MoveTables show
Aliases:
show, Show
Examples:
vtctldclient --server localhost:15999 MoveTables --workflow commerce2customer --target-keyspace customer show
Flags:
-h, --help help for show
--include-logs Include recent logs for the workflow. (default true)
--shards strings (Optional) Specifies a comma-separated list of shards to operate on.
We might want to test --shards at some point here.
|
One other note, I think that we should backport this to v18 if possible. I say that as that's when |
|
Adding backport to 18.0 as suggested above, at least for the core logic fix. Will address the nits along with the suggested addition to test coverage in follow-up tests, so as to merge this for the GA release. |
…scovered issues. (#15353) Signed-off-by: Rohit Nayak <[email protected]>
…ions and fix discovered issues. (#15353) (#15364) Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Description
Expands #14957 to check
Reshard. Found a couple of issues which are also fixed in this PR:Reshard Createwas not returning any response, resulting in incorrect outputs from the command.Backport to v19 since this is a bug in the vtctldclient-based commands, which were recommended in that version over the vtctlclient ones.
Related Issue(s)
#12152
#14957
Checklist