Throttler: Expose Tablet's Config & Leverage to Deflake Tests #12737

mattlord · 2023-03-27T20:38:38Z

Description

The primary thing that this PR does is expose the tablet's current throttler config — query and threshold — via its /throttler/status http endpoint. This allows operators and test creators to confirm and wait for specific configurations to be live on all tablets.

The secondary thing that this PR does is disable vtorc recoveries in the throttler tests when needed (may be other reasons as yet unknown or that unintentionally arise later) because we stop replication, wait a few seconds, then expect there to be lag. But vtorc could repair replication during that wait and then the lag is gone. We had previously explicitly disabled the legacy tablet repair by using the --disable_active_reparents flag, now in this PR we also disable any vtorc repairs.

Finally, a minor thing that this PR does is enable the tablet throttler configuration via the topo in the local examples. This does not alter the default behavior, but it allows users to test out this new feature without having to modify any files or restart any processes. This will help with local testing of the new feature as we move it towards GA (currently experimental).

The relevant throttler_topo workflow now passed 15 times in a row: https://github.com/vitessio/vitess/actions/runs/4557359892?pr=12737

The failures now seem due to some odd/unexpected behavior of the throttler itself during test runs. We'll track any bugs down over time -- which will now be much easier with the updated tests.

Related Issue(s)

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: Throttler: Add query and threshold to example status output website#1427

For example, we stop replication, wait a few seconds, then expect there to be lag. But vtorc could repair replication during that wait and then the lag is gone. Signed-off-by: Matt Lord <mattalord@gmail.com>

vitess-bot · 2023-03-27T20:38:42Z

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.

Signed-off-by: Matt Lord <mattalord@gmail.com>

go/test/endtoend/tabletmanager/throttler_topo/throttler_test.go

Signed-off-by: Matt Lord <mattalord@gmail.com>

shlomi-noach

Looks good, thank you @mattlord !

Signed-off-by: Matt Lord <mattalord@gmail.com>

Which seemed to revolve around NOT sleeping long enough after starting all the sleep queries. Signed-off-by: Matt Lord <mattalord@gmail.com>

Signed-off-by: Matt Lord <mattalord@gmail.com>

shlomi-noach

There's been some changes since my earlier review, so I dismissed it and am submitting this new review. A couple of those changes are a bit unclear to me and I'm grateful if you could answer my inline questions.

shlomi-noach · 2023-03-29T10:10:53Z

go/test/endtoend/tabletmanager/throttler_topo/throttler_test.go

-	t.Run("enabling throttler with low threshold", func(t *testing.T) {
-		_, err := onlineddl.UpdateThrottlerTopoConfig(clusterInstance, true, false, unreasonablyLowThreshold.Seconds(), "", false)
+	t.Run("enabling throttler with very low threshold", func(t *testing.T) {
+		_, err := throttler.UpdateThrottlerTopoConfig(clusterInstance, true, false, unreasonablyLowThreshold.Seconds(), useDefaultQuery, false)


shlomi-noach · 2023-03-29T10:11:03Z

go/test/endtoend/tabletmanager/throttler_topo/throttler_test.go

+		// Wait for the throttler to be enabled everywhere with the new config.
+		for _, tablet := range clusterInstance.Keyspaces[0].Shards[0].Vttablets {
+			throttler.WaitForThrottlerStatusEnabled(t, tablet, true, &throttler.Config{Query: throttler.DefaultQuery, Threshold: unreasonablyLowThreshold.Seconds()}, throttlerEnabledTimeout)
+		}


shlomi-noach · 2023-03-29T10:11:12Z

go/test/endtoend/tabletmanager/throttler_topo/throttler_test.go

+		// Wait for the throttler to be disabled everywhere.
+		for _, tablet := range clusterInstance.Keyspaces[0].Shards[0].Vttablets {
+			throttler.WaitForThrottlerStatusEnabled(t, tablet, false, nil, throttlerEnabledTimeout)
+		}


go/test/endtoend/tabletmanager/throttler_topo/throttler_test.go

Signed-off-by: Matt Lord <mattalord@gmail.com>

shlomi-noach

And adjust timing Signed-off-by: Matt Lord <mattalord@gmail.com>

Signed-off-by: Matt Lord <mattalord@gmail.com>

examples/common/scripts/vttablet-up.sh

deepthi · 2023-03-29T23:29:03Z

go/vt/vttablet/tabletserver/throttle/throttler.go

@@ -84,7 +84,7 @@ func registerThrottlerFlags(fs *pflag.FlagSet) {

 	fs.DurationVar(&throttleThreshold, "throttle_threshold", throttleThreshold, "Replication lag threshold for default lag throttling")
 	fs.StringVar(&throttleMetricQuery, "throttle_metrics_query", throttleMetricQuery, "Override default heartbeat/lag metric. Use either `SELECT` (must return single row, single value) or `SHOW GLOBAL ... LIKE ...` queries. Set -throttle_metrics_threshold respectively.")
-	fs.Float64Var(&throttleMetricThreshold, "throttle_metrics_threshold", throttleMetricThreshold, "Override default throttle threshold, respective to -throttle_metrics_query")
+	fs.Float64Var(&throttleMetricThreshold, "throttle_metrics_threshold", throttleMetricThreshold, "Override default throttle threshold, respective to --throttle_metrics_query")


not actually relevant to the changes in this PR - what does respective to --throttle_metrics_query mean?
maybe a question more for @shlomi-noach

It comes hand-in-hand with --throttle_metrics_query. If you define a --throttle_metrics_query, then you should also say what's the --throttle_metrics_threshold at which the throttle will engage. Not sure how to phrase it otherwise.

This is because enabling heartbeats with --heartbeat_enable also results in the replication reporter being enabled: https://github.com/vitessio/vitess/blob/3d9ef871e42bd20a60ec95997c97ecf0694c1e78/go/vt/vttablet/tabletserver/tabletenv/config.go#L235-L237 Signed-off-by: Matt Lord <mattalord@gmail.com>

Signed-off-by: Matt Lord <mattalord@gmail.com>

vitess-bot · 2023-03-30T17:54:32Z

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

…io#12737) * Flakes: effectively disable vtorc for deterministic behavior For example, we stop replication, wait a few seconds, then expect there to be lag. But vtorc could repair replication during that wait and then the lag is gone. Signed-off-by: Matt Lord <mattalord@gmail.com> * Wait for the throttler to be up and running everywhere Signed-off-by: Matt Lord <mattalord@gmail.com> * Expose tablet's throttler config and leverage to deflake tests Signed-off-by: Matt Lord <mattalord@gmail.com> * Apply various corrections Signed-off-by: Matt Lord <mattalord@gmail.com> * Be more explicit about VTOrc behavior changes Signed-off-by: Matt Lord <mattalord@gmail.com> * Note received throttler response when it is unexpected Signed-off-by: Matt Lord <mattalord@gmail.com> * Fixes from local testing Signed-off-by: Matt Lord <mattalord@gmail.com> * Nits from self review Signed-off-by: Matt Lord <mattalord@gmail.com> * Use assert.Equalf on failed assertions Signed-off-by: Matt Lord <mattalord@gmail.com> * Ummm, duh. Signed-off-by: Matt Lord <mattalord@gmail.com> * Try to get rid of last bit of flakiness Which seemed to revolve around NOT sleeping long enough after starting all the sleep queries. Signed-off-by: Matt Lord <mattalord@gmail.com> * Nits from self review Signed-off-by: Matt Lord <mattalord@gmail.com> * Address review comments Signed-off-by: Matt Lord <mattalord@gmail.com> * Adjust test for behavior and comment it And adjust timing Signed-off-by: Matt Lord <mattalord@gmail.com> * Align both stale hearbeat checks Signed-off-by: Matt Lord <mattalord@gmail.com> * Remove no longer needed flag This is because enabling heartbeats with --heartbeat_enable also results in the replication reporter being enabled: https://github.com/vitessio/vitess/blob/3d9ef871e42bd20a60ec95997c97ecf0694c1e78/go/vt/vttablet/tabletserver/tabletenv/config.go#L235-L237 Signed-off-by: Matt Lord <mattalord@gmail.com> * Correct comment Signed-off-by: Matt Lord <mattalord@gmail.com> * Correct comment part II: electric boogaloo Signed-off-by: Matt Lord <mattalord@gmail.com> * Revert one other minor unnecessary change. Signed-off-by: Matt Lord <mattalord@gmail.com> --------- Signed-off-by: Matt Lord <mattalord@gmail.com>

…e Tests (#12791) * Throttler: Expose Tablet's Config & Leverage to Deflake Tests (#12737) * Flakes: effectively disable vtorc for deterministic behavior For example, we stop replication, wait a few seconds, then expect there to be lag. But vtorc could repair replication during that wait and then the lag is gone. Signed-off-by: Matt Lord <mattalord@gmail.com> * Wait for the throttler to be up and running everywhere Signed-off-by: Matt Lord <mattalord@gmail.com> * Expose tablet's throttler config and leverage to deflake tests Signed-off-by: Matt Lord <mattalord@gmail.com> * Apply various corrections Signed-off-by: Matt Lord <mattalord@gmail.com> * Be more explicit about VTOrc behavior changes Signed-off-by: Matt Lord <mattalord@gmail.com> * Note received throttler response when it is unexpected Signed-off-by: Matt Lord <mattalord@gmail.com> * Fixes from local testing Signed-off-by: Matt Lord <mattalord@gmail.com> * Nits from self review Signed-off-by: Matt Lord <mattalord@gmail.com> * Use assert.Equalf on failed assertions Signed-off-by: Matt Lord <mattalord@gmail.com> * Ummm, duh. Signed-off-by: Matt Lord <mattalord@gmail.com> * Try to get rid of last bit of flakiness Which seemed to revolve around NOT sleeping long enough after starting all the sleep queries. Signed-off-by: Matt Lord <mattalord@gmail.com> * Nits from self review Signed-off-by: Matt Lord <mattalord@gmail.com> * Address review comments Signed-off-by: Matt Lord <mattalord@gmail.com> * Adjust test for behavior and comment it And adjust timing Signed-off-by: Matt Lord <mattalord@gmail.com> * Align both stale hearbeat checks Signed-off-by: Matt Lord <mattalord@gmail.com> * Remove no longer needed flag This is because enabling heartbeats with --heartbeat_enable also results in the replication reporter being enabled: https://github.com/vitessio/vitess/blob/3d9ef871e42bd20a60ec95997c97ecf0694c1e78/go/vt/vttablet/tabletserver/tabletenv/config.go#L235-L237 Signed-off-by: Matt Lord <mattalord@gmail.com> * Correct comment Signed-off-by: Matt Lord <mattalord@gmail.com> * Correct comment part II: electric boogaloo Signed-off-by: Matt Lord <mattalord@gmail.com> * Revert one other minor unnecessary change. Signed-off-by: Matt Lord <mattalord@gmail.com> --------- Signed-off-by: Matt Lord <mattalord@gmail.com> * Post cherry-pick fixup Signed-off-by: Matt Lord <mattalord@gmail.com> --------- Signed-off-by: Matt Lord <mattalord@gmail.com>

Flakes: effectively disable vtorc for deterministic behavior

a75191c

For example, we stop replication, wait a few seconds, then expect there to be lag. But vtorc could repair replication during that wait and then the lag is gone. Signed-off-by: Matt Lord <mattalord@gmail.com>

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Mar 27, 2023

mattlord added Flakes Component: Build/CI Type: Testing and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Mar 27, 2023

mattlord requested review from GuptaManan100 and shlomi-noach March 27, 2023 20:43

mattlord added the Backport to: release-16.0 label Mar 27, 2023

Wait for the throttler to be up and running everywhere

e3b999a

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord changed the title ~~Flakes: effectively disable vtorc for deterministic behavior in throttler e2e tests~~ Throttler: Expose Tablet's Config & Leverage to Deflake Tests Mar 28, 2023

mattlord force-pushed the throttler_vtorc_flakes branch 7 times, most recently from becac96 to 4ebc2fe Compare March 28, 2023 04:54

Expose tablet's throttler config and leverage to deflake tests

d303444

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord force-pushed the throttler_vtorc_flakes branch from 4ebc2fe to d303444 Compare March 28, 2023 04:56

shlomi-noach reviewed Mar 28, 2023

View reviewed changes

go/test/endtoend/tabletmanager/throttler_topo/throttler_test.go Outdated Show resolved Hide resolved

shlomi-noach reviewed Mar 28, 2023

View reviewed changes

go/test/endtoend/tabletmanager/throttler_topo/throttler_test.go Show resolved Hide resolved

mattlord force-pushed the throttler_vtorc_flakes branch from e20fc72 to 73420fa Compare March 28, 2023 06:15

Apply various corrections

c623dc9

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord force-pushed the throttler_vtorc_flakes branch from 73420fa to c623dc9 Compare March 28, 2023 06:18

shlomi-noach approved these changes Mar 28, 2023

View reviewed changes

shlomi-noach marked this pull request as ready for review March 28, 2023 06:33

Nits from self review

4f98ae1

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord marked this pull request as ready for review March 28, 2023 19:28

mattlord added 2 commits March 28, 2023 16:31

Use assert.Equalf on failed assertions

d28de5a

Signed-off-by: Matt Lord <mattalord@gmail.com>

Ummm, duh.

d613f94

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord added the Backport to: release-16.0 label Mar 28, 2023

mattlord force-pushed the throttler_vtorc_flakes branch from de6ddce to f7303ba Compare March 29, 2023 00:28

Try to get rid of last bit of flakiness

3a793f8

Which seemed to revolve around NOT sleeping long enough after starting all the sleep queries. Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord force-pushed the throttler_vtorc_flakes branch from f7303ba to 3a793f8 Compare March 29, 2023 01:04

Nits from self review

07333c2

Signed-off-by: Matt Lord <mattalord@gmail.com>

shlomi-noach self-requested a review March 29, 2023 10:17

shlomi-noach reviewed Mar 29, 2023

View reviewed changes

Address review comments

181a5cd

Signed-off-by: Matt Lord <mattalord@gmail.com>

shlomi-noach approved these changes Mar 29, 2023

View reviewed changes

mattlord added 2 commits March 29, 2023 13:22

Adjust test for behavior and comment it

a3017f2

And adjust timing Signed-off-by: Matt Lord <mattalord@gmail.com>

Align both stale hearbeat checks

19fe7c8

Signed-off-by: Matt Lord <mattalord@gmail.com>

deepthi approved these changes Mar 29, 2023

View reviewed changes

mattlord added 4 commits March 29, 2023 19:51

Correct comment

7f41207

Signed-off-by: Matt Lord <mattalord@gmail.com>

Correct comment part II: electric boogaloo

db8e13d

Signed-off-by: Matt Lord <mattalord@gmail.com>

Revert one other minor unnecessary change.

06628d8

Signed-off-by: Matt Lord <mattalord@gmail.com>

mattlord mentioned this pull request Mar 30, 2023

Throttler: Add query and threshold to example status output vitessio/website#1427

Merged

mattlord merged commit 1a0c3fe into vitessio:main Mar 30, 2023
112 checks passed

mattlord deleted the throttler_vtorc_flakes branch March 30, 2023 17:53

rohit-nayak-ps mentioned this pull request Mar 30, 2023

[release-16.0] Throttler: Expose Tablet's Config & Leverage to Deflake Tests #12791

Merged

mattlord mentioned this pull request Apr 18, 2023

Feature Request: GetThrottlerConfig vtctld RPC #12925

Closed

maxenglander mentioned this pull request Jan 18, 2024

examples: rm heartbeat flags #14980

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throttler: Expose Tablet's Config & Leverage to Deflake Tests #12737

Throttler: Expose Tablet's Config & Leverage to Deflake Tests #12737

mattlord commented Mar 27, 2023 •

edited

vitess-bot bot commented Mar 27, 2023

shlomi-noach left a comment •

edited

shlomi-noach left a comment

shlomi-noach Mar 29, 2023

shlomi-noach Mar 29, 2023

shlomi-noach Mar 29, 2023

shlomi-noach left a comment

deepthi Mar 29, 2023

shlomi-noach Mar 30, 2023

vitess-bot bot commented Mar 30, 2023

Throttler: Expose Tablet's Config & Leverage to Deflake Tests #12737

Throttler: Expose Tablet's Config & Leverage to Deflake Tests #12737

Conversation

mattlord commented Mar 27, 2023 • edited

Description

Related Issue(s)

Checklist

vitess-bot bot commented Mar 27, 2023

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

shlomi-noach left a comment • edited

Choose a reason for hiding this comment

shlomi-noach left a comment

Choose a reason for hiding this comment

shlomi-noach Mar 29, 2023

Choose a reason for hiding this comment

shlomi-noach Mar 29, 2023

Choose a reason for hiding this comment

shlomi-noach Mar 29, 2023

Choose a reason for hiding this comment

shlomi-noach left a comment

Choose a reason for hiding this comment

deepthi Mar 29, 2023

Choose a reason for hiding this comment

shlomi-noach Mar 30, 2023

Choose a reason for hiding this comment

vitess-bot bot commented Mar 30, 2023

mattlord commented Mar 27, 2023 •

edited

shlomi-noach left a comment •

edited