Fix: Errant GTID detection on the replicas when they set replication source #16833

GuptaManan100 · 2024-09-24T13:04:07Z

Description

This PR adds errant GTID detection logic in setReplicationSourceLocked call. The intent is that we want to prevent any replica that has an errant GTID from even joining the replication stream. This is because we want to prevent this replica from sending semi-sync ACKs, which can land us into some really annoying and hard to deal with situations.

To do the errant GTID detection, the tablets first query their own position and then ask the primary for its position. Then they compare the two to see if they have an errant GTID or not. If they do, they fail the RPC without setting the new source.

Related Issue(s)

Part of an effort to address the issue Bug Report: vtorc ERS incorrectly flags replica as having errant GTIDs #16724

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

Signed-off-by: Manan Gupta <[email protected]>

vitess-bot · 2024-09-24T13:04:10Z

Signed-off-by: Manan Gupta <[email protected]>

…ationSource RPC Signed-off-by: Manan Gupta <[email protected]>

Signed-off-by: Manan Gupta <[email protected]>

codecov · 2024-09-26T12:12:12Z

Codecov Report

Attention: Patch coverage is 43.28358% with 38 lines in your changes missing coverage. Please review.

Project coverage is 69.42%. Comparing base (766bc64) to head (e1e560c).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vttablet/tabletmanager/rpc_replication.go	0.00%	15 Missing ⚠️
go/vt/vttablet/tabletmanager/tm_init.go	48.27%	15 Missing ⚠️
go/vt/vttablet/tabletmanager/restore.go	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16833      +/-   ##
==========================================
- Coverage   69.43%   69.42%   -0.01%     
==========================================
  Files        1571     1571              
  Lines      203086   203232     +146     
==========================================
+ Hits       141013   141101      +88     
- Misses      62073    62131      +58

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

Signed-off-by: Manan Gupta <[email protected]>

shlomi-noach

LGTM (with comments). Are we convinced that setReplicationSourceLocked is the only path (other than restore.go which I've noticed is handled as well)?

shlomi-noach · 2024-10-01T11:25:04Z

go/mysql/replication/mysql56_gtid_set.go

+
+	// Calculate the difference between the replica and primary GTID sets.
+	diffSet := replicaGTIDSet.Difference(primaryGTIDSet)
+	return diffSet.String(), nil


shlomi-noach · 2024-10-01T11:25:08Z

go/mysql/replication/mysql56_gtid_set.go

+		newSet[sid] = intervals
+	}
+	return newSet
+}


shlomi-noach · 2024-10-01T11:28:24Z

go/mysql/replication/mysql56_gtid_set_test.go

+			name:               "Single errant GTID",
+			replicaPosition:    "MySQL56/8bc65c84-3fe4-11ed-a912-257f0fcdd6c9:1-8,8bc65cca-3fe4-11ed-bbfb-091034d48b3e:1,8bc65cca-3fe4-11ed-bbfb-091034d48bd3:34",
+			primaryPositionStr: "MySQL56/8bc65c84-3fe4-11ed-a912-257f0fcdd6c9:1-50,8bc65cca-3fe4-11ed-bbfb-091034d48b3e:1-30",
+			errantGtidWanted:   "8bc65cca-3fe4-11ed-bbfb-091034d48bd3:34",


shlomi-noach · 2024-10-01T11:29:06Z

go/mysql/replication/mysql56_gtid_set_test.go

+			name:               "Multiple errant GTID",
+			replicaPosition:    "MySQL56/8bc65c84-3fe4-11ed-a912-257f0fcdd6c9:1-8,8bc65cca-3fe4-11ed-bbfb-091034d48b3e:1-32,8bc65cca-3fe4-11ed-bbfb-091034d48bd3:3-35",
+			primaryPositionStr: "MySQL56/8bc65c84-3fe4-11ed-a912-257f0fcdd6c9:1-50,8bc65cca-3fe4-11ed-bbfb-091034d48b3e:1-30,8bc65cca-3fe4-11ed-bbfb-091034d48bd3:34",
+			errantGtidWanted:   "8bc65cca-3fe4-11ed-bbfb-091034d48b3e:31-32,8bc65cca-3fe4-11ed-bbfb-091034d48bd3:3-33:35",


shlomi-noach · 2024-10-01T11:30:10Z

go/mysql/replication/mysql56_gtid_set_test.go

+			name:       "Remove unknown UUID",
+			initialSet: "8bc65c84-3fe4-11ed-a912-257f0fcdd6c9:1-8,8bc65cca-3fe4-11ed-bbfb-091034d48b3e:1:4-24",
+			uuid:       "8bc65c84-3fe4-11ed-a912-257f0fcde6c9",
+			wantSet:    "8bc65c84-3fe4-11ed-a912-257f0fcdd6c9:1-8,8bc65cca-3fe4-11ed-bbfb-091034d48b3e:1:4-24",


shlomi-noach · 2024-10-01T11:30:37Z

go/vt/vttablet/tabletmanager/restore.go

@@ -326,7 +323,7 @@ func (tm *TabletManager) restoreDataLocked(ctx context.Context, logger logutil.L
 		} else if keyspaceInfo.KeyspaceType == topodatapb.KeyspaceType_NORMAL {
 			// Reconnect to primary only for "NORMAL" keyspaces
 			params.Logger.Infof("Restore: starting replication at position %v", pos)
-			if err := tm.startReplication(context.Background(), pos, originalType); err != nil {
+			if err := tm.startReplication(ctx, pos, originalType); err != nil {


go/vt/vttablet/tabletmanager/rpc_replication.go

go/vt/vttablet/tabletmanager/tm_init.go

deepthi

We should note in the release notes that for people running with vtop on k8s, this change means that the replica with broken replication will show as unready. There will be some kind of manual intervention needed to clean up these errant replica tablets, even if using the vtorc feature of marking them as drained.

go/mysql/replication/mysql56_gtid_set.go

Signed-off-by: Manan Gupta <[email protected]>

GuptaManan100 · 2024-10-03T06:02:02Z

Thank you for the reviews! I've addressed all review comments and added the release notes changes as requested. I will merge the PR once the tests are green. ♥️

…ication source (vitessio#16833)" This reverts commit eab262e. Signed-off-by: Manan Gupta <[email protected]>

GuptaManan100 added 3 commits September 24, 2024 15:54

feat: add code to run errant gtid detection for a replica

09e9244

Signed-off-by: Manan Gupta <[email protected]>

feat: use errant gtid detection logic in setreplicationsourcelocked

9c6ad9e

Signed-off-by: Manan Gupta <[email protected]>

feat: make changes for initialize replication too

734bd86

Signed-off-by: Manan Gupta <[email protected]>

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

GuptaManan100 added 5 commits September 25, 2024 10:25

Merge remote-tracking branch 'upstream/main' into errant-gtid-fix

b7928ec

Signed-off-by: Manan Gupta <[email protected]>

feat: fix encoding of primary position that we send down

66f0b6d

Signed-off-by: Manan Gupta <[email protected]>

feat: remove sending primary position from PRS and ERS into SetReplic…

38983b7

…ationSource RPC Signed-off-by: Manan Gupta <[email protected]>

comment: add code comments

ff452cc

Signed-off-by: Manan Gupta <[email protected]>

test: add tests

b2e5b20

Signed-off-by: Manan Gupta <[email protected]>

GuptaManan100 marked this pull request as ready for review September 30, 2024 05:02

GuptaManan100 requested review from deepthi, mattlord, rohit-nayak-ps, shlomi-noach, harshit-gangal and systay as code owners September 30, 2024 05:02

GuptaManan100 added 2 commits September 30, 2024 12:17

feat: fix context passing and test expectations

5854c45

Signed-off-by: Manan Gupta <[email protected]>

Merge remote-tracking branch 'upstream/main' into errant-gtid-fix

f8b5129

Signed-off-by: Manan Gupta <[email protected]>

GuptaManan100 mentioned this pull request Oct 1, 2024

Bug Report: vtorc ERS incorrectly flags replica as having errant GTIDs #16724

Closed

shlomi-noach approved these changes Oct 1, 2024

View reviewed changes

deepthi added the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Oct 1, 2024

deepthi approved these changes Oct 1, 2024

View reviewed changes

go/mysql/replication/mysql56_gtid_set.go Outdated Show resolved Hide resolved

rohit-nayak-ps mentioned this pull request Oct 2, 2024

Vitess Release v21 Tracking Issue #16795

Closed

57 tasks

GuptaManan100 added 2 commits October 3, 2024 11:23

feat: address review comments

f280d6d

Signed-off-by: Manan Gupta <[email protected]>

docs: add summary changes

e1e560c

Signed-off-by: Manan Gupta <[email protected]>

GuptaManan100 removed the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Oct 3, 2024

GuptaManan100 merged commit eab262e into vitessio:main Oct 3, 2024
98 of 99 checks passed

GuptaManan100 deleted the errant-gtid-fix branch October 3, 2024 06:31

GuptaManan100 mentioned this pull request Oct 21, 2024

Bug Report: Incorrect errant GTID detection because of how semi-sync replication works #17018

Closed

GuptaManan100 added a commit to planetscale/vitess that referenced this pull request Oct 21, 2024

Revert "Fix: Errant GTID detection on the replicas when they set repl…

b5c1bd0

…ication source (vitessio#16833)" This reverts commit eab262e. Signed-off-by: Manan Gupta <[email protected]>

GuptaManan100 mentioned this pull request Oct 21, 2024

[Direct PR] [release-21.0] Revert Errant GTID detection in vttablets #17019

Merged

5 tasks

GuptaManan100 mentioned this pull request Nov 21, 2024

Fix errant GTID detection in VTOrc to also work with a replica not connected to any primary #17267

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Errant GTID detection on the replicas when they set replication source #16833

Fix: Errant GTID detection on the replicas when they set replication source #16833

GuptaManan100 commented Sep 24, 2024 •

edited

Loading

vitess-bot bot commented Sep 24, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

shlomi-noach left a comment

shlomi-noach Oct 1, 2024

shlomi-noach Oct 1, 2024

shlomi-noach Oct 1, 2024

shlomi-noach Oct 1, 2024

shlomi-noach Oct 1, 2024

shlomi-noach Oct 1, 2024

deepthi left a comment

GuptaManan100 commented Oct 3, 2024 •

edited

Loading

Fix: Errant GTID detection on the replicas when they set replication source #16833

Fix: Errant GTID detection on the replicas when they set replication source #16833

Conversation

GuptaManan100 commented Sep 24, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Sep 24, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

shlomi-noach left a comment

Choose a reason for hiding this comment

shlomi-noach Oct 1, 2024

Choose a reason for hiding this comment

shlomi-noach Oct 1, 2024

Choose a reason for hiding this comment

shlomi-noach Oct 1, 2024

Choose a reason for hiding this comment

shlomi-noach Oct 1, 2024

Choose a reason for hiding this comment

shlomi-noach Oct 1, 2024

Choose a reason for hiding this comment

shlomi-noach Oct 1, 2024

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

GuptaManan100 commented Oct 3, 2024 • edited Loading

GuptaManan100 commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

GuptaManan100 commented Oct 3, 2024 •

edited

Loading