Add timeout to all the contexts used for RPC calls in vtorc

[GuptaManan100]

Description

This PR fixes the bug described in #15990. It adds the RemoteOperationTimeout to all the RPC calls.

Related Issue(s)

• Fixes Bug Report: VTOrc is not setting the correct timeout for RPC calls which can get stuck #15990

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

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

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

Project coverage is 68.25%. Comparing base (3b09eb2) to head (acfcfc0).
Report is 3 commits behind head on main.

Files	Patch %	Lines
go/vt/vtorc/logic/tablet_discovery.go	0.00%	15 Missing ⚠️
...t/vtctl/grpcvtctldserver/testutil/test_tmclient.go	37.50%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15991      +/-   ##
==========================================
- Coverage   68.47%   68.25%   -0.23%     
==========================================
  Files        1562     1562              
  Lines      197083   197097      +14     
==========================================
- Hits       134962   134524     -438     
- Misses      62121    62573     +452     

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

[mattlord]

❤️ Thanks, @GuptaManan100 ! I double checked a few of the RPCs we're making with the new context and confirmed that we're checking to see if the context is done as we go up the call stack on the tmc server side.

Do we not also need to do it for other things like LockShard() ? That's making a topo call and we use context.Background() here:

vitess/go/vt/vtorc/logic/topology_recovery.go

Line 490 in cf06f60

ctx, unlock, err := LockShard(context.Background(), analysisEntry.AnalyzedInstanceAlias, getLockAction(analysisEntry.AnalyzedInstanceAlias, analysisEntry.Analysis))

If that hangs then it could block many other operations both within vtorc and without (at least until the lock's lease expired in the topo server). I didn't look at everything so that's just one of N other potential places where we should probably use a context with a timeout/deadline too.

I also think it probably makes sense to create a context earlier/lower in the call stack at various points, which then gets passed along. For example here:

vitess/go/vt/vtorc/logic/topology_recovery.go

Line 446 in cf06f60

func executeCheckAndRecoverFunction(analysisEntry *inst.ReplicationAnalysis) (err error) {

Or even lower in the call stack at CheckAndRecover(). That would then get passed on to LockShard, RefreshKeyspaceShard, and all the recovery functions. It could be topo.RemoteOperationTimeout * X or a fixed time of e.g. 60s — whatever makes sense for the larger overall operation with functions up the stack like tabletUndoDemotePrimary() still able to create child contexts with shorter timeouts.

We don't have to fix/improve everything all at once, but I think it's at least something worth looking into and considering. If you do feel up for a more exhaustive context examination and refactoring I'm happy to help if/as needed (certainly at least with the review).

[mattlord]

There also appears to be a data race in one of the new tests that we should address before merging:

2024-05-22T04:28:30.9053460Z ==================
2024-05-22T04:28:30.9053564Z WARNING: DATA RACE
2024-05-22T04:28:30.9053700Z Read at 0x000002d0e9e0 by goroutine 587:
2024-05-22T04:28:30.9054125Z   vitess.io/vitess/go/vt/vtctl/reparentutil.(*PlannedReparenter).performGracefulPromotion()
2024-05-22T04:28:30.9054645Z       /home/runner/work/vitess/vitess/go/vt/vtctl/reparentutil/planned_reparenter.go:302 +0xd64
2024-05-22T04:28:30.9055191Z   vitess.io/vitess/go/vt/vtctl/reparentutil.TestPlannedReparenter_performGracefulPromotion.func3()
2024-05-22T04:28:30.9055788Z       /home/runner/work/vitess/vitess/go/vt/vtctl/reparentutil/planned_reparenter_flaky_test.go:1709 +0xace
...
2024-05-22T04:28:30.9056814Z 
2024-05-22T04:28:30.9056987Z Previous write at 0x000002d0e9e0 by goroutine 888:
2024-05-22T04:28:30.9057432Z   vitess.io/vitess/go/vt/vtctl/reparentutil.TestEmergencyReparenter_reparentReplicas.func1.1()
2024-05-22T04:28:30.9057994Z       /home/runner/work/vitess/vitess/go/vt/vtctl/reparentutil/emergency_reparenter_test.go:3475 +0x30
2024-05-22T04:28:30.9058119Z   runtime.deferreturn()
...
2024-05-22T04:28:30.9064675Z ==================
2024-05-22T04:28:30.9065077Z === NAME  TestPlannedReparenter_performGracefulPromotion/demotion_succeeds_but_parent_context_times_out
2024-05-22T04:28:30.9065334Z     testing.go:1398: race detected during execution of test
2024-05-22T04:28:30.9065626Z --- FAIL: TestPlannedReparenter_performGracefulPromotion (0.01s)

[GuptaManan100]

The data race actually got introduced in #15934, I'll do a PR to fix it separately, that we can merge ASAP

[GuptaManan100]

@mattlord We don't need a timeout for LockShard because internally it adds the appropriate timeout -

func (l *Lock) internalLockShard(ctx context.Context, ts *Server, keyspace, shard string, isBlocking bool) (LockDescriptor, error) {
	log.Infof("Locking shard %v/%v for action %v", keyspace, shard, l.Action)

	ctx, cancel := context.WithTimeout(ctx, getLockTimeout())
	defer cancel()

Having a timeout at a higher level in CheckAndRecover is something that I'm considering. Not sure whether we'll add one or not, and if we do, what that timeout would be. In any case, like you said, adding timeouts to each RPC is still something we should do! ❤️