Only refresh required tablet's information in VTOrc by GuptaManan100 · Pull Request #11220 · vitessio/vitess

GuptaManan100 · 2022-09-14T11:46:19Z

Description

In #10115 and #10150 we added the capability of refreshing VTOrc's ephemeral information before it ran any fix. This was required to help us guarantee safety however, the first iteration of this change was inefficient.

We used to refresh all the tablets that are in the VTOrc instance's purview for each and every recovery. As part of the PRs a TODO for fixing this was also added -

// TODO (@GuptaManan100): Refresh only the shard tablet information instead of all the tablets

This change couldn't be immediately accomplished because we first required the cleanup of cluster_alias, cluster_name, and suggested_cluster_alias. This cleanup was addressed in #11193.

This PR addresses this TODO that was introduced then in order to make the recoveries more efficient and faster. Instead of the proposed refreshing all tablets in a shard in the TODO, this PR takes it a step further and only refreshes the tablets that are required.

To this end, all the recoveries have been categorized into two types, the ones that are cluster-wide recoveries and the ones that aren't.

If we are about to run a cluster-wide recovery like electNewPrimary or recoverDeadPrimary, then it is imperative to first refresh all the tablets of a shard because a new tablet could have been promoted, and we need to have this visibility before we run a cluster operation of our own.

Non-cluster-wide recoveries are only concerned with the specific tablet on which the failure occurred and the primary instance of the shard. For example, ConnectedToWrongPrimary analysis only cares for which tablet is the current primary and the host-port set on the tablet in question. So, we only need to refresh the tablet info records (to know if the primary tablet has changed), and the replication data of the new primary and this tablet.

These changes make VTOrc recoveries much faster, while still guaranteeing correctness like they used to 🧞

Related Issue(s)

RFC - Durability and consensus in Vtorc #8975

Checklist

"Backport me!" label has been added if this change should be backported
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

… code and use it for finding if their is an actionable recovery and the recovery function Signed-off-by: Manan Gupta <manan@planetscale.com>

…e when getting replication analysis Signed-off-by: Manan Gupta <manan@planetscale.com>

…n instead of the big-hammer approach of refreshing everything Signed-off-by: Manan Gupta <manan@planetscale.com>

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

vitess-bot · 2022-09-14T11:46:22Z

GuptaManan100 · 2022-09-14T11:47:58Z

go/vt/orchestrator/inst/analysis.go

 	AnalyzedInstanceDataCenter                string
 	AnalyzedInstanceRegion                    string
 	AnalyzedKeyspace                          string
+	AnalyzedShard                             string


We need the name of the shard that was analyzed too now that we want to restrict the number of tablets we want to refresh

GuptaManan100 · 2022-09-14T11:49:17Z

go/vt/orchestrator/logic/orchestrator.go

+	if forceDiscovery {
+		log.Infof("Force discovered - %+v", instance)
+	}


This addition of logging is intentional. Until we have a metrics page where we export the internal database information of VTOrc, this is going to be very useful in debugging. I had it in my mind to add this log and I am just piggy-backing on this PR.

GuptaManan100 · 2022-09-14T11:51:09Z

go/vt/orchestrator/logic/tablet_discovery.go

+func shardPrimary(keyspace string, shard string) (primary *topodatapb.Tablet, err error) {
+	query := `SELECT
+		info,
+		hostname,
+		port,
+		tablet_type,
+		primary_timestamp
+	FROM 
+		vitess_tablet
+	WHERE
+		keyspace = ? AND shard = ?
+	ORDER BY
+		tablet_type ASC,
+		primary_timestamp DESC
+	LIMIT 1
+`
+	err = db.Db.QueryOrchestrator(query, sqlutils.Args(keyspace, shard), func(m sqlutils.RowMap) error {
+		if primary == nil {
+			primary = &topodatapb.Tablet{}
+			return prototext.Unmarshal([]byte(m.GetString("info")), primary)
+		}
+		return nil
+	})
+	return primary, err


This is another small enhancement that was made in this PR, wherein we can use the tablet information we have collected to find the shard primary tablet. We use something similar in GetReplicationAnalysis to find who the primary is.
Previously we used to do a topo-server call to read the shard and tablet record, but it isn't required.

I have added tests for this function and when I added the tests I also found a bug in my original implementation 🤣. I don't just need to descending sort the tablet_types, we need to filter on them!

GuptaManan100 · 2022-09-14T11:52:13Z

go/vt/orchestrator/logic/topology_recovery.go

-	// Can't do this now since SuggestedClusterAlias, ClusterName, ClusterAlias aren't consistent
-	// and passing any one causes issues in some failures
-	analysisEntries, err := inst.GetReplicationAnalysis("", &inst.ReplicationAnalysisHints{})
+	analysisEntries, err := inst.GetReplicationAnalysis(analysisEntry.ClusterDetails.ClusterName, &inst.ReplicationAnalysisHints{})


This is a change that could have happened in #11193 but I am piggy-backing on this PR to not create a separate one just for this change.

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

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

rsajwani

Overall LGTM.

go/vt/orchestrator/logic/topology_recovery.go

go/vt/orchestrator/logic/tablet_discovery.go

rsajwani · 2022-09-14T21:21:13Z

go/vt/orchestrator/logic/tablet_discovery.go

-		return nil, err
+// shardPrimary finds the primary of the given keyspace-shard by reading the orchestrator backend
+func shardPrimary(keyspace string, shard string) (primary *topodatapb.Tablet, err error) {
+	query := `SELECT


general comment: should we not put all query execution in some retryable template function?

Yes, that would be a good addition, but so far we have not really needed, because even if the read fails, we just fail the recovery and then retry later.

go/vt/orchestrator/logic/tablet_discovery.go

deepthi · 2022-09-15T01:44:16Z

go/vt/orchestrator/logic/tablet_discovery.go

+		primary_timestamp DESC
+	LIMIT 1
+`
+	err = db.Db.QueryOrchestrator(query, sqlutils.Args(keyspace, shard, topodatapb.TabletType_PRIMARY), func(m sqlutils.RowMap) error {


As part of cleaning up, we should rename some of these functions. QueryOrchestrator is not the right name for this function. Similarly we have OpenOrchestrator too.

Yes, I had it in my mind to get rid of the Orchestrator references everywhere, from Parameters, flags, package names, function names, etc. I'll do in a follow-up PR so that it is easier to review

go/vt/topotools/tablet.go

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

…itessio#1061) * refactor: make recovery Function code as the identifier of a function code and use it for finding if their is an actionable recovery and the recovery function Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: remvoe a TODO in checkIfAlreadyFixed by sending the cluster name when getting replication analysis Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: refactor refersh code logic to only refresh required information instead of the big-hammer approach of refreshing everything Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: add logs and refresh for analyzed instance after a recovery Signed-off-by: Manan Gupta <manan@planetscale.com> * refactor: fix typing error in comments Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: use context.Background() instead of nil Signed-off-by: Manan Gupta <manan@planetscale.com> * test: add testing for refreshTabletsInKeyspaceShard Signed-off-by: Manan Gupta <manan@planetscale.com> * test: add tests for shardPrimary function and also fix its implementation Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: address review comments Signed-off-by: Manan Gupta <manan@planetscale.com> * test: use cmp with proto.Equal Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 added 4 commits September 12, 2022 16:01

refactor: make recovery Function code as the identifier of a function…

ac661ec

… code and use it for finding if their is an actionable recovery and the recovery function Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: remvoe a TODO in checkIfAlreadyFixed by sending the cluster nam…

89fb875

…e when getting replication analysis Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: refactor refersh code logic to only refresh required informatio…

7b24036

…n instead of the big-hammer approach of refreshing everything Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: add logs and refresh for analyzed instance after a recovery

817fa7a

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

GuptaManan100 added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: VTOrc Vitess Orchestrator integration labels Sep 14, 2022

GuptaManan100 requested review from deepthi and shlomi-noach as code owners September 14, 2022 11:46

GuptaManan100 commented Sep 14, 2022

View reviewed changes

GuptaManan100 added 4 commits September 14, 2022 17:53

refactor: fix typing error in comments

f0f97b8

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

feat: use context.Background() instead of nil

d6ff07a

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

test: add testing for refreshTabletsInKeyspaceShard

7b26517

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

test: add tests for shardPrimary function and also fix its implementa…

2049e16

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

GuptaManan100 requested a review from rsajwani September 14, 2022 13:50

rsajwani approved these changes Sep 14, 2022

View reviewed changes

deepthi reviewed Sep 15, 2022

View reviewed changes

GuptaManan100 added 2 commits September 15, 2022 11:16

feat: address review comments

4a09c3f

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

test: use cmp with proto.Equal

e851828

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

GuptaManan100 merged commit b5d8281 into vitessio:main Sep 15, 2022

GuptaManan100 deleted the vtorc-conservative-refresh branch September 15, 2022 07:50

maxenglander mentioned this pull request Jul 6, 2023

Various vtorc backports into v14 planetscale/vitess#82

Merged

Conversation

GuptaManan100 commented Sep 14, 2022 • edited by deepthi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

vitess-bot bot commented Sep 14, 2022 • edited by deepthi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Checklist

General

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsajwani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuptaManan100 Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GuptaManan100 commented Sep 14, 2022 •

edited by deepthi

Loading

vitess-bot bot commented Sep 14, 2022 •

edited by deepthi

Loading

GuptaManan100 Sep 15, 2022 •

edited

Loading