VReplication: Fix workflow update changed handling #15621

mattlord · 2024-04-03T04:15:47Z

Description

The rows affected value in the UpdateVReplicationWorkflow tabletmanager RPC (see the PR changes) which is then turned into a boolean changed value in the corresponding UpdateWorkflow vtctld RPC was inadvertently broken in: #14447

This PR corrects that and also addresses an oversight in adding support for changing the State in the tabletmanager RPC (we should be passing the simulated NULL for the client command as we don't allow you to modify the state via the workflow update client command). Note that this had no real practical impact as the workflow is stopped and started as part of the update so the intermediate state of Unknown was extremely brief.

Lastly, we close the testing gap here by confirming the changed value from the client command.

I wanted to backport this to v18 as it's a small and safe change and that's where these bugs were introduced.

Related Issue(s)

Fixes: vtctldclient: WorkflowUpdate response does not set Changed #15619

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 · 2024-04-03T04:15:50Z

This was inadvertently broken in vitessio#14447 Signed-off-by: Matt Lord <[email protected]>

codecov · 2024-04-03T04:53:18Z

Codecov Report

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

Project coverage is 68.12%. Comparing base (37cc00a) to head (a57df52).
Report is 11 commits behind head on main.

Files	Patch %	Lines
...ctldclient/command/vreplication/workflow/update.go	0.00%	1 Missing ⚠️
go/vt/vtctl/vtctl.go	0.00%	1 Missing ⚠️
go/vt/vttablet/tabletmanager/rpc_vreplication.go	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15621      +/-   ##
==========================================
+ Coverage   68.03%   68.12%   +0.09%     
==========================================
  Files        1561     1556       -5     
  Lines      195526   194984     -542     
==========================================
- Hits       133023   132840     -183     
+ Misses      62503    62144     -359

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

derekperkins

lgtm

derekperkins · 2024-04-03T06:04:11Z

go/cmd/vtctldclient/command/vreplication/workflow/update.go

@@ -112,6 +112,7 @@ func commandUpdate(cmd *cobra.Command, args []string) error {
 			TabletSelectionPreference: tsp,
 			OnDdl:                     binlogdatapb.OnDDLAction(onddl),
 			Shards:                    baseOptions.Shards,
+			State:                     binlogdatapb.VReplicationWorkflowState(textutil.SimulatedNullInt), // We don't allow changing this in the client command


If we're not using the cli, is there documentation for when the zero value isn't sufficient for a no-op? Does it make sense to at least add field level comments to the proto?

Good idea! I've added comments here: 3dd0e13

That's helpful. Is it backwards compatible if we add optional to those fields?

In the go protobuf implementation everything is optional. And unfortunately it does not support the more recent optional protobuf behavior where code is then injected to detect if a value was explicitly provided in the message. That's what the simulated NULL stuff is all about, because there's literally no way to determine if a value was actually provided or not (type's ZeroValue). I'm not terribly happy with it, but I could not find a less bad option today. I'd love to get rid of the simulated NULL whenever a better option appears.

Sorry, my understanding is now dated! The go implementation DOES now support truly optional field behavior: https://github.com/protocolbuffers/protobuf/blob/v3.25.0/docs/field_presence.md

It does that by making the specified type, e.g. string, a pointer (*string). I'm looking into whether or not that's backward compatible now.

#15627 ❤️

Added a TODO in the comments as well to link everything together.

1. Add comments to the workflow update related proto messages 2. Remove/reserve an unused shards field in the WorkflowUpdateRequest msg 3. Add a wait to the mulit_tenant_test to address an obvserved flake Signed-off-by: Matt Lord <[email protected]>

Signed-off-by: Matt Lord <[email protected]>

mattlord · 2024-04-03T17:29:00Z

go/test/endtoend/vreplication/multi_tenant_test.go

@@ -190,6 +190,7 @@ func TestMultiTenantSimple(t *testing.T) {
 	for _, ks := range []string{sourceKeyspace, sourceAliasKeyspace} {
 		lastIndex = insertRows(lastIndex, ks)
 	}
+	waitForWorkflowState(t, vc, fmt.Sprintf("%s.%s", targetKeyspace, mt.workflowName), binlogdatapb.VReplicationWorkflowState_Running.String())


This is unrelated, but addresses a test flake that was observed on the PR CI runs.

mattlord · 2024-04-03T17:29:28Z

proto/tabletmanagerdata.proto

 message UpdateVReplicationWorkflowRequest {
  string workflow = 1;
  repeated string cells = 2;
  repeated topodata.TabletType tablet_types = 3;
  TabletSelectionPreference tablet_selection_preference = 4;
  binlogdata.OnDDLAction on_ddl = 5;
  binlogdata.VReplicationWorkflowState state = 6;
-  repeated string shards = 7;
+  reserved 7; // unused, was: repeated string shards


This was errantly added in v19 and was missed during review. It caused no harm, but was entirely unnecessary/unused.

…5621) (#15629) Signed-off-by: Matt Lord <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Matt Lord <[email protected]>

…5621) (#15628) Signed-off-by: Matt Lord <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Matt Lord <[email protected]>

mattlord added Type: Bug Component: VReplication Backport to: release-18.0 Backport to: release-19.0 Needs to be back ported to release-19.0 labels Apr 3, 2024

github-actions bot added this to the v20.0.0 milestone Apr 3, 2024

mattlord force-pushed the workflow_update branch from 6f22505 to 2965e9c Compare April 3, 2024 04:24

mattlord marked this pull request as ready for review April 3, 2024 04:25

mattlord requested review from deepthi, GuptaManan100, rohit-nayak-ps, shlomi-noach and notfelineit as code owners April 3, 2024 04:25

mattlord changed the title ~~Fix workflow update changed handling~~ VReplication: Fix workflow update changed handling Apr 3, 2024

Fix workflow update changed handling

8074898

This was inadvertently broken in vitessio#14447 Signed-off-by: Matt Lord <[email protected]>

mattlord force-pushed the workflow_update branch from 2965e9c to 8074898 Compare April 3, 2024 04:32

derekperkins approved these changes Apr 3, 2024

View reviewed changes

derekperkins reviewed Apr 3, 2024

View reviewed changes

rohit-nayak-ps approved these changes Apr 3, 2024

View reviewed changes

Several minor changes...

3dd0e13

1. Add comments to the workflow update related proto messages 2. Remove/reserve an unused shards field in the WorkflowUpdateRequest msg 3. Add a wait to the mulit_tenant_test to address an obvserved flake Signed-off-by: Matt Lord <[email protected]>

mattlord requested review from harshit-gangal and ajm188 as code owners April 3, 2024 14:26

Add TODO to proto message comments

a57df52

Signed-off-by: Matt Lord <[email protected]>

mattlord commented Apr 3, 2024

View reviewed changes

mattlord merged commit 0e2f175 into vitessio:main Apr 3, 2024
106 checks passed

mattlord deleted the workflow_update branch April 3, 2024 17:55

This was referenced Apr 3, 2024

[release-18.0] VReplication: Fix workflow update changed handling (#15621) #15628

Merged

[release-19.0] VReplication: Fix workflow update changed handling (#15621) #15629

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VReplication: Fix workflow update changed handling #15621

VReplication: Fix workflow update changed handling #15621

mattlord commented Apr 3, 2024 •

edited

Loading

vitess-bot bot commented Apr 3, 2024

codecov bot commented Apr 3, 2024 •

edited

Loading

derekperkins left a comment

derekperkins Apr 3, 2024

mattlord Apr 3, 2024

derekperkins Apr 3, 2024

mattlord Apr 3, 2024 •

edited

Loading

mattlord Apr 3, 2024

mattlord Apr 3, 2024

mattlord Apr 3, 2024

mattlord Apr 3, 2024

mattlord Apr 3, 2024 •

edited

Loading

VReplication: Fix workflow update changed handling #15621

VReplication: Fix workflow update changed handling #15621

Conversation

mattlord commented Apr 3, 2024 • edited Loading

Description

Related Issue(s)

Checklist

vitess-bot bot commented Apr 3, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Apr 3, 2024 • edited Loading

Codecov Report

derekperkins left a comment

Choose a reason for hiding this comment

derekperkins Apr 3, 2024

Choose a reason for hiding this comment

mattlord Apr 3, 2024

Choose a reason for hiding this comment

derekperkins Apr 3, 2024

Choose a reason for hiding this comment

mattlord Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

mattlord Apr 3, 2024

Choose a reason for hiding this comment

mattlord Apr 3, 2024

Choose a reason for hiding this comment

mattlord Apr 3, 2024

Choose a reason for hiding this comment

mattlord Apr 3, 2024

Choose a reason for hiding this comment

mattlord Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

mattlord commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading

mattlord Apr 3, 2024 •

edited

Loading

mattlord Apr 3, 2024 •

edited

Loading