VTOrc checks and fixes replication misconfiguration issues #15881

GuptaManan100 · 2024-05-08T09:43:48Z

Description

This PR adds the feature request for #15880.

If the user changes replic_net_timeout (https://dev.mysql.com/doc/refman/8.0/en/replication-options-replica.html#sysvar_replica_net_timeout) settings, then VTOrc should detect the misconfiguration of heartbeat intervals, and should set it correctly in https://dev.mysql.com/doc/refman/8.0/en/change-replication-source-to.html#crs-opt-source_heartbeat_period.

This PR adds this capability. As part of FullStatus RPC, we now also read the heartbeat interval, and the replica net timeout values. These are used by VTOrc to figure out if there is a misconfiguration. If there is, then VTOrc goes ahead and fixes the problem.

VTOrc will only be able to fix the problems properly after both VTOrc and vttablet have been upgraded.

If only VTOrc is upgraded (and not VTTablets), then VTOrc will register the misconfiguration issue, but won't be able to fix it, since the vttablet won't have the RPC changes to accept the heartbeat interval to set.

If only VTTablet is upgraded, old VTOrc won't even register the issue.

Related Issue(s)

Fixes Feature Request: Get VTOrc to fix replication misconfiguration issues on replicas #15880

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

…rtbeat interval Signed-off-by: Manan Gupta <manan@planetscale.com>

…ormation for the tablet Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

vitess-bot · 2024-05-08T09:43:51Z

GuptaManan100 · 2024-05-08T09:51:36Z

go/vt/vtorc/logic/topology_recovery.go

+	if analysisEntry.Analysis == inst.ReplicaMisconfigured {
+		heartBeatInterval = float64(analysisEntry.ReplicaNetTimeout) / 2
+	}
+


I could use some advice/opinions on this front. I have so far chosen to only specify heartBeatInterval in the express case where VTOrc has registered a misconfiguration.

The advantage of this approach is that if the replica only has its replication stopped, then VTOrc won't make it run a CHANGE REPLICATION SOURCE command.

The drawback is that if the replica has no replication source configured and also heartbeat interval misconfigured, then VTOrc will first register NotConnectedToPrimary and it won't change the heartbeat settings. Only after the replication source has been fixed, will it register ReplicaMisconfigured. So the whole process of the fix will take 2 fixes.

Another disadvantage is that so far fixReplica has been fixing all the problems on the replica by configuring it again without caring for what was the initial problem that triggered this fix. We will be changing this assumption, as the analysis will also impact how the fix runs. Moreover, this adds another layer of complexity in replication analysis detection wherein VTOrc must register misconfiguration issues before registering replication not running issues. Because if the replication is not running because of misconfiguration, we want to fix the misconfiguration and not just restart replication. So we must register the misconfiguration problem before.

The alternative to this approach is to set the heartbeat interval value always. This would mean that whenever we try to fix the replica, we run the CHANGE SOURCE REPLICATION command always.

Another approach that we could take is to add a method to the analysis like HeartbeatMisconfigured which returns a boolean if the heartbeat settings are incorrect, and we use this method in detecting the misconfiguration and also in the fixReplica phase to only set the heartbeat value if it is misconfigured.

I suggest taking the simpler code path, even if that means that the process fix takes 2 steps. That's immaterial IMO.

As discussed, we have decided to take the simpler, least risk approach of just fixing everything on the replica when we notice anything is broken

Signed-off-by: Manan Gupta <manan@planetscale.com>

shlomi-noach

LGTM with just a bit of concern comapring float64 values.

shlomi-noach · 2024-05-08T09:55:03Z

go/test/endtoend/vtorc/general/vtorc_test.go

+
+		// check that writes succeed
+		utils.VerifyWritesSucceed(t, clusterInfo, curPrimary, []*cluster.Vttablet{replica, otherReplica}, 15*time.Second)
+	})


go/test/endtoend/vtorc/utils/utils.go

shlomi-noach · 2024-05-08T09:58:35Z

go/vt/vtorc/db/generate_base.go

@@ -55,6 +55,8 @@ CREATE TABLE database_instance (
 	binary_log_pos bigint NOT NULL,
 	source_host varchar(128) NOT NULL,
 	source_port smallint NOT NULL,
+	replica_net_timeout int NOT NULL,
+	heartbeat_interval decimal(11,4) NOT NULL,


👍

Although: if we use decimal, then we should not be doing parseFloat nor using float64 in the rest of the code.

shlomi-noach · 2024-05-08T09:59:46Z

go/vt/vtorc/inst/analysis_dao.go

@@ -468,6 +473,10 @@ func GetReplicationAnalysis(keyspace string, shard string, hints *ReplicationAna
 			a.Analysis = NotConnectedToPrimary
 			a.Description = "Not connected to the primary"
 			//
+		} else if topo.IsReplicaType(a.TabletType) && !a.IsPrimary && math.Round(a.HeartbeatInterval*2) != float64(a.ReplicaNetTimeout) {


If the db value is decimal, and we read and compare it with a float64(...) then we lose the decimal accuracy.

If the heartbeat interval is configured correctly then it should either be an integer or have .5 as its fractional part because it is half of an integral value. Because we lose some precision, we're using math.Round to round it to the nearest integer after multiplying by 2.

shlomi-noach · 2024-05-08T10:01:21Z

go/vt/vtorc/inst/instance_dao.go

@@ -495,6 +500,8 @@ func readInstanceRow(m sqlutils.RowMap) *Instance {
 	instance.LogReplicationUpdatesEnabled = m.GetBool("log_replica_updates")
 	instance.SourceHost = m.GetString("source_host")
 	instance.SourcePort = m.GetInt("source_port")
+	instance.ReplicaNetTimeout = m.GetInt32("replica_net_timeout")
+	instance.HeartbeatInterval = m.GetFloat64("heartbeat_interval")


shlomi-noach · 2024-05-08T10:03:26Z

go/vt/vtorc/logic/topology_recovery.go

+	if analysisEntry.Analysis == inst.ReplicaMisconfigured {
+		heartBeatInterval = float64(analysisEntry.ReplicaNetTimeout) / 2
+	}
+


I suggest taking the simpler code path, even if that means that the process fix takes 2 steps. That's immaterial IMO.

codecov · 2024-05-08T10:18:00Z

Codecov Report

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

Project coverage is 68.46%. Comparing base (9bfc18c) to head (d59ccd4).
Report is 11 commits behind head on main.

Files	Patch %	Lines
go/mysql/flavor_mariadb.go	12.00%	22 Missing ⚠️
go/mysql/flavor_mysql.go	65.71%	12 Missing ⚠️
go/vt/vttablet/tabletmanager/rpc_replication.go	0.00%	11 Missing ⚠️
go/mysql/flavor_filepos.go	0.00%	5 Missing ⚠️
go/vt/vtorc/inst/instance_dao.go	66.66%	3 Missing ⚠️
go/mysql/flavor_mysqlgr.go	0.00%	2 Missing ⚠️
go/vt/mysqlctl/fakemysqldaemon.go	33.33%	2 Missing ⚠️
go/vt/vtorc/logic/tablet_discovery.go	0.00%	2 Missing ⚠️
go/cmd/vtbackup/cli/vtbackup.go	0.00%	1 Missing ⚠️
go/cmd/vtcombo/cli/main.go	0.00%	1 Missing ⚠️
... and 7 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15881      +/-   ##
==========================================
+ Coverage   68.43%   68.46%   +0.02%     
==========================================
  Files        1559     1559              
  Lines      196516   196753     +237     
==========================================
+ Hits       134479   134699     +220     
- Misses      62037    62054      +17

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

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 added 4 commits May 8, 2024 12:16

feat: add the capability to track the replica net timeout and the hea…

032b662

…rtbeat interval Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: store the replica net timeout and heartbeat interval in the inf…

5a30d31

…ormation for the tablet Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: add the capability to vtorc to detect replica misconfiguratin

12d13ae

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: add capability to vtorc to fix the replication misconfiguration

7523d10

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 added Type: Feature Component: VTorc Vitess Orchestrator integration labels May 8, 2024

GuptaManan100 requested review from deepthi, harshit-gangal, mattlord, rohit-nayak-ps, ajm188, shlomi-noach, notfelineit and systay as code owners May 8, 2024 09:43

GuptaManan100 commented May 8, 2024

View reviewed changes

feat: register misconfiguration problem before

27f663d

Signed-off-by: Manan Gupta <manan@planetscale.com>

github-actions bot added this to the v20.0.0 milestone May 8, 2024

shlomi-noach approved these changes May 8, 2024

View reviewed changes

GuptaManan100 added 2 commits May 8, 2024 16:17

feat: make the code simpler

b74c9db

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: use context with timeout

d59ccd4

Signed-off-by: Manan Gupta <manan@planetscale.com>

dbussink approved these changes May 8, 2024

View reviewed changes

GuptaManan100 merged commit cbf89bd into vitessio:main May 8, 2024
96 checks passed

GuptaManan100 deleted the vtorc-check-configs branch May 8, 2024 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VTOrc checks and fixes replication misconfiguration issues #15881

VTOrc checks and fixes replication misconfiguration issues #15881

GuptaManan100 commented May 8, 2024

vitess-bot bot commented May 8, 2024

GuptaManan100 May 8, 2024

GuptaManan100 May 8, 2024

GuptaManan100 May 8, 2024

shlomi-noach May 8, 2024

GuptaManan100 May 8, 2024

shlomi-noach left a comment

shlomi-noach May 8, 2024

shlomi-noach May 8, 2024

shlomi-noach May 8, 2024

GuptaManan100 May 8, 2024

shlomi-noach May 8, 2024

shlomi-noach May 8, 2024

codecov bot commented May 8, 2024 •

edited

Loading

VTOrc checks and fixes replication misconfiguration issues #15881

VTOrc checks and fixes replication misconfiguration issues #15881

Conversation

GuptaManan100 commented May 8, 2024

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented May 8, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

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

shlomi-noach left a 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

codecov bot commented May 8, 2024 • edited Loading

Codecov Report

codecov bot commented May 8, 2024 •

edited

Loading