flow based tablet load balancer

[demmer]

Description

Implement an optional flow-based tablet balancer.

The motivation and algorithm is described more in #12241 and in a long comment in the code.

In brief, the goal of this implementation is to more evenly balance out the queries to a the replicas in a shard when the tablets are not evenly distributed across the various cells where vtgates are receiving application traffic, because the default routing algorithm will always prefer an available tablet in the same cell as the vtgate, even if there are more idle tablets in other cells. The balancer addresses this by attempting to route an equal share to each tablet, preferring the same-cell where possible, but crossing cells in order to achieve better global balance.

Slack production results

Note that the original algorithm was developed many months ago, but only recently have we made the time to deploy this within Slack and spent some time to verify it works as expected.

The results from our initial testing are very encouraging and we have been running with this feature enabled for some time now on one of our main production keyspaces. This is a 16 shard keyspace with between 9-11 tablets per shard spread among our 4 active cells. Each cell receives the same amount of inbound traffic.

When we first enabled the feature shows that the wide disparity in QPS per replica goes away and instead the QPS is only based on the number of replicas per shard, as expected:

Since then we have actually normalized our deployment so each shard has 9 replicas and the QPS is quite even across all hosts, even though again they do not map evenly to each cell:

Related Issue(s)

#12241

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 Tabletbalancer docs website#1804

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 83.22148% with 25 lines in your changes missing coverage. Please review.

Project coverage is 68.93%. Comparing base (feb845a) to head (707d77b).
Report is 34 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtgate/tabletgateway.go	64.51%	11 Missing ⚠️
go/vt/vtgate/balancer/balancer.go	92.03%	9 Missing ⚠️
go/vt/vtgate/vtgate.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16351      +/-   ##
==========================================
- Coverage   68.98%   68.93%   -0.05%     
==========================================
  Files        1562     1565       +3     
  Lines      200697   201473     +776     
==========================================
+ Hits       138446   138886     +440     
- Misses      62251    62587     +336     

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

[systay]

This looks great @demmer

Just need to use rand/v2 to make CI happy, and then we need a website PR for the new flags, right?

[demmer]

@systay based on the test coverage reports, @venkatraju and I added some more tests for the balancer, and I beefed up some of the existing tests for the retry logic, including running both with and without the balancer. might be overkill but this is kinda a critical part of the code :)

[deepthi]

Suggested change

	fs.StringSliceVar(&balancerVtgateCells, "balancer_vtgate_cells", []string{}, "When in balanced mode, a comma-separated list of cells that contain vtgates (required)")
	fs.StringSliceVar(&balancerVtgateCells, "balancer-vtgate-cells", []string{}, "When in balanced mode, a comma-separated list of cells that contain vtgates (required)")

[deepthi]

Everything else is cosmetic, but this seems important.
What happens if you add vtgates to a cell that didn't previously have any? You would have to restart every vtgate with a new value for this flag?
It may not be very common in practice, but it should be documented somewhere.
Of course, the real answer is that vtgates should be given a list of all cells and discover which ones contain vtgates, but we've so far avoided adding vtgates to the topo.

[demmer]

What happens if you add vtgates to a cell that didn't previously have any? You would have to restart every vtgate with a new value for this flag?

Yep. For the current algorithm to work as designed, it's required that all the vtgates know the fully set of cells that are running vtgates and that the load among these cells is roughly even. This allows each individual vtgate to make the assumption about what the others will do without needing signaling.

If you want to change this assumption therefore, you do have to restart the whole fleet to take it into account, just like any other vtgate flag change.

It may not be very common in practice, but it should be documented somewhere.

That makes sense. I'll include something to this effect when we write the website docs.

[demmer]

Of course, the real answer is that vtgates should be given a list of all cells and discover which ones contain vtgates, but we've so far avoided adding vtgates to the topo.

Yeah - that's obviously beyond the scope of this PR, and it would in theory reduce the dependency that query inflows per cell need to be balanced (i.e. you could use the number of gates as a proxy for how much load you expect).

[deepthi]

Suggested change

	fs.StringSliceVar(&balancerKeyspaces, "balancer_keyspaces", []string{}, "When in balanced mode, a comma-separated list of keyspaces for which to use the balancer (optional)")
	fs.StringSliceVar(&balancerKeyspaces, "balancer-keyspaces", []string{}, "When in balanced mode, a comma-separated list of keyspaces for which to use the balancer (optional)")

[deepthi]

I'm curious, why was this added? So that you can opt in and out per keyspace as a phased rollout?

[demmer]

Yep exactly. That's how we did the rollout testing inside Slack -- started with a loadtest keyspace only and then moved to the one production keyspace where we saw the most imbalance, so we could focus on that without worrying about potential negative impact on the rest of the (thousands of) shards.

[deepthi]

@venkatraju looks like this may need either a merge from main, or a simpler code change to pass linter checks.

[venkatraju]

@venkatraju looks like this may need either a merge from main, or a simpler code change to pass linter checks.

@deepthi Fixed the log line that was triggering the linter error.

[deepthi]

@systay @rohit-nayak-ps do either of you think this needs a release note?

I've added the label to block merging. This will be a nice thing to add to the release notes.
@venkatraju can you please add a section to the summary release notes doc for this feature? It should go into https://github.com/vitessio/vitess/blob/main/changelog/21.0/21.0.0/summary.md

[venkatraju]

@venkatraju can you please add a section to the summary release notes doc for this feature? It should go into https://github.com/vitessio/vitess/blob/main/changelog/21.0/21.0.0/summary.md

@deepthi added release notes. Could you review the text, and also comment on whether you want the new flags called out there?

(sorry about the messed up git history from a rebase that went wrong)

[deepthi]

@venkatraju can you please add a section to the summary release notes doc for this feature? It should go into https://github.com/vitessio/vitess/blob/main/changelog/21.0/21.0.0/summary.md

@deepthi added release notes. Could you review the text, and also comment on whether you want the new flags called out there?

(sorry about the messed up git history from a rebase that went wrong)

This looks fine. Details on flags will be in the vtgate command reference (auto generated).