RFC: access data via an interface in vtorc

[timvaillancourt]

Feature Description

This RFC proposes a new golang interface is added to go/vt/vtorc/db to allow:

1. Easier mocking of the backend store, for tests, etc
2. The ability to use a different implementation
3. A full separation of VTOrc logic vs. data access - right now go/vt/vtorc/logic has some database logic in it

The move to this would involve:

1. Defining the new interface, I'm thinking type DB interface
2. Make the existing logic an implementation of the new interface
3. Update tests

I've done a quick scan over the code, and I probably missed something, but here is a rough draft interface that should support the existing code. There are some small tweaks such as:

1. Moving long signatures to opts structs
2. Using real *topodatapb.TabletAlias vs the opinionated tabletAlias string
3. Adding ctx context.Context to everything

cc @GuptaManan100 for thoughts 🙇

Rough draft:

type Backend interface {
	// Discovery
	DeleteDiscoveredTablets(ctx context.Context) error
	GetShardPrimary(ctx context.Context, keyspace string, shard string) (*topodatapb.Tablet, error)
	GetTabletAliasesByCell(ctx context.Context) ([]*topodatapb.TabletAlias, error)
	GetTabletAliasesByKeyspaceShard(ctx context.Context) ([]*topodatapb.TabletAlias, error)

	// Detection
	AttemptFailureDetectionRegistration(ctx context.Context, analysisEntry *inst.ReplicationAnalysis) (bool, error)
	ClearActiveFailureDetections(ctx context.Context) error

	// Analysis
	AuditInstanceAnalysisInChangelog(ctx context.Context, tabletAlias *topodatapb.TabletAlias, analysisCode AnalysisCode) error
	ExpireAuditData(ctx context.Context) error
	ExpireInstanceAnalysisChangelog(ctx context.Context) error
	GetReplicationAnalysis(ctx context.Context, keyspace string, shard string, hints *inst.ReplicationAnalysisHints) ([]*inst.ReplicationAnalysis, error)

	// Audit
	AuditOperation(ctx context.Context, opts *AuditOperationOpts) error

	// Instance
	ExpireStaleInstanceBinlogCoordinates(ctx context.Context) error
	ForgetInstance(ctx context.Context, tabletAlias *topodatapb.TabletAlias) error
	ForgetLongUnseenInstances(ctx context.Context) error
	GetKeyspaceShardName(ctx context.Context, tabletAlias *topodatapb.TabletAlias) (string, string, error) {
        LegacyReadInstanceClusterAttributes(ctx context.Context, primaryHost string, primaryPort int) (*inst.Instance, error)
	ReadInstanceClusterAttributes(ctx context.Context, primaryAlias *topodatapb.TabletAlias) (*inst.Instance, error)
	ReadInstance(ctx context.Context, tabletAlias *topodatapb.TabletAlias) (*inst.Instance, bool, error) 
	ReadReplicaInstances(ctx context.Context, opts *ReadReplicaInstancesOpts) ([]*inst.Instance, error)
	ReadProblemInstances(ctx context.Context, keyspace string, shard string) ([]*inst.Instance, error)
	ReadOutdatedInstanceKeys(ctx context.Context) ([]*topodatapb.TabletAlias, error)
	ReadInstancesWithErrantGTIDs(ctx context.Context, keyspace string, shard string) ([]*inst.Instance, error)
	RecordStaleInstanceBinlogCoordinates(ctx context.Context, tabletAlias *topodatapb.TabletAlias, binlogCoordinates *inst.BinlogCoordinates) error
	SnapshotTopologies(ctx context.Context) error
	UpdateInstanceLastAttemptedCheck(ctx context.Context, tabletAlias *topodatapb.TabletAlias)
	UpdateInstanceLastChecked(ctx context.Context, tabletAlias *topodatapb.TabletAlias, partialSuccess bool) error
	WriteInstances(ctx context.Context, instances []*inst.Instance, instanceWasActuallyFound, updateLastSeen bool) error

	// Keyspace
	ReadKeyspace(ctx context.Context, keyspace string) (*topo.KeyspaceInfo, error)
	SaveKeyspace(ctx context.Context, keyspace *topo.KeyspaceInfo) error
	GetDurabilityPolicy(ctx context.Context, keyspace string) (reparentutil.Durabler, error)

	// Shard
	ReadShardPrimaryInformation(ctx context.Context, keyspaceName, shardName string) (*topodatapb.TabletAlias, string, error)
	SaveShard(ctx context.Context, shard *topo.ShardInfo) error

	// Tablet
	ReadTablet(ctx context.Context, tabletAlias *topodatapb.TabletAlias) (*topodatapb.Tablet, error)
	SaveTablet(ctx context.Context, tablet *topodatapb.Tablet) error

	// Recovery
	AcknowledgeRecoveries(ctx context.Context, opts *AcknowledgeRecoveriesOpts) (int64, error)
	ClearActiveRecoveries(ctx context.Context) error
	DisableRecovery(ctx context.Context) error
	EnableRecovery(ctx context.Context) error
	ExpireBlockedRecoveries(ctx context.Context) error
	ExpireRecoveries(ctx context.Context) error
	ExpireRecoverySteps(ctx context.Context) error
	IsRecoveryDisabled(ctx context.Context) (bool, error)
	ReadRecoveries(ctx context.Context, opts *ReadRecoveriesOpts) ([]*logic.TopologyRecovery, error)
	RegisterBlockedRecoveries(ctx context.Context, analysisEntry *inst.ReplicationAnalysis, blockingRecoveries []*logic.TopologyRecovery) error
	WriteResolveRecovery(ctx context.Context, topologyRecovery *logic.TopologyRecovery) error
	WriteTopologyRecovery(ctx context.Context, topologyRecovery *logic.TopologyRecovery) (*logic.TopologyRecovery, error)
	WriteTopologyRecoveryStep(ctx context.Context, topologyRecoveryStep *logic.TopologyRecoveryStep) error
}

type AcknowledgeRecoveriesOpts struct {
	Owner           string
	Comment         string
	MarkEndRecovery bool
}

type AuditOperationOpts struct {
	// TODO: define
}

type ReadRecoveriesOpts struct {
	// TODO: define
}

type ReadReplicaInstancesOpts struct {
	PrimaryHost                    string
	PrimaryPort                    int
	IncludeBinlogServerSubReplicas bool
}

Use Case(s)

Developers and indirectly users of vtorc

[timvaillancourt]

A bit unrelated, but maybe time to fix this too:

ReadInstanceClusterAttributes(ctx context.Context, primaryHost string, primaryPort int)

and

ReadReplicaInstances(ctx context.Context, primaryHost string, primaryPort int) ([]*Instance, error)

..should just use a *topodatapb.TabletAlias of the primary, but FullStatus doesn't include it, so VTOrc doesn't have it in the backend database

Related to this, it seems we have no indices on hostname and port (but we sometimes query/join on it) - I'm on the fence if I should add that index or fix it properly now (return/store alias of the primary)

cc @GuptaManan100 for 💡s

[GuptaManan100]

I like the idea of the RFC, just one thing I'd like to correct.

but FullStatus doesn't include it, so VTOrc doesn't have it in the backend database

This is not true. We have all the information that we store in a tablet record in vitess_tablet table. Look at SaveTablet for more information. We store the entire tablet record by marshaling it from proto into string. We also store individual fields that we deem worthy of reading without needing to deserialize the marshalled proto string. We store the alias as a string, and the cell in the table. If we want, we can also store the UID, and that gives us all the infomration to construct the *topodatapb.TabletAlias back. (We can still do that by unmarshaling the info field, but we can store uid explicitly too).

[GuptaManan100]

A refactor that makes things more testable is always welcome! I have always thought that the way VTOrc DB code is written was a little bit hard to test. I had introduced an interface for testing before as well. It is present in db.go -

type DB interface {
	QueryVTOrc(query string, argsArray []any, onRow func(sqlutils.RowMap) error) error
}

but your proposal is way better.

[timvaillancourt]

This is not true. We have all the information that we store in a tablet record in vitess_tablet table. Look at SaveTablet for more information.

@GuptaManan100 great, good to know! Another refactor I'd like to do is stop using the unindexed hostname and port columns in queries. I assume those were indexed but they aren't anymore. It seems hostname/port is used to join to the primary tablet in some cases, so I thought alias just wasn't available and we were stuck

If we JOIN on the primary alias there would be an index

[timvaillancourt]

We store the alias as a string, and the cell in the table.

To be clear, I mean storing the alias of each tablet's current primary. I'll review where it's available

[timvaillancourt]

@GuptaManan100 I've moved the discussion re: the primary alias in FullStatus responses to this issue: #17297. Let me know what you think there

For the backend interface this issue describes, I'll get started on it 👍

[timvaillancourt]

Renamed ReadInstanceClusterAttributes to LegacyReadInstanceClusterAttributes:

        LegacyReadInstanceClusterAttributes(ctx context.Context, primaryHost string, primaryPort int) (*inst.Instance, error)

And added future-replacement that uses *topodatapb.TabletAlias (for when #17297 add this to FullStatus):

	ReadInstanceClusterAttributes(ctx context.Context, primaryAlias *topodatapb.TabletAlias) (*inst.Instance, error)