Tablet throttler: throttled app configuration via vtctl UpdateThrottlerConfig

[shlomi-noach]

Description

Per #13341, we want to move throttled apps configuration into topo. Right now, to throttle a specific app, the user must access the PRIMARY tablet's HTTP endpoint, per shard. The throttled app information is only stored locally on the tablet and does not survive PRS or ERS.

With this PR, we will store throttled app info in topo, as part of the already existing ThrottlerConfig, which is then read by all tablets in all shards.

Example vtctld invocation:

$ vtctldclient UpdateThrottlerConfig --throttle-app="vreplication" --throttle-app-ratio=0.5 --throttle-app-duration="30m" commerce

$ vtctldclient GetKeyspace commerce

{
  "name": "commerce",
  "keyspace": {
    "served_froms": [],
    "keyspace_type": 0,
    "base_keyspace": "",
    "snapshot_time": null,
    "durability_policy": "semi_sync",
    "throttler_config": {
      "enabled": true,
      "threshold": 15,
      "custom_query": "",
      "check_as_check_self": false,
      "throttled_apps": {
        "vreplication": {
          "name": "vreplication",
          "ratio": 0.5,
          "expires_at": {
            "seconds": "1687864412",
            "nanoseconds": 142717831
          }
        }
      }
    },
    "sidecar_db_name": "_vt"
  }
}

Related Issue(s)

• Tablet Throttler: v18 features and objectives #13341

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 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.
• 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]

Extracted throttler race condition fix to #13384

[shlomi-noach]

In a followup PR, we will need to reroute ALTER VITESS_MIGRATION ... THROTTLE ... query, so that instead of going from vtgate to the tablets, it will invoke an UpdateKeyspace command on topo.

I'm leaving this to a different PR because we need to handle backwards compatibility and I don't want to overload this PR any more.

[shlomi-noach]

Ready to review!

[shlomi-noach]

Docs PR: vitessio/website#1512

[mattlord]

I like it! I had some comments and suggestions. Let me know what you think.

[mattlord]

How would a user know what valid app names exist? Without that, I think that this has limited value to the average user.

Can we add an exported variable or function here: https://github.com/vitessio/vitess/blob/main/go/vt/vttablet/tabletserver/throttle/throttlerapp/app.go#L29-L51

And then use that in the help output, e.g.:

throttledApp := subFlags.String("throttle-app", "", "an app name to throttle; valid values are %s", strings.Join(throttlerapp.GetNames(), ","))

Same for vtctldclient.

With that, we could also add some basic input validation so that a user is aware if they specified an invalid app name (e.g. mistyped vstraemer).

IMO these minor improvements would provide a lot of value.

[shlomi-noach]

In the current design, app names are arbitrary. And while you have those well-known (how are they "well known" is a discussion of its own) apps like online-ddl or vreplication, there are also variants. Like, an Online DDL migration that uses vreplication answers both to online-ddl and to vreplication, and also it answers to the migration's UUID.

So, you can throttle all online-ddl operations ; or you can throttle all vreplication operations; or you can throttle a specific migration.

The list in

vitess/go/vt/vttablet/tabletserver/throttle/throttlerapp/app.go

Lines 29 to 51 in 1518cc1

	const (
	// DefaultName is the app name used by vitess when app doesn't indicate its name
	DefaultName Name = "default"
	VitessName Name = "vitess"
	TableGCName Name = "tablegc"
	OnlineDDLName Name = "online-ddl"
	GhostName Name = "gh-ost"
	PTOSCName Name = "pt-osc"
	VStreamerName Name = "vstreamer"
	VReplicationName Name = "vreplication"
	VPlayerName Name = "vplayer"
	VCopierName Name = "vcopier"
	ResultStreamerName Name = "resultstreamer"
	RowStreamerName Name = "rowstreamer"
	ExternalConnectorName Name = "external-connector"
	ReplicaConnectorName Name = "replica-connector"
	BinlogWatcherName Name = "binlog-watcher"
	MessagerName Name = "messager"
	SchemaTrackerName Name = "schema-tracker"
	)

is IMO pretty long to print out in a single CLI line.

I do see your point here, though! And I agree. But I don't have a good grasp of what would be the right solution. Most app names are irrelevant to the user and are really internal (e.g. I don't really want a user messing with schema-tracker, and if they do, that's on them). Maybe we should just let them know about the obvious ones?

The other thing is that we have a SQL interface to throttling migrations: ALTER VITESS_MIGRATION [UUID] THROTTLE [ALL]. In a followup PR I will make that SQL command go through topo (right now it goes directly to the tablets). So that's perhaps a formalization where the user doesn't even need to know the app name.

Open to discuss further.

[mattlord]

Ah, I see. Thanks! In that case then, I'd call them built-in / vitess / internal app names -- as we want Vitess operators to be able to use this with some ease. If you think it's a lot of work, we could also create a PR to make this improvement later (and think more about how best to do it).

What do you think? We could also cover this in the website docs. If you think it's cumbersome / difficult in Vitess, we could at least do it in the docs and then that would be good enough IMO.

[shlomi-noach]

Let's think about this a bit more. I think we need to understand better the usage patterns. For example, just the other day I've been asekd whether it's possible (it is with this PR) to only throttle vcopier but not vplayer as part of a VReplication / OnlineDDL workflow -- the reasoning for which was that they wanted to throttle for a duration of a few days, but did not want to break the workflow as binary logs were expected to rotate & purge.

[shlomi-noach]

So I suggest let's document what we have for now, and follow up with another PR as needed.

[mattlord]

IMO this is probably also a good place to validate the app name to be sure that it's valid.

[shlomi-noach]

commented in #13351 (comment), but technically any arbitrary string is valid...

[mattlord]

I guess that most of the changes in this file helped to deflake the tests? Maybe it's another cherry-pick?

[shlomi-noach]

With the new design, it's impossible to tell vitess "throttle one shard but not this other shard". Previously, it was possible since we hit a shard's PRIMARY tablet's HTTP API directly.

So the new design means we need to change the tests. And we don't need to test something that we don't support...

[mattlord]

Might be worth doing each of these in a goroutine. Although I don't think we have enough shards and tablets in the tests today that it should matter too much.

[shlomi-noach]

Yeah, the good news is that those tablets do, meanwhile, update their configs concurrently. Say we had to wait full 42sec for the first tablet; chances are that by the time we check the next tablets, they will have already picked the change. So this loop really just waits for the slowest tablet, which is the same as if we'd run goroutines. We just have a subsecond overhead for serially checking those successful tablets.

[mattlord]

Approving since the only open comment is this one: #13351 (comment)

And we may end up resolving that with docs only changes.

[shlomi-noach]

Docs PR in vitessio/website#1512