Tablet throttler: throttler-config-via-topo defaults 'true', deprecation message for old flags

[shlomi-noach]

Description

• v16 introduced a new vttablet flag: --throttler-config-via-topo, see the docs: https://vitess.io/docs/16.0/reference/features/tablet-throttler/
• In v16 this flag defaults false, and the old per-tablet --enable-lag-throttler configuration is still supported.
• For v17, the target of this PR, this PR sets the default for --throttler-config-via-topo to true. The old configuration is still supported but if used there's a deprecation warning.
• In v18, the old configuration & logic will be compeletely removed and it will be assumed that --throttler-config-via-topo is always true whether specified or not. The flag will issue a deprecation message.
• In v19 we will remove the flag --throttler-config-via-topo.

We remove all references to --enable-lag-throttler in Vitess's own endtoend tests, and use the dynamic throttler config everywhere.

Related Issue(s)

Follow up to:

• Dynamic tablet throttler config: enable/disable, set metrics query/threshold #11604
• Throttler: Store Config in Global Keyspace Topo Record #12520

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on the CI
• Documentation: Update throttler config docs for v17 website#1489

[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.
• If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
• If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

• Is it really necessary to add this flag?
• Flag names should be clear and intuitive (as far as possible)
• Help text should be descriptive.
• Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

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

Bug fixes

• There should be at least one unit or end-to-end test.
• The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

• There should be some code comments as to why things are implemented the way they are.

New/Existing features

• Should be documented, either by modifying the existing documentation or creating new documentation.
• New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

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

[shlomi-noach]

Various vreplication tests are failing. We enable the throttler in endtoend/vreplication/cluster_test.go and then check all tablets to see that the configuration is applied. On some tablets this test times out (at 1min timeout which is painfully long). Not sure yet what's up, still looking.

[shlomi-noach]

Sample failure: https://github.com/vitessio/vitess/actions/runs/5056630919/jobs/9075115936?pr=13130

2023-05-23T12:14:38.2318934Z I0523 12:11:43.411558   17733 cluster_test.go:568] Finished creating shard c0-
2023-05-23T12:14:38.2319318Z I0523 12:11:43.411573   17733 cluster_test.go:571] Applying throttler config for keyspace customer
2023-05-23T12:14:38.2320142Z I0523 12:11:43.413036   17733 vtctldclient_process.go:67] Executing vtctldclient with command: vtctldclient --server 127.0.0.1:15999 UpdateThrottlerConfig --enable --threshold 30.000000 --custom-query  --check-as-check-shard customer (attempt 1 of 10)
2023-05-23T12:14:38.2320755Z I0523 12:11:43.703824   17733 cluster_test.go:574] Waiting for throttler config to be applied on all shards
2023-05-23T12:14:38.2321292Z I0523 12:11:43.704212   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone2-603
2023-05-23T12:14:38.2321826Z I0523 12:11:43.708998   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-600
2023-05-23T12:14:38.2322334Z I0523 12:11:43.709716   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-601
2023-05-23T12:14:38.2322838Z I0523 12:11:43.710200   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-602
2023-05-23T12:14:38.2323334Z I0523 12:11:43.710920   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-701
2023-05-23T12:14:38.2323827Z I0523 12:11:43.711373   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-702
2023-05-23T12:14:38.2374812Z I0523 12:11:43.711896   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone2-703
2023-05-23T12:14:38.2375439Z I0523 12:11:43.712458   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-700
2023-05-23T12:14:38.2376094Z I0523 12:11:43.713035   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-200
2023-05-23T12:14:38.2376590Z I0523 12:11:43.714059   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-201
2023-05-23T12:14:38.2376907Z === NAME  TestBasicV2Workflows
2023-05-23T12:14:38.2378407Z     util.go:131: timed out waiting for the zone1-201 tablet's throttler status enabled to be true with the correct config after 1m0s; last seen value: {"Keyspace":"customer","Shard":"-80","IsLeader":false,"IsOpen":true,"IsEnabled":false,"IsDormant":true,"Query":"select unix_timestamp(now(6))-max(ts/1000000000) as replication_lag from `__vt_e2e-test`.heartbeat","Threshold":1,"AggregatedMetrics":{},"MetricsHealth":{}}
2023-05-23T12:14:38.2379264Z I0523 12:12:43.719734   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-202
2023-05-23T12:14:38.2379778Z I0523 12:12:43.723084   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone2-203
2023-05-23T12:14:38.2381306Z     util.go:131: timed out waiting for the zone2-203 tablet's throttler status enabled to be true with the correct config after 1m0s; last seen value: {"Keyspace":"customer","Shard":"-80","IsLeader":false,"IsOpen":true,"IsEnabled":false,"IsDormant":true,"Query":"select unix_timestamp(now(6))-max(ts/1000000000) as replication_lag from `__vt_e2e-test`.heartbeat","Threshold":1,"AggregatedMetrics":{},"MetricsHealth":{}}
2023-05-23T12:14:38.2382164Z I0523 12:13:43.725539   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-300
2023-05-23T12:14:38.2382770Z I0523 12:13:43.730041   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-301
2023-05-23T12:14:38.2447747Z I0523 12:13:43.731270   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-302
2023-05-23T12:14:38.2448279Z I0523 12:13:43.732833   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone2-303
2023-05-23T12:14:38.2448792Z I0523 12:13:43.733302   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-400
2023-05-23T12:14:38.2449300Z I0523 12:13:43.734463   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-401
2023-05-23T12:14:38.2449803Z I0523 12:13:43.735122   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-402
2023-05-23T12:14:38.2450286Z I0523 12:13:43.736168   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone2-403
2023-05-23T12:14:38.2450768Z I0523 12:13:43.736691   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-502
2023-05-23T12:14:38.2451248Z I0523 12:13:43.737214   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone2-503
2023-05-23T12:14:38.2451734Z I0523 12:13:43.738068   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-500
2023-05-23T12:14:38.2452227Z I0523 12:13:43.738804   17733 cluster_test.go:581] + Waiting for throttler config to be applied on zone1-501
2023-05-23T12:14:38.2452615Z I0523 12:13:43.739572   17733 cluster_test.go:585] Throttler config applied on all shards
2023-05-23T12:14:38.2453263Z I0523 12:13:43.749920   17733 vtctldclient_process.go:67] Executing vtctldclient with command: vtctldclient --server 127.0.0.1:15999 GetShard customer/-40 (attempt 1 of 10)
2023-05-23T12:14:38.2453892Z I0523 12:13:43.887528   17733 vtgate_process.go:205] Waiting for healthy status of 2 customer.-40.replica tablets in cell zone1
2023-05-23T12:14:38.2454480Z I0523 12:13:43.909585   17733 vtgate_process.go:205] Waiting for healthy status of 1 customer.-40.rdonly tablets in cell zone1
2023-05-23T12:14:38.2455154Z I0523 12:13:43.916846   17733 vtctldclient_process.go:67] Executing vtctldclient with command: vtctldclient --server 127.0.0.1:15999 GetShard customer/40-80 (attempt 1 of 10)
2023-05-23T12:14:38.2455782Z I0523 12:13:43.970841   17733 vtgate_process.go:205] Waiting for healthy status of 2 customer.40-80.replica tablets in cell zone1
2023-05-23T12:14:38.2456504Z I0523 12:13:43.978908   17733 vtgate_process.go:205] Waiting for healthy status of 1 customer.40-80.rdonly tablets in cell zone1
2023-05-23T12:14:38.2457171Z I0523 12:13:43.983193   17733 vtctldclient_process.go:67] Executing vtctldclient with command: vtctldclient --server 127.0.0.1:15999 GetShard customer/80-c0 (attempt 1 of 10)
2023-05-23T12:14:38.2457802Z I0523 12:13:44.014598   17733 vtgate_process.go:205] Waiting for healthy status of 2 customer.80-c0.replica tablets in cell zone1
2023-05-23T12:14:38.2458386Z I0523 12:13:44.016839   17733 vtgate_process.go:205] Waiting for healthy status of 1 customer.80-c0.rdonly tablets in cell zone1
2023-05-23T12:14:38.2459060Z I0523 12:13:44.018391   17733 vtctldclient_process.go:67] Executing vtctldclient with command: vtctldclient --server 127.0.0.1:15999 GetShard customer/c0- (attempt 1 of 10)
2023-05-23T12:14:38.2459689Z I0523 12:13:44.048688   17733 vtgate_process.go:205] Waiting for healthy status of 2 customer.c0-.replica tablets in cell zone1
2023-05-23T12:14:38.2460282Z I0523 12:13:44.050931   17733 vtgate_process.go:205] Waiting for healthy status of 1 customer.c0-.rdonly tablets in cell zone1
2023-05-23T12:14:38.2461276Z I0523 12:13:44.053676   17733 vtctlclient_process.go:210] Executing vtctlclient with command: vtctlclient --server 127.0.0.1:15999 Reshard -- --max_replication_lag_allowed=2542087h --source_shards -80,80- --target_shards -40,40-80,80-c0,c0- --defer-secondary-keys --cells zone1 Create customer.wf1 (attempt 1 of 10)
2023-05-23T12:14:38.2461900Z     resharding_workflows_v2_test.go:65: 
2023-05-23T12:14:38.2462673Z         	Error Trace:	/home/runner/work/vitess/vitess/go/test/endtoend/vreplication/resharding_workflows_v2_test.go:65
2023-05-23T12:14:38.2463860Z         	            				/home/runner/work/vitess/vitess/go/test/endtoend/vreplication/resharding_workflows_v2_test.go:404
2023-05-23T12:14:38.2465025Z         	            				/home/runner/work/vitess/vitess/go/test/endtoend/vreplication/resharding_workflows_v2_test.go:282
2023-05-23T12:14:38.2465482Z         	Error:      	Received unexpected error:
2023-05-23T12:14:38.2466091Z         	            	exit status 1: Waiting for workflow to start:
2023-05-23T12:14:38.2531170Z         	            	0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 0% ... 
2023-05-23T12:14:38.2532227Z         	            	The workflow did not start within 30s. The workflow may simply be slow to start or there may be an issue.
2023-05-23T12:14:38.2533510Z         	            	Check the status using the 'Workflow customer.wf1 show' client command for details.
2023-05-23T12:14:38.2535406Z         	            	Reshard Error: rpc error: code = Unknown desc = timed out waiting for workflow to start
2023-05-23T12:14:38.2536682Z         	            	E0523 12:14:15.417566   40839 main.go:105] remote error: rpc error: code = Unknown desc = timed out waiting for workflow to start
2023-05-23T12:14:38.2537130Z         	Test:       	TestBasicV2Workflows
2023-05-23T12:14:38.2537454Z I0523 12:14:25.479757   17733 cluster_test.go:640] vtgate teardown successful
2023-05-23T12:14:38.2537809Z I0523 12:14:25.479788   17733 cluster_test.go:640] vtgate teardown successful

As you can see above, the test timed out at 1min waiting for 2 tablets to pick up the throttler config changes. Because they didn't get the config changes, the subsequent workflow fails to make progress.

[mattlord]

@shlomi-noach finally all the tests are passing!

Here's a summary of my findings when investigating the vreplication e2e test failures -- please let me know if you think I'm wrong/mistaken on any part(s).

In the end there were 5 main issues:

1. The way we were updating the throttler in the vreplication e2e tests -- when adding each shard, after adding the tablets for it -- was triggering a race condition that could be encountered while the throttler was still opening and the SrvKeyspace record was updated. This caused the goroutine executing the SrvKeyspaceWatcher callback function to try and update the throttler config concurrent with the retryReadAndApplyThrottlerConfig function in Open(). This was addressed by ensuring that the initMutex is always taken and held when applying throttler config changes in: cd2cc1b and 261b06a and 48e71a5
2. The http response in our e2e Wait function would at times not contain one or more of the keys we were looking for/at, which does not really indicate a failure, but would cause the test to fail. That was addressed in: 8309b8b
3. By default vreplication will choose a rdonly or replica tablet when available -- and they are available in the vreplication tests -- to be used as the vstreamer (source) for workflows and vdiff2 runs. This meant that we were hitting this bug: VReplication Workflows and VStream API gets stuck in the copy phase if tablet type is set only as a replica #13175 That was addressed temporarily here: 46a4c17 (and subsequently rolled back when we merged the fix: Tablet throttler: be explicit about client app name, exempt some apps from checks and heartbeat renewals #13195)
4. When doing Reshards, the new shards are created with tablets that explicitly have their QueryService disabled. So they are e.g. PRIMARY Not Serving. This means that the throttler is not Open. This is fine, as the throttler should return OK/200 in this case. The problem was that we waited for these tablets to have the throttler enabled and failed the test after the timeout. This was addressed in: 0fc37df (and in follow-ups to also ignore non-primary non-serving types)
5. We should be considering isOpen in the ready/running/CanServe/CheckResult determination since that’s the only time the throttler can function properly (with open DB conn pools etc). There were a few spots where we were instead treating isEnabled alone as that signal. So we were acting AS IF the throttler was ready/running when it was ONLY enabled -- which is now dynamic with the topo method. I believe that distinction was papered over before because isEnabled was not dynamic and was controlled entirely through Open()/Close() (based on the static tablet flags). This was addressed in: 261b06a and 48e71a5

Side Note: I saw that some of the tests that look at log messages became flaky as they would sometimes see this as the most recent log message instead of what they expected: WatchSrvKeyspace clearing cached entry for. It appears that this was likely caused by us at times trying to watch the SrvKeyspace record when it did not (yet) exist. This would then lead to a NoNode error from the topo server and that log message in this scenario. So I elided that useless message when it occurs from that code path (end result being that value is already nil'd out when that code block hits) in bbbb351

[shlomi-noach]

Here's a summary of my findings when investigating the vreplication e2e test failures -- please let me know if you think I'm wrong/mistaken on any part(s).

@mattlord thank you for the summary! I have further refinements, pushed in ba7aa05, all relating to the newly introduced IsOpen()/Isrunning() functions:

1. The various throttler queries: SHOW VITESS_THROTTLER STATUS, SHOW VITESS_THROTTLED_APPS, ALTER VITESS_MIGRATION .. [THROTTLE|UNTHROTTLE] - all should be able to run as long as the throttler IsOpen(), and irrespective of whether the throttler IsEnabled() or not. The user should be allowed to throttler a migration even if at that moment the throttler is actually disabled. Likewise, you should definitely be able to see throttled apps/status when the throttler is disabled.
2. There is an old, existing safety mechanism in Online DDL, that mitigates the scenario where a steamer reads from a replica. The scenario is of course now fixed in Tablet throttler: be explicit about client app name, exempt some apps from checks and heartbeat renewals #13195, but due to compatibility we have to keep this specific Online DDL mitigation in place in v17. So far so good. However, a slight simplification is that the mitigation is only required when the throttler IsRunning(): ba7aa05#diff-059c9f46e8d270d9c5514ef2b08679035eb0daaa8d95074e34ef43a81d50dc37L3596

[shlomi-noach]

Doc updates are in vitessio/website#1489. Removing the NeedsWebsiteDocUpdate label.

[mattlord]

Yay! ❤️

[vitess-bot]

I was unable to backport this Pull Request to the following branches: release-17.0.